Return error if calling function will block host task#1136
Open
Return error if calling function will block host task#1136
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughArr — This change records the NimBLE host task handle at startup and adds Changes
Sequence Diagram(s)sequenceDiagram
participant HostTask as "Host Task\n(caller)"
participant NimBLEDevice as "NimBLEDevice\n(startup)"
participant NimBLEUtils as "NimBLEUtils\n(utils)"
participant Component as "Client/Service/Scan\n(operation)"
HostTask->>NimBLEDevice: start host task
NimBLEDevice->>NimBLEUtils: set m_hostTaskHandle (ble_npl_get_current_task_id)
Note over NimBLEUtils,HostTask: later, caller runs in host task
HostTask->>Component: invoke operation (connect/read/scan/etc.)
Component->>NimBLEUtils: inHostTask()? (compare current task id)
NimBLEUtils-->>Component: true (matches host handle)
Component-->>HostTask: log error & early return (error/null/empty)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 6
🤖 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/NimBLEClient.cpp`:
- Around line 683-685: The current NimBLEUtils::inHostTask() guard in
getService() runs before checking cached services (m_svcVec), causing host-task
callbacks to get nullptr even when service is cached; move the cached-lookup
(m_svcVec) before the inHostTask() check so cached services are returned without
blocking, and change the guard to only prevent synchronous discovery (i.e.,
perform the inHostTask() check only when discovery is requested). Also add the
same host-task guard to the remaining synchronous discovery entry points
(getServices(true) and discoverAttributes()) so they block discovery-only calls
from the host task but allow cache reads. Ensure you reference getService(),
m_svcVec, getServices(true), discoverAttributes(), and NimBLEUtils::inHostTask()
when making the changes.
In `@src/NimBLEDevice.cpp`:
- Around line 882-884: The stored host-task handle NimBLEUtils::m_hostTaskHandle
must be cleared when the host task exits to avoid stale handles; update the exit
sequence so that immediately after nimble_port_run() returns (i.e., when the
host task is stopping) you reset NimBLEUtils::m_hostTaskHandle to the "no task"
value used by inHostTask() (e.g., nullptr/0 or the equivalent task-handle
sentinel) before calling nimble_port_freertos_deinit(), ensuring inHostTask() no
longer matches a recycled task handle.
In `@src/NimBLERemoteCharacteristic.cpp`:
- Around line 158-160: The current NimBLEUtils::inHostTask() check in
getDescriptor short-circuits and returns nullptr before consulting
m_vDescriptors, preventing host-task callbacks from accessing already-cached
descriptors; change the logic so getDescriptor first checks m_vDescriptors and
returns the cached descriptor if present, and only perform the
NimBLEUtils::inHostTask() guard immediately before any blocking discovery (e.g.,
calls to getDescriptors(true) or discoverDescriptors), refusing/blocking that
path on the host task while allowing reads from m_vDescriptors; ensure
getDescriptors(true) is not invoked from host task (or switch it to a
non-blocking variant) so the hang path is fully closed.
In `@src/NimBLERemoteService.cpp`:
- Around line 78-80: Move the NimBLEUtils::inHostTask() guard so cached lookups
aren’t blocked: in getCharacteristic() perform the cache walk first and return
the cached characteristic if found; only when the code needs to call the
blocking path (retrieveCharacteristics(...)) check NimBLEUtils::inHostTask() and
reject that specific retrieveCharacteristics(...) call with the error. In short,
keep the cache lookup before the inHostTask() check and apply the host-task
rejection only to the retrieveCharacteristics(...) code path.
In `@src/NimBLEUtils.h`:
- Around line 79-81: The static member declaration void*
NimBLEUtils::m_hostTaskHandle in the header is missing an out-of-class
definition; add a single definition in the implementation file (e.g.,
src/NimBLEUtils.cpp) to resolve the linker error when getHostTaskHandle() or
host_task() reference m_hostTaskHandle by defining NimBLEUtils::m_hostTaskHandle
and initializing it (e.g., to nullptr).
- Around line 32-33: The GNU attribute syntax on the nimble_cpp_assert
declaration is malformed; replace the single-parenthesis form
`__attribute((weak, noreturn))` with the correct double-parenthesis form so the
declaration for nimble_cpp_assert(const char* file, unsigned line) uses
`__attribute__((weak, noreturn))`; update that attribute usage to fix
compilation when NIMBLE_CPP_DEBUG_ASSERT_ENABLED is set.
🪄 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: ad4022c7-47f4-4108-aa3b-c38db9a32397
📒 Files selected for processing (8)
src/NimBLEClient.cppsrc/NimBLEDevice.cppsrc/NimBLERemoteCharacteristic.cppsrc/NimBLERemoteService.cppsrc/NimBLERemoteValueAttribute.cppsrc/NimBLEScan.cppsrc/NimBLEUtils.cppsrc/NimBLEUtils.h
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/NimBLEUtils.h (1)
32-33:⚠️ Potential issue | 🔴 CriticalArrr, Line 33 still needs the proper GNU attribute spelling.
__attribute((weak, noreturn))is malformed here; when debug asserts are enabled, this declaration will fail to compile.🛠️ Suggested fix
-void nimble_cpp_assert(const char* file, unsigned line) __attribute((weak, noreturn)); +void nimble_cpp_assert(const char* file, unsigned line) __attribute__((weak, noreturn));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEUtils.h` around lines 32 - 33, The declaration of nimble_cpp_assert under the MYNEWT_VAL(NIMBLE_CPP_DEBUG_ASSERT_ENABLED) && !defined NDEBUG guard uses a malformed GNU attribute syntax; update the attribute on the nimble_cpp_assert declaration to use the correct double-parentheses GNU form (e.g., __attribute__((weak, noreturn))) so the declaration compiles when debug asserts are enabled, keeping the function name nimble_cpp_assert and the surrounding conditional macros unchanged.
🤖 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/NimBLEUtils.h`:
- Around line 35-38: The NIMBLE_CPP_DEBUG_ASSERT macro expands to a bare if
which can cause dangling-else bugs; make it statement-safe by wrapping the body
in a do { ... } while (0) block (preserving the existing call to
nimble_cpp_assert with NIMBLE_ATT_VAL_FILE and __LINE__) so callers can use
NIMBLE_CPP_DEBUG_ASSERT(cond); inside if/else chains without altering control
flow.
---
Duplicate comments:
In `@src/NimBLEUtils.h`:
- Around line 32-33: The declaration of nimble_cpp_assert under the
MYNEWT_VAL(NIMBLE_CPP_DEBUG_ASSERT_ENABLED) && !defined NDEBUG guard uses a
malformed GNU attribute syntax; update the attribute on the nimble_cpp_assert
declaration to use the correct double-parentheses GNU form (e.g.,
__attribute__((weak, noreturn))) so the declaration compiles when debug asserts
are enabled, keeping the function name nimble_cpp_assert and the surrounding
conditional macros unchanged.
🪄 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: b581a2e0-0c8a-4af9-bd81-261f2496f00a
📒 Files selected for processing (8)
src/NimBLEClient.cppsrc/NimBLEDevice.cppsrc/NimBLERemoteCharacteristic.cppsrc/NimBLERemoteService.cppsrc/NimBLERemoteValueAttribute.cppsrc/NimBLEScan.cppsrc/NimBLEUtils.cppsrc/NimBLEUtils.h
✅ Files skipped from review due to trivial changes (1)
- src/NimBLERemoteService.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/NimBLERemoteCharacteristic.cpp
- src/NimBLERemoteValueAttribute.cpp
- src/NimBLEUtils.cpp
- src/NimBLEScan.cpp
This will prevent applications from hanging forever when calling a blocking function from the host task.
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 applications from hanging forever when calling a blocking function from the host task.
Summary by CodeRabbit