-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
feat: Add ESP_RETURN_ON_ERR macro (IDFGH-14787) #15525
base: master
Are you sure you want to change the base?
Conversation
👋 Hello robotman2412, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. LGTM! Just one comment is about renaming and adding an example of usage in the macro description.
* Macro that returns the error code if an error occurs, optionally running extra code. | ||
* The extra code can be placed after the function thay may error and it will only run if the error occurs. | ||
*/ | ||
#define ESP_RETURN_ON_ERR(x, ...) do { \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest renaming it as the macros in this file start with ESP_ERROR_CHECK_ . It could be something like ESP_ERROR_RETURN, or ESP_ERROR_CHECK_RETURN.
Additionally, it would be good to have an example of usage. Could you add in the description of the macro how to use it, something like this, I guess:
* @example
* esp_err_t get_data()
* {
* ESP_ERROR_CHECK_RETURN(read_file())
* ESP_ERROR_CHECK_RETURN(read_file(), close_file())
* return ESP_OK;
* }
You may want to move your code to esp-idf/components/esp_common/include/esp_check.h Lines 19 to 25 in 94cfe39
|
Part of the reason for this PR is that this new macro does not log at all, as opposed to with a menuconfig option. I would be open to moving it to that file if that would get your approval for merging, but I do think it's important to have this non-log variant. |
Well, my intention was to make sure you considered the existing macro. I'm fine with having both. |
Hi @robotman2412! I am ok with merging it when it is updated (rename or move to esp_check.h). Thanks. |
Description
This simple macro is a safe way of returning en error code rather than aborting like
ESP_ERROR_CHECK
would. It is intended to allow future pull requests by myself and/or my friend to replace unnecessaryESP_ERROR_CHECK
s with this newESP_RETURN_ON_ERR
macro.Related
N/A
Testing
This does not involve changing any C files and should not affect any existing code.
Checklist
Before submitting a Pull Request, please ensure the following: