NimBLE peripheral shim: run firmware BLE code in the simulator - #36
NimBLE peripheral shim: run firmware BLE code in the simulator#36rfordinal wants to merge 13 commits into
Conversation
SimBleLink.h is the interface between the socket transport and the GATT model. Both halves are built in parallel against it, so it is frozen before either starts. docs/ble-shim.md seeds the wire protocol, the threading model and the four fidelity items the shim has to reproduce.
SimBleLink.cpp is the socket half of the frozen seam: a loopback TCP
listener, one reader thread that lives in poll(), line framing over
recv boundaries, and a thread safe emit(). SimBleProtocol.{h,cpp} is
the hand rolled JSON codec for the ten client ops. No JSON library is
added: the ops are flat and this branch must not grow a dependency.
Transport decisions worth naming:
- The listener binds 127.0.0.1, never INADDR_ANY. This process runs
firmware command handling.
- stop() wakes the reader with a self pipe byte. shutdown() on a
listening socket does not portably wake accept(), and closing a fd
another thread is polling races a new socket onto that number.
- One line caps at 65536 bytes. Past it the buffer is dropped, one
error event goes out, and bytes up to the next newline are discarded.
- The reader alone closes the client fd. emit() shuts it down on a
write failure so teardown stays in one place, and carries a 5 s send
timeout so a wedged client cannot hang a firmware thread.
- A lost socket synthesizes a disconnect op, so the GATT model does not
keep believing a central is connected.
tests/sim_ble_link_selftest.{cpp,py} is the gate. It builds against
those two sources alone, so the transport is provable before the GATT
model exists: 65 checks covering every op explicit and defaulted, 20
malformed lines, the cap, the second client refusal, prompt stop() and
concurrent emit. Clean under ASan+UBSan and under TSan.
Adds a "Transport specifics" section with the settled numbers: the 65536 byte line cap and what a longer line does, how stop() wakes a blocked reader and why the self pipe beats the alternatives, the loopback only bind and its reason, the second client refusal, the malformed line answer, emit()'s line atomicity, the synthetic disconnect on socket loss, and the full default and range table for every op field. Flips those claims to [verified] and names the command that showed them. The GATT half stays [contract]. Every claim carries a file:line.
Header-compatible fake for NimBLE-Arduino's peripheral API, so the firmware's BLE code runs on the host. No NimBLE source is compiled. The NimBLE* classes forward; SimBleGatt holds the model, the host thread, the single per-connection indication slot and the outgoing event JSON. Callbacks are dispatched on the host thread, never inline on the caller's, because the firmware is built around the disconnect callback and the sync event it waits for sharing one task. Three fidelity points the shim reproduces rather than smooths over: the indication confirm is out of band and can be withheld, a second indicate() before a confirm clobbers the first and still returns true, and the client owns the MTU (default 23). Self-test: 40 checks, 0 failures, clean under ThreadSanitizer. The real firmware translation unit compiles against these headers.
Flips the threading model, the four fidelity items and the enforced refusals from contract to verified, each naming the self-test check that showed it. Adds what building it turned up: two NimBLE calls the frozen contract list missed (NimBLEServer::start and setCallbacks' second argument), the indication slot being per connection rather than per characteristic, and two FreeRTOS symbols the simulator still lacks that the firmware BLE file needs. Extends "What it cannot answer" with the one refusal this shim cannot produce: indicate() never returns false for a busy slot, so the firmware's park-and-flush path is not exercised here.
The self-test defined a second main() and a second SimBleLink under src/. library.json has no srcFilter, so both landed in the library archive next to simulator_main.o and the linker satisfied main() from the test: the simulator built clean, never ran, printed one line from a global constructor and aborted in a destructor of code it had never entered. Moved to tests/, which is where the transport self-test already lives and which is why only this half broke. No srcFilter: an allowlist has to be remembered when the next file appears, a directory that is not compiled does not. The test now ends with fflush(stdout) and _exit() instead of returning, so its verdict cannot be lost to someone else's teardown.
NimBLEDevice::init() emits stack/up, and init() is also what starts the listener, so no client can be attached when that line is written and it is dropped. The event was never racy, it was structurally undeliverable. The gatt table and the first advertising line have the same problem whenever the firmware builds them before a client turns up, which is the normal case. A real central does not have this problem: it scans. The accept path now synthesizes an `attach` op into the sink, the mirror of the synthetic disconnect a lost socket already produces, and the GATT model answers it by emitting what is currently true: stack up, the gatt table if one is built, then the current advertising state. Not a real BLE event, same status as clobber. No change to the frozen SimBleLink.h: the op name is a string. Nothing is replayed about a live connection. A client that was not there for the connect is not the central that made it. The transport gate's two connect-time baselines were snapshotted before the reader had accepted, so they measured the next op against the attach line and failed intermittently. They now wait for the attach op and assert it arrives, which adds a check rather than removing one. 65/65 again, plain, ASan/UBSan and TSan. tests/sim_ble_gatt_attach_selftest.cpp proves the replay over a real socket: it builds a table with nobody connected, connects to itself and reads the three lines. 9 checks.
Records the state replay as its own section, labelled not-a-real-BLE-event the same way clobber is, so a client author reading a packet trace does not mistake it for something a phone would see. Says what is not replayed and what a client that attaches before init() gets. Rewrites the self-test commands for the new tests/ paths and adds the socket test. States why test files live in tests/ rather than behind a srcFilter. Re-checks every file:line citation in the file. The accept-path insertion moved 21 lines of src/SimBleLink.cpp, which stale-dated eleven citations in the transport section, and the GATT edits moved eighteen of mine. One citation was already off by one before this pass.
The bullet claiming the simulator lacked pdMS_TO_TICKS and xSemaphoreCreateBinary, and that the firmware BLE file therefore did not build in-tree, is false on all three counts now. Deleted rather than softened. The firmware side documents its own fix; a second account of it here would drift. The same claim was embedded in the firmware syntax-check recipe as a placeholder -include line. Removed. The recipe now says only what it is for: anything the -include flags miss surfaces as a FreeRTOS name, not a NimBLE one. The stronger evidence replaces it: the whole firmware builds against the shim with no NimBLE symbol missing. Three more stale facts found by auditing content rather than line numbers: - The status line still called the file a seed written before the implementation, contradicting its own next sentence. Both halves are built. - Fault injection was marked all contract. Withholding a confirm and sending a malformed frame are both demonstrated now; the other two need a transfer, so they need the firmware. - The busy-slot bullet listed its refusals as if exhaustive and omitted the stack being down. The heap, radio, peer-GATT-stack and busy-slot bullets are unchanged in substance. All 54 citations still resolve.
The shim's own doc was written from its self-tests. Real firmware has now been driven through it: pin the gatt event's props type on the wire (the one field nobody typed, and it broke every client), record what the socket structurally cannot answer (throughput, MTU negotiation, an over-MTU refusal, radio loss, a hole from a withheld confirm, storage failures), note that auto_confirm survives a client disconnect, add speed as a fifth fidelity hazard in both directions, and promote the four fault-injection cases that needed firmware from contract to verified. Also: a grep over call syntax cannot detect a stale API contract, and six firmware citations had drifted.
NIMBLE_PROPERTY::READ was 0x0001. That is BLE_GATT_CHR_F_BROADCAST. Read is 0x0002 (nimble/host/include/host/ble_gatt.h:133), and standard GATT agrees, so it was not a NimBLE quirk being matched. Harmless today and wrong for upstream. No firmware characteristic sets READ, so nothing observable rode on it and no test could have caught it. But the property bitmask now goes out on the wire as an integer and a client decodes it back into names, so a readable characteristic would have made the shim say "broadcast" where the device meant "read". WRITE, NOTIFY and INDICATE were already right: 0x0008, 0x0010 and 0x0020, at ble_gatt.h:139, :142 and :145. Each constant now carries its source line, and the block says why 0x0001 is not read. WRITE_NO_RSP and the four flags above INDICATE stay undeclared: the firmware does not use them. The doc's props decoding table said `1` read, propagated from the same mistake. Corrected, and it now names the header to check rather than asking the reader to trust the table.
The shim returned false from indicate() when nothing was connected, when nobody was subscribed, and when the characteristic lacked NOTIFY/INDICATE, under a comment claiming those were refusals the real stack makes. It makes none of them. The firmware calls the two-argument indicate(), so connHandle defaults to BLE_HS_CONN_HANDLE_NONE (NimBLECharacteristic.h:60) and the call lands in sendValue (NimBLECharacteristic.cpp:272-328). That function has no connection check, no CCCD check and no property check: rc starts at 0, only the peer loop can move it, and with no peers the loop body never runs. It returns true. ble_gatts_indicate_custom refuses only on out of memory or, under BLE_GATT_CACHING, an unaware peer (ble_gattc.c:4888-4936), so an unsubscribed peer gets a real indication PDU and the pending slot is genuinely taken. Measured against the real firmware with a connected-but-unsubscribed peer: false: reply indicate failed after 40 attempts ~1008 ms, flag never set true: reply unconfirmed after 3000 ms 3000 ms, flag set Different duration, different branch, different log line, and different persistent state: only the second sets lastConfirmTimedOut_, which is what suppresses FETCH_CANCEL downstream. The simulator was sending developers into code the device never reaches. Nothing connected returns true and leaves the slot alone, emitting nothing: real NimBLE builds and sends nothing there, and filling the slot would let a later connect-then-confirm confirm a payload from before the link existed. A connected but unsubscribed peer takes the slot and gets the indicate event, because the PDU really does go out -- but no auto-confirm is scheduled, since a client with the CCCD off has nothing to confirm. That is what reproduces the firmware's confirm timeout. Two more corrections found in the same read. An empty payload is a different operation in real NimBLE, not a refusal: it falls through to ble_gatts_chr_updated and still returns true. The stack-down and null-pointer guards stay false and are now labelled as shim guards, because real NimBLE dereferences a null server there rather than returning anything. Clobber semantics are untouched: a second indicate() before a confirm still overwrites and still returns true. The two self-test checks that asserted the inversion are inverted rather than deleted, with the consequence added: unsubscribed indicate is true, the event still goes out, and nothing confirms it. The disconnect check that leaned on the old return value now proves the subscription cleared by its consequence -- no confirm on the new link, and a confirm again after re-subscribing. 50 checks.
Nineteen comment and doc references pointed at BlePositionServer.cpp and FREEINK_CAP_BLE_PERIPHERAL -- a file and a build flag that do not exist upstream, which made the branch unreviewable by anyone outside this fork. Each one now states the behaviour instead of the address. Nothing load-bearing was lost: the authority in these comments was always NimBLE's own source (NimBLECharacteristic.cpp, ble_gattc.c, ble_gatt.h), which an upstream reader can check, and the firmware was only ever provenance. The syntax-check recipe keeps its shape with placeholders rather than one fork's paths. Gates unchanged: 50 GATT checks, 9 attach checks, 65 transport checks.
|
Warning Review limit reached
Next review available in: 59 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
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 |
PR 2 of 2 — A NimBLE peripheral shim, so firmware BLE code runs in the simulator
This is the second of two, and it depends on #35 (the FreeRTOS shim). This
branch alone has no
xSemaphoreCreateBinaryand nopdMS_TO_TICKS, so afirmware BLE file does not build against it until that one lands. Merging this
first will look broken and will not be.
Where this comes from
We maintain ExplorInk, a fork of
CrossPoint Reader that turns an Xteink X3/X4 into an offline map and navigation
device, and our fork of this simulator is
explorink-simulator. The
firmware referred to below as "the firmware this was developed against" is that
one, and it is public, so every claim here is checkable rather than asserted.
The code and docs in this PR name none of that on purpose. Anything you
merge you then have to maintain, and a downstream project's file paths and build
flags in your tree would be noise you did not sign up for. So the diff is
generic, the citations point at NimBLE and FreeRTOS rather than at us, and the
provenance lives here in the description where it costs you nothing.
What it is
The simulator gains a shim for the Bluetooth radio: a header-compatible
fake, meaning the same C++ API as NimBLE-Arduino with a different implementation
underneath. No NimBLE source is compiled. A client connects to a TCP socket
on loopback, speaks newline-delimited JSON, and plays the part of the central.
So firmware BLE code runs unchanged on the host. Everything above the radio is
the firmware's own; only the radio is faked.
Off unless asked:
CROSSPOINT_SIM_BLE_PORT=8765. Absent or 0 and the simulatorbehaves exactly as it does today, no listener, no thread.
Why it might interest you rather than just us
Anything in a firmware that talks over BLE is currently untestable in the
simulator, and the shim makes the whole path executable on a laptop: a command
channel, a file push, a request-and-fetch loop. It also buys fault injection
that hardware cannot: withhold an indication confirm, drop the link mid-transfer,
send a malformed frame, subscribe to nothing and then transfer.
The four things it has to get right, or it lies
A polite fake is worse than none, because it hides exactly the bugs the real
stack produces. These are reproduced deliberately:
Inline dispatch makes a whole class of deadlock unreproducible.
indicate()returns when the pending slot accepted the payload, not when the peer got it;
the confirm arrives later through
onStatus.indicate()before a confirm clobbers the first. Measured onreal hardware: back-to-back calls all return true and the peer sees the first
and the last. The shim reproduces the clobber and emits a
clobberevent soit is observable instead of silent.
wrong default tests different arithmetic than a device runs.
One bug worth reading about, because it is the failure mode of this whole idea
indicate()originally returned false when nobody was subscribed, whennothing was connected, and when the characteristic lacked NOTIFY/INDICATE, under
a comment reading "refusals the real stack makes".
Real NimBLE makes none of those three checks.
NimBLECharacteristic::sendValuehas no CCCD check, no property check and no connection check; with the default
connHandleit falls into the peer loop, and an empty peer list leavesrc = 0and returns true. The value came from a comment in the firmware this was
developed against, which is itself wrong, rather than from the library on disk.
The cost, measured with a connected-but-unsubscribed peer: the shim drove the
firmware's 40 x 25 ms retry loop, about 1 s per line, and never set the
firmware's confirm-timed-out flag. Real hardware takes the 3000 ms confirm wait
and does set it. Different duration, different branch, different persistent
state.
No test could have caught it: two self-test checks asserted the inversion as
correct, so the suite agreed with the bug. A test written from the same belief as
the code confirms the belief, not the behaviour. It is fixed, the checks are
inverted rather than deleted, and the story is in
docs/ble-shim.mdbecause itis the most useful thing in there for the next person.
Verification
connect, subscribe, write, indicate, confirm, clobber, disconnect against a
fake sink, and asserts a callback ran on a different thread than the caller.
table is built receives the current state unasked.
explicit and defaulted fields, a second client refused, 20 malformed inputs,
an over-long line, a prompt
stop()with a client connected, and concurrentemit()from two threads arriving unspliced.API surface complete rather than plausible.
PR, it lives downstream) bridges a real BlueZ GATT peripheral to this socket,
so an Android app connected over actual Bluetooth to firmware running as a host
binary: it negotiated MTU 517, subscribed, and its GPS fix reached the firmware
and redrew the map. That is what convinced us the wire contract is right.
Notes for review, including two things you may not want
docs/andtests/.tests/is the one toquestion:
CLAUDE.mdsays this repo has no tests and that a change is testedby running the simulator, so this introduces a convention rather than following
one. We have kept the tests because they are how the bug above was caught and
how the fix is pinned, but we are not attached to the layout. Say the word and
they move, merge into the sample runner, or go.
because a dependency for this would be a poor trade.
INADDR_LOOPBACK, neverINADDR_ANY. The process runsfirmware command handling, so exposing it on a LAN interface would be a real
hazard rather than a style point.
docs/ble-shim.mdrather thanleft to be discovered: heap (the simulator reports a flat figure, and a real
BT host is usually the biggest single RAM consumer), the radio itself, and a
peer's real GATT stack. A python client agreeing with a firmware proves the
firmware self-consistent, not interoperable.
NIMBLE_PROPERTY::READwas0x0001, which is broadcast's bit. Read is0x0002perhost/ble_gatt.h.Nothing observable rode on it, and it would have misled the first reader who
made a characteristic readable.