Conversation
1b7cb22 to
6a67147
Compare
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:
📝 WalkthroughWalkthroughYarrr: Introduces a new NimBLEStream subsystem (server + client) with ring-buffered TX/RX, MTU-aware chunking, drain/callout logic; adds three Arduino examples (Server, Client, Echo) with READMEs; small console/nimconfig preprocessor formatting tweaks; and updates .gitignore. 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, set callbacks
Peer->>BLE: connect & subscribe
BLE->>Stream: onSubscribe -> update peer handle
App->>Stream: write(data)
Stream->>Stream: enqueue TX, schedule drain
Stream->>BLE: send notification (MTU-limited chunks)
BLE->>Peer: deliver notification
Peer->>BLE: write characteristic (RX)
BLE->>Stream: onWrite -> push RX into buffer
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, subscribe=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) | ⏱️ ~45 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 |
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
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
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.
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.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/NimBLE_Stream_Server/NimBLE_Stream_Server.ino (1)
132-132: Consider platform portability forESP.getFreeHeap().This call is specific to ESP32/ESP8266 platforms. While the NimBLE-Arduino library primarily targets ESP32, adding a brief comment or a preprocessor guard would clarify the platform requirement for users porting to other Arduino-compatible boards.
💡 Optional: Add platform context
// You can also use printf - bleStream.printf("Free heap: %d bytes\n", ESP.getFreeHeap()); + // Note: ESP.getFreeHeap() is ESP32/ESP8266 specific + bleStream.printf("Free heap: %d bytes\n", ESP.getFreeHeap());Or use a guard:
`#if` defined(ESP32) || defined(ESP8266) bleStream.printf("Free heap: %d bytes\n", ESP.getFreeHeap()); `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Server/NimBLE_Stream_Server.ino` at line 132, The call to ESP.getFreeHeap() inside bleStream.printf is ESP-specific and may break portability; update the code around bleStream.printf("Free heap: %d bytes\n", ESP.getFreeHeap()) to either wrap the call in a preprocessor guard (e.g., only compile when ESP32 or ESP8266 macros are defined) or add a clarifying comment that this line requires ESP platforms and should be removed/adjusted when porting to other boards so compilation and runtime behavior remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/NimBLE_Stream_Server/NimBLE_Stream_Server.ino`:
- Line 132: The call to ESP.getFreeHeap() inside bleStream.printf is
ESP-specific and may break portability; update the code around
bleStream.printf("Free heap: %d bytes\n", ESP.getFreeHeap()) to either wrap the
call in a preprocessor guard (e.g., only compile when ESP32 or ESP8266 macros
are defined) or add a clarifying comment that this line requires ESP platforms
and should be removed/adjusted when porting to other boards so compilation and
runtime behavior remain correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32444b8d-96f5-496e-a45e-10c9a2279824
📒 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.
Pull request overview
Copilot reviewed 10 out of 12 changed files in this pull request and generated 5 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.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 804-807: The ready() implementation incorrectly returns true for
RX-only streams (m_txBufSize == 0) by returning m_rxBufSize > 0, which makes
operator bool() report "ready" without any subscriber/notification context;
change the RX-only branch in ready() to return false so readiness remains
subscriber/notification-centric (consistent with NimBLEStreamServer behavior)
and rely on subscriber notification logic to indicate readiness for RX-only
streams; update ready() where m_txBufSize is checked (and any callers like
operator bool()) to preserve this contract.
- Around line 483-488: NimBLEStream::pushRx currently early-returns on ready()
and dereferences m_rxBuf without null-check; change it to first verify m_rxBuf
is non-null and return 0 if it is null, then obtain ByteRingBuffer::Guard
g(*m_rxBuf) and push data into the buffer unconditionally (do not gate on
ready()), mirroring NimBLEStreamServer behavior; update references in pushRx to
use the null-checked m_rxBuf before creating the Guard so no null deref occurs,
and ensure any existing ready()-based logic that dropped RX data is removed or
relocated so onWrite from clients always queues incoming data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8b37a14e-0540-46e5-89f5-59dae4f5d0bf
📒 Files selected for processing (3)
examples/NimBLE_Stream_Client/NimBLE_Stream_Client.inosrc/NimBLEStream.cppsrc/NimBLEStream.h
Co-authored-by: doudar <17362216+doudar@users.noreply.github.com>
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