Open
Conversation
h2zero
force-pushed
the
stream-class
branch
3 times, most recently
from
1b7cb22 to
6a67147
Compare
h2zero
force-pushed
the
stream-class
branch
2 times, most recently
from
bcfcf75 to
9d832b4
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a multi-role NimBLE streaming subsystem (NimBLEStream: server and client) with ring-buffered TX/RX, MTU-aware chunking and drain logic; three Arduino examples (Server, Client, Echo) with READMEs; minor console and nimconfig preprocessor formatting tweaks; and an added .gitignore entry. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Stream as NimBLEStreamServer
participant BLE as BLE Stack
participant Peer as Remote BLE Peer
App->>Stream: begin(svcUUID, chrUUID)
Stream->>BLE: register service/characteristic & callbacks
Peer->>BLE: connect & subscribe
BLE->>Stream: onSubscribe
App->>Stream: write(data)
Stream->>Stream: enqueue TX, schedule drain
Stream->>BLE: send notification (MTU-limited)
BLE->>Peer: deliver notification
Peer->>BLE: write characteristic (RX)
BLE->>Stream: onWrite -> push RX
App->>Stream: read() -> return RX data
Peer->>BLE: disconnect
BLE->>Stream: onDisconnect -> clear state
sequenceDiagram
participant App as Application
participant Stream as NimBLEStreamClient
participant BLE as BLE Stack
participant Server as Remote BLE Server
App->>BLE: scan for server
BLE->>App: report discovered server
App->>Stream: begin(remoteChar, subscribeNotify=true)
Stream->>BLE: subscribe to notifications
App->>Stream: write(data)
Stream->>Stream: enqueue TX, schedule drain
Stream->>BLE: write characteristic (MTU-limited)
BLE->>Server: deliver write
Server->>BLE: notify
BLE->>Stream: notifyCallback -> push RX
App->>Stream: read() -> return RX data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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/NimBLEStream.h (1)
110-111: Use{}for proper zero-initialization of aggregate types.Initializing
ble_npl_eventandble_npl_calloutwith{NULL}only initializes the first member and leaves remaining members uninitialized. Both are aggregate structs with multiple members that should be zero-initialized. Use{}instead for standard C++ aggregate initialization that ensures all members are properly zeroed.♻️ Suggested change
- ble_npl_event m_txDrainEvent{NULL}; - ble_npl_callout m_txDrainCallout{NULL}; + ble_npl_event m_txDrainEvent{}; + ble_npl_callout m_txDrainCallout{};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEStream.h` around lines 110 - 111, The member initializers m_txDrainEvent and m_txDrainCallout should use aggregate zero-initialization instead of {NULL}; update the declarations of ble_npl_event m_txDrainEvent and ble_npl_callout m_txDrainCallout in NimBLEStream.h to initialize with {} so all struct members are zero-initialized (replace the current {NULL} initializers with {} for the symbols m_txDrainEvent and m_txDrainCallout).examples/NimBLE_Stream_Client/NimBLE_Stream_Client.ino (1)
75-77: Add a defensive null check forpServerDeviceinconnectToServer().
connectToServer()dereferencespServerDeviceimmediately (Line 76). A guard prevents accidental null dereference if call flow changes later.🔧 Proposed fix
bool connectToServer() { + if (pServerDevice == nullptr) { + Serial.println("No server selected"); + return false; + } Serial.printf("Connecting to: %s\n", pServerDevice->getAddress().toString().c_str());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Client/NimBLE_Stream_Client.ino` around lines 75 - 77, The connectToServer() function dereferences pServerDevice immediately; add a defensive null check at the start of connectToServer() to verify pServerDevice is not null (e.g., if (pServerDevice == nullptr) { Serial.println("No server device available"); return false; }) so you avoid null dereference, log a clear message and return false early; update any callers if they rely on a different return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/NimBLE_Stream_Client/NimBLE_Stream_Client.ino`:
- Around line 155-162: The doConnect handling block (where doConnect is set
false and connectToServer() is called) can leave scanning stopped if onScanEnd
already ran; update the failure branch inside that block (in the same function
handling doConnect) to explicitly restart BLE scanning when connectToServer()
returns false by invoking the scan restart API used elsewhere in the code (e.g.,
the NimBLE scan start call or Scan->start()), and keep the existing logging;
reference the doConnect check and the connectToServer() call to locate where to
add the explicit scan restart.
In `@examples/NimBLE_Stream_Echo/NimBLE_Stream_Echo.ino`:
- Around line 25-26: The server is created in setup() via
NimBLEDevice::createServer() but advertising is not restarted after a client
disconnects, preventing new connections; add a BLE server callback by
subclassing NimBLEServerCallbacks (implement onDisconnect) and in onDisconnect
call the server advertising restart (e.g., server->getAdvertising()->start() or
NimBLEDevice::startAdvertising()) so advertising is resumed after disconnect;
register this callback with the server returned by NimBLEDevice::createServer()
in setup().
In `@src/NimBLEStream.cpp`:
- Around line 50-71: The destructor calls ble_npl_mutex_deinit unconditionally
even if ble_npl_mutex_init failed; add a member flag (e.g., bool
m_mutexInitialized) to ByteRingBuffer, set it true only after ble_npl_mutex_init
returns BLE_NPL_OK in the constructor, and in the ~ByteRingBuffer() only call
ble_npl_mutex_deinit(&m_mutex) when m_mutexInitialized is true (leave existing
m_buf free logic as-is).
- Around line 483-494: The pushRx implementation currently drops the whole
packet when m_rxBuf->freeSize() < len; change it to accept a partial write
instead: compute accepted = std::min(len, m_rxBuf->freeSize()), call
m_rxBuf->write(data, accepted) and return the number of bytes written;
optionally update the log to indicate a partial drop when accepted < len. Update
NimBLEStream::pushRx to use m_rxBuf->freeSize() and ByteRingBuffer::write()
accordingly.
---
Nitpick comments:
In `@examples/NimBLE_Stream_Client/NimBLE_Stream_Client.ino`:
- Around line 75-77: The connectToServer() function dereferences pServerDevice
immediately; add a defensive null check at the start of connectToServer() to
verify pServerDevice is not null (e.g., if (pServerDevice == nullptr) {
Serial.println("No server device available"); return false; }) so you avoid null
dereference, log a clear message and return false early; update any callers if
they rely on a different return behavior.
In `@src/NimBLEStream.h`:
- Around line 110-111: The member initializers m_txDrainEvent and
m_txDrainCallout should use aggregate zero-initialization instead of {NULL};
update the declarations of ble_npl_event m_txDrainEvent and ble_npl_callout
m_txDrainCallout in NimBLEStream.h to initialize with {} so all struct members
are zero-initialized (replace the current {NULL} initializers with {} for the
symbols m_txDrainEvent and m_txDrainCallout).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57228ec8-04c6-4beb-abb7-4aa6254cd25d
📒 Files selected for processing (12)
.gitignoreexamples/NimBLE_Stream_Client/NimBLE_Stream_Client.inoexamples/NimBLE_Stream_Client/README.mdexamples/NimBLE_Stream_Echo/NimBLE_Stream_Echo.inoexamples/NimBLE_Stream_Echo/README.mdexamples/NimBLE_Stream_Server/NimBLE_Stream_Server.inoexamples/NimBLE_Stream_Server/README.mdsrc/NimBLEDevice.hsrc/NimBLEStream.cppsrc/NimBLEStream.hsrc/nimble/console/console.hsrc/nimconfig.h
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
examples/NimBLE_Stream_Client/NimBLE_Stream_Client.ino (1)
38-43: Consider copying device address instead of storing pointer.The
pServerDevicepointer references memory managed by the NimBLE scan results. While this pattern is common in NimBLE examples and typically works, the pointer may become invalid if scan results are cleared beforeconnectToServer()is called. For more robust code, consider storing the address directly:static NimBLEAddress serverAddress; // In onResult: serverAddress = *advertisedDevice->getAddress();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Client/NimBLE_Stream_Client.ino` around lines 38 - 43, The code stores a pointer to the scan result in pServerDevice which may become invalid later; instead, copy and store the device address (use NimBLEAddress) in onResult and use that address in connectToServer(), replacing uses of pServerDevice with the saved serverAddress; update any references in functions like connectToServer() and the onResult handler to construct/lookup a NimBLEDevice from the saved NimBLEAddress rather than dereferencing the original advertisedDevice pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/NimBLE_Stream_Client/NimBLE_Stream_Client.ino`:
- Around line 75-77: connectToServer() dereferences pServerDevice without
checking for null; add a null-check at the start of connectToServer() to handle
the case where pServerDevice is nullptr (log an error via Serial or return
false) before calling pServerDevice->getAddress(), ensuring the function returns
cleanly (e.g., false) when pServerDevice is not set.
---
Nitpick comments:
In `@examples/NimBLE_Stream_Client/NimBLE_Stream_Client.ino`:
- Around line 38-43: The code stores a pointer to the scan result in
pServerDevice which may become invalid later; instead, copy and store the device
address (use NimBLEAddress) in onResult and use that address in
connectToServer(), replacing uses of pServerDevice with the saved serverAddress;
update any references in functions like connectToServer() and the onResult
handler to construct/lookup a NimBLEDevice from the saved NimBLEAddress rather
than dereferencing the original advertisedDevice pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7e95589-ed6d-4143-ae99-cee4bb3fb91b
📒 Files selected for processing (3)
examples/NimBLE_Stream_Client/NimBLE_Stream_Client.inosrc/NimBLEStream.cppsrc/NimBLEStream.h
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/NimBLEStream.cpp (1)
682-693: Note: Debug log recursion prevention filter.The string filtering to prevent log recursion is a pragmatic solution. Consider adding a brief comment explaining why this specific string triggers recursion (GAP event logging → characteristic write → more GAP events) for future maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEStream.cpp` around lines 682 - 693, Add a brief explanatory comment above the recursion-prevention filter: explain that the literal "handleGapEvent" (variable filterStr) corresponds to GAP event logging paths which can cause a feedback loop where logging the GAP event triggers a characteristic write and thus generates further GAP events, so the code checks data/len for that substring and returns early to drop the log entry and avoid recursion; reference the variables filterStr, filterLen, data, and len so maintainers can find the filter logic quickly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/NimBLEStream.cpp`:
- Around line 682-693: Add a brief explanatory comment above the
recursion-prevention filter: explain that the literal "handleGapEvent" (variable
filterStr) corresponds to GAP event logging paths which can cause a feedback
loop where logging the GAP event triggers a characteristic write and thus
generates further GAP events, so the code checks data/len for that substring and
returns early to drop the log entry and avoid recursion; reference the variables
filterStr, filterLen, data, and len so maintainers can find the filter logic
quickly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44e19378-3c8c-438e-a9c5-2fcfec21372c
📒 Files selected for processing (2)
src/NimBLEStream.cppsrc/NimBLEStream.h
Co-authored-by: doudar <17362216+doudar@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/NimBLEStream.h (1)
93-98: Tighten the overflow callback contract.The callback fires when a new chunk no longer fits in the available RX space, not just when
lenexceeds the total buffer capacity. Calling out the two outcomes—drop current data vs drop oldest buffered data—would make the header matchpushRx()and set the right expectations for integrators.Based on learnings, the preferred direction for RX overflow in NimBLEStream is a user-settable callback so the application can react when data is dropped.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEStream.h` around lines 93 - 98, Update the setRxOverflowCallback comment to exactly match pushRx() semantics: state that the rx_overflow_callback_t is invoked when an incoming chunk cannot fit in the available RX space (not merely when len > total buffer capacity), and document the two outcomes the callback controls—return true to drop the oldest buffered data to make room for the incoming chunk, or return false to drop the incoming chunk; reference setRxOverflowCallback, rx_overflow_callback_t, pushRx(), and m_rxOverflowCallback so integrators know where this behavior is enforced and that they can react to dropped data.
🤖 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/NimBLEStream.cpp`:
- Around line 488-514: The RX overflow handling in NimBLEStream.cpp uses
separate ByteRingBuffer calls (freeSize(), drop(), write()) allowing a
concurrent read() to make requiredSpace stale; fix by adding a single atomic
ByteRingBuffer method (e.g., dropAndWrite or ensureSpaceThenWrite) that acquires
the ring-buffer mutex and performs the drop + write decision and operations
under one lock, then replace the overflow branch in the function handling
m_rxBuf with a single call to that new atomic method; reference m_rxBuf,
freeSize(), drop(), write(), and read() when locating changes.
- Around line 758-765: In NimBLEStreamServer::ChrCallbacks::onWrite, detect
write-only mode (begin called with txBufSize == 0 and rxBufSize > 0) and
populate the peer handle so ready()/operator bool() reflect an active peer: if
m_parent's TX is disabled and m_parent->m_peerHandle is not set, call
m_parent->setPeerHandle(connInfo.getConnHandle()) (or assign
m_parent->m_peerHandle = connInfo.getConnHandle() using the class' existing
accessor) when handling onWrite; otherwise leave behavior unchanged. This
ensures write-only streams become "ready" even without onSubscribe(); if you
don't want to support write-only streams, instead update the API
contract/comments on ready()/operator bool() to state they only track
subscribers from onSubscribe().
- Around line 776-795: When NimBLEStreamServer::ChrCallbacks::onSubscribe
accepts a new subscriber (m_peerHandle updated) it must drop any queued unsent
bytes left from the previous peer to avoid delivering them to the new
connection; update onSubscribe to clear m_txBuf (or call the method that empties
it) whenever you retire the old peer (i.e., when ble_gap_conn_find fails and you
set m_peerHandle = BLE_HS_CONN_HANDLE_NONE, and also when assigning m_peerHandle
= connInfo.getConnHandle() for a new subscriber), and ensure send() behavior
remains unchanged except that queued data is flushed/cleared on peer rollover.
---
Nitpick comments:
In `@src/NimBLEStream.h`:
- Around line 93-98: Update the setRxOverflowCallback comment to exactly match
pushRx() semantics: state that the rx_overflow_callback_t is invoked when an
incoming chunk cannot fit in the available RX space (not merely when len > total
buffer capacity), and document the two outcomes the callback controls—return
true to drop the oldest buffered data to make room for the incoming chunk, or
return false to drop the incoming chunk; reference setRxOverflowCallback,
rx_overflow_callback_t, pushRx(), and m_rxOverflowCallback so integrators know
where this behavior is enforced and that they can react to dropped data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ca87acc-8895-41c5-836f-899a61bc43aa
📒 Files selected for processing (12)
.gitignoreexamples/NimBLE_Stream_Client/NimBLE_Stream_Client.inoexamples/NimBLE_Stream_Client/README.mdexamples/NimBLE_Stream_Echo/NimBLE_Stream_Echo.inoexamples/NimBLE_Stream_Echo/README.mdexamples/NimBLE_Stream_Server/NimBLE_Stream_Server.inoexamples/NimBLE_Stream_Server/README.mdsrc/NimBLEDevice.hsrc/NimBLEStream.cppsrc/NimBLEStream.hsrc/nimble/console/console.hsrc/nimconfig.h
✅ Files skipped from review due to trivial changes (2)
- examples/NimBLE_Stream_Server/README.md
- src/nimconfig.h
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/NimBLE_Stream_Echo/README.md
- .gitignore
- examples/NimBLE_Stream_Echo/NimBLE_Stream_Echo.ino
- src/nimble/console/console.h
Comment on lines
+488
to
+514
| size_t freeSize = m_rxBuf->freeSize(); | ||
| if (len > freeSize) { | ||
| const bool dropOlderData = m_rxOverflowCallback && m_rxOverflowCallback(data, len); | ||
| if (!dropOlderData) { | ||
| NIMBLE_LOGE(LOG_TAG, "RX buffer overflow, dropping current data"); | ||
| return 0; | ||
| } | ||
|
|
||
| if (len >= m_rxBuf->capacity()) { | ||
| m_rxBuf->drop(m_rxBuf->size()); | ||
| const uint8_t* tail = data + (len - m_rxBuf->capacity()); | ||
| size_t written = m_rxBuf->write(tail, m_rxBuf->capacity()); | ||
| if (written != m_rxBuf->capacity()) { | ||
| NIMBLE_LOGE(LOG_TAG, "RX buffer overflow, %zu bytes dropped", m_rxBuf->capacity() - written); | ||
| } | ||
| return written; | ||
| } | ||
|
|
||
| const size_t requiredSpace = len - freeSize; | ||
| size_t dropped = m_rxBuf->drop(requiredSpace); | ||
| if (dropped < requiredSpace) { | ||
| NIMBLE_LOGE(LOG_TAG, "RX buffer overflow, failed to drop enough buffered data"); | ||
| return 0; | ||
| } | ||
| } | ||
|
|
||
| return m_rxBuf->write(data, len); |
There was a problem hiding this comment.
Make the RX overflow path atomic.
freeSize(), drop(), and write() each take the ring-buffer mutex separately. A concurrent read() between those calls can make requiredSpace stale, so this branch can drop more buffered data than necessary or reject the incoming chunk even though enough space exists by the time the write happens. Move the overflow decision and the drop/write sequence into one ByteRingBuffer operation under a single lock.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEStream.cpp` around lines 488 - 514, The RX overflow handling in
NimBLEStream.cpp uses separate ByteRingBuffer calls (freeSize(), drop(),
write()) allowing a concurrent read() to make requiredSpace stale; fix by adding
a single atomic ByteRingBuffer method (e.g., dropAndWrite or
ensureSpaceThenWrite) that acquires the ring-buffer mutex and performs the drop
+ write decision and operations under one lock, then replace the overflow branch
in the function handling m_rxBuf with a single call to that new atomic method;
reference m_rxBuf, freeSize(), drop(), write(), and read() when locating
changes.
Owner
Author
There was a problem hiding this comment.
it's a recursive mutex, recheck
There was a problem hiding this comment.
Let me check the NPL mutex implementation to verify this.
@h2zero, you're correct that ble_npl_mutex uses xSemaphoreCreateRecursiveMutex() and xSemaphoreTakeRecursive on FreeRTOS — a recursive mutex. However, a recursive mutex only prevents the same task from deadlocking when it re-acquires the lock. It does not prevent two different tasks from interleaving between separate lock/unlock cycles.
The scenario I flagged is a cross-task race:
pushRxruns on the NimBLE host task (dispatched via the event queue fromonWrite/notifyCallback).read()/available()are called from the user's Arduino loop task.
Between the separate mutex acquisitions for freeSize(), drop(), and write() in pushRx, the user task can acquire the mutex and call read(), changing the buffer's occupancy. The recursive nature of the mutex doesn't help here — it only allows the same task to re-enter, not serialize across tasks.
So the sequence:
[NimBLE task] freeSize() → unlock → ...
[User task] read() → unlock ← can sneak in here
[NimBLE task] drop(requiredSpace) → unlock → write() → unlock
…still produces a stale requiredSpace calculation. An atomic dropAndWrite method that holds the lock across the full drop+write sequence is still the correct fix. The concern stands.
✏️ Learnings added
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:488-514
Timestamp: 2026-03-15T00:51:54.533Z
Learning: In NimBLEStream.cpp (NimBLE-Arduino), the ble_npl_mutex used in ByteRingBuffer is a recursive mutex (xSemaphoreCreateRecursiveMutex on FreeRTOS). This prevents same-task deadlocks on re-entry but does NOT prevent cross-task interleaving between separate lock/unlock cycles in pushRx (NimBLE task) and read() (user/Arduino task).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:483-494
Timestamp: 2026-03-14T23:33:16.711Z
Learning: In NimBLEStream (src/NimBLEStream.cpp, NimBLE-Arduino), the author's preference regarding RX buffer overflow in pushRx is that the bigger concern is not partial vs. full drops, but that the application layer has no notification mechanism when data is dropped. A user-settable overflow callback is the preferred direction over silently accepting partial writes.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:50-71
Timestamp: 2026-03-14T23:36:28.104Z
Learning: Guideline: In NimBLEStream.cpp, it's acceptable for ble_npl_mutex_deinit to be called on a zero-initialized mutex (e.g., when ble_npl_mutex_init failed). Ensure review confirms that the ByteRingBuffer constructor zeroes the mutex memory before init (memset to 0) so both fresh construction and reinit cases are safe. If applicable, consider adding a defensive memset to 0 after deinitialization in the destructor to guard against memory reuse, but only if this aligns with the project’s memory handling policy. Verify that the NPL layer handles null/zero checks and that the codepaths consistently treat an uninitialized mutex as safe to deinit.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:776-795
Timestamp: 2026-03-15T00:44:23.085Z
Learning: In NimBLEStream.cpp (NimBLE-Arduino, src/NimBLEStream.cpp), when flushing the TX buffer on peer rollover in NimBLEStreamServer::ChrCallbacks::onSubscribe, the flush should only occur when the new subscriber's connection handle differs from the previous one. If the same peer re-subscribes after a momentary connection loss, the TX buffer should be preserved so transmission can resume seamlessly. Explicit unsubscribes (subValue == 0) should still flush the buffer. Save prevHandle before clearing m_peerHandle so the comparison is possible.
Comment on lines
+758
to
+765
| void NimBLEStreamServer::ChrCallbacks::onWrite(NimBLECharacteristic* pChr, NimBLEConnInfo& connInfo) { | ||
| // Push received data into RX buffer | ||
| auto val = pChr->getValue(); | ||
| m_parent->pushRx(val.data(), val.size()); | ||
|
|
||
| if (m_userCallbacks) { | ||
| m_userCallbacks->onWrite(pChr, connInfo); | ||
| } |
There was a problem hiding this comment.
Track an RX peer for write-only streams.
begin(..., txBufSize = 0, rxBufSize > 0) explicitly allows a write-only stream, but m_peerHandle is only populated from onSubscribe(). In that mode RX data can arrive here and be buffered while ready() / operator bool() stay false forever. If write-only mode is meant to be supported, capture connInfo.getConnHandle() here when TX is disabled; otherwise the API contract should say that ready() only reflects a notification subscriber.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEStream.cpp` around lines 758 - 765, In
NimBLEStreamServer::ChrCallbacks::onWrite, detect write-only mode (begin called
with txBufSize == 0 and rxBufSize > 0) and populate the peer handle so
ready()/operator bool() reflect an active peer: if m_parent's TX is disabled and
m_parent->m_peerHandle is not set, call
m_parent->setPeerHandle(connInfo.getConnHandle()) (or assign
m_parent->m_peerHandle = connInfo.getConnHandle() using the class' existing
accessor) when handling onWrite; otherwise leave behavior unchanged. This
ensures write-only streams become "ready" even without onSubscribe(); if you
don't want to support write-only streams, instead update the API
contract/comments on ready()/operator bool() to state they only track
subscribers from onSubscribe().
There was a problem hiding this comment.
@h2zero, you're right — my comment was off-base. Re-reading the code more carefully:
onWrite()pushes data intom_rxBufunconditionally, without anyready()check, so incoming client writes are buffered regardless of peer-handle state.write()(server → client, notifications) does checkready(), but in a TX-disabled / RX-only streamm_txBufis null, so it returns0even before reachingready().ready()is purely a "do I have an active notification subscriber?" sentinel — which is correct and intentional; it has no bearing on the RX path.
There's nothing to fix here. I'll withdraw the comment.
✏️ Learnings added
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:758-765
Timestamp: 2026-03-15T00:58:38.537Z
Learning: In NimBLEStreamServer (src/NimBLEStream.cpp, NimBLE-Arduino), ready() is intentionally subscriber/notification-centric: m_peerHandle is set only in onSubscribe(). In a write-only (RX-only) server stream (txBufSize==0, rxBufSize>0), ready() correctly returns false permanently — this is by design. Incoming client writes are pushed into m_rxBuf via onWrite() unconditionally and do not require ready() to be true. There is no need to track a peer handle from onWrite() for RX-only mode.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:483-494
Timestamp: 2026-03-14T23:33:16.711Z
Learning: In NimBLEStream (src/NimBLEStream.cpp, NimBLE-Arduino), the author's preference regarding RX buffer overflow in pushRx is that the bigger concern is not partial vs. full drops, but that the application layer has no notification mechanism when data is dropped. A user-settable overflow callback is the preferred direction over silently accepting partial writes.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:776-795
Timestamp: 2026-03-15T00:44:23.085Z
Learning: In NimBLEStream.cpp (NimBLE-Arduino, src/NimBLEStream.cpp), when flushing the TX buffer on peer rollover in NimBLEStreamServer::ChrCallbacks::onSubscribe, the flush should only occur when the new subscriber's connection handle differs from the previous one. If the same peer re-subscribes after a momentary connection loss, the TX buffer should be preserved so transmission can resume seamlessly. Explicit unsubscribes (subValue == 0) should still flush the buffer. Save prevHandle before clearing m_peerHandle so the comparison is possible.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:50-71
Timestamp: 2026-03-14T23:36:28.104Z
Learning: Guideline: In NimBLEStream.cpp, it's acceptable for ble_npl_mutex_deinit to be called on a zero-initialized mutex (e.g., when ble_npl_mutex_init failed). Ensure review confirms that the ByteRingBuffer constructor zeroes the mutex memory before init (memset to 0) so both fresh construction and reinit cases are safe. If applicable, consider adding a defensive memset to 0 after deinitialization in the destructor to guard against memory reuse, but only if this aligns with the project’s memory handling policy. Verify that the NPL layer handles null/zero checks and that the codepaths consistently treat an uninitialized mutex as safe to deinit.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:488-514
Timestamp: 2026-03-15T00:51:54.533Z
Learning: In NimBLEStream.cpp (NimBLE-Arduino), the ble_npl_mutex used in ByteRingBuffer is a recursive mutex (xSemaphoreCreateRecursiveMutex on FreeRTOS). This prevents same-task deadlocks on re-entry but does NOT prevent cross-task interleaving between separate lock/unlock cycles in pushRx (NimBLE task) and read() (user/Arduino task).
Comment on lines
+776
to
+795
| void NimBLEStreamServer::ChrCallbacks::onSubscribe(NimBLECharacteristic* pChr, NimBLEConnInfo& connInfo, uint16_t subValue) { | ||
| // If we have a stored peer handle, ensure it still refers to an active connection. | ||
| // If the connection has gone away without an explicit unsubscribe, clear it so a new | ||
| // subscriber can be accepted. | ||
| if (m_peerHandle != BLE_HS_CONN_HANDLE_NONE) { | ||
| if (ble_gap_conn_find(m_peerHandle, nullptr) != 0) { | ||
| m_peerHandle = BLE_HS_CONN_HANDLE_NONE; | ||
| } | ||
| } | ||
|
|
||
| // only one subscriber supported | ||
| if (subValue && m_peerHandle != BLE_HS_CONN_HANDLE_NONE) { | ||
| NIMBLE_LOGI(LOG_TAG, | ||
| "Already have a subscriber, rejecting new subscription from conn handle %d", | ||
| connInfo.getConnHandle()); | ||
| return; | ||
| } | ||
|
|
||
| m_peerHandle = subValue ? connInfo.getConnHandle() : BLE_HS_CONN_HANDLE_NONE; | ||
| if (m_userCallbacks) { |
There was a problem hiding this comment.
Clear queued TX data when the tracked peer rolls over.
send() keeps unsent bytes in m_txBuf when a notification fails. If the previous peer disappears or unsubscribes and a new peer is accepted here, those bytes stay queued and can be delivered to the next subscriber. Dropping the pending TX queue when the old peer is retired avoids cross-session leakage.
Proposed fix
if (m_peerHandle != BLE_HS_CONN_HANDLE_NONE) {
if (ble_gap_conn_find(m_peerHandle, nullptr) != 0) {
+ if (m_parent->m_txBuf) {
+ m_parent->m_txBuf->drop(m_parent->m_txBuf->size());
+ }
m_peerHandle = BLE_HS_CONN_HANDLE_NONE;
}
}
@@
- m_peerHandle = subValue ? connInfo.getConnHandle() : BLE_HS_CONN_HANDLE_NONE;
+ if (!subValue && m_parent->m_txBuf) {
+ m_parent->m_txBuf->drop(m_parent->m_txBuf->size());
+ }
+ m_peerHandle = subValue ? connInfo.getConnHandle() : BLE_HS_CONN_HANDLE_NONE;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/NimBLEStream.cpp` around lines 776 - 795, When
NimBLEStreamServer::ChrCallbacks::onSubscribe accepts a new subscriber
(m_peerHandle updated) it must drop any queued unsent bytes left from the
previous peer to avoid delivering them to the new connection; update onSubscribe
to clear m_txBuf (or call the method that empties it) whenever you retire the
old peer (i.e., when ble_gap_conn_find fails and you set m_peerHandle =
BLE_HS_CONN_HANDLE_NONE, and also when assigning m_peerHandle =
connInfo.getConnHandle() for a new subscriber), and ensure send() behavior
remains unchanged except that queued data is flushed/cleared on peer rollover.
Owner
Author
There was a problem hiding this comment.
if the new subscriber is the same, then it shouldn't be dropped as we can continue normally if it was a momentary loss
There was a problem hiding this comment.
@h2zero, that's a fair point — if the same connection handle re-subscribes (momentary loss/reconnect), there's no reason to discard data that can still be delivered. The fix should only flush the TX buffer when it's genuinely a different peer taking over the subscription.
Here's a revised proposal that saves the previous handle before clearing it and only drops queued data on a handle mismatch (or on an explicit unsubscribe):
void NimBLEStreamServer::ChrCallbacks::onSubscribe(
NimBLECharacteristic* pChr, NimBLEConnInfo& connInfo, uint16_t subValue) {
uint16_t prevHandle = m_peerHandle; // save before any modification
// If we have a stored peer handle, ensure it still refers to an active connection.
if (m_peerHandle != BLE_HS_CONN_HANDLE_NONE) {
if (ble_gap_conn_find(m_peerHandle, nullptr) != 0) {
m_peerHandle = BLE_HS_CONN_HANDLE_NONE;
}
}
// Only one subscriber supported.
if (subValue && m_peerHandle != BLE_HS_CONN_HANDLE_NONE) {
NIMBLE_LOGI(LOG_TAG,
"Already have a subscriber, rejecting new subscription from conn handle %d",
connInfo.getConnHandle());
return;
}
if (subValue) {
uint16_t newHandle = connInfo.getConnHandle();
// Only flush stale TX data when it's a genuinely different peer.
if (newHandle != prevHandle && m_parent->m_txBuf) {
m_parent->m_txBuf->drop(m_parent->m_txBuf->size());
}
m_peerHandle = newHandle;
} else {
// Explicit unsubscribe — peer opted out, flush queued data.
if (m_parent->m_txBuf) {
m_parent->m_txBuf->drop(m_parent->m_txBuf->size());
}
m_peerHandle = BLE_HS_CONN_HANDLE_NONE;
}
if (m_userCallbacks) { ... }
}Key changes vs. the original proposal:
prevHandleis captured before any mutation.- TX flush only happens when
newHandle != prevHandle(different peer taking over), preserving buffered data for the same peer resuming after a transient drop. - Explicit unsubscribes still flush, since the peer has actively opted out of receiving.
✏️ Learnings added
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:776-795
Timestamp: 2026-03-15T00:44:23.085Z
Learning: In NimBLEStream.cpp (NimBLE-Arduino, src/NimBLEStream.cpp), when flushing the TX buffer on peer rollover in NimBLEStreamServer::ChrCallbacks::onSubscribe, the flush should only occur when the new subscriber's connection handle differs from the previous one. If the same peer re-subscribes after a momentary connection loss, the TX buffer should be preserved so transmission can resume seamlessly. Explicit unsubscribes (subValue == 0) should still flush the buffer. Save prevHandle before clearing m_peerHandle so the comparison is possible.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:483-494
Timestamp: 2026-03-14T23:33:16.711Z
Learning: In NimBLEStream (src/NimBLEStream.cpp, NimBLE-Arduino), the author's preference regarding RX buffer overflow in pushRx is that the bigger concern is not partial vs. full drops, but that the application layer has no notification mechanism when data is dropped. A user-settable overflow callback is the preferred direction over silently accepting partial writes.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:50-71
Timestamp: 2026-03-14T23:36:28.104Z
Learning: Guideline: In NimBLEStream.cpp, it's acceptable for ble_npl_mutex_deinit to be called on a zero-initialized mutex (e.g., when ble_npl_mutex_init failed). Ensure review confirms that the ByteRingBuffer constructor zeroes the mutex memory before init (memset to 0) so both fresh construction and reinit cases are safe. If applicable, consider adding a defensive memset to 0 after deinitialization in the destructor to guard against memory reuse, but only if this aligns with the project’s memory handling policy. Verify that the NPL layer handles null/zero checks and that the codepaths consistently treat an uninitialized mutex as safe to deinit.
Owner
Author
There was a problem hiding this comment.
the handle can change, the address is the only identifier that might be useful
There was a problem hiding this comment.
@h2zero, you're right — the connection handle is re-assigned by the controller on every new connection, so two successive subscriptions from the same physical device will carry different handles. Comparing handles to detect "same peer" is unreliable. Storing and comparing the peer address is the correct approach.
Revised proposal — add a NimBLEAddress m_peerAddress member to ChrCallbacks and compare addresses instead of handles:
// In the header, add to ChrCallbacks:
NimBLEAddress m_peerAddress{}; // address of the current subscriber
// In onSubscribe:
void NimBLEStreamServer::ChrCallbacks::onSubscribe(
NimBLECharacteristic* pChr, NimBLEConnInfo& connInfo, uint16_t subValue) {
NimBLEAddress prevAddress = m_peerAddress; // save before any modification
// If stored handle is no longer valid, clear it.
if (m_peerHandle != BLE_HS_CONN_HANDLE_NONE) {
if (ble_gap_conn_find(m_peerHandle, nullptr) != 0) {
m_peerHandle = BLE_HS_CONN_HANDLE_NONE;
}
}
// Only one subscriber supported.
if (subValue && m_peerHandle != BLE_HS_CONN_HANDLE_NONE) {
NIMBLE_LOGI(LOG_TAG,
"Already have a subscriber, rejecting new subscription from conn handle %d",
connInfo.getConnHandle());
return;
}
if (subValue) {
// Flush TX data only if this is a genuinely different peer (address differs).
if (connInfo.getAddress() != prevAddress && m_parent->m_txBuf) {
m_parent->m_txBuf->drop(m_parent->m_txBuf->size());
}
m_peerHandle = connInfo.getConnHandle();
m_peerAddress = connInfo.getAddress();
} else {
// Explicit unsubscribe — peer opted out, flush queued data.
if (m_parent->m_txBuf) {
m_parent->m_txBuf->drop(m_parent->m_txBuf->size());
}
m_peerHandle = BLE_HS_CONN_HANDLE_NONE;
m_peerAddress = NimBLEAddress{};
}
if (m_userCallbacks) {
m_userCallbacks->onSubscribe(pChr, connInfo, subValue);
}
}Key points:
m_peerAddressis stored alongsidem_peerHandleand cleared on unsubscribe.- A re-subscribing peer with the same address (momentary reconnect) won't lose buffered TX data.
- A different address taking over, or an explicit unsubscribe, still flushes the buffer.
✏️ Learnings added
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:776-795
Timestamp: 2026-03-15T01:06:46.397Z
Learning: In NimBLEStream.cpp (NimBLE-Arduino, src/NimBLEStream.cpp), when deciding whether to flush the TX buffer on peer rollover in NimBLEStreamServer::ChrCallbacks::onSubscribe, compare the peer BLE *address* (NimBLEAddress via connInfo.getAddress()), not the connection handle. Connection handles are re-assigned by the BLE controller on every new connection, even for the same physical device, so handle comparison is unreliable for detecting "same peer". Store a NimBLEAddress member (e.g. m_peerAddress) in ChrCallbacks alongside m_peerHandle, update it on each subscription, and clear it (set to NimBLEAddress{}) on explicit unsubscribe. Only flush TX data when the new subscriber's address differs from the stored address or on an explicit unsubscribe (subValue == 0).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:758-765
Timestamp: 2026-03-15T00:58:38.537Z
Learning: In NimBLEStreamServer (src/NimBLEStream.cpp, NimBLE-Arduino), ready() is intentionally subscriber/notification-centric: m_peerHandle is set only in onSubscribe(). In a write-only (RX-only) server stream (txBufSize==0, rxBufSize>0), ready() correctly returns false permanently — this is by design. Incoming client writes are pushed into m_rxBuf via onWrite() unconditionally and do not require ready() to be true. There is no need to track a peer handle from onWrite() for RX-only mode.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:483-494
Timestamp: 2026-03-14T23:33:16.711Z
Learning: In NimBLEStream (src/NimBLEStream.cpp, NimBLE-Arduino), the author's preference regarding RX buffer overflow in pushRx is that the bigger concern is not partial vs. full drops, but that the application layer has no notification mechanism when data is dropped. A user-settable overflow callback is the preferred direction over silently accepting partial writes.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:776-795
Timestamp: 2026-03-15T00:44:23.085Z
Learning: In NimBLEStream.cpp (NimBLE-Arduino, src/NimBLEStream.cpp), when flushing the TX buffer on peer rollover in NimBLEStreamServer::ChrCallbacks::onSubscribe, the flush should only occur when the new subscriber's connection handle differs from the previous one. If the same peer re-subscribes after a momentary connection loss, the TX buffer should be preserved so transmission can resume seamlessly. Explicit unsubscribes (subValue == 0) should still flush the buffer. Save prevHandle before clearing m_peerHandle so the comparison is possible.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:50-71
Timestamp: 2026-03-14T23:36:28.104Z
Learning: Guideline: In NimBLEStream.cpp, it's acceptable for ble_npl_mutex_deinit to be called on a zero-initialized mutex (e.g., when ble_npl_mutex_init failed). Ensure review confirms that the ByteRingBuffer constructor zeroes the mutex memory before init (memset to 0) so both fresh construction and reinit cases are safe. If applicable, consider adding a defensive memset to 0 after deinitialization in the destructor to guard against memory reuse, but only if this aligns with the project’s memory handling policy. Verify that the NPL layer handles null/zero checks and that the codepaths consistently treat an uninitialized mutex as safe to deinit.
Learnt from: h2zero
Repo: h2zero/NimBLE-Arduino PR: 1068
File: src/NimBLEStream.cpp:488-514
Timestamp: 2026-03-15T00:51:54.533Z
Learning: In NimBLEStream.cpp (NimBLE-Arduino), the ble_npl_mutex used in ByteRingBuffer is a recursive mutex (xSemaphoreCreateRecursiveMutex on FreeRTOS). This prevents same-task deadlocks on re-entry but does NOT prevent cross-task interleaving between separate lock/unlock cycles in pushRx (NimBLE task) and read() (user/Arduino task).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+683
to
+692
| // Skip server gap events to avoid log recursion | ||
| static const char filterStr[] = "handleGapEvent"; | ||
| constexpr size_t filterLen = sizeof(filterStr) - 1; | ||
| if (len >= filterLen + 3) { | ||
| for (size_t i = 3; i <= len - filterLen; i++) { | ||
| if (memcmp(data + i, filterStr, filterLen) == 0) { | ||
| return len; // drop to avoid recursion | ||
| } | ||
| } | ||
| } |
Comment on lines
+725
to
+731
| if (m_rc == BLE_HS_ENOMEM || os_msys_num_free() <= 2) { | ||
| // NimBLE stack out of buffers, likely due to pending notifications/indications | ||
| // Don't drop data, but wait for stack to free buffers and try again later | ||
| return true; | ||
| } | ||
|
|
||
| return false; // disconnect or other error don't retry send, preserve data for next attempt |
Comment on lines
+141
to
+146
| // Convenience overload to create service/characteristic internally; service will be deleted on end() | ||
| bool begin(const NimBLEUUID& svcUuid, | ||
| const NimBLEUUID& chrUuid, | ||
| uint32_t txBufSize = 1024, | ||
| uint32_t rxBufSize = 1024, | ||
| bool secure = false); |
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
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.
Adds a data stream client and server classes.
The server class sends data to clients through notifications and accepts data written to the characteristic by clients.
The client writes data to the server characteristic and receives data through notifications.
Summary by CodeRabbit
New Features
Documentation
Chores