diff --git a/.github/workflows/l2cap-tests.yml b/.github/workflows/l2cap-tests.yml new file mode 100644 index 00000000..05e72a35 --- /dev/null +++ b/.github/workflows/l2cap-tests.yml @@ -0,0 +1,22 @@ +name: L2CAP native tests +on: + workflow_dispatch: + pull_request: + push: + branches: [master] +permissions: + contents: read + +jobs: + native: + runs-on: ubuntu-latest + strategy: + matrix: + cxx_flags: ["", "-fsanitize=address,undefined -fno-omit-frame-pointer"] + steps: + - uses: actions/checkout@v5 + with: + persist-credentials: false + - run: cmake -S tests/l2cap -B build/l2cap-tests -DCMAKE_CXX_FLAGS="${{ matrix.cxx_flags }}" + - run: cmake --build build/l2cap-tests + - run: ctest --test-dir build/l2cap-tests --output-on-failure diff --git a/src/NimBLEL2CAPChannel.cpp b/src/NimBLEL2CAPChannel.cpp index 36b740ad..8868ad86 100644 --- a/src/NimBLEL2CAPChannel.cpp +++ b/src/NimBLEL2CAPChannel.cpp @@ -8,6 +8,7 @@ # include "NimBLEClient.h" # include "NimBLELog.h" # include "NimBLEUtils.h" +# include # ifdef USING_NIMBLE_ARDUINO_HEADERS # include "nimble/nimble/host/include/host/ble_gap.h" @@ -21,9 +22,6 @@ // Round-up integer division # define CEIL_DIVIDE(a, b) (((a) + (b) - 1) / (b)) # define ROUND_DIVIDE(a, b) (((a) + (b) / 2) / (b)) -// Retry -constexpr uint32_t RetryTimeout = 50; -constexpr int RetryCounter = 3; NimBLEL2CAPChannel::NimBLEL2CAPChannel(uint16_t psm, uint16_t mtu, NimBLEL2CAPChannelCallbacks* callbacks) : psm(psm), mtu(mtu), callbacks(callbacks) { @@ -105,54 +103,30 @@ int NimBLEL2CAPChannel::writeFragment(std::vector::const_iterator begin return -BLE_HS_EBADDATA; } - auto retries = RetryCounter; - - while (retries--) { - auto txd = os_mbuf_get_pkthdr(&_coc_mbuf_pool, 0); - if (!txd) { - NIMBLE_LOGE(LOG_TAG, "Can't os_mbuf_get_pkthdr."); - return -BLE_HS_ENOMEM; - } - auto append = os_mbuf_append(txd, &(*begin), toSend); - if (append != 0) { - NIMBLE_LOGE(LOG_TAG, "Can't os_mbuf_append: %d", append); - return append; - } + auto* txd = os_mbuf_get_pkthdr(&_coc_mbuf_pool, 0); + if (!txd) { + return BLE_HS_ENOMEM; + } + const int append = os_mbuf_append(txd, &*begin, toSend); + if (append != 0) { + os_mbuf_free_chain(txd); + return append; + } - auto res = ble_l2cap_send(channel, txd); - switch (res) { - case 0: - NIMBLE_LOGD(LOG_TAG, "L2CAP COC 0x%04X sent %d bytes.", this->psm, toSend); - return 0; - - case BLE_HS_ESTALLED: - stalled = true; - NIMBLE_LOGD(LOG_TAG, "L2CAP COC 0x%04X sent %d bytes.", this->psm, toSend); - NIMBLE_LOGW(LOG_TAG, - "ble_l2cap_send returned BLE_HS_ESTALLED. Next send will wait for unstalled event..."); - return 0; - - case BLE_HS_ENOMEM: - case BLE_HS_EAGAIN: - /* ble_l2cap_send already consumed and freed txd on these errors */ - NIMBLE_LOGD(LOG_TAG, "ble_l2cap_send returned %d (consumed buffer). Retrying shortly...", res); - ble_npl_time_delay(ble_npl_time_ms_to_ticks32(RetryTimeout)); - continue; - - case BLE_HS_EBUSY: - /* Channel busy; txd not consumed */ - NIMBLE_LOGD(LOG_TAG, "ble_l2cap_send returned %d (busy). Retrying shortly...", res); - os_mbuf_free_chain(txd); - ble_npl_time_delay(ble_npl_time_ms_to_ticks32(RetryTimeout)); - continue; - - default: - NIMBLE_LOGE(LOG_TAG, "ble_l2cap_send failed: %d", res); - return res; - } + const int rc = ble_l2cap_send(channel, txd); + if (rc == BLE_HS_ESTALLED) { + // NimBLE owns the SDU and will finish it when credits arrive. + stalled = true; + return 0; + } + // These early validation/busy errors retain caller ownership. All other + // native send results consume the SDU, including ENOMEM and EAGAIN. + if (rc == BLE_HS_EBADDATA || rc == BLE_HS_EBUSY) { + os_mbuf_free_chain(txd); } - NIMBLE_LOGE(LOG_TAG, "Retries exhausted, dropping %d bytes to send.", toSend); - return -BLE_HS_EREJECT; + // A native error can follow partial transmission of this SDU. Resubmitting + // it could duplicate bytes; let write() abort the incomplete stream. + return rc; } # if MYNEWT_VAL(BLE_ROLE_CENTRAL) @@ -194,8 +168,9 @@ bool NimBLEL2CAPChannel::write(const std::vector& bytes) { auto start = bytes.begin(); while (start != bytes.end()) { - auto end = start + mtu < bytes.end() ? start + mtu : bytes.end(); - if (writeFragment(start, end) < 0) { + auto end = start + std::min(mtu, bytes.end() - start); + if (writeFragment(start, end) != 0) { + disconnect(); return false; } start = end; diff --git a/tests/l2cap/CMakeLists.txt b/tests/l2cap/CMakeLists.txt new file mode 100644 index 00000000..8bfc65fa --- /dev/null +++ b/tests/l2cap/CMakeLists.txt @@ -0,0 +1,10 @@ +cmake_minimum_required(VERSION 3.20) +project(L2CAPTests LANGUAGES CXX) +enable_testing() +add_executable(l2cap_send_tests SendErrorsTests.cpp ../../src/NimBLEL2CAPChannel.cpp) +target_compile_features(l2cap_send_tests PRIVATE cxx_std_17) +target_include_directories(l2cap_send_tests PRIVATE stubs ../../src) +target_compile_options(l2cap_send_tests PRIVATE -UNDEBUG -include ${CMAKE_CURRENT_SOURCE_DIR}/stubs/Host.h) +foreach(case errors partial ownership append success stalled) + add_test(NAME l2cap_send_${case} COMMAND l2cap_send_tests ${case}) +endforeach() diff --git a/tests/l2cap/README.md b/tests/l2cap/README.md new file mode 100644 index 00000000..50e710d2 --- /dev/null +++ b/tests/l2cap/README.md @@ -0,0 +1,13 @@ +# L2CAP send regression tests + +The tests compile the production `NimBLEL2CAPChannel.cpp` with minimal native +host replacements. They check positive native error propagation, abort after a +partly submitted SDU without replay, caller/native mbuf ownership, append +failure cleanup, successful fragmentation and accepted credit-stalled sends. +They do not simulate RF, controller buffers or concurrency. + +```sh +cmake -S tests/l2cap -B build/l2cap-tests +cmake --build build/l2cap-tests +ctest --test-dir build/l2cap-tests --output-on-failure +``` diff --git a/tests/l2cap/SendErrorsTests.cpp b/tests/l2cap/SendErrorsTests.cpp new file mode 100644 index 00000000..2236d733 --- /dev/null +++ b/tests/l2cap/SendErrorsTests.cpp @@ -0,0 +1,60 @@ +#include "NimBLEL2CAPChannel.h" +#include +#include + +static void require(bool value,const char* message) { if (!value) throw std::runtime_error(message); } +struct Channel : NimBLEL2CAPChannel { + ble_l2cap_chan native; + Channel() : NimBLEL2CAPChannel(129,64,new NimBLEL2CAPChannelCallbacks) { + ble_l2cap_event e{};e.connect.chan=&native;handleConnectionEvent(&e); + } +}; +int main(int argc,char** argv) { + try { + const std::string test=argc>1?argv[1]:"errors"; + int sends=0; + if (test=="errors" || test=="ownership") { + const std::vector codes=test=="errors" ? + std::vector{BLE_HS_ENOMEM,BLE_HS_EAGAIN,BLE_HS_ENOTCONN,BLE_HS_EINVAL} : + std::vector{BLE_HS_EBADDATA,BLE_HS_EBUSY}; + for (int error:codes) { + Channel c;sends=0;hostDisconnects=0; + hostSend=[&](os_mbuf* b) { + ++sends; + if (error!=BLE_HS_EBADDATA && error!=BLE_HS_EBUSY) os_mbuf_free_chain(b); + return error; + }; + require(!c.write(std::vector(20,0x55)),"positive error reported success"); + require(sends==1,"failed SDU was retried or later fragment submitted"); + require(hostDisconnects==1,"failed stream was not disconnected"); + require(liveMbufs==0,"send buffer leaked or freed twice"); + } + } else if (test=="partial") { + Channel c;std::vector delivered; + hostSend=[&](os_mbuf* b) { + ++sends; + const size_t length=sends==1?b->bytes.size():2; + delivered.insert(delivered.end(),b->bytes.begin(),b->bytes.begin()+length); + os_mbuf_free_chain(b);return sends==1?0:BLE_HS_ENOMEM; + }; + require(!c.write(std::vector(24,0x55)),"partially sent stream reported success"); + require(sends==2 && delivered.size()==10,"partial SDU prefix replayed or remaining fragment sent"); + require(hostDisconnects==1 && liveMbufs==0,"partial-send abort/ownership failed"); + } else if (test=="append") { + Channel c;appendError=BLE_HS_ENOMEM; + hostSend=[&](os_mbuf*) { ++sends;return 0; }; + require(!c.write({1,2,3}),"append failure reported success"); + require(sends==0 && liveMbufs==0 && hostDisconnects==1,"append error leaked buffer or submitted data"); + } else if (test=="success") { + Channel c;std::vector received;std::vector input; + for (int i=0;i<20;++i) input.push_back(uint8_t(i)); + hostSend=[&](os_mbuf* b) {++sends;received.insert(received.end(),b->bytes.begin(),b->bytes.end());return os_mbuf_free_chain(b);}; + require(c.write(input) && sends==3 && received==input && liveMbufs==0 && hostDisconnects==0,"valid fragmented write failed"); + } else if (test=="stalled") { + Channel c; + hostSend=[&](os_mbuf* b) {++sends;os_mbuf_free_chain(b);return BLE_HS_ESTALLED;}; + require(c.write({1,2,3}) && sends==1 && liveMbufs==0 && hostDisconnects==0,"accepted stalled SDU incorrectly treated as error"); + } else { throw std::runtime_error("unknown test"); } + std::cout< +#include +#include +#include +#include +#include +#include +#include +struct os_mempool {}; +struct os_mbuf_pool {}; +using os_membuf_t = uint8_t; +struct os_mbuf { std::vector bytes; }; +#define OS_MEMPOOL_SIZE(n,s) ((n)*(s)) +#define OS_MBUF_PKTLEN(m) ((m)->bytes.size()) +inline int liveMbufs=0, appendError=0; +inline int os_mempool_init(os_mempool*,size_t,size_t,void*,const char*) { return 0; } +inline int os_mbuf_pool_init(os_mbuf_pool*,os_mempool*,size_t,size_t) { return 0; } +inline os_mbuf* os_mbuf_get_pkthdr(os_mbuf_pool*,int) { ++liveMbufs; return new os_mbuf; } +inline int os_mbuf_append(os_mbuf* b,const void* p,size_t n) { + if (appendError) return appendError; + auto a=static_cast(p); b->bytes.insert(b->bytes.end(),a,a+n); return 0; +} +inline int os_mbuf_free_chain(os_mbuf* b) { --liveMbufs; delete b; return 0; } +inline int os_mbuf_copydata(os_mbuf* b,size_t off,size_t n,void* p) { memcpy(p,b->bytes.data()+off,n); return 0; } +constexpr int BLE_HS_EAGAIN=1, BLE_HS_ENOMEM=6, BLE_HS_ENOTCONN=7, BLE_HS_EINVAL=3, + BLE_HS_EALREADY=2, BLE_HS_EBADDATA=10, BLE_HS_EBUSY=15, BLE_HS_EREJECT=14, + BLE_HS_ESTALLED=31, BLE_HS_EUNKNOWN=17; +constexpr uint16_t BLE_HS_CONN_HANDLE_NONE=0xffff; +constexpr uint32_t BLE_NPL_TIME_FOREVER=UINT32_MAX; +inline uint32_t ble_npl_time_ms_to_ticks32(uint32_t ms) { return ms; } +inline void ble_npl_time_delay(uint32_t) {} // Retry count, not elapsed radio time, is tested. +struct ble_l2cap_chan {}; +struct ble_l2cap_chan_info { uint16_t peer_coc_mtu=8,our_coc_mtu=8,peer_l2cap_mtu=8,our_l2cap_mtu=8; }; +constexpr int BLE_L2CAP_EVENT_COC_CONNECTED=1,BLE_L2CAP_EVENT_COC_DISCONNECTED=2, + BLE_L2CAP_EVENT_COC_ACCEPT=3,BLE_L2CAP_EVENT_COC_DATA_RECEIVED=4,BLE_L2CAP_EVENT_COC_TX_UNSTALLED=5; +struct ble_l2cap_event { + int type=0; + struct { ble_l2cap_chan* chan; } connect{},accept{},disconnect{}; + struct { os_mbuf* sdu_rx; } receive{}; + struct { int status; ble_l2cap_chan* chan; } tx_unstalled{}; +}; +inline std::function hostSend; +inline int hostDisconnects=0; +inline int ble_l2cap_get_chan_info(ble_l2cap_chan*,ble_l2cap_chan_info* i) { *i={}; return 0; } +inline uint16_t ble_l2cap_get_conn_handle(ble_l2cap_chan*) { return 1; } +inline int ble_l2cap_send(ble_l2cap_chan*,os_mbuf* b) { return hostSend(b); } +inline int ble_l2cap_disconnect(ble_l2cap_chan*) { ++hostDisconnects; return 0; } +inline int ble_l2cap_recv_ready(ble_l2cap_chan*,os_mbuf* b) { return os_mbuf_free_chain(b); } +class NimBLEUtils { + public: + struct TaskData { mutable int m_flags=0; }; + static bool taskWait(const TaskData&,uint32_t) { assert(false && "credit wait not exercised by send error tests"); return false; } + static void taskRelease(const TaskData&,int=0) {} + static const char* returnCodeToString(int) { return "stub"; } +}; diff --git a/tests/l2cap/stubs/host/ble_gap.h b/tests/l2cap/stubs/host/ble_gap.h new file mode 100644 index 00000000..dcdc9c8a --- /dev/null +++ b/tests/l2cap/stubs/host/ble_gap.h @@ -0,0 +1 @@ +#include "Host.h" diff --git a/tests/l2cap/stubs/host/ble_l2cap.h b/tests/l2cap/stubs/host/ble_l2cap.h new file mode 100644 index 00000000..dcdc9c8a --- /dev/null +++ b/tests/l2cap/stubs/host/ble_l2cap.h @@ -0,0 +1 @@ +#include "Host.h" diff --git a/tests/l2cap/stubs/os/os_mbuf.h b/tests/l2cap/stubs/os/os_mbuf.h new file mode 100644 index 00000000..dcdc9c8a --- /dev/null +++ b/tests/l2cap/stubs/os/os_mbuf.h @@ -0,0 +1 @@ +#include "Host.h" diff --git a/tests/l2cap/stubs/syscfg/syscfg.h b/tests/l2cap/stubs/syscfg/syscfg.h new file mode 100644 index 00000000..dcdc9c8a --- /dev/null +++ b/tests/l2cap/stubs/syscfg/syscfg.h @@ -0,0 +1 @@ +#include "Host.h"