-
Notifications
You must be signed in to change notification settings - Fork 7.3k
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
Move xSemaphoreGive out of configASSERT (IDFGH-7988) #9497
Conversation
xSemaphoreGive won't be executed in configASSERT and semaphore will stay locked if NDEBUG (idf v5) or CONFIG_FREERTOS_ASSERT_DISABLE (idf v3, v4) are defined.
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.
Thank you for noticing the bug and submitting the fix! Have left just one request about making the code more readable.
components/esp_ringbuf/ringbuf.c
Outdated
@@ -1358,7 +1358,8 @@ BaseType_t xRingbufferAddToQueueSetRead(RingbufHandle_t xRingbuffer, QueueSetHan | |||
xReturn = xQueueAddToSet(rbGET_RX_SEM_HANDLE(pxRingbuffer), xQueueSet); | |||
if (xHoldSemaphore == pdTRUE) { | |||
//Return semaphore if temporarily held | |||
configASSERT(xSemaphoreGive(rbGET_RX_SEM_HANDLE(pxRingbuffer)) == pdTRUE); | |||
xHoldSemaphore = xSemaphoreGive(rbGET_RX_SEM_HANDLE(pxRingbuffer)); |
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.
Would you consider changing this assignment to some new variable? Repurposing the xHoldSemaphore variable for the return code looks a bit odd here.
(same in the case below)
Thanks for your contribution. |
LGTM |
sha=4837ba9b84aa00ff230e729f15728b93d327b352 |
xSemaphoreGive won't be executed in configASSERT and semaphore will stay locked if NDEBUG (idf v5) or CONFIG_FREERTOS_ASSERT_DISABLE (idf v3, v4) are defined.