feat(L2CAP): add disconnect API and harden CoC send/error handling#396
Open
mickeyl wants to merge 8 commits intoh2zero:masterfrom
Open
feat(L2CAP): add disconnect API and harden CoC send/error handling#396mickeyl wants to merge 8 commits intoh2zero:masterfrom
mickeyl wants to merge 8 commits intoh2zero:masterfrom
Conversation
…nitoring
This commit improves the L2CAP testing framework to help debug (suspected)
L2CAP issues in the NimBLE stack. The examples implement a client-server pair
that can stress test L2CAP connections with progressive payload sizes.
Client (L2CAP_Client):
- Connects to devices advertising with name "l2cap"
- Uses PSM 192 for L2CAP connection
- Implements framed protocol: [seqno:8bit][length:16bit BE][payload]
- Starts with 64-byte payloads, doubles every 50 blocks up to 1024 bytes
- Sends data continuously without delays for maximum throughput
- Includes real-time bandwidth and performance monitoring
Server (L2CAP_Server):
- Advertises as "l2cap" to match client scanning
- Listens on PSM 192
- Parses framed protocol with proper buffering for fragmented frames
- Validates sequence numbers and reports errors
- Tracks frames per second, total bytes, and bandwidth
Both examples include:
- Heap memory monitoring with leak detection
- Warns after 10 consecutive heap decreases
- Status updates every second showing:
- Bandwidth in KB/s and Mbps
- Current/minimum heap memory
- Memory used since start
- Frame/block statistics
Testing results:
- Stable operation at ~84 KB/s (0.69 Mbps) with 1024-byte payloads
- Identified crash when attempting 2048-byte payloads (memory allocation issue)
- No memory leaks detected during extended operation
This framework enables systematic debugging of L2CAP implementation issues
by providing detailed metrics and stress testing capabilities.
…l for minimal-impact-debug-logs
Handle ENOMEM/EAGAIN from `ble_l2cap_send` as “mbuf consumed” and only free tx buffers on EBUSY; prevents double-free pool corruption during CoC stress. ESP-IDF NimBLE host change needed separately: In components/bt/host/nimble/nimble/nimble/host/src/ble_l2cap_coc.c, when an SDU completes (RX path), clear rx->sdus[current_sdu_idx] = NULL before advancing the index so cleanup won’t free a buffer already handed to the application.
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? |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
📝 WalkthroughWalkthroughAdds a Git dependency (mickeyl/esp-hpl) to both L2CAP examples, implements a framed L2CAP protocol with dynamic payload sizing and runtime statistics in client/server examples, and exposes two new NimBLEL2CAPChannel APIs (disconnect, getConnHandle) plus improved transient-send error handling. 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\ncreate connectTask & statusTask
Client->>Channel: connect()/open channel (uses L2CAP_PSM)
Channel->>Server: BLE connection established
loop Send framed packets
Client->>Client: build frame (seq + len(2) + payload)
Client->>Channel: write(frame)
alt write success
Channel->>Client: ack success
Client->>Client: increment counters, maybe double 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 to buffer, parse frames, validate seq, update stats
end
Client->>Channel: disconnect()
Channel->>Server: teardown
Server->>Server: print final stats, reset state, restart advertising
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
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 Tip CodeRabbit can generate a title for your PR based on the changes.Add |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
![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
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