Merged
Conversation
📝 WalkthroughWalkthroughArr, matey — this PR adds a full NimBLE streaming subsystem (new header + implementation), integrates it into the build, and lands three example apps (Server, Client, Echo) with sdkconfig.defaults and docs; plus small repo housekeeping and CI matrix updates. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(46, 117, 182, 0.5)
actor App as Application<br/>(Central)
participant Scanner as NimBLE<br/>Scanner
participant Remote as BLE<br/>Server
participant RemoteChr as Remote<br/>Characteristic
participant StreamClt as NimBLEStreamClient<br/>Instance
end
App->>Scanner: start scan for service UUID
Scanner->>Remote: discover service/characteristic
Remote-->>Scanner: advertise/service found
Scanner-->>App: onResult callback
App->>Remote: connect
Remote-->>App: connection established
App->>RemoteChr: discover characteristic
RemoteChr-->>App: characteristic found
App->>StreamClt: begin(pRemoteChr, subscribe)
StreamClt->>RemoteChr: subscribe to notifications
RemoteChr-->>StreamClt: notifications enabled
App->>StreamClt: write(data)
StreamClt->>RemoteChr: write (no response / chunked)
RemoteChr-->>Remote: deliver data
Remote->>RemoteChr: notify client with RX data
RemoteChr-->>StreamClt: onNotify -> pushRx()
StreamClt->>App: data available in RX buffer
sequenceDiagram
rect rgba(46, 160, 67, 0.5)
actor App as Application<br/>(Peripheral)
participant NimBLEDev as NimBLEDevice
participant Server as NimBLE<br/>Server
participant Chr as BLE<br/>Characteristic
participant StreamSrv as NimBLEStreamServer<br/>Instance
actor RemoteClient as Remote BLE<br/>Client
end
App->>NimBLEDev: init device & server
App->>Chr: create service/characteristic
App->>StreamSrv: begin(characteristic)
StreamSrv->>Chr: register callbacks (onWrite/onSubscribe/onStatus)
App->>Server: start advertising
RemoteClient->>Server: connect
Chr->>StreamSrv: onSubscribe callback
RemoteClient->>Chr: write(data)
Chr->>StreamSrv: onWrite -> pushRx()
StreamSrv->>App: data available in RX buffer
App->>StreamSrv: read() and write(response)
StreamSrv->>Chr: send notification (chunked / drain)
Chr-->>RemoteClient: data delivered
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
h2zero
force-pushed
the
stream-class
branch
9 times, most recently
from
c141b96 to
5949e51
Compare
![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 (10)
examples/NimBLE_Stream_Client/main/CMakeLists.txt (1)
1-4: Ahoy! This old-timey CMake be usin' the legacy ESP-IDF API, matey!Arr,
register_component()be the old way of doin' things. The modern ESP-IDF (4.0+) prefersidf_component_register(). If ye be only supportin' ESP-IDF 5.x as yer workflow suggests, ye might want to modernize this. But if ye need backward compatibility with older build systems, this be fine as-is, savvy?⚓ Modern CMake approach (optional)
-set(COMPONENT_SRCS "main.cpp") -set(COMPONENT_ADD_INCLUDEDIRS ".") - -register_component() +idf_component_register(SRCS "main.cpp" + INCLUDE_DIRS ".")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Client/main/CMakeLists.txt` around lines 1 - 4, The CMake file uses the legacy register_component() API; replace it with the modern idf_component_register() call and pass the component sources and include dirs via the SRCS and INCLUDE_DIRS keywords (i.e., replace register_component() and the separate set(COMPONENT_SRCS ...) / set(COMPONENT_ADD_INCLUDEDIRS ...) pattern by a single idf_component_register(...) invocation that references the same source file name and include directory), or conditionally wrap the new call if you must preserve backward compatibility with older ESP-IDF versions..github/workflows/build.yml (2)
23-27: Arr, where be the new stream examples in yer CI treasure map?The new
NimBLE_Stream_Server,NimBLE_Stream_Client, andNimBLE_Stream_Echoexamples be added to the codebase, but they ain't bein' tested in yer CI matrix, savvy? Consider addin' at least one of 'em to ensure the stream functionality be properly built and tested across the fleet.🏴☠️ Proposed addition to include stream examples
example: - NimBLE_Client - NimBLE_Server - Bluetooth_5/NimBLE_extended_client - Bluetooth_5/NimBLE_extended_server + - NimBLE_Stream_Server + - NimBLE_Stream_Client🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 23 - 27, Add the new stream examples to the CI job matrix so they get built/tested: update the "example" matrix entries (currently containing NimBLE_Client, NimBLE_Server, Bluetooth_5/NimBLE_extended_client, Bluetooth_5/NimBLE_extended_server) to include one or more of NimBLE_Stream_Server, NimBLE_Stream_Client, and NimBLE_Stream_Echo (preferably at least NimBLE_Stream_Server) so stream functionality is exercised in CI.
33-48: Avast! Ye got some barnacles clingin' to yer hull, matey!These exclude rules be referrin' to
release-v4.4andrelease-v5.1, but those scallywags ain't in yer matrix no more (onlyrelease-v5.4andrelease-v5.5remain). These dead excludes be cluttering up yer workflow like barnacles on a ship's bottom.🧹 Proposed cleanup to remove stale excludes
exclude: - idf_target: "esp32" example: Bluetooth_5/NimBLE_extended_client - idf_target: "esp32" example: Bluetooth_5/NimBLE_extended_server - - idf_ver: release-v4.4 - idf_target: "esp32c2" - - idf_ver: release-v4.4 - idf_target: "esp32c5" - - idf_ver: release-v4.4 - idf_target: "esp32c6" - - idf_ver: release-v4.4 - idf_target: "esp32c61" - idf_ver: release-v5.4 idf_target: "esp32c61" - - idf_ver: release-v4.4 - idf_target: "esp32h2" - - idf_ver: release-v4.4 - idf_target: "esp32p4" - - idf_ver: release-v5.1 - idf_target: "esp32p4"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 33 - 48, The workflow contains stale exclude entries referencing idf_ver values "release-v4.4" and "release-v5.1" (rows paired with idf_target values like "esp32c2", "esp32c5", "esp32c6", "esp32c61", "esp32h2", "esp32p4") that no longer exist in the matrix; remove those exclude blocks or replace them with the current idf_ver values (e.g., "release-v5.4" or "release-v5.5") so excludes match actual matrix entries and avoid dead rules.examples/NimBLE_Stream_Echo/main/main.cpp (2)
19-36: Arr, ye scallywag! ThedroppedNewfield be collectin' barnacles - never gets incremented!The
RxOverflowStatsstruct declaresdroppedNewbut onlydroppedOldbe gettin' updated in the overflow callback. Since this example always returnsDROP_OLDER_DATA, thedroppedNewcounter will forever remain at zero, makin' it dead weight in yer cargo hold.Either increment
droppedNewwhen appropriate (if ye plan to supportDROP_NEW_DATAin the future), or remove the unused field to keep the ship trim.🏴☠️ Proposed fix to simplify the stats structure
struct RxOverflowStats { uint32_t droppedOld{0}; - uint32_t droppedNew{0}; }; RxOverflowStats g_rxOverflowStats; NimBLEStream::RxOverflowAction onRxOverflow(const uint8_t* data, size_t len, void* userArg) { auto* stats = static_cast<RxOverflowStats*>(userArg); if (stats) { stats->droppedOld++; } // Echo mode prefers the latest incoming bytes. (void)data; (void)len; return NimBLEStream::DROP_OLDER_DATA; }And update the log statement accordingly:
- if (g_rxOverflowStats.droppedOld != lastDroppedOld || g_rxOverflowStats.droppedNew != lastDroppedNew) { - lastDroppedOld = g_rxOverflowStats.droppedOld; - lastDroppedNew = g_rxOverflowStats.droppedNew; - printf("RX overflow handled (drop-old=%" PRIu32 ", drop-new=%" PRIu32 ")\n", lastDroppedOld, lastDroppedNew); + if (g_rxOverflowStats.droppedOld != lastDroppedOld) { + lastDroppedOld = g_rxOverflowStats.droppedOld; + printf("RX overflow handled (drop-old=%" PRIu32 ")\n", lastDroppedOld); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Echo/main/main.cpp` around lines 19 - 36, The RxOverflowStats struct declares droppedNew but only droppedOld is ever updated in the onRxOverflow callback (which always returns NimBLEStream::DROP_OLDER_DATA), so either remove the unused droppedNew field or update onRxOverflow to increment droppedNew when returning DROP_NEW_DATA; locate RxOverflowStats and the global g_rxOverflowStats and modify the struct to remove droppedNew (and update any logging) OR add logic in onRxOverflow to inspect which RxOverflowAction will be used and increment stats->droppedNew accordingly (while keeping the current increment of stats->droppedOld when DROP_OLDER_DATA is chosen).
71-80: Consider readin' the whole chest of treasure at once, matey!The current echo loop reads and writes one byte at a time in a tight while loop. While this be functionin' correctly, it could be more efficient to use the bulk
read(uint8_t*, size_t)method to hoist the data in larger chunks, especially when there be plenty of bytes awaitin' in the buffer.⚓ Proposed optimization for bulk read/write
// Echo any received data back to the client. if (bleStream.ready() && bleStream.available()) { printf("Echo: "); - while (bleStream.available()) { - char c = bleStream.read(); - putchar(c); - bleStream.write(c); + uint8_t buf[128]; + size_t n; + while ((n = bleStream.read(buf, sizeof(buf))) > 0) { + fwrite(buf, 1, n, stdout); + bleStream.write(buf, n); } printf("\n"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Echo/main/main.cpp` around lines 71 - 80, The per-byte echo loop is inefficient; replace it with a chunked read/write using bleStream.available() to determine how many bytes to pull and the bulk methods bleStream.read(uint8_t*, size_t) and bleStream.write(const uint8_t*, size_t). Allocate a temporary buffer (stack or fixed-size) and in a loop read up to buffer size or available(), write the returned length back to the stream, and print the buffer contents (keeping the "Echo:"/newline output) until no more bytes are available. Ensure you handle the returned read length (may be less than requested) and loop until bleStream.available() == 0.examples/NimBLE_Stream_Server/main/main.cpp (1)
22-39: Same barnacle-covereddroppedNewfield as the other examples!For consistency across yer fleet of examples, consider either usin' this field properly or removin' it from all three examples.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Server/main/main.cpp` around lines 22 - 39, The struct RxOverflowStats contains an unused field droppedNew; either remove droppedNew from RxOverflowStats or update the onRxOverflow handler (and any related overflow logic) to increment droppedNew where appropriate (e.g., when newer incoming bytes are dropped instead of older bytes). Locate the RxOverflowStats definition and the NimBLEStream::RxOverflowAction onRxOverflow(const uint8_t*, size_t, void*) function and make the change consistently across all examples so the field is either used (incremented) or deleted everywhere.examples/NimBLE_Stream_Client/main/main.cpp (2)
58-80: Beware the danglin' pointer to advertised device, cap'n!The
pServerDevicepointer be stored from the scan callback, butNimBLEAdvertisedDevicepointers from scan results may become invalid once a new scan starts or results are cleared. While yer code stops the scan immediately and setsdoConnect = true, there be a small window where the pointer could become stale if timin' be unlucky.Consider copyin' the address instead of storin' the pointer, which be safer for voyage across async waters.
🗺️ Proposed fix to store address instead of pointer
-static const NimBLEAdvertisedDevice* pServerDevice = nullptr; +static NimBLEAddress serverAddress; +static bool haveServerAddress = false; /** Scan callbacks to find the server */ class ScanCallbacks : public NimBLEScanCallbacks { void onResult(const NimBLEAdvertisedDevice* advertisedDevice) override { printf("Advertised Device: %s\n", advertisedDevice->toString().c_str()); // Check if this device advertises our service. if (advertisedDevice->isAdvertisingService(NimBLEUUID(SERVICE_UUID))) { printf("Found our stream server!\n"); - pServerDevice = advertisedDevice; + serverAddress = advertisedDevice->getAddress(); + haveServerAddress = true; NimBLEDevice::getScan()->stop(); doConnect = true; } }Then update
connectToServer()to use the address directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Client/main/main.cpp` around lines 58 - 80, The scan callback stores a pointer pServerDevice to a NimBLEAdvertisedDevice which can become invalid when scans end or results are cleared; instead capture and store the device address (e.g., advertisedDevice->getAddress() or its string) inside ScanCallbacks::onResult and clear any use of pServerDevice, then update connectToServer() to resolve/connect using that stored address (use NimBLEAddress/string -> connect flow) so you no longer rely on a dangling NimBLEAdvertisedDevice pointer.
27-45: Shiver me timbers! Same ghost field as the Echo example, me hearty!The
droppedNewfield be unused here too - onlydroppedOldgets incremented. This be a pattern across all three examples that could use some consistency cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Client/main/main.cpp` around lines 27 - 45, The RxOverflowStats.droppedNew field is never updated; update the overflow handling so the counter matches the chosen action or remove the unused field: in onRxOverflow (returning NimBLEStream::DROP_OLDER_DATA) increment stats->droppedOld as already done and, if you intend to support DROP_NEWER_DATA, branch on the returned NimBLEStream::RxOverflowAction to increment stats->droppedNew accordingly, otherwise remove droppedNew from RxOverflowStats and g_rxOverflowStats to keep examples consistent (references: struct RxOverflowStats, g_rxOverflowStats, function onRxOverflow, NimBLEStream::RxOverflowAction).src/NimBLEStream.cpp (1)
711-747: Thar be duplicate code sailin' in both ships!The
flush()methods inNimBLEStreamServer(lines 711-747) andNimBLEStreamClient(lines 958-994) be nearly identical twins separated at birth. Consider hoisting this common logic up to the baseNimBLEStreamclass to keep yer code DRY as hardtack.♻️ Proposed refactor to move flush() to base class
Move the common flush logic to
NimBLEStream:// In NimBLEStream base class void NimBLEStream::flush() { if (!m_txBuf || m_txBuf->size() == 0) { return; } const uint32_t timeoutMs = static_cast<uint32_t>(std::min<unsigned long>(getTimeout(), 0xFFFFFFFFUL)); const uint32_t retryDelay = std::max<uint32_t>(1, ble_npl_time_ms_to_ticks32(5)); uint32_t waitStart = ble_npl_time_get(); while (m_txBuf->size() > 0) { size_t before = m_txBuf->size(); bool retry = send(); size_t after = m_txBuf->size(); if (after == 0) { return; } if (after < before) { waitStart = ble_npl_time_get(); continue; } if (retry && timeoutMs > 0) { const uint32_t elapsed = ble_npl_time_get() - waitStart; if (elapsed < ble_npl_time_ms_to_ticks32(timeoutMs)) { ble_npl_time_delay(retryDelay); continue; } } m_txBuf->drop(m_txBuf->size()); if (m_rxBuf) { m_rxBuf->drop(m_rxBuf->size()); } return; } }Then remove the duplicate implementations from both derived classes.
Also applies to: 958-994
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEStream.cpp` around lines 711 - 747, The flush() implementations in NimBLEStreamServer::flush and NimBLEStreamClient::flush are duplicate; move the shared logic into a single NimBLEStream::flush method and remove the two derived copies. Ensure NimBLEStream has access to m_txBuf and m_rxBuf (make them protected if necessary), call the virtual send() and getTimeout() methods from the base implementation, and keep the existing use of ble_npl_time_get/ble_npl_time_ms_to_ticks32/ble_npl_time_delay and buffer drop logic intact; if either derived class needs special behavior, override NimBLEStream::flush and call the base implementation.src/NimBLEStream.h (1)
101-104: Consider thread safety for callback registration, cap'n!The
setRxOverflowCallbackmethod assigns bothm_rxOverflowCallbackandm_rxOverflowUserArgwithout synchronization. If called while data be flowin' in, there be a small window where the callback might be called with mismatched arguments (new callback with old userArg or vice versa).For example code this be likely fine, but in production scenarios with dynamic callback changes, ye might want to protect this with a lock or use atomic operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEStream.h` around lines 101 - 104, The setter setRxOverflowCallback currently writes m_rxOverflowCallback and m_rxOverflowUserArg unsafely; protect these two related members with synchronization so they cannot be observed in a mixed state. Add a mutex (e.g., m_callbackMutex) and lock it in setRxOverflowCallback while assigning both m_rxOverflowCallback and m_rxOverflowUserArg, and also lock (or atomically copy the pair under the same mutex) wherever the RX overflow callback is invoked so the caller always sees a consistent pair; alternatively implement an atomic small struct (callback+userArg) and swap it atomically in setRxOverflowCallback and when invoking.
🤖 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/main/main.cpp`:
- Around line 184-194: There’s a race where resetting pServerDevice to nullptr
before restarting a scan can cause onScanEnd to also restart scanning
concurrently; change the sequence in the failure branch of connectToServer()
handling in main loop so you either (a) set doConnect = false and start the scan
before clearing pServerDevice (keep pServerDevice valid until after
NimBLEDevice::getScan()->start(...) completes), or (b) add an explicit guard
using NimBLEDevice::getScan()->isScanning() (or a new boolean like scanning) so
NimBLEDevice::getScan()->start(scanTime, false, true) is only called when no
scan is already running; reference symbols: doConnect, connectToServer(),
pServerDevice, onScanEnd, connected, and NimBLEDevice::getScan()->start.
In `@examples/NimBLE_Stream_Server/main/main.cpp`:
- Around line 126-128: The log strings are reversed for the server: update the
messages in the BLE send block (the bleStream.printf calls and the printf("Sent
data to server via BLE stream")) to reference the server role; change "Hello
from client! Uptime..." to "Hello from server! Uptime..." (or similar) and
change the final printf to indicate data was sent to the client (e.g., "Sent
data to client via BLE stream") so bleStream.printf and the printf reflect the
SERVER role.
In `@src/NimBLEStream.cpp`:
- Around line 49-63: The constructor ByteRingBuffer(size_t) initializes m_mutex
via ble_npl_mutex_init then may return early if malloc fails, leaking the
initialized mutex; fix by ensuring the mutex is cleaned up on allocation failure
(call the matching deinit/cleanup function for ble_npl_mutex, e.g.
ble_npl_mutex_deinit(&m_mutex) or the appropriate API) before returning, or
track mutex initialization with a flag (e.g. m_mutex_initialized) and use that
flag in the destructor/early returns to call ble_npl_mutex_deinit when needed;
update the ByteRingBuffer constructor (where ble_npl_mutex_init and m_buf
allocation occur) and the destructor/valid() logic accordingly.
- Around line 483-522: pushRx() holds ByteRingBuffer::Guard g and then calls
m_rxBuf methods (freeSize, drop, write) that internally re-acquire the same
mutex causing deadlock; fix by adding and calling unlocked/internal variants
(e.g., freeSizeUnlocked(), dropUnlocked(size_t), writeUnlocked(const uint8_t*,
size_t)) or by directly accessing the ring buffer's internal fields while
holding g, then replace calls to m_rxBuf->freeSize(), m_rxBuf->drop(...), and
m_rxBuf->write(...) with those unlocked/internal operations inside
NimBLEStream::pushRx so no nested Guard is taken; ensure the same semantics
(capacity(), size(), and write return value and error logging) are preserved.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 23-27: Add the new stream examples to the CI job matrix so they
get built/tested: update the "example" matrix entries (currently containing
NimBLE_Client, NimBLE_Server, Bluetooth_5/NimBLE_extended_client,
Bluetooth_5/NimBLE_extended_server) to include one or more of
NimBLE_Stream_Server, NimBLE_Stream_Client, and NimBLE_Stream_Echo (preferably
at least NimBLE_Stream_Server) so stream functionality is exercised in CI.
- Around line 33-48: The workflow contains stale exclude entries referencing
idf_ver values "release-v4.4" and "release-v5.1" (rows paired with idf_target
values like "esp32c2", "esp32c5", "esp32c6", "esp32c61", "esp32h2", "esp32p4")
that no longer exist in the matrix; remove those exclude blocks or replace them
with the current idf_ver values (e.g., "release-v5.4" or "release-v5.5") so
excludes match actual matrix entries and avoid dead rules.
In `@examples/NimBLE_Stream_Client/main/CMakeLists.txt`:
- Around line 1-4: The CMake file uses the legacy register_component() API;
replace it with the modern idf_component_register() call and pass the component
sources and include dirs via the SRCS and INCLUDE_DIRS keywords (i.e., replace
register_component() and the separate set(COMPONENT_SRCS ...) /
set(COMPONENT_ADD_INCLUDEDIRS ...) pattern by a single
idf_component_register(...) invocation that references the same source file name
and include directory), or conditionally wrap the new call if you must preserve
backward compatibility with older ESP-IDF versions.
In `@examples/NimBLE_Stream_Client/main/main.cpp`:
- Around line 58-80: The scan callback stores a pointer pServerDevice to a
NimBLEAdvertisedDevice which can become invalid when scans end or results are
cleared; instead capture and store the device address (e.g.,
advertisedDevice->getAddress() or its string) inside ScanCallbacks::onResult and
clear any use of pServerDevice, then update connectToServer() to resolve/connect
using that stored address (use NimBLEAddress/string -> connect flow) so you no
longer rely on a dangling NimBLEAdvertisedDevice pointer.
- Around line 27-45: The RxOverflowStats.droppedNew field is never updated;
update the overflow handling so the counter matches the chosen action or remove
the unused field: in onRxOverflow (returning NimBLEStream::DROP_OLDER_DATA)
increment stats->droppedOld as already done and, if you intend to support
DROP_NEWER_DATA, branch on the returned NimBLEStream::RxOverflowAction to
increment stats->droppedNew accordingly, otherwise remove droppedNew from
RxOverflowStats and g_rxOverflowStats to keep examples consistent (references:
struct RxOverflowStats, g_rxOverflowStats, function onRxOverflow,
NimBLEStream::RxOverflowAction).
In `@examples/NimBLE_Stream_Echo/main/main.cpp`:
- Around line 19-36: The RxOverflowStats struct declares droppedNew but only
droppedOld is ever updated in the onRxOverflow callback (which always returns
NimBLEStream::DROP_OLDER_DATA), so either remove the unused droppedNew field or
update onRxOverflow to increment droppedNew when returning DROP_NEW_DATA; locate
RxOverflowStats and the global g_rxOverflowStats and modify the struct to remove
droppedNew (and update any logging) OR add logic in onRxOverflow to inspect
which RxOverflowAction will be used and increment stats->droppedNew accordingly
(while keeping the current increment of stats->droppedOld when DROP_OLDER_DATA
is chosen).
- Around line 71-80: The per-byte echo loop is inefficient; replace it with a
chunked read/write using bleStream.available() to determine how many bytes to
pull and the bulk methods bleStream.read(uint8_t*, size_t) and
bleStream.write(const uint8_t*, size_t). Allocate a temporary buffer (stack or
fixed-size) and in a loop read up to buffer size or available(), write the
returned length back to the stream, and print the buffer contents (keeping the
"Echo:"/newline output) until no more bytes are available. Ensure you handle the
returned read length (may be less than requested) and loop until
bleStream.available() == 0.
In `@examples/NimBLE_Stream_Server/main/main.cpp`:
- Around line 22-39: The struct RxOverflowStats contains an unused field
droppedNew; either remove droppedNew from RxOverflowStats or update the
onRxOverflow handler (and any related overflow logic) to increment droppedNew
where appropriate (e.g., when newer incoming bytes are dropped instead of older
bytes). Locate the RxOverflowStats definition and the
NimBLEStream::RxOverflowAction onRxOverflow(const uint8_t*, size_t, void*)
function and make the change consistently across all examples so the field is
either used (incremented) or deleted everywhere.
In `@src/NimBLEStream.cpp`:
- Around line 711-747: The flush() implementations in NimBLEStreamServer::flush
and NimBLEStreamClient::flush are duplicate; move the shared logic into a single
NimBLEStream::flush method and remove the two derived copies. Ensure
NimBLEStream has access to m_txBuf and m_rxBuf (make them protected if
necessary), call the virtual send() and getTimeout() methods from the base
implementation, and keep the existing use of
ble_npl_time_get/ble_npl_time_ms_to_ticks32/ble_npl_time_delay and buffer drop
logic intact; if either derived class needs special behavior, override
NimBLEStream::flush and call the base implementation.
In `@src/NimBLEStream.h`:
- Around line 101-104: The setter setRxOverflowCallback currently writes
m_rxOverflowCallback and m_rxOverflowUserArg unsafely; protect these two related
members with synchronization so they cannot be observed in a mixed state. Add a
mutex (e.g., m_callbackMutex) and lock it in setRxOverflowCallback while
assigning both m_rxOverflowCallback and m_rxOverflowUserArg, and also lock (or
atomically copy the pair under the same mutex) wherever the RX overflow callback
is invoked so the caller always sees a consistent pair; alternatively implement
an atomic small struct (callback+userArg) and swap it atomically in
setRxOverflowCallback and when invoking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b78d7b1f-f9b5-4fe5-bf09-4273e9aedd98
📒 Files selected for processing (21)
.github/workflows/build.yml.gitignoreCMakeLists.txtexamples/NimBLE_Stream_Client/CMakeLists.txtexamples/NimBLE_Stream_Client/README.mdexamples/NimBLE_Stream_Client/main/CMakeLists.txtexamples/NimBLE_Stream_Client/main/main.cppexamples/NimBLE_Stream_Client/sdkconfig.defaultsexamples/NimBLE_Stream_Echo/CMakeLists.txtexamples/NimBLE_Stream_Echo/README.mdexamples/NimBLE_Stream_Echo/main/CMakeLists.txtexamples/NimBLE_Stream_Echo/main/main.cppexamples/NimBLE_Stream_Echo/sdkconfig.defaultsexamples/NimBLE_Stream_Server/CMakeLists.txtexamples/NimBLE_Stream_Server/README.mdexamples/NimBLE_Stream_Server/main/CMakeLists.txtexamples/NimBLE_Stream_Server/main/main.cppexamples/NimBLE_Stream_Server/sdkconfig.defaultssrc/NimBLEDevice.hsrc/NimBLEStream.cppsrc/NimBLEStream.h
Co-authored-by: doudar <17362216+doudar@users.noreply.github.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
examples/NimBLE_Stream_Client/main/main.cpp (1)
27-45: A minor observation about yer overflow counters, cap'n.The
RxOverflowStatsstruct has bothdroppedOldanddroppedNewfields, but theonRxOverflowcallback only ever incrementsdroppedOld(since it always returnsDROP_OLDER_DATA). ThedroppedNewcounter will forever remain at zero in this example.This be perfectly fine for demonstration purposes, but ye might want to add a comment explainin' that
droppedNewbe there for cases where the callback might returnDROP_NEW_DATAinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Client/main/main.cpp` around lines 27 - 45, The overflow counters are misleading because RxOverflowStats defines droppedOld and droppedNew but onRxOverflow only increments droppedOld and always returns DROP_OLDER_DATA; update the code by adding a concise comment near RxOverflowStats or onRxOverflow (referencing RxOverflowStats, g_rxOverflowStats, droppedOld, droppedNew, onRxOverflow, and DROP_OLDER_DATA) explaining that droppedNew is intentionally unused in this example and exists for callbacks that return DROP_NEW_DATA, or alternatively branch in onRxOverflow to increment droppedNew when returning DROP_NEW_DATA if you prefer a symmetric example.examples/NimBLE_Stream_Echo/main/main.cpp (1)
71-80: The echo loop works, but could be more efficient, matey.Ye be readin' and writin' one byte at a time in the echo loop. For a simple demo this be fine, but for larger payloads ye might want to batch the echo operation to reduce overhead.
🏴☠️ Optional optimization for batch echo
// Echo any received data back to the client. if (bleStream.ready() && bleStream.available()) { printf("Echo: "); + uint8_t buf[128]; + size_t len = bleStream.read(buf, sizeof(buf)); + for (size_t i = 0; i < len; i++) { + putchar(buf[i]); + } + bleStream.write(buf, len); - while (bleStream.available()) { - char c = bleStream.read(); - putchar(c); - bleStream.write(c); - } printf("\n"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/NimBLE_Stream_Echo/main/main.cpp` around lines 71 - 80, The echo loop in main.cpp is inefficient because it reads and writes one byte at a time using bleStream.available(), bleStream.read(), and bleStream.write(); change it to batch reads and writes by allocating a temporary buffer, determine a chunk size from bleStream.available() (capped to a sensible max), call the stream's bulk-read into the buffer (e.g., read into buffer or readBytes if available) and then write the entire buffer back with a single bleStream.write(buffer, length) call; keep the bleStream.ready() check and fall back to the byte-by-byte loop only if bulk read APIs are unavailable.src/NimBLEStream.cpp (1)
712-748: Avast! Code duplication spotted on the horizon!The
flush()implementations inNimBLEStreamServer(lines 712-748) andNimBLEStreamClient(lines 959-995) be nearly identical, ye scallywag! Consider hoisting this common logic up to the baseNimBLEStreamclass to avoid maintainin' two copies of the same code.This be optional since the implementations work correctly, but it would make future changes easier to manage.
Also applies to: 959-995
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/NimBLEStream.cpp` around lines 712 - 748, Both NimBLEStreamServer::flush and NimBLEStreamClient::flush contain the same loop/timeout/retry/drop logic; extract that common algorithm into a single protected method on the base NimBLEStream (e.g., NimBLEStream::flushImpl or make NimBLEStream::flush the shared implementation) that relies on the virtual send(), getTimeout(), m_txBuf/m_rxBuf members and helper functions (ble_npl_time_get, ble_npl_time_ms_to_ticks32, ble_npl_time_delay); then have NimBLEStreamServer::flush and NimBLEStreamClient::flush simply call the base implementation (or remove them if no subclass-specific behavior remains) to eliminate duplication and keep behavior unchanged.
🤖 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_Client/main/main.cpp`:
- Around line 27-45: The overflow counters are misleading because
RxOverflowStats defines droppedOld and droppedNew but onRxOverflow only
increments droppedOld and always returns DROP_OLDER_DATA; update the code by
adding a concise comment near RxOverflowStats or onRxOverflow (referencing
RxOverflowStats, g_rxOverflowStats, droppedOld, droppedNew, onRxOverflow, and
DROP_OLDER_DATA) explaining that droppedNew is intentionally unused in this
example and exists for callbacks that return DROP_NEW_DATA, or alternatively
branch in onRxOverflow to increment droppedNew when returning DROP_NEW_DATA if
you prefer a symmetric example.
In `@examples/NimBLE_Stream_Echo/main/main.cpp`:
- Around line 71-80: The echo loop in main.cpp is inefficient because it reads
and writes one byte at a time using bleStream.available(), bleStream.read(), and
bleStream.write(); change it to batch reads and writes by allocating a temporary
buffer, determine a chunk size from bleStream.available() (capped to a sensible
max), call the stream's bulk-read into the buffer (e.g., read into buffer or
readBytes if available) and then write the entire buffer back with a single
bleStream.write(buffer, length) call; keep the bleStream.ready() check and fall
back to the byte-by-byte loop only if bulk read APIs are unavailable.
In `@src/NimBLEStream.cpp`:
- Around line 712-748: Both NimBLEStreamServer::flush and
NimBLEStreamClient::flush contain the same loop/timeout/retry/drop logic;
extract that common algorithm into a single protected method on the base
NimBLEStream (e.g., NimBLEStream::flushImpl or make NimBLEStream::flush the
shared implementation) that relies on the virtual send(), getTimeout(),
m_txBuf/m_rxBuf members and helper functions (ble_npl_time_get,
ble_npl_time_ms_to_ticks32, ble_npl_time_delay); then have
NimBLEStreamServer::flush and NimBLEStreamClient::flush simply call the base
implementation (or remove them if no subclass-specific behavior remains) to
eliminate duplication and keep behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6c1d5e0a-2586-4097-9037-d7d17ec849ed
📒 Files selected for processing (21)
.github/workflows/build.yml.gitignoreCMakeLists.txtexamples/NimBLE_Stream_Client/CMakeLists.txtexamples/NimBLE_Stream_Client/README.mdexamples/NimBLE_Stream_Client/main/CMakeLists.txtexamples/NimBLE_Stream_Client/main/main.cppexamples/NimBLE_Stream_Client/sdkconfig.defaultsexamples/NimBLE_Stream_Echo/CMakeLists.txtexamples/NimBLE_Stream_Echo/README.mdexamples/NimBLE_Stream_Echo/main/CMakeLists.txtexamples/NimBLE_Stream_Echo/main/main.cppexamples/NimBLE_Stream_Echo/sdkconfig.defaultsexamples/NimBLE_Stream_Server/CMakeLists.txtexamples/NimBLE_Stream_Server/README.mdexamples/NimBLE_Stream_Server/main/CMakeLists.txtexamples/NimBLE_Stream_Server/main/main.cppexamples/NimBLE_Stream_Server/sdkconfig.defaultssrc/NimBLEDevice.hsrc/NimBLEStream.cppsrc/NimBLEStream.h
🚧 Files skipped from review as they are similar to previous changes (13)
- examples/NimBLE_Stream_Client/main/CMakeLists.txt
- examples/NimBLE_Stream_Echo/CMakeLists.txt
- examples/NimBLE_Stream_Echo/sdkconfig.defaults
- examples/NimBLE_Stream_Client/CMakeLists.txt
- examples/NimBLE_Stream_Server/CMakeLists.txt
- .github/workflows/build.yml
- CMakeLists.txt
- examples/NimBLE_Stream_Server/sdkconfig.defaults
- src/NimBLEDevice.h
- examples/NimBLE_Stream_Echo/main/CMakeLists.txt
- examples/NimBLE_Stream_Echo/README.md
- examples/NimBLE_Stream_Server/main/CMakeLists.txt
- examples/NimBLE_Stream_Client/sdkconfig.defaults
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.
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