Replace task noitifcations with semaphores to avoid conflicts.#421
Merged
Replace task noitifcations with semaphores to avoid conflicts.#421
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughArr: Removed the Kconfig integer option and refactored task synchronization to use a centralized semaphore registry inside NimBLEUtils ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/NimBLEUtils.cpp (1)
138-147: LGTM on the release path, just one pedantic nit.Lookup-by-taskID, null-check, error log with return-code string — all shipshape. One tiny polish: the logged "task handle" wording in
taskWait(Line 123) and "in taskRelease" here be slightly inconsistent; consider unifyin' for easier triage when a user grep's the logs. Yarrr.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEUtils.cpp` around lines 138 - 147, Log wording is inconsistent between the task wait and release paths; update the error message in NimBLEUtils::taskRelease that currently calls NIMBLE_LOGE(LOG_TAG, "Semaphore lookup failed in taskRelease") to use the same "task handle" phrasing as the message in NimBLEUtils::taskWait so grepping/troubleshooting is consistent (locate NimBLEUtils::getTaskSem, taskRelease, taskWait and make the taskRelease NIMBLE_LOGE message mirror the wording used by taskWait).src/NimBLEUtils.h (1)
78-89: Aye, a tidy encapsulation — but the registry's cargo hold be unguarded.The
frienddeclarations andprivatehelpers keep the surface clean, butm_taskSemEntriesis a shared mutable container with no synchronization primitive beside it. SincegetTaskSem()can be invoked concurrently from app tasks callin' BLE ops on the same device, consider documentin' (or addin') a mutex here to fend off concurrentpush_back/iteration. Full concern be filed onNimBLEUtils.cpp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEUtils.h` around lines 78 - 89, The static vector m_taskSemEntries is accessed concurrently by getTaskSem (and deleteTaskSems) and needs synchronization: add a static mutex (e.g., std::mutex m_taskSemEntriesMutex) alongside the TaskSemEntry/m_taskSemEntries declarations, include <mutex>, and guard all accesses/iterations/modifications of m_taskSemEntries inside getTaskSem and deleteTaskSems (and any other functions touching it) with std::lock_guard<std::mutex> to prevent concurrent push_back/iteration races.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEDevice.cpp`:
- Line 1075: The call to NimBLEUtils::deleteTaskSems() can free semaphores while
other threads may still be blocked in NimBLEUtils::taskWait(); update teardown
so role objects explicitly release waiting tasks before the global semaphore
registry is wiped: in the destructors for NimBLEScan, NimBLEClient (and any
class owning m_pTaskData) call NimBLEUtils::taskRelease(m_pTaskData, /*error*/
true) or equivalent to wake/punt any pending ble_npl_sem_pend, and/or modify
NimBLEUtils::deleteTaskSems() to skip deleting entries whose m_pTaskData
indicates a pending waiter; ensure all waits are released/unwound prior to
removing semaphore memory to avoid dangling penders.
In `@src/NimBLEUtils.cpp`:
- Around line 62-95: The m_taskSemEntries vector is not synchronized and can be
mutated concurrently by getTaskSem() and deleteTaskSems(); protect all accesses
by using a mutex (e.g., a ble_npl_mutex) to avoid races: acquire the mutex at
the start of NimBLEUtils::getTaskSem() and hold it through the lookup and any
creation/push_back (release after insertion), and acquire the same mutex in
NimBLEUtils::deleteTaskSems() while deleting entries and clearing the vector;
ensure you initialize/destroy the mutex in the NimBLEUtils module setup/teardown
so the lock is available when getTaskSem() runs.
- Around line 120-128: In taskWait (use NimBLEUtils::getTaskSem and
ble_npl_sem_pend), drain any leftover semaphore count before the timed wait by
repeatedly performing a non-blocking ble_npl_sem_pend (timeout 0) loop until it
returns not-OK, ensuring the semaphore starts in a non-signaled state for this
wait; then log and perform the actual timed ble_npl_sem_pend(sem, ticks) as now
implemented.
- Around line 90-95: deleteTaskSems currently only deletes the C++ wrapper
(ble_npl_sem) and leaks the underlying FreeRTOS SemaphoreHandle_t; before
deleting each entry in m_taskSemEntries, call vSemaphoreDelete(...) on the
wrapper's underlying handle (e.g., entry.sem->sem or the actual handle field
inside ble_npl_sem) if non-null, then delete the wrapper and finally clear
m_taskSemEntries; reference the ble_npl_sem wrapper, the ble_npl_sem_init
creator, and deleteTaskSems to locate the code and use vSemaphoreDelete to free
the kernel semaphore prior to deleting the wrapper.
---
Nitpick comments:
In `@src/NimBLEUtils.cpp`:
- Around line 138-147: Log wording is inconsistent between the task wait and
release paths; update the error message in NimBLEUtils::taskRelease that
currently calls NIMBLE_LOGE(LOG_TAG, "Semaphore lookup failed in taskRelease")
to use the same "task handle" phrasing as the message in NimBLEUtils::taskWait
so grepping/troubleshooting is consistent (locate NimBLEUtils::getTaskSem,
taskRelease, taskWait and make the taskRelease NIMBLE_LOGE message mirror the
wording used by taskWait).
In `@src/NimBLEUtils.h`:
- Around line 78-89: The static vector m_taskSemEntries is accessed concurrently
by getTaskSem (and deleteTaskSems) and needs synchronization: add a static mutex
(e.g., std::mutex m_taskSemEntriesMutex) alongside the
TaskSemEntry/m_taskSemEntries declarations, include <mutex>, and guard all
accesses/iterations/modifications of m_taskSemEntries inside getTaskSem and
deleteTaskSems (and any other functions touching it) with
std::lock_guard<std::mutex> to prevent concurrent push_back/iteration races.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c6f045a-c4dd-4bb5-bdf0-88e128a4f16f
📒 Files selected for processing (4)
Kconfigsrc/NimBLEDevice.cppsrc/NimBLEUtils.cppsrc/NimBLEUtils.h
💤 Files with no reviewable changes (1)
- Kconfig
h2zero
force-pushed
the
taskwait-semaphores
branch
from
7000dc1 to
fbd39bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/NimBLEL2CAPChannel.h`:
- Line 30: Remove the invalid forward declaration "struct
NimBLEUtils::TaskData;" and instead include the full declaration by adding
`#include` "NimBLEUtils.h" to NimBLEL2CAPChannel.h so NimBLEUtils::TaskData is
visible; update the NimBLEL2CAPChannel class members and any construction/usage
(the member that uses NimBLEUtils::TaskData* and the places that create TaskData
instances) to rely on the real definition from NimBLEUtils.h rather than a
nested forward-declare.
In `@src/NimBLEUtils.cpp`:
- Around line 123-131: NimBLEUtils::taskRelease currently dereferences
taskData.m_pSem without nullptr checks and early-returns before clearing inUse
on ble_npl_sem_release failure; fix by first checking if taskData.m_pSem is
nullptr and log/return if so, then call ble_npl_sem_release, and regardless of
the return code always set taskData.m_pSem->inUse = false (after the nullptr
check) so entries aren't left stuck; reference NimBLEUtils::taskRelease,
taskWait (for the init path that may leave m_pSem null), taskData.m_pSem, and
ble_npl_sem_release when locating the change.
- Around line 137-145: NimBLEUtils::deleteTaskSems is leaking the C++ heap
object allocated for each entry->sem; after calling
ble_npl_sem_deinit(entry->sem) you must also delete the semaphore pointer before
deleting the wrapper. Update NimBLEUtils::deleteTaskSems to, for each entry in
m_taskSemEntries, release and deinit the semaphore
(ble_npl_sem_release/ble_npl_sem_deinit) and then delete entry->sem (or check
for non-null and delete) prior to delete entry; finally clear m_taskSemEntries.
In `@src/NimBLEUtils.h`:
- Around line 59-63: The comment says TaskData's documentation mentions a
non-existent member m_pTaskID; update the docblock for struct TaskData to
accurately reflect current fields and behavior: remove the reference to
m_pTaskID and state that taskWait() sets the private m_pSem semaphore (or other
actual members) and that TaskData is used with NimBLEUtils::taskWait() and
NimBLEUtils::taskRelease() to block/unblock tasks; ensure you reference the
struct name TaskData and the methods NimBLEUtils::taskWait() and
NimBLEUtils::taskRelease() so future readers can locate the logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d2b2048-841d-48be-9d33-42b2cb064a7c
📒 Files selected for processing (13)
Kconfigsrc/NimBLEClient.cppsrc/NimBLEClient.hsrc/NimBLEDevice.cppsrc/NimBLEL2CAPChannel.cppsrc/NimBLEL2CAPChannel.hsrc/NimBLERemoteCharacteristic.cppsrc/NimBLERemoteService.cppsrc/NimBLERemoteValueAttribute.cppsrc/NimBLEScan.cppsrc/NimBLEScan.hsrc/NimBLEUtils.cppsrc/NimBLEUtils.h
💤 Files with no reviewable changes (1)
- Kconfig
✅ Files skipped from review due to trivial changes (2)
- src/NimBLEDevice.cpp
- src/NimBLEL2CAPChannel.cpp
This will prevent a case where applications may notify a task that is blocking on a BLE operation before it completes. This can occur when writing to a characteristic in the application and blocking with task notifications until a notification occurs that releases it and the notification arrives before the write response.
h2zero
force-pushed
the
taskwait-semaphores
branch
from
fbd39bb to
16ab3f6
Compare
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ Actions performedReview triggered.
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This will prevent a case where applications may notify a task that is blocking on a BLE operation before it completes.
This can occur when writing to a characteristic in the application and blocking with task notifications until a notification occurs that releases it and the notification arrives before the write response.
Summary by CodeRabbit
Chores
Bug Fixes
Refactor