feat(L2CAP): add disconnect API and harden CoC send/error handling#396
Open
mickeyl wants to merge 1 commit intoh2zero:masterfrom
Open
feat(L2CAP): add disconnect API and harden CoC send/error handling#396mickeyl wants to merge 1 commit intoh2zero:masterfrom
mickeyl wants to merge 1 commit intoh2zero:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new L2CAP CoC channel control APIs and hardens CoC TX buffer ownership semantics, while updating the L2CAP client/server examples to support stress testing and runtime statistics.
Changes:
- Add
NimBLEL2CAPChannel::disconnect()andNimBLEL2CAPChannel::getConnHandle(). - Adjust
writeFragment()retry/error handling to reflect NimBLE mbuf ownership on specific return codes. - Refresh L2CAP client/server examples (framing, stats, heap monitoring) and pin
mickeyl/esp-hplto1.1.0.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/NimBLEL2CAPChannel.h | Declares new channel APIs (disconnect, getConnHandle). |
| src/NimBLEL2CAPChannel.cpp | Implements new APIs and updates CoC send retry / mbuf ownership handling. |
| examples/L2CAP/L2CAP_Server/main/main.cpp | Updates server example for framed RX parsing, stats, heap monitoring, and advertising behavior. |
| examples/L2CAP/L2CAP_Server/main/idf_component.yml | Adds/pins mickeyl/esp-hpl dependency for the server example. |
| examples/L2CAP/L2CAP_Client/main/main.cpp | Updates client example for framed TX, status task, heap monitoring, and scan-by-name discovery. |
| examples/L2CAP/L2CAP_Client/main/idf_component.yml | Adds/pins mickeyl/esp-hpl dependency for the client example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+32
to
+35
| uint8_t expectedSequenceNumber = 0; | ||
| size_t sequenceErrors = 0; | ||
| size_t frameErrors = 0; | ||
| uint64_t startTime = 0; |
There was a problem hiding this comment.
frameErrors is tracked and printed but never incremented anywhere in the new framing parser, so the statistic will always stay at 0. Either increment it on invalid framing conditions (e.g., impossible length) or remove the counter to avoid misleading output.
Owner
|
@mickeyl Thanks for this, would you like to address the comments above or are those not relevant in your testing? |
📝 WalkthroughWalkthroughAdds a Git dependency ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client as L2CAP Client
participant Channel as NimBLEL2CAPChannel
participant Server as L2CAP Server
Client->>Client: init logger, framing vars\nstart connectTask & statusTask
Client->>Channel: open/connect using L2CAP_PSM
Channel->>Server: establish BLE L2CAP connection
loop Send framed packets
Client->>Client: build frame (seq + len(2) + payload)
Client->>Channel: write(frame)
alt write success
Channel->>Client: success
Client->>Client: increment counters, adjust payload
else ENOMEM/EAGAIN
Channel->>Channel: log "buffer consumed", delay, retry
else EBUSY
Channel->>Channel: log "busy", delay, retry
end
Channel->>Server: deliver frame
Server->>Server: append buffer, parse complete frames, validate seq, update stats
end
Client->>Channel: disconnect()
Channel->>Server: teardown channel
Server->>Server: print final stats, reset state, restart advertising
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/L2CAP/L2CAP_Client/main/main.cpp`:
- Around line 212-214: When write() fails in the send branch (currently printing
"failed to send!" then calling abort()), don't terminate the process; instead
unwind into the reconnect logic: remove the abort() call, keep the failure log,
update any send-failure counters/statistics, close the connection/socket
resource, set the client state to disconnected (the same state used by your
reconnect path), and return from the send routine so the existing reconnect
logic will run; reference the write() call and the abort() call in your changes
to locate and replace the behavior.
- Around line 79-123: The statusTask reads globals bytesSent, blocksSent,
currentPayloadSize, and startTime without synchronization while connectTask
writes them, causing data races (and unsafe 64-bit access on 32-bit MCUs);
create a FreeRTOS mutex (e.g., xSemaphoreHandle / SemaphoreHandle_t via
xSemaphoreCreateMutex) during initialization, then wrap all reads in statusTask
and all writes/updates in connectTask with xSemaphoreTake(..., portMAX_DELAY) /
xSemaphoreGive(...) to protect access to those variables and ensure the mutex is
created before tasks start.
In `@examples/L2CAP/L2CAP_Server/main/main.cpp`:
- Around line 61-70: Reject excessively large or invalid advertised payload
lengths instead of trusting payloadLen: define a reasonable MAX_PAYLOAD_SIZE
and, after computing payloadLen and frameSize, validate payloadLen <=
MAX_PAYLOAD_SIZE and frameSize does not exceed that limit or a total-buffer cap;
if the check fails increment frameErrors and tear down or resync the session
(e.g., close the connection or drop enough bytes to find the next valid header)
to avoid unbounded buffering. Apply this logic around the existing variables
buffer, payloadLen, frameSize and update frameErrors where frames are rejected.
- Around line 39-49: The onConnect implementation does not match the base hook
signature so it doesn't override; change the method signature for onConnect in
this file to match the base declaration: onConnect(NimBLEL2CAPChannel* channel,
uint16_t negotiatedMTU) (and add/keep the override specifier if present). Update
the function header to accept the negotiatedMTU parameter (use or explicitly
ignore it) so the connection-state initialization (connected,
totalBytesReceived, startTime, buffer.clear(), etc.) runs when the base calls
onConnect.
- Around line 160-200: The status-report loop is reading shared fields on
l2capChannelCallbacks (connected, totalBytesReceived, totalFramesReceived,
startTime, totalPayloadBytes, sequenceErrors) without synchronization, causing
data races and possible 64-bit tearing on 32-bit targets; fix by protecting
access with a lock (e.g., create/declare a mutex or spinlock like
l2capChannelMutex) or by entering a critical section around reads and copying
the needed fields into local variables/struct before computing elapsedSeconds
and printing, and ensure every writer to l2capChannelCallbacks uses the same
mutex/critical section so reads/writes are synchronized and the uint64_t
startTime is always read atomically.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7e62836-f4a7-42f9-b4ca-01bc93943943
📒 Files selected for processing (6)
examples/L2CAP/L2CAP_Client/main/idf_component.ymlexamples/L2CAP/L2CAP_Client/main/main.cppexamples/L2CAP/L2CAP_Server/main/idf_component.ymlexamples/L2CAP/L2CAP_Server/main/main.cppsrc/NimBLEL2CAPChannel.cppsrc/NimBLEL2CAPChannel.h
Comment on lines
+79
to
123
| void statusTask(void *pvParameters) { | ||
| while (true) { | ||
| vTaskDelay(1000 / portTICK_PERIOD_MS); | ||
|
|
||
| printf("Found the device we're interested in!\n"); | ||
| BLEDevice::getScan()->stop(); | ||
| if (startTime > 0 && blocksSent > 0) { | ||
| uint64_t currentTime = esp_timer_get_time(); | ||
| double elapsedSeconds = (currentTime - startTime) / 1000000.0; | ||
| double bytesPerSecond = bytesSent / elapsedSeconds; | ||
| double kbPerSecond = bytesPerSecond / 1024.0; | ||
|
|
||
| // Hand over the device to the other task | ||
| theDevice = advertisedDevice; | ||
| // Heap monitoring | ||
| size_t currentHeap = esp_get_free_heap_size(); | ||
| size_t minHeap = esp_get_minimum_free_heap_size(); | ||
|
|
||
| // Track heap for leak detection | ||
| if (initialHeap == 0) { | ||
| initialHeap = currentHeap; | ||
| lastHeap = currentHeap; | ||
| } | ||
|
|
||
| // Check for consistent heap decrease | ||
| if (currentHeap < lastHeap) { | ||
| heapDecreaseCount++; | ||
| if (heapDecreaseCount >= HEAP_LEAK_THRESHOLD) { | ||
| printf("\n⚠️ WARNING: POSSIBLE MEMORY LEAK DETECTED! ⚠️\n"); | ||
| printf("Heap has decreased %zu times in a row\n", heapDecreaseCount); | ||
| printf("Initial heap: %zu, Current heap: %zu, Lost: %zu bytes\n", | ||
| initialHeap, currentHeap, initialHeap - currentHeap); | ||
| } | ||
| } else if (currentHeap >= lastHeap) { | ||
| heapDecreaseCount = 0; // Reset counter if heap stabilizes or increases | ||
| } | ||
| lastHeap = currentHeap; | ||
|
|
||
| printf("\n=== STATUS UPDATE ===\n"); | ||
| printf("Blocks sent: %lu\n", (unsigned long)blocksSent); | ||
| printf("Total bytes sent: %zu\n", bytesSent); | ||
| printf("Current payload size: %zu bytes\n", currentPayloadSize); | ||
| printf("Elapsed time: %.1f seconds\n", elapsedSeconds); | ||
| printf("Bandwidth: %.2f KB/s (%.2f Mbps)\n", kbPerSecond, (bytesPerSecond * 8) / 1000000.0); | ||
| printf("Heap: %zu free (min: %zu), Used since start: %zu\n", | ||
| currentHeap, minHeap, initialHeap > 0 ? initialHeap - currentHeap : 0); | ||
| printf("==================\n\n"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'size_t bytesSent|size_t currentPayloadSize|uint32_t blocksSent|uint64_t startTime' examples/L2CAP/L2CAP_Client/main/main.cpp
rg -n -C2 'bytesSent \+=|blocksSent\+\+|currentPayloadSize =|startTime =' examples/L2CAP/L2CAP_Client/main/main.cpp
rg -n -C2 'xTaskCreate\(connectTask|xTaskCreate\(statusTask' examples/L2CAP/L2CAP_Client/main/main.cppRepository: h2zero/esp-nimble-cpp
Length of output: 1794
Protect the shared cargo with a lock, ye scurvy dog.
connectTask writes bytesSent, blocksSent, currentPayloadSize, and startTime whilst statusTask reads 'em concurrently without synchronization. These be racy globals, and the 64-bit startTime be especially treacherous on 32-bit MCUs where reads and writes of that treasure require multiple instructions.
Use xSemaphoreMutex or similar protection to guard these shared variables.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/L2CAP/L2CAP_Client/main/main.cpp` around lines 79 - 123, The
statusTask reads globals bytesSent, blocksSent, currentPayloadSize, and
startTime without synchronization while connectTask writes them, causing data
races (and unsafe 64-bit access on 32-bit MCUs); create a FreeRTOS mutex (e.g.,
xSemaphoreHandle / SemaphoreHandle_t via xSemaphoreCreateMutex) during
initialization, then wrap all reads in statusTask and all writes/updates in
connectTask with xSemaphoreTake(..., portMAX_DELAY) / xSemaphoreGive(...) to
protect access to those variables and ensure the mutex is created before tasks
start.
Comment on lines
212
to
+214
| } else { | ||
| printf("failed to send!\n"); | ||
| abort(); | ||
| abort(); |
There was a problem hiding this comment.
Don’t abort() on a routine send failure, matey.
write() already collapses disconnects and exhausted retries into false. Crashing the whole app here throws away the very stats this stress example is trying to gather; back off and unwind to the reconnect path instead.
🏴☠️ Proposed fix
} else {
- printf("failed to send!\n");
- abort();
+ printf("failed to send, backing off\n");
+ vTaskDelay(1000 / portTICK_PERIOD_MS);
+ break;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else { | |
| printf("failed to send!\n"); | |
| abort(); | |
| abort(); | |
| } else { | |
| printf("failed to send, backing off\n"); | |
| vTaskDelay(1000 / portTICK_PERIOD_MS); | |
| break; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/L2CAP/L2CAP_Client/main/main.cpp` around lines 212 - 214, When
write() fails in the send branch (currently printing "failed to send!" then
calling abort()), don't terminate the process; instead unwind into the reconnect
logic: remove the abort() call, keep the failure log, update any send-failure
counters/statistics, close the connection/socket resource, set the client state
to disconnected (the same state used by your reconnect path), and return from
the send routine so the existing reconnect logic will run; reference the write()
call and the abort() call in your changes to locate and replace the behavior.
Comment on lines
39
to
+49
| void onConnect(NimBLEL2CAPChannel* channel) { | ||
| printf("L2CAP connection established\n"); | ||
| printf("L2CAP connection established on PSM %d\n", L2CAP_PSM); | ||
| connected = true; | ||
| numberOfReceivedBytes = nextSequenceNumber = 0; | ||
| totalBytesReceived = 0; | ||
| totalFramesReceived = 0; | ||
| totalPayloadBytes = 0; | ||
| expectedSequenceNumber = 0; | ||
| sequenceErrors = 0; | ||
| frameErrors = 0; | ||
| startTime = esp_timer_get_time(); | ||
| buffer.clear(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'virtual void onConnect\(NimBLEL2CAPChannel\* channel, uint16_t negotiatedMTU\)' src/NimBLEL2CAPChannel.h
rg -n -C2 'void onConnect\(NimBLEL2CAPChannel\* channel\)' examples/L2CAP/L2CAP_Server/main/main.cppRepository: h2zero/esp-nimble-cpp
Length of output: 535
Arr, fix the onConnect override signature, ye scallywag!
The base class in src/NimBLEL2CAPChannel.h declares onConnect(NimBLEL2CAPChannel* channel, uint16_t negotiatedMTU), but yer implementation at lines 39-49 only takes (NimBLEL2CAPChannel* channel). Without the negotiatedMTU parameter, this don't override the base hook at all—the connection-state initialization code stays dead, and connected never flips true when a client dares to connect to yer vessel.
🧰 Tools
🪛 Cppcheck (2.20.0)
[error] 43-43: failed to evaluate #if condition, undefined function-like macro invocation
(syntaxError)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/L2CAP/L2CAP_Server/main/main.cpp` around lines 39 - 49, The
onConnect implementation does not match the base hook signature so it doesn't
override; change the method signature for onConnect in this file to match the
base declaration: onConnect(NimBLEL2CAPChannel* channel, uint16_t negotiatedMTU)
(and add/keep the override specifier if present). Update the function header to
accept the negotiatedMTU parameter (use or explicitly ignore it) so the
connection-state initialization (connected, totalBytesReceived, startTime,
buffer.clear(), etc.) runs when the base calls onConnect.
Comment on lines
+61
to
+70
| while (buffer.size() >= 3) { // Minimum frame size: seqno(1) + len(2) | ||
| // Parse frame header | ||
| uint8_t seqno = buffer[0]; | ||
| uint16_t payloadLen = (buffer[1] << 8) | buffer[2]; // Big-endian | ||
|
|
||
| size_t frameSize = 3 + payloadLen; | ||
|
|
||
| // Check if we have complete frame | ||
| if (buffer.size() < frameSize) { | ||
| break; // Wait for more data |
There was a problem hiding this comment.
Reject bogus frame lengths before ye buffer them.
payloadLen is trusted blindly here. A malformed header can advertise a frame far larger than this example intends to handle, and buffer will just keep growing while the server waits for bytes that should never arrive. This is also the natural place to increment frameErrors and tear down or resync the session.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/L2CAP/L2CAP_Server/main/main.cpp` around lines 61 - 70, Reject
excessively large or invalid advertised payload lengths instead of trusting
payloadLen: define a reasonable MAX_PAYLOAD_SIZE and, after computing payloadLen
and frameSize, validate payloadLen <= MAX_PAYLOAD_SIZE and frameSize does not
exceed that limit or a total-buffer cap; if the check fails increment
frameErrors and tear down or resync the session (e.g., close the connection or
drop enough bytes to find the next valid header) to avoid unbounded buffering.
Apply this logic around the existing variables buffer, payloadLen, frameSize and
update frameErrors where frames are rejected.
Comment on lines
+160
to
+200
| if (l2capChannelCallbacks->connected && l2capChannelCallbacks->totalBytesReceived > 0) { | ||
| uint64_t currentTime = esp_timer_get_time(); | ||
| double elapsedSeconds = (currentTime - l2capChannelCallbacks->startTime) / 1000000.0; | ||
|
|
||
| if (elapsedSeconds > 0) { | ||
| double bytesPerSecond = l2capChannelCallbacks->totalBytesReceived / elapsedSeconds; | ||
| double framesPerSecond = l2capChannelCallbacks->totalFramesReceived / elapsedSeconds; | ||
|
|
||
| // Heap monitoring | ||
| size_t currentHeap = esp_get_free_heap_size(); | ||
| size_t minHeap = esp_get_minimum_free_heap_size(); | ||
|
|
||
| // Track heap for leak detection | ||
| if (initialHeap == 0) { | ||
| initialHeap = currentHeap; | ||
| lastHeap = currentHeap; | ||
| } | ||
|
|
||
| // Check for consistent heap decrease | ||
| if (currentHeap < lastHeap) { | ||
| heapDecreaseCount++; | ||
| if (heapDecreaseCount >= HEAP_LEAK_THRESHOLD) { | ||
| printf("\n⚠️ WARNING: POSSIBLE MEMORY LEAK DETECTED! ⚠️\n"); | ||
| printf("Heap has decreased %zu times in a row\n", heapDecreaseCount); | ||
| printf("Initial heap: %zu, Current heap: %zu, Lost: %zu bytes\n", | ||
| initialHeap, currentHeap, initialHeap - currentHeap); | ||
| } | ||
| } else if (currentHeap >= lastHeap) { | ||
| heapDecreaseCount = 0; // Reset counter if heap stabilizes or increases | ||
| } | ||
| lastHeap = currentHeap; | ||
|
|
||
| printf("\n=== STATUS UPDATE ===\n"); | ||
| printf("Frames received: %zu (%.1f fps)\n", l2capChannelCallbacks->totalFramesReceived, framesPerSecond); | ||
| printf("Total bytes: %zu\n", l2capChannelCallbacks->totalBytesReceived); | ||
| printf("Payload bytes: %zu\n", l2capChannelCallbacks->totalPayloadBytes); | ||
| printf("Bandwidth: %.2f KB/s (%.2f Mbps)\n", bytesPerSecond / 1024.0, (bytesPerSecond * 8) / 1000000.0); | ||
| printf("Sequence errors: %zu\n", l2capChannelCallbacks->sequenceErrors); | ||
| printf("Heap: %zu free (min: %zu), Used since start: %zu\n", | ||
| currentHeap, minHeap, initialHeap > 0 ? initialHeap - currentHeap : 0); | ||
| printf("==================\n"); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'context of the NimBLE bluetooth task' src/NimBLEL2CAPChannel.h
rg -n -C2 'connected = true|totalBytesReceived \+=|totalFramesReceived\+\+|startTime =|connected &&|totalBytesReceived > 0' examples/L2CAP/L2CAP_Server/main/main.cppRepository: h2zero/esp-nimble-cpp
Length of output: 1990
Arr, shiver me timbers! Guard these stats across tasks, matey.
The docs in src/NimBLEL2CAPChannel.h (line 46) say the channel callbacks run on nimble_host, but this loop be readin' connected, totalBytesReceived, totalFramesReceived, startTime, and friends from the app task with nary a lock in sight. That be makin' the numbers racy as Davy Jones' locker, and uint64_t startTime can be tearin' asunder on 32-bit targets where the whole thing don't load in one instruction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/L2CAP/L2CAP_Server/main/main.cpp` around lines 160 - 200, The
status-report loop is reading shared fields on l2capChannelCallbacks (connected,
totalBytesReceived, totalFramesReceived, startTime, totalPayloadBytes,
sequenceErrors) without synchronization, causing data races and possible 64-bit
tearing on 32-bit targets; fix by protecting access with a lock (e.g.,
create/declare a mutex or spinlock like l2capChannelMutex) or by entering a
critical section around reads and copying the needed fields into local
variables/struct before computing elapsedSeconds and printing, and ensure every
writer to l2capChannelCallbacks uses the same mutex/critical section so
reads/writes are synchronized and the uint64_t startTime is always read
atomically.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
examples/L2CAP/L2CAP_Client/main/main.cpp (2)
216-219:⚠️ Potential issue | 🟠 MajorDon’t sink the ship with
abort()on send failure, matey.Line 218 kills the whole app on a routine send miss; better unwind and let reconnect flow handle it.
🏴☠️ Proposed fix
} else { printf("failed to send!\n"); - abort(); + vTaskDelay(1000 / portTICK_PERIOD_MS); + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/L2CAP/L2CAP_Client/main/main.cpp` around lines 216 - 219, The else branch that currently calls abort() on a failed send in main() should be changed to gracefully handle the send error: remove abort(), log the failure (keep the printf or use the existing logger), set/notify the connection state so the existing reconnect logic can run (e.g., set a should_reconnect/connected flag or call the module's connection teardown helper), and return/continue from the send routine or propagate an error code so higher-level code can attempt reconnect; locate the send-result check that currently reads "printf(\"failed to send!\\n\"); abort();" and replace abort() with a non-fatal error path that triggers the reconnect flow.
83-126:⚠️ Potential issue | 🟠 MajorGuard yer shared stats with a mutex, matey.
Line 83 and Line 187 touch the same globals from different tasks without protection. That be a race (and Line 19
uint64_t startTimebe extra risky on 32-bit targets).🏴☠️ Proposed fix
+SemaphoreHandle_t statsMutex = nullptr; + void statusTask(void *pvParameters) { while (true) { vTaskDelay(1000 / portTICK_PERIOD_MS); - - if (startTime > 0 && blocksSent > 0) { + size_t localBytesSent = 0; + size_t localPayloadSize = 0; + uint32_t localBlocksSent = 0; + uint64_t localStartTime = 0; + if (xSemaphoreTake(statsMutex, portMAX_DELAY) == pdTRUE) { + localBytesSent = bytesSent; + localPayloadSize = currentPayloadSize; + localBlocksSent = blocksSent; + localStartTime = startTime; + xSemaphoreGive(statsMutex); + } + if (localStartTime > 0 && localBlocksSent > 0) { uint64_t currentTime = esp_timer_get_time(); - double elapsedSeconds = (currentTime - startTime) / 1000000.0; + double elapsedSeconds = (currentTime - localStartTime) / 1000000.0; ... - bytesPerSecond = bytesSent / elapsedSeconds; + bytesPerSecond = localBytesSent / elapsedSeconds; ... - printf("Blocks sent: %lu\n", (unsigned long)blocksSent); - printf("Total bytes sent: %zu\n", bytesSent); - printf("Current payload size: %zu bytes\n", currentPayloadSize); + printf("Blocks sent: %lu\n", (unsigned long)localBlocksSent); + printf("Total bytes sent: %zu\n", localBytesSent); + printf("Current payload size: %zu bytes\n", localPayloadSize); } } } if (theChannel->write(packet)) { - if (startTime == 0) { - startTime = esp_timer_get_time(); - } - bytesSent += packet.size(); - blocksSent++; + if (xSemaphoreTake(statsMutex, portMAX_DELAY) == pdTRUE) { + if (startTime == 0) { + startTime = esp_timer_get_time(); + } + bytesSent += packet.size(); + blocksSent++; + ... + xSemaphoreGive(statsMutex); + } ... } void app_main(void) { + statsMutex = xSemaphoreCreateMutex(); + assert(statsMutex != nullptr); xTaskCreate(connectTask, "connectTask", 5000, NULL, 1, NULL); xTaskCreate(statusTask, "statusTask", 3000, NULL, 1, NULL);Also applies to: 187-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/L2CAP/L2CAP_Client/main/main.cpp` around lines 83 - 126, The status code reads and writes shared globals (startTime, blocksSent, bytesSent, currentPayloadSize, initialHeap, lastHeap, heapDecreaseCount, HEAP_LEAK_THRESHOLD) from multiple tasks and must be serialized to avoid races; create a global mutex (e.g., statsMutex) and wrap every access/update to those symbols (both in the status-print block and the other task(s) around line ~187) with mutex take/give. Also change startTime from uint64_t to a safe signed 64-bit type (e.g., int64_t) or use an atomic type to avoid tearing on 32-bit targets, and ensure any increments/reads of counters (bytesSent, blocksSent, heapDecreaseCount) are done while holding the mutex (or use atomics consistently) so the leak-detection logic (initialHeap/lastHeap/heapDecreaseCount vs HEAP_LEAK_THRESHOLD) sees consistent values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/L2CAP/L2CAP_Client/main/main.cpp`:
- Around line 216-219: The else branch that currently calls abort() on a failed
send in main() should be changed to gracefully handle the send error: remove
abort(), log the failure (keep the printf or use the existing logger),
set/notify the connection state so the existing reconnect logic can run (e.g.,
set a should_reconnect/connected flag or call the module's connection teardown
helper), and return/continue from the send routine or propagate an error code so
higher-level code can attempt reconnect; locate the send-result check that
currently reads "printf(\"failed to send!\\n\"); abort();" and replace abort()
with a non-fatal error path that triggers the reconnect flow.
- Around line 83-126: The status code reads and writes shared globals
(startTime, blocksSent, bytesSent, currentPayloadSize, initialHeap, lastHeap,
heapDecreaseCount, HEAP_LEAK_THRESHOLD) from multiple tasks and must be
serialized to avoid races; create a global mutex (e.g., statsMutex) and wrap
every access/update to those symbols (both in the status-print block and the
other task(s) around line ~187) with mutex take/give. Also change startTime from
uint64_t to a safe signed 64-bit type (e.g., int64_t) or use an atomic type to
avoid tearing on 32-bit targets, and ensure any increments/reads of counters
(bytesSent, blocksSent, heapDecreaseCount) are done while holding the mutex (or
use atomics consistently) so the leak-detection logic
(initialHeap/lastHeap/heapDecreaseCount vs HEAP_LEAK_THRESHOLD) sees consistent
values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fb18b49-9ecf-47aa-8f42-3b6d42740c89
📒 Files selected for processing (1)
examples/L2CAP/L2CAP_Client/main/main.cpp
Contributor
Author
|
Anything holding this back? The remaining copilot hints are at the (completely not production ready) testing tools which are subject to another PR. |
Owner
|
Nothing blocking this, just wasn't sure if you were done with it. Could you squash these commits please? |
- Add NimBLEL2CAPChannel::disconnect() and getConnHandle(). - Fix CoC TX mbuf ownership handling in writeFragment(): treat BLE_HS_ENOMEM / BLE_HS_EAGAIN as consumed buffer, only free local tx mbuf on BLE_HS_EBUSY. - Refresh L2CAP client/server examples for stress testing and runtime stats. - Pin example dependency mickeyl/esp-hpl to tag 1.1.0. - Clean trailing whitespace in updated example sources. Closes h2zero#391
There was a problem hiding this comment.
♻️ Duplicate comments (5)
examples/L2CAP/L2CAP_Server/main/main.cpp (3)
160-200:⚠️ Potential issue | 🟠 MajorSnapshot the callback stats under a lock, matey.
Once the connect hook above actually fires, this loop reads
connected,totalBytesReceived,totalFramesReceived,totalPayloadBytes,sequenceErrors, and the 64-bitstartTimewhile the L2CAP callbacks write them on another path. That be a plain data race, andstartTimecan tear on 32-bit ESP targets. Copy the fields under the same mutex or critical section used by the callbacks before computing the report.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/L2CAP/L2CAP_Server/main/main.cpp` around lines 160 - 200, The status-report code reads shared fields (l2capChannelCallbacks->connected, totalBytesReceived, totalFramesReceived, totalPayloadBytes, sequenceErrors, startTime) while callbacks may write them, causing a data race and potential 32-bit tear on startTime; fix by taking the same mutex/critical section used by the L2CAP callbacks, copy those fields into local variables (snapshot) while holding the lock, release the lock, then perform elapsedSeconds, bandwidth and heap calculations and printf using the locals; ensure you reference the same lock/critical API used by the callbacks so synchronization is consistent with the writers.
39-49:⚠️ Potential issue | 🟠 MajorFix the
onConnectoverride signature, matey.
NimBLEL2CAPChannel::handleConnectionEvent()callscallbacks->onConnect(this, mtu), so this one-argument method ne'er overrides the base hook.connectedstays false, the counters are never reset on connect, and the status loop below never reports a live session.🛠️ Proposed fix
- void onConnect(NimBLEL2CAPChannel* channel) { + void onConnect(NimBLEL2CAPChannel* channel, uint16_t negotiatedMTU) override { + (void)negotiatedMTU; printf("L2CAP connection established on PSM %d\n", L2CAP_PSM); connected = true;Run this to confirm the two-argument callback contract and the one-argument method here; expect the base declaration/call site to take
uint16_t, while this example does not.#!/bin/bash rg -n -C2 'onConnect\(NimBLEL2CAPChannel\*.*uint16_t' src/NimBLEL2CAPChannel.h src/NimBLEL2CAPChannel.cpp rg -n -C2 'void onConnect\(NimBLEL2CAPChannel\* channel\)' examples/L2CAP/L2CAP_Server/main/main.cpp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/L2CAP/L2CAP_Server/main/main.cpp` around lines 39 - 49, The onConnect handler in main.cpp currently has the wrong signature (onConnect(NimBLEL2CAPChannel* channel)) and therefore does not override the base callback; change it to match the declared callback: onConnect(NimBLEL2CAPChannel* channel, uint16_t mtu) (and add override) so the base call to callbacks->onConnect(this, mtu) invokes your method; inside the corrected onConnect(NimBLEL2CAPChannel*, uint16_t) keep the existing initialization of connected, totalBytesReceived, totalFramesReceived, totalPayloadBytes, expectedSequenceNumber, sequenceErrors, frameErrors, startTime and buffer.clear(), and use the mtu parameter if you need to record or log the negotiated MTU.
61-70:⚠️ Potential issue | 🟠 MajorReject impossible payload lengths afore ye stash them.
payloadLencomes straight off the wire. Anything aboveL2CAP_MTU - 3can never become a valid frame on this channel, so the buffer just keeps swelling andframeErrorsnever moves. Cap it as soon as the header is parsed and drop/reset the buffered stream on bogus lengths.🧰 Proposed fix
uint16_t payloadLen = (buffer[1] << 8) | buffer[2]; // Big-endian - size_t frameSize = 3 + payloadLen; + if (payloadLen > L2CAP_MTU - 3) { + frameErrors++; + printf("Invalid frame length %u, dropping buffered data\n", payloadLen); + buffer.clear(); + return; + } // Check if we have complete frame🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/L2CAP/L2CAP_Server/main/main.cpp` around lines 61 - 70, When parsing the frame header in the receive loop, validate payloadLen against the channel maximum (L2CAP_MTU - 3) immediately after computing it; if payloadLen is greater than that cap treat the header as invalid, drop or reset the buffered stream (clear buffer or advance past the bad header), increment frameErrors, and log/handle the error instead of waiting for more data. Update the code around the buffer parsing (the loop that reads seqno, payloadLen and computes frameSize) to perform this bounds check and early discard so impossible lengths cannot cause the buffer to grow forever.examples/L2CAP/L2CAP_Client/main/main.cpp (2)
216-218:⚠️ Potential issue | 🟠 MajorDon’t scuttle the whole demo on a routine send failure.
write()already reports disconnects and exhausted retries withfalse.abort()here crashes the stress run and throws away the very telemetry this example is meant to gather.🏴☠️ Proposed fix
} else { - printf("failed to send!\n"); - abort(); + printf("failed to send, backing off\n"); + vTaskDelay(1000 / portTICK_PERIOD_MS); + break; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/L2CAP/L2CAP_Client/main/main.cpp` around lines 216 - 218, The demo currently calls abort() on a routine send failure (in the else branch following write()/send), which crashes the stress run and loses telemetry; replace abort() with non-fatal handling: log the error (keep or expand the printf), update or increment a send-failure counter/state, and continue or gracefully close the connection instead of aborting so the example can keep running and collecting metrics; locate the else branch that prints "failed to send!" (the write()/send call and its immediate else) and change abort() to a graceful error path that records the failure and proceeds without terminating the process.
79-128:⚠️ Potential issue | 🔴 CriticalGuard this shared client state, matey.
statusTaskreadsbytesSent,blocksSent,currentPayloadSize, and the 64-bitstartTimewhileconnectTaskupdates them, and the same unsynchronized globals letonDisconnect()cleartheChannelbetween the loop check and Line 187. That can both print torn stats and turntheChannel->write(packet)into a null dereference. Put the channel pointer and counters behind one mutex/critical section, or hand disconnect notifications back toconnectTaskthrough a task-safe queue or flag.Also applies to: 170-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/L2CAP/L2CAP_Client/main/main.cpp` around lines 79 - 128, statusTask reads globals (bytesSent, blocksSent, currentPayloadSize, startTime) and theChannel while connectTask/onDisconnect update them, causing torn reads and potential null deref; protect shared state by introducing a FreeRTOS mutex (e.g., statsMutex) and use it to guard all accesses to bytesSent, blocksSent, currentPayloadSize, startTime and theChannel: take the mutex in statusTask before reading these variables and release it after printing, and also take the same mutex in connectTask when updating counters/currentPayloadSize/startTime and in onDisconnect when clearing theChannel; alternatively use a task-safe queue or atomic flag to hand disconnect notifications to connectTask instead of clearing theChannel from onDisconnect.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/L2CAP/L2CAP_Client/main/main.cpp`:
- Around line 216-218: The demo currently calls abort() on a routine send
failure (in the else branch following write()/send), which crashes the stress
run and loses telemetry; replace abort() with non-fatal handling: log the error
(keep or expand the printf), update or increment a send-failure counter/state,
and continue or gracefully close the connection instead of aborting so the
example can keep running and collecting metrics; locate the else branch that
prints "failed to send!" (the write()/send call and its immediate else) and
change abort() to a graceful error path that records the failure and proceeds
without terminating the process.
- Around line 79-128: statusTask reads globals (bytesSent, blocksSent,
currentPayloadSize, startTime) and theChannel while connectTask/onDisconnect
update them, causing torn reads and potential null deref; protect shared state
by introducing a FreeRTOS mutex (e.g., statsMutex) and use it to guard all
accesses to bytesSent, blocksSent, currentPayloadSize, startTime and theChannel:
take the mutex in statusTask before reading these variables and release it after
printing, and also take the same mutex in connectTask when updating
counters/currentPayloadSize/startTime and in onDisconnect when clearing
theChannel; alternatively use a task-safe queue or atomic flag to hand
disconnect notifications to connectTask instead of clearing theChannel from
onDisconnect.
In `@examples/L2CAP/L2CAP_Server/main/main.cpp`:
- Around line 160-200: The status-report code reads shared fields
(l2capChannelCallbacks->connected, totalBytesReceived, totalFramesReceived,
totalPayloadBytes, sequenceErrors, startTime) while callbacks may write them,
causing a data race and potential 32-bit tear on startTime; fix by taking the
same mutex/critical section used by the L2CAP callbacks, copy those fields into
local variables (snapshot) while holding the lock, release the lock, then
perform elapsedSeconds, bandwidth and heap calculations and printf using the
locals; ensure you reference the same lock/critical API used by the callbacks so
synchronization is consistent with the writers.
- Around line 39-49: The onConnect handler in main.cpp currently has the wrong
signature (onConnect(NimBLEL2CAPChannel* channel)) and therefore does not
override the base callback; change it to match the declared callback:
onConnect(NimBLEL2CAPChannel* channel, uint16_t mtu) (and add override) so the
base call to callbacks->onConnect(this, mtu) invokes your method; inside the
corrected onConnect(NimBLEL2CAPChannel*, uint16_t) keep the existing
initialization of connected, totalBytesReceived, totalFramesReceived,
totalPayloadBytes, expectedSequenceNumber, sequenceErrors, frameErrors,
startTime and buffer.clear(), and use the mtu parameter if you need to record or
log the negotiated MTU.
- Around line 61-70: When parsing the frame header in the receive loop, validate
payloadLen against the channel maximum (L2CAP_MTU - 3) immediately after
computing it; if payloadLen is greater than that cap treat the header as
invalid, drop or reset the buffered stream (clear buffer or advance past the bad
header), increment frameErrors, and log/handle the error instead of waiting for
more data. Update the code around the buffer parsing (the loop that reads seqno,
payloadLen and computes frameSize) to perform this bounds check and early
discard so impossible lengths cannot cause the buffer to grow forever.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52b98978-f6b2-4917-a347-b242ecd96962
📒 Files selected for processing (6)
examples/L2CAP/L2CAP_Client/main/idf_component.ymlexamples/L2CAP/L2CAP_Client/main/main.cppexamples/L2CAP/L2CAP_Server/main/idf_component.ymlexamples/L2CAP/L2CAP_Server/main/main.cppsrc/NimBLEL2CAPChannel.cppsrc/NimBLEL2CAPChannel.h
✅ Files skipped from review due to trivial changes (2)
- examples/L2CAP/L2CAP_Server/main/idf_component.yml
- examples/L2CAP/L2CAP_Client/main/idf_component.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NimBLEL2CAPChannel.h
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.
This PR bundles the recent L2CAP CoC work from
l2cap-updateson top of release2.3.4.What is in here
NimBLEL2CAPChannel::disconnect().NimBLEL2CAPChannel::getConnHandle().writeFragment():BLE_HS_ENOMEM/BLE_HS_EAGAINas consumed buffer,BLE_HS_EBUSY.mickeyl/esp-hplto tag1.1.0.Verification
L2CAP_Clientbuilds.L2CAP_Serverbuilds.Important caveat
For heavy CoC stress scenarios, some ESP-IDF NimBLE host revisions may still require a host-side fix in
ble_l2cap_coc.c(RX SDU ring handling): clearrx->sdus[current_sdu_idx] = NULLbefore advancing the index after handing a completed SDU to the app.This PR keeps esp-nimble-cpp-side ownership handling correct; host-side behavior depends on the NimBLE revision shipped with ESP-IDF.
Summary by CodeRabbit
New Features
Bug Fixes
Chores