Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

openamp: add error log when ept->cb return error #519

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

CV-Bowen
Copy link
Contributor

Add helpful log when rpmsg endpoint callback return error.

@CV-Bowen
Copy link
Contributor Author

@arnopo Hi, why the docs/readthedocs is always failed. Did I miss some things to do?

@arnopo
Copy link
Collaborator

arnopo commented Oct 16, 2023

@arnopo Hi, why the docs/readthedocs is always failed. Did I miss some things to do?

you should rebase your work on main branch, seems that your 6 commits behind

@CV-Bowen
Copy link
Contributor Author

@arnopo Thanks, now CI passed.

"unexpected callback status\r\n");
if (status < 0) {
metal_log(METAL_LOG_ERROR,
"ept %s, cb %p, return status %d\r\n",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to print ept->cb function pointer address ? only printing ept->name should be enough.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, you could use "ml_err" macro in place of "metal_log(METAL_LOG_ERROR",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some thinking, only print ept->name is enough. Thanks.

add helpful log when rpmsg endpoint callback return error

Signed-off-by: Guiding Li <[email protected]>
Signed-off-by: Bowen Wang <[email protected]>
@xiaoxiang781216
Copy link
Collaborator

@arnopo how about this patch?

@arnopo arnopo requested a review from tnmysh November 10, 2023 09:23
@arnopo
Copy link
Collaborator

arnopo commented Nov 10, 2023

@arnopo how about this patch?

we have a duplication with the assert and the error message. The question is what is the expected behavior: Crash on error, just display an error message or both?

Another question is why this error message is not displayed in the callback itself? Seems to me that this would be more efficient for the debug. And the user can decide to print an error and return a status 0 to not crash the system.

an alternative could be to update the RPMSG_ASSERT macro to have variadic arguments, something like this ( not tested)

#define RPMSG_ASSERT(_exp, format, ...) do { \
		if (!(_exp)) { \
			metal_log(METAL_LOG_EMERGENCY, \
				  "FATAL: %s - " format,  __func__, ##__VA_ARGS__); \
			metal_assert(_exp); \
		} \
	} while (0)

but sound to me better to display the message in the endpoint callback

@CV-Bowen
Copy link
Contributor Author

CV-Bowen commented Dec 6, 2023

@arnopo OK, maybe add error log to the endpoint callback is better although it needs more works.

@CV-Bowen CV-Bowen marked this pull request as draft December 6, 2023 02:53
Copy link

This pull request has been marked as a stale pull request because it has been open (more than) 45 days with no activity.

@github-actions github-actions bot added the Stale label Jan 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants