twai: Don't abort on interrupt allocation failure (IDFGH-10231) #11494
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
changed the title
Dazza0
reviewed
May 26, 2023
There was a problem hiding this comment.
@nebkat Thanks for your contribution, the requested changes looks reasonable to me.
I would probably want to rearrange the order of operations in the twai_driver_install() so that all resource allocations are done at the start. Furthermore, would you be able to target this PR for the master branch (it would work better with our backporting workflow)?
| @@ -446,7 +449,8 @@ esp_err_t twai_driver_install(const twai_general_config_t *g_config, const twai_ | |||
|
|
|||
| //Allocate GPIO and Interrupts | |||
| twai_configure_gpio(g_config->tx_io, g_config->rx_io, g_config->clkout_io, g_config->bus_off_io); | |||
| ESP_ERROR_CHECK(esp_intr_alloc(ETS_TWAI_INTR_SOURCE, g_config->intr_flags, twai_intr_handler_main, NULL, &p_twai_obj->isr_handle)); | |||
| ret = esp_intr_alloc(ETS_TWAI_INTR_SOURCE, g_config->intr_flags, twai_intr_handler_main, NULL, &p_twai_obj->isr_handle); | |||
There was a problem hiding this comment.
Prefer if we
- Allocated interrupts immediately after allocating the driver object. This way, if any resource allocation fails, we can return early
- Added the
ESP_INTR_FLAG_INTRDISABLEDflag so that an allocated interrupt remains disabled until we explicitly enable it usingesp_intr_enable()
So something like...
Suggested change
| ret = esp_intr_alloc(ETS_TWAI_INTR_SOURCE, g_config->intr_flags, twai_intr_handler_main, NULL, &p_twai_obj->isr_handle); | |
| //Create a TWAI object (including queues and semaphores) | |
| p_twai_obj_dummy = twai_alloc_driver_obj(g_config->tx_queue_len, g_config->rx_queue_len); | |
| TWAI_CHECK(p_twai_obj_dummy != NULL, ESP_ERR_NO_MEM); | |
| //Allocate interrupt for TWAI driver | |
| intr_handle_t isr_hdl; | |
| ret = esp_intr_alloc(ETS_TWAI_INTR_SOURCE, g_config->intr_flags | ESP_INTR_FLAG_INTRDISABLED, twai_intr_handler_main, NULL, &isr_hdl); | |
| if (ret != ESP_OK) { | |
| ESP_LOGE(TWAI_TAG, "Interrupt allocation failed"); | |
| goto intr_alloc_err; | |
| } |
espressif-bot
added
Status: Done
|
Sorry never got around to finishing this, thanks for the change! |
|
@nebkat No worries. I've made some modifications to your PR commit, but commit author is still attributed to you. Thanks for the contribution |
espressif-bot
pushed a commit
that referenced
this pull request
Aug 22, 2023
…lure This commit updates twai_driver_install() so that an error is returned when esp_intr_alloc() fails, instead of aborting. Closes #11494 [darian@espressif.com: Refactored object allocation and free procedures] [darian@espressif.com: Updated commit message] Signed-off-by: Darian Leung <darian@espressif.com>
espressif-bot
pushed a commit
that referenced
this pull request
Sep 1, 2023
…lure This commit updates twai_driver_install() so that an error is returned when esp_intr_alloc() fails, instead of aborting. Closes #11494 [darian@espressif.com: Refactored object allocation and free procedures] [darian@espressif.com: Updated commit message] Signed-off-by: Darian Leung <darian@espressif.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
twai_driver_install()currently aborts if there is a failure allocation the interrupt handler, unlike for example UART driver which simply returns the error code:esp-idf/components/driver/uart/uart.c
Lines 1588 to 1591 in d00e7b5
With this change execution can continue even if the allocation fails.