From 46f07e9b0be97e488edafa2a859a2f620f96d45d Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:36:33 -0500 Subject: [PATCH] Sanitize the UDP ingress the way the BLE ingress does UdpMulticastHandler clears transport_mechanism, pki_encrypted and public_key on arrival but leaves priority, via_mqtt and tx_after as the sender set them. All three are local-only, and the reasoning the BLE ingress already carries applies unchanged here: multicast carries the full proto, so the sender chooses. priority is the one that bites. It is not in the LoRa header, so fixPriority derives it locally for a radio arrival and nothing on that path can be chosen by a stranger. Over UDP a crafted packet can ask for MAX, which outranks the ceiling fixPriority assigns, and replaceLowerPriorityPacket then evicts one of our own frames to make room for it once perhapsRebroadcast queues it. via_mqtt suppresses our uplink for that packet and tx_after schedules our transmit. None of it needs a key, and multicast reaches anyone on the LAN. develop has the same gap, so this is not something the BLE spike introduced - the spike's own bearer was simply hardened and the shipped one was not. Worth its own PR against develop. Also restores two firmware anchors a comment sweep dropped from the BLE priority guard: perhapsRebroadcast is what copies the crafted value into the TX queue and replaceLowerPriorityPacket is what evicts. Native suite 1430/1430. --- src/mesh/BLEGattMeshHandler.cpp | 12 ++- src/mesh/BLEGattMeshHandler.h | 20 ++--- src/mesh/BLEMeshHandler.cpp | 74 +++++++---------- src/mesh/BLEMeshHandler.h | 91 ++++++++------------- src/mesh/FloodingRouter.cpp | 6 +- src/mesh/MeshTransportBase.cpp | 9 +- src/mesh/MeshTransportBase.h | 62 ++++++-------- src/mesh/udp/UdpMulticastHandler.h | 14 ++-- src/platform/esp32/ESP32BLEMesh.cpp | 39 ++++----- src/platform/esp32/ESP32BLEMesh.h | 12 +-- src/platform/nrf52/NRF52BLEMesh.cpp | 15 ++-- src/platform/nrf52/NRF52BLEMesh.h | 10 +-- test/test_ble_mesh/BleMesh.cpp | 122 ++++++++++++---------------- variants/esp32/esp32-common.ini | 33 ++++---- 14 files changed, 211 insertions(+), 308 deletions(-) diff --git a/src/mesh/BLEGattMeshHandler.cpp b/src/mesh/BLEGattMeshHandler.cpp index e1e922628b..64a02a4e8f 100644 --- a/src/mesh/BLEGattMeshHandler.cpp +++ b/src/mesh/BLEGattMeshHandler.cpp @@ -19,7 +19,7 @@ bool BLEGattMeshHandler::parseFragment(const uint8_t *chunk, size_t len, Fragmen hdr.index = chunk[3]; hdr.total = chunk[4]; // A total of zero describes nothing and an index outside it can never complete; either would sit - // in the table until expiry, which is exactly the buffer a hostile peer wants to fill. + // in the table until expiry, which is the buffer a hostile peer wants to fill. return hdr.total != 0 && hdr.index < hdr.total; } @@ -339,15 +339,14 @@ void BLEGattMeshHandler::deliverToRouter(BLEGattPeerId peer, const uint8_t *data if (!isRunning || !nodeDB || !data) return; - // Validate before relay: nothing is forwarded that did not decode as a whole packet. meshtastic_MeshPacket mp = meshtastic_MeshPacket_init_zero; if (!pb_decode_from_bytes(data, len, &meshtastic_MeshPacket_msg, &mp)) return; if (mp.which_payload_variant != meshtastic_MeshPacket_encrypted_tag) return; - // The same guards the UDP and advertisement transports apply. A spoofed local origin would let a - // peer reach paths that trust isFromUs; an out-of-range hop count is not relayable. + // The same guards the UDP and advertisement transports apply: a spoofed local origin reaches + // paths that trust isFromUs, and an out-of-range hop count is not relayable. if (mp.from == 0) { LOG_WARN("BLE GATT mesh: packet with no sender from peer %u, dropping", peer); return; @@ -366,9 +365,8 @@ void BLEGattMeshHandler::deliverToRouter(BLEGattPeerId peer, const uint8_t *data // or schedule our transmit. mp.via_mqtt = false; mp.tx_after = 0; - // Same reason as the advertisement bearer: priority is not on the LoRa wire, so this is the - // first path that lets a sender pick it, and priority MAX outranks the ACK ceiling fixPriority - // assigns locally. + // priority is not carried in the LoRa header, so here a sender can choose it, and priority MAX + // outranks the ACK ceiling fixPriority assigns locally. mp.priority = meshtastic_MeshPacket_Priority_UNSET; // Authentication metadata is local-only; the Router re-establishes it after a PKI decrypt. diff --git a/src/mesh/BLEGattMeshHandler.h b/src/mesh/BLEGattMeshHandler.h index 43fe5900b3..94588c6cb9 100644 --- a/src/mesh/BLEGattMeshHandler.h +++ b/src/mesh/BLEGattMeshHandler.h @@ -12,8 +12,7 @@ #include // The mesh-peer service a phone connects to. Private UUIDs shared with the client library -// (node-transport-ble-gatt); deliberately not the phone-API service, which a stock app would -// otherwise mistake this node for. +// (node-transport-ble-gatt); deliberately not the phone-API service, which a stock app dials. #define BLE_GATT_MESH_SERVICE_UUID "4d657368-4e6f-6465-4741-545400000001" #define BLE_GATT_MESH_CHARACTERISTIC_UUID "4d657368-4e6f-6465-4741-545400000002" @@ -64,18 +63,18 @@ struct BLEGattMeshPeer { }; /** - * Carries mesh frames between this node and phones connected over a BLE GATT link - the SIG Mesh + * Carries mesh frames between this node and phones connected over a BLE GATT link, the SIG Mesh * "GATT proxy" role. The node is the GATT server; each phone is a central that writes fragments to - * the mesh characteristic and subscribes to it for what the node sends. Firmware never dials out. + * the mesh characteristic and subscribes to it. Firmware never dials out. * * Point-to-point where LoRa and the advertisement transport are one-to-many: a broadcast here is N - * notifies to N peers. Egress skips the peer a relayed packet arrived from. Everything a stranger - * can influence - framing, reassembly bounds, ingress sanitising - lives here in platform-neutral - * code so the native suite covers it; the platform half only moves opaque chunks. + * notifies to N peers. Egress skips the peer a relayed packet arrived from. Framing, reassembly + * bounds and ingress sanitising live here in platform-neutral code so the native suite covers them; + * the platform half only moves opaque chunks. * * Both ends of every ring run on the main task: onSend() is reached from Router::send(), runOnce() - * is an OSThread on the same task, and the platform hands received chunks over via - * platformPollInbound() from runOnce(). The BLE stack's own task never touches this class. + * is an OSThread on the same task, and platformPollInbound() is called from runOnce(). The BLE + * stack's own task never touches this class. */ class BLEGattMeshHandler : private concurrency::OSThread, public MeshTransportBase { @@ -86,7 +85,6 @@ class BLEGattMeshHandler : private concurrency::OSThread, public MeshTransportBa virtual void start() = 0; virtual void stop() = 0; - // Registry gate: this transport carries outgoing packets only while mesh peers are served. bool isEnabled() const override { return config.network.enabled_protocols & meshtastic_Config_NetworkConfig_ProtocolFlags_BLE_GATT_PEER; @@ -180,7 +178,7 @@ class BLEGattMeshHandler : private concurrency::OSThread, public MeshTransportBa size_t txTail = 0; size_t txCount = 0; - // The send in progress: the peer snapshot taken when it started, and where we are in it. + // The send in progress: the peer snapshot taken when it started, and the position within it. bool txActive = false; std::array txPeers{}; size_t txPeerCount = 0; diff --git a/src/mesh/BLEMeshHandler.cpp b/src/mesh/BLEMeshHandler.cpp index 0da3b6c9ad..931a7a6070 100644 --- a/src/mesh/BLEMeshHandler.cpp +++ b/src/mesh/BLEMeshHandler.cpp @@ -7,8 +7,8 @@ BLEMeshHandler *bleMeshHandler = nullptr; -// AD type constants, spelled locally so this file does not have to pick between the NimBLE and -// SoftDevice headers - the values are from the Bluetooth Core Supplement, not from either stack. +// AD type constants from the Bluetooth Core Supplement, spelled locally so this file need not pick +// between the NimBLE and SoftDevice headers. #define BLE_MESH_AD_TYPE_FLAGS 0x01 #define BLE_MESH_AD_TYPE_MFG_DATA 0xFF #define BLE_MESH_AD_FLAGS_LE_GENERAL_DISC_BREDR_UNSUP 0x06 @@ -16,16 +16,12 @@ BLEMeshHandler *bleMeshHandler = nullptr; namespace { /** - * The packet as it should go on the air: everything the far side is going to overwrite, removed. + * The packet as it goes on the air: every field the receiver overwrites, removed. * - * deliverToRouter() rewrites transport_mechanism, via_mqtt, tx_after, priority, pki_encrypted, - * public_key, rx_snr and rx_rssi on every arrival, and Router::handleReceived stamps rx_time - * (Router.cpp:1493). Every one of those is budget spent on bytes the receiver throws away, and this - * bearer has 243 of them against LoRa's 239 of ciphertext. rx_rssi is the worst of them twice over: - * a negative int32 is a ten-byte varint, and it publishes the relayer's own link quality. + * deliverToRouter() rewrites all of these on arrival and Router::handleReceived stamps rx_time. + * rx_rssi would additionally publish this node's own link quality. * - * Keep this in step with the ingress guards. A field added to one belongs in the other, and the - * native suite asserts the two agree. + * Keep in step with the ingress guards: a field added to one belongs in the other. */ meshtastic_MeshPacket strippedForAir(const meshtastic_MeshPacket &mp) { @@ -60,9 +56,8 @@ uint8_t BLEMeshHandler::buildAdvPayload(const meshtastic_MeshPacket *mp, uint8_t const meshtastic_MeshPacket air = strippedForAir(*mp); - // Sized before encoding rather than inferred from a short buffer, because pb_encode_to_bytes - // returns 0 for a genuine encode failure and for an over-budget packet alike - and the two want - // different answers. Over-budget is routine and countable; an encode failure is a bug. + // Sized before encoding: pb_encode_to_bytes returns 0 for an encode failure and for an + // over-budget packet alike, and only one of those is a bug. size_t needed = 0; if (!pb_get_encoded_size(&needed, &meshtastic_MeshPacket_msg, &air)) { LOG_ERROR("BLE mesh: cannot size packet 0x%08x", mp->id); @@ -90,12 +85,11 @@ uint8_t BLEMeshHandler::buildAdvPayload(const meshtastic_MeshPacket *mp, uint8_t } uint8_t *p = out; - // Flags AD structure. *p++ = 2; *p++ = BLE_MESH_AD_TYPE_FLAGS; *p++ = BLE_MESH_AD_FLAGS_LE_GENERAL_DISC_BREDR_UNSUP; - // Manufacturer-specific data AD structure: length covers everything after the length byte. + // Manufacturer-specific data: the length byte covers everything after itself. *p++ = (uint8_t)(1 /* type */ + 2 /* company */ + 1 /* version */ + protoLen); *p++ = BLE_MESH_AD_TYPE_MFG_DATA; *p++ = (uint8_t)(BLE_MESH_COMPANY_ID & 0xFF); @@ -113,12 +107,10 @@ bool BLEMeshHandler::onSend(const meshtastic_MeshPacket *mp) if (!isRunning || !mp) return false; - // Deliberately NOT the guard UdpMulticastHandler carries. A packet that arrived over BLE and - // comes back through Router::send is a rebroadcast: NextHopRouter::perhapsRebroadcast allocCopy()s - // the received packet, and nothing on the TX path rewrites transport_mechanism (RadioInterface - // stamps TRANSPORT_LORA in deliverToReceiver, which is RX-only). Refusing it caps the BLE mesh at - // a single hop. Loop protection is the same as LoRa's: PacketHistory drops a packet seen - // recently, hop_limit decrements per relay, and deliverToRouter ignores frames sent by us. + // Deliberately NOT the "arrived on this medium" guard UdpMulticastHandler carries: a rebroadcast + // still carries TRANSPORT_BLE_ADV, because nothing on the TX path rewrites transport_mechanism, + // so refusing it would cap the BLE mesh at one hop. Loop protection is LoRa's: PacketHistory, + // hop_limit, and deliverToRouter ignoring frames this node sent. if (mp->transport_mechanism == meshtastic_MeshPacket_TransportMechanism_TRANSPORT_BLE_ADV) LOG_DEBUG("BLE mesh: re-advertising relayed packet 0x%08x", mp->id); @@ -126,17 +118,15 @@ bool BLEMeshHandler::onSend(const meshtastic_MeshPacket *mp) slot.len = buildAdvPayload(mp, slot.data.data(), slot.data.size()); if (slot.len == 0) return false; - // mp->from, not getFrom(mp): buildAdvPayload has already refused from == 0, and this has to be - // the same key perhapsCancelDupe cancels with. + // mp->from, not getFrom(mp): the same key perhapsCancelDupe cancels with. slot.from = mp->from; slot.id = mp->id; slot.priority = (uint8_t)mp->priority; if (txCount >= BLE_MESH_TX_QUEUE_SIZE) { - // Make room only by displacing something strictly less important, the same trade - // MeshPacketQueue::replaceLowerPriorityPacket makes for the LoRa queue. A queue full of work - // at least as important refuses the newcomer rather than shuffling equals. + // Displace only something strictly less important, as + // MeshPacketQueue::replaceLowerPriorityPacket does for LoRa. const size_t worst = lowestPrioritySlot(); if (txQueue[worst].priority >= slot.priority) { txDroppedQueueFull++; @@ -155,7 +145,7 @@ bool BLEMeshHandler::onSend(const meshtastic_MeshPacket *mp) return true; } -/// The slot that should go out next: highest priority, oldest first within a priority. +/// Highest priority, oldest first within a priority. size_t BLEMeshHandler::highestPrioritySlot() const { size_t best = 0; @@ -166,8 +156,7 @@ size_t BLEMeshHandler::highestPrioritySlot() const return best; } -/// The slot to displace when a more important frame arrives: lowest priority, newest first, so a -/// frame that has already waited is not the one thrown away. +/// Lowest priority, newest first, so a frame that has already waited is not the one displaced. size_t BLEMeshHandler::lowestPrioritySlot() const { size_t worst = 0; @@ -225,7 +214,7 @@ bool BLEMeshHandler::onCancelSending(meshtastic_MeshPacket_TransportMechanism me bool canceled = false; - // Compact in place, keeping arrival order so equal priorities still leave oldest-first. + // Arrival order is kept, so equal priorities still leave oldest-first. size_t kept = 0; for (size_t i = 0; i < txCount; i++) { if (txQueue[i].from == from && txQueue[i].id == id) { @@ -238,9 +227,7 @@ bool BLEMeshHandler::onCancelSending(meshtastic_MeshPacket_TransportMechanism me } txCount = kept; - // Cut a burst already on air short too. Extended advertising repeats one payload for - // BLE_MESH_ADV_EVENTS events, so the copies still to come are exactly what the overhear says are - // unnecessary. runOnce() pops before it advertises, so this frame is no longer in the ring. + // runOnce() pops before it advertises, so a frame already on air is no longer in the queue. if (advertising && advertisingFrom == from && advertisingId == id) { platformEndAdvertising(); advertising = false; @@ -265,8 +252,8 @@ void BLEMeshHandler::deliverToRouter(const uint8_t *data, size_t len, int8_t rss if (mp.which_payload_variant != meshtastic_MeshPacket_encrypted_tag) return; - // Guard 1 (mirrors UdpMulticastHandler): spoofed local origin. Nothing legitimate advertises - // from=0, and our own advertisement echoing back into our own scanner would loop. + // Guard 1 (mirrors UdpMulticastHandler): nothing legitimate advertises from=0, and our own + // advertisement echoing back into our own scanner would loop. if (mp.from == 0) { LOG_WARN("BLE mesh: advertisement with no sender, dropping"); return; @@ -285,22 +272,19 @@ void BLEMeshHandler::deliverToRouter(const uint8_t *data, size_t len, int8_t rss // or schedule our transmit. mp.via_mqtt = false; mp.tx_after = 0; - // priority is local-only too, and unlike want_ack/next_hop/relay_node it is NOT a field the LoRa - // header carries - so fixPriority() always derives it locally for a LoRa arrival, and this bearer - // is the first that lets a sender choose it. Left as sent, a crafted frame with priority MAX - // outranks ACK (the ceiling fixPriority assigns) and, once perhapsRebroadcast copies it into the - // TX queue, replaceLowerPriorityPacket evicts one of ours to make room for it. + // priority is local-only and, unlike want_ack/next_hop/relay_node, is NOT carried in the LoRa + // header, so here a sender can choose it. Left as sent, MAX outranks the ceiling fixPriority + // assigns, and replaceLowerPriorityPacket evicts one of ours once perhapsRebroadcast queues it. mp.priority = meshtastic_MeshPacket_Priority_UNSET; - // Guard 3 (mirrors UdpMulticastHandler): authentication metadata is local-only. The Router - // re-establishes it after a successful PKI decrypt; carrying it in from the wire would let a - // sender assert its own packet was PKI-authenticated. + // Guard 3 (mirrors UdpMulticastHandler): authentication metadata is local-only, or a sender + // could assert its own packet was PKI-authenticated. The Router re-establishes it after decrypt. mp.pki_encrypted = false; mp.public_key.size = 0; memset(mp.public_key.bytes, 0, sizeof(mp.public_key.bytes)); - // Guard 4: no LoRa measurement exists for a BLE arrival. Unlike the UDP case there IS a real - // measurement of this hop, so rx_rssi is populated and has_rx_rssi set rather than cleared. + // Guard 4: no LoRa measurement exists for a BLE arrival, but the BLE hop itself is measured, so + // rx_rssi is populated rather than cleared as in the UDP case. mp.rx_snr = 0; mp.rx_rssi = rssi; mp.has_rx_rssi = true; diff --git a/src/mesh/BLEMeshHandler.h b/src/mesh/BLEMeshHandler.h index 67779d2d6c..26ce2bad93 100644 --- a/src/mesh/BLEMeshHandler.h +++ b/src/mesh/BLEMeshHandler.h @@ -12,18 +12,15 @@ #include -// Meshtastic BLE mesh manufacturer data identifier. -// 0xFFFF is the SIG-reserved "internal/test" company ID. A shipping build needs either a member -// company ID or - better, because iOS can only scan in the background when filtering by service -// UUID - an assigned 16-bit service UUID with the payload as service data. +// Meshtastic BLE mesh manufacturer data identifier. 0xFFFF is the SIG-reserved "internal/test" +// company ID, not usable in a shipping build. #define BLE_MESH_COMPANY_ID 0xFFFF #define BLE_MESH_PROTOCOL_VERSION 1 -// A single unfragmented extended advertising payload is capped at 251 bytes, not the 254 an -// AUX_ADV_IND could hold: the HCI LE Set Extended Advertising Data command spends four of its 255 -// parameter bytes on handle, operation, fragment preference and length. Each platform static_asserts -// this against its own stack's constant (NimBLE's BLE_HCI_MAX_EXT_ADV_DATA_LEN, the SoftDevice's -// BLE_GAP_ADV_SET_DATA_SIZE_EXTENDED_MAX_SUPPORTED). +// One unfragmented extended advertising payload caps at 251 bytes, not the 254 an AUX_ADV_IND +// holds: HCI LE Set Extended Advertising Data spends four of its 255 parameter bytes on handle, +// operation, fragment preference and length. Each platform static_asserts this against its own +// stack's constant. #define BLE_MESH_ADV_TOTAL_MAX 251 // Flags AD structure (3) + manufacturer-data AD header (2) + company ID (2) + version (1). @@ -31,13 +28,12 @@ #define BLE_MESH_MAX_PROTO_LEN (BLE_MESH_ADV_TOTAL_MAX - BLE_MESH_ADV_OVERHEAD) // Outbound frames waiting for the advertiser. Extended advertising is set-and-repeat, not a packet -// queue - the instance holds one payload and repeats it - so a burst has to be clocked through one -// frame at a time. +// queue, so a burst has to be clocked through one frame at a time. #ifndef BLE_MESH_TX_QUEUE_SIZE #define BLE_MESH_TX_QUEUE_SIZE 8 #endif -// Repeats per queued frame, standing in for the natural redundancy LoRa gets from its own retries. +// Repeats per queued frame, standing in for the redundancy LoRa gets from its own retries. #ifndef BLE_MESH_ADV_EVENTS #define BLE_MESH_ADV_EVENTS 3 #endif @@ -45,21 +41,14 @@ /** * Carries mesh frames between nodes over connectionless BLE extended advertisements. * - * A second broadcast transport alongside LoRa, wired the way UdpMulticastHandler is: ingress hands - * decoded frames to Router::enqueueReceivedMessage, egress is a copy taken in Router::send. It is - * never the only path to the mesh - Router::send still asserts a LoRa iface. + * A second broadcast transport alongside LoRa, wired as UdpMulticastHandler is. Never the only path + * to the mesh: Router::send() still asserts a LoRa iface. * - * Connectionless, not GATT. GATT is point-to-point: reaching N peers costs N writes and no peer - * overhears another, where one advertisement reaches every neighbour at once - the same one-to-many - * shape LoRa has. That one-to-many shape is what makes dupe suppression possible, and - * onCancelSending() is where it lands: a duplicate overheard on BLE drops our own queued copy, the - * same way FloodingRouter cancels a queued LoRa rebroadcast. Strictly same-medium - hearing a - * neighbour on BLE says nothing about who heard us on LoRa. + * One advertisement reaches every neighbour at once, the same one-to-many shape LoRa has, which is + * what makes onCancelSending() dupe suppression possible. Strictly same-medium. * - * onSend() only encodes and queues. The advertising itself is clocked by runOnce() on the main - * thread, because Router::send() is not a place to block: an implementation that advertises - * synchronously stalls the router - and therefore LoRa timing and the whole main loop - for the - * length of every burst. + * onSend() only encodes and queues; runOnce() clocks the advertising on the main thread. Advertising + * inline would stall Router::send(), and with it LoRa timing and the whole main loop. */ class BLEMeshHandler : private concurrency::OSThread, public MeshTransportBase { @@ -71,21 +60,17 @@ class BLEMeshHandler : private concurrency::OSThread, public MeshTransportBase virtual void stop() = 0; virtual void onBluetoothReady() {} - // Registry gate: this transport carries outgoing packets only while BLE mesh is enabled. bool isEnabled() const override { return config.network.enabled_protocols & meshtastic_Config_NetworkConfig_ProtocolFlags_BLE_BROADCAST; } - /// Packets refused because they do not fit one unfragmented advertisement. Counted rather than - /// only logged: this is a routine, silent loss of the top of the payload range, not an anomaly. - /// The ceiling is BLE_MESH_MAX_PROTO_LEN for the whole encoded MeshPacket, so the usable - /// ciphertext is that minus the envelope - well under LoRa's MAX_RADIO_PAYLOAD_LEN. + /// Packets refused for not fitting one unfragmented advertisement. The ceiling is + /// BLE_MESH_MAX_PROTO_LEN for the whole encoded MeshPacket, below LoRa's MAX_RADIO_PAYLOAD_LEN. uint32_t txDroppedTooLarge = 0; - /// Frames refused because the queue was full of work at least as important. Distinct from - /// txDroppedTooLarge: that one says the bearer cannot carry this packet at all, this one says it - /// could not carry it right now. + /// Frames lost to a full TX queue, refused or displaced. txDroppedTooLarge means the bearer + /// cannot carry the packet at all; this means not right now. uint32_t txDroppedQueueFull = 0; /// Called from Router::send(). Encodes and queues; never transmits inline. @@ -96,10 +81,8 @@ class BLEMeshHandler : private concurrency::OSThread, public MeshTransportBase bool onCancelSending(meshtastic_MeshPacket_TransportMechanism medium, NodeNum from, PacketId id) override; protected: - /// One queued outbound frame, already built into a complete AD payload. `from`/`id` are kept - /// alongside the bytes so a cancel does not have to decode its own queue back out again, and - /// `priority` because the air copy no longer carries it - strippedForAir() drops it, since it is - /// a local scheduling property that the receiver overwrites anyway. + /// One queued outbound frame, built into a complete AD payload. `from`/`id` let a cancel match + /// without decoding the queue; `priority` is here because strippedForAir() keeps it off the air. struct AdvSlot { std::array data; uint8_t len; @@ -116,13 +99,11 @@ class BLEMeshHandler : private concurrency::OSThread, public MeshTransportBase /// Tear the burst down and hand the radio back to scanning / phone advertising. virtual void platformEndAdvertising() = 0; /// True once the stack is up and it is safe to touch the GAP API. Must query the BLE stack - /// itself, NOT a flag set by onBluetoothReady() - the whole point is that this works no matter - /// which of the two was constructed first. + /// itself, NOT a flag set by onBluetoothReady(): either may be constructed first. virtual bool platformReady() = 0; int32_t runOnce() override; - /// Queue order, kept out of runOnce() so the policy is one readable thing. size_t highestPrioritySlot() const; size_t lowestPrioritySlot() const; void removeSlot(size_t index); @@ -140,31 +121,25 @@ class BLEMeshHandler : private concurrency::OSThread, public MeshTransportBase bool isRunning = false; private: - // No lock. Both ends of this ring run on the main task: onSend() is reached from Router::send(), - // and runOnce() is an OSThread on the same task. The BLE callbacks (NimBLE host task on ESP32, - // SoftDevice on nRF52) only ever reach deliverToRouter(), which touches the packet pool and the - // router's FreeRTOS queue - both explicitly safe from other contexts - and never this ring. - // - // / are also actively harmful here: they pull in , which does not survive - // Arduino's round()/abs() macros on the nRF52 arm-none-eabi toolchain. - // A bag, not a ring. Frames leave in priority order rather than arrival order, so there is no - // head to advance: runOnce() picks the best slot and closes the gap. Eight slots of 256 bytes - // makes the worst-case compaction under 2 KB on the main task, once per burst, which is cheaper - // than the index arithmetic an ordered ring would need. + // No lock. Both ends run on the main task: onSend() is reached from Router::send(), runOnce() is + // an OSThread on the same task, and the BLE callbacks only ever reach deliverToRouter(), which + // touches the packet pool and the router's FreeRTOS queue, never this array. / + // also pull in , which does not survive Arduino's round()/abs() macros on the nRF52 + // arm-none-eabi toolchain. + // A bag, not a ring: frames leave in priority order, so runOnce() picks the best slot and closes + // the gap rather than advancing a head. std::array txQueue{}; size_t txCount = 0; bool advertising = false; - // runOnce() pops a slot before it begins the burst, so a frame already on air is no longer in - // the ring. Its identity is kept here so a cancel can cut a live burst short as well. + // runOnce() pops before advertising, so a frame on air is no longer queued; its identity lives + // here so a cancel can end a live burst. NodeNum advertisingFrom = 0; PacketId advertisingId = 0; - // The BLE stack is brought up by setBluetoothEnable(), which on ESP32 runs *before* main() - // constructs this handler - so a one-shot "bluetooth is ready" callback into the handler is a - // race, and lost the coin flip about half the time: the handler sat in "waiting for Bluetooth - // ready" forever while the stack was already up. runOnce() polls platformReady() instead and - // calls onBluetoothReady() itself, exactly once, whenever readiness actually arrives. + // setBluetoothEnable() can bring the stack up before main() constructs this handler, so a + // one-shot readiness callback would race. runOnce() polls platformReady() and calls + // onBluetoothReady() itself, exactly once. bool readyHandled = false; }; diff --git a/src/mesh/FloodingRouter.cpp b/src/mesh/FloodingRouter.cpp index 7695e767c5..41228194d6 100644 --- a/src/mesh/FloodingRouter.cpp +++ b/src/mesh/FloodingRouter.cpp @@ -138,10 +138,8 @@ bool FloodingRouter::roleAllowsCancelingDupe(const meshtastic_MeshPacket *p) void FloodingRouter::perhapsCancelDupe(const meshtastic_MeshPacket *p) { if (roleAllowsCancelingDupe(p)) { - // Cancel rebroadcast of this message *if* there was already one, unless we're a router. - // Strictly same-medium: overhearing a neighbour relay this on BLE is evidence that our BLE - // neighbours have it, and no evidence at all about who heard us on LoRa. Cancelling across - // media would silently thin the LoRa flood. + // Strictly same-medium: an overhear on one medium is no evidence about who heard this node + // on another. switch (p->transport_mechanism) { case meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA: if (Router::cancelSending(p->from, p->id)) diff --git a/src/mesh/MeshTransportBase.cpp b/src/mesh/MeshTransportBase.cpp index d26511a23b..05ff047427 100644 --- a/src/mesh/MeshTransportBase.cpp +++ b/src/mesh/MeshTransportBase.cpp @@ -6,7 +6,7 @@ std::vector *MeshTransportBase::preEncodeTransports; MeshTransportBase::MeshTransportBase(HookPoint hook) : hookPoint(hook) { - // Can't trust static initializer order, so we check each time (same as MeshModule). + // Static initializer order is not guaranteed, so the list is created on first use (as MeshModule does). std::vector *&list = (hook == PreEncode) ? preEncodeTransports : postEncodeTransports; if (!list) list = new std::vector(); @@ -29,7 +29,6 @@ void MeshTransportBase::callTransports(const meshtastic_MeshPacket *mp) if (!postEncodeTransports) return; - // Every enabled transport gets every packet; a transport accepting it never suppresses another. for (auto *t : *postEncodeTransports) { if (t->isEnabled()) t->onSend(mp); @@ -41,8 +40,7 @@ bool MeshTransportBase::cancelTransportsOn(meshtastic_MeshPacket_TransportMechan if (!postEncodeTransports) return false; - // No isEnabled() gate: a transport disabled since the packet was queued still holds it, and a - // frame we have decided not to relay should not go out when it is re-enabled. + // No isEnabled() gate: a transport disabled since queueing still holds the frame. bool canceled = false; for (auto *t : *postEncodeTransports) canceled |= t->onCancelSending(medium, from, id); @@ -55,8 +53,7 @@ void MeshTransportBase::callTransportsPreEncode(const meshtastic_MeshPacket &mp_ if (!preEncodeTransports) return; - // No isEnabled() gate here (see header): the call site applies the transport-specific gate, each - // transport applies the rest of its policy inside onSendPreEncode. + // No isEnabled() gate: the call site applies the transport-specific gate. for (auto *t : *preEncodeTransports) t->onSendPreEncode(mp_encrypted, mp_decoded, chIndex); } diff --git a/src/mesh/MeshTransportBase.h b/src/mesh/MeshTransportBase.h index 35bca873c3..06dcafd976 100644 --- a/src/mesh/MeshTransportBase.h +++ b/src/mesh/MeshTransportBase.h @@ -6,25 +6,22 @@ /** * Base class for a non-LoRa transport that Router::send() fans an outgoing packet out to, alongside - * the mandatory LoRa iface. Today: UDP multicast and BLE mesh (post-encode) and MQTT (pre-encode). + * the mandatory LoRa iface. * * Registration copies MeshModule's idiom: each instance self-registers in its constructor into a * static vector, so adding a transport never touches Router::send(). Unlike MeshModule there is no - * CONTINUE/STOP contract - these are parallel media, not a handler chain, so a call hands every packet - * to every enabled transport and ignores each hook's return. + * CONTINUE/STOP contract: these are parallel media, not a handler chain, so every enabled transport + * gets every packet and each hook's return is ignored. * - * There are two fan-out points because Router::send() reaches them at different packet states: - * - PostEncode: the very end, with the final (already-encrypted, or relayed-encrypted) packet. The - * broadcast media (UDP, BLE) live here - they re-emit exactly what LoRa would. - * - PreEncode: inside the decoded-tag block, after perhapsEncode(), while the decoded copy is still - * alive. MQTT lives here because it needs the decoded packet plus the channel index. "PreEncode" - * names the hook's purpose (acting on decoded content), NOT the packet state: the packet has - * already been encrypted by the time this fires - callTransportsPreEncode still receives it. - * A transport opts into exactly one point via its constructor argument, so the two never cross: a - * PreEncode transport is invisible to callTransports(), and vice versa. + * Two fan-out points, because Router::send() reaches them at different packet states: + * - PostEncode: the very end, with the final encrypted packet. The broadcast media live here. + * - PreEncode: inside the decoded-tag block, after perhapsEncode(), while the decoded copy is + * still alive. "PreEncode" names the hook's purpose (acting on decoded content), NOT the packet + * state: the packet has already been encrypted by the time it fires. + * A transport opts into exactly one point via its constructor argument, so the two never cross. * - * Not a RadioInterface: that is the LoRa physical layer (getPacketTime, ~30 radio members). A - * transport here only needs "is this transport active" and "queue/emit this packet". + * Not a RadioInterface: that is the LoRa physical layer. A transport here only needs "is this + * transport active" and "queue/emit this packet". */ class MeshTransportBase { @@ -41,41 +38,32 @@ class MeshTransportBase explicit MeshTransportBase(HookPoint hook = PostEncode); virtual ~MeshTransportBase(); - /** Called from Router::send() with a packet that has already been encrypted (or is being relayed - * already-encrypted). Fans it out to every registered PostEncode transport whose isEnabled() is - * true. Never gates on packet contents - each transport applies its own policy in onSend(). */ + /** Fans an already-encrypted packet out to every registered PostEncode transport whose + * isEnabled() is true. Never gates on packet contents; each transport applies its own policy. */ static void callTransports(const meshtastic_MeshPacket *mp); - /** Called from Router::send() inside the decoded-tag block, after perhapsEncode() and before the - * decoded copy is released. Hands both the now-encrypted packet and the decoded copy (plus the - * channel index) to every registered PreEncode transport. Unlike callTransports() this does NOT - * gate on isEnabled(): the caller applies the transport-specific gate (e.g. moduleConfig.mqtt.enabled - * && isFromUs) at the call site, and each transport applies the rest of its policy inside its hook. */ + /** Hands the encrypted packet, the decoded copy and the channel index to every registered + * PreEncode transport, before the decoded copy is released. Unlike callTransports() this does NOT + * gate on isEnabled(): the call site applies the transport-specific gate. */ static void callTransportsPreEncode(const meshtastic_MeshPacket &mp_encrypted, const meshtastic_MeshPacket &mp_decoded, ChannelIndex chIndex); - /** Ask every PostEncode transport to drop a queued copy of (from, id) it has not yet sent, - * because a duplicate was overheard on `medium`. - * - * An overhear is evidence about one medium only: hearing a neighbour rebroadcast over BLE says - * nothing about who heard us on LoRa. So the medium is passed through and each transport ignores - * a cancel for a medium that is not its own. Returns true if any transport dropped something. */ + /** Drop any queued copy of (from, id) not yet sent, because a duplicate was overheard on + * `medium`. An overhear is evidence about one medium only, so each transport ignores a cancel + * for a medium that is not its own. True if any transport dropped something. */ static bool cancelTransportsOn(meshtastic_MeshPacket_TransportMechanism medium, NodeNum from, PacketId id); protected: - /** True when this transport should receive outgoing packets right now (typically its - * config.network.enabled_protocols flag). Checked by callTransports before each onSend(). Only the - * PostEncode path consults this. */ + /** True when this transport should receive outgoing packets right now. Only the PostEncode path + * consults this. */ virtual bool isEnabled() const = 0; - /** Queue or emit an outgoing (encrypted) packet. Must not block Router::send(). The return value is - * ignored by callTransports - one transport accepting a packet never suppresses another. Only the - * PostEncode path calls this. */ + /** Queue or emit an outgoing (encrypted) packet. Must not block Router::send(). The return value + * is ignored: one transport accepting a packet never suppresses another. */ virtual bool onSend(const meshtastic_MeshPacket *mp) = 0; - /** Pre-encode hook: act on the decoded packet (with its encrypted copy and channel index) before the - * decoded copy is freed. Default no-op, so a PostEncode transport never sees it. A PreEncode transport - * overrides this and applies its own policy inside. Must not block Router::send(). */ + /** Act on the decoded packet, with its encrypted copy and channel index, before the decoded copy + * is freed. Default no-op. Must not block Router::send(). */ virtual void onSendPreEncode(const meshtastic_MeshPacket &mp_encrypted, const meshtastic_MeshPacket &mp_decoded, ChannelIndex chIndex) { diff --git a/src/mesh/udp/UdpMulticastHandler.h b/src/mesh/udp/UdpMulticastHandler.h index 1961726f3d..3e58a12246 100644 --- a/src/mesh/udp/UdpMulticastHandler.h +++ b/src/mesh/udp/UdpMulticastHandler.h @@ -25,7 +25,6 @@ class UdpMulticastHandler final : public MeshTransportBase public: UdpMulticastHandler() : isRunning(false) { udpIpAddress = IPAddress(239, 0, 0, 69); } - // Registry gate: this transport carries outgoing packets only while UDP multicast is enabled. bool isEnabled() const override { return config.network.enabled_protocols & meshtastic_Config_NetworkConfig_ProtocolFlags_UDP_BROADCAST; @@ -96,13 +95,18 @@ class UdpMulticastHandler final : public MeshTransportBase // Authentication metadata is local-only; Router re-establishes it after successful PKI decryption. mp.pki_encrypted = false; mp.public_key.size = 0; + // Wire-carried flags only the local stack may set. A sender must not suppress our MQTT + // uplink or schedule our transmit, and priority is not in the LoRa header, so fixPriority + // derives it locally for a radio arrival: left as sent, MAX outranks the ACK ceiling and + // replaceLowerPriorityPacket evicts one of ours once perhapsRebroadcast queues it. + mp.via_mqtt = false; + mp.tx_after = 0; + mp.priority = meshtastic_MeshPacket_Priority_UNSET; UniquePacketPoolPacket p = packetPool.allocUniqueCopy(mp); if (!p) return; - // Unset received SNR/RSSI - no local RF measurement exists for a UDP arrival. rx_rssi - // has explicit presence, so also clear has_rx_rssi: `mp` may have arrived already - // carrying a real measurement from whichever node forwarded it onto UDP, and leaving - // the presence bit set would misrepresent that stale value as "0 dBm over UDP". + // No local RF measurement exists for a UDP arrival. rx_rssi has explicit presence, so + // clear has_rx_rssi too, or a value forwarded in reads as "0 dBm over UDP". p->rx_snr = 0; p->rx_rssi = 0; p->has_rx_rssi = false; diff --git a/src/platform/esp32/ESP32BLEMesh.cpp b/src/platform/esp32/ESP32BLEMesh.cpp index e586483f98..5d22059b3d 100644 --- a/src/platform/esp32/ESP32BLEMesh.cpp +++ b/src/platform/esp32/ESP32BLEMesh.cpp @@ -32,12 +32,9 @@ void ESP32BLEMesh::start() bool ESP32BLEMesh::platformReady() { - // Poll rather than wait for a callback, so this does not depend on whether NimbleBluetooth or - // this handler was constructed first. - // - // isActive(), not ble_hs_synced(): the host syncs well before NimbleBluetooth::setup() has - // registered its service and started advertising, and starting a scan in that window races the - // stack's own GAP configuration. Wait for the PhoneAPI side to be fully up. + // Polled, not a callback: construction order against NimbleBluetooth is not fixed. + // isActive(), not ble_hs_synced() - the host syncs before NimbleBluetooth::setup() registers its + // service, and starting a scan in that window races the stack's own GAP configuration. return nimbleBluetooth && nimbleBluetooth->isActive(); } @@ -68,8 +65,8 @@ bool ESP32BLEMesh::configureAdvInstance() return true; struct ble_gap_ext_adv_params params = {}; - // Non-connectable, non-scannable, non-legacy: a pure broadcast. legacy_pdu must stay 0 or we are - // back to the 31-byte limit, which cannot hold a mesh frame at all. + // legacy_pdu must stay 0, or the payload is back to the 31-byte limit, which cannot hold a mesh + // frame at all. params.connectable = 0; params.scannable = 0; params.directed = 0; @@ -94,8 +91,8 @@ bool ESP32BLEMesh::configureAdvInstance() return false; } - // Configured once and left in place. Reconfiguring per packet costs a full GAP round trip on - // every send for no benefit - only the data changes between frames. + // Left configured: only the data changes between frames, so reconfiguring per packet is a GAP + // round trip for nothing. advInstanceConfigured = true; return true; } @@ -124,9 +121,8 @@ bool ESP32BLEMesh::platformBeginAdvertising(const uint8_t *adv, size_t len) return false; } - // (instance, duration, max_events). Bounded by max_events, NOT by duration: duration is in 10ms - // units, so passing the event count there advertises for 30ms and then stops, which is not what - // a repeat count means. + // Bounded by max_events, not by duration: duration is the middle argument and is in 10ms units, + // not a repeat count. rc = ble_gap_ext_adv_start(BLE_MESH_ADV_INSTANCE, 0, BLE_MESH_ADV_EVENTS); if (rc != 0) { LOG_WARN("BLE mesh ext adv start failed: %d", rc); @@ -134,9 +130,8 @@ bool ESP32BLEMesh::platformBeginAdvertising(const uint8_t *adv, size_t len) } return true; #else - // Legacy advertising fallback (ESP32 classic - BLE 4.2, 31 bytes total). A mesh frame is far - // larger than that, so this path effectively never carries one; it exists so the build is - // uniform across ESP32 parts rather than because classic ESP32 can join a BLE mesh. + // Legacy advertising (BLE 4.2, 31 bytes total) cannot hold a mesh frame. This path exists to + // keep the build uniform across ESP32 parts, not because classic ESP32 can join a BLE mesh. if (len > 31) { LOG_DEBUG("BLE mesh: %u bytes exceeds legacy advertising capacity, not sent", (unsigned)len); return false; @@ -181,8 +176,6 @@ void ESP32BLEMesh::startScanning() return; #ifdef BLE_MESH_TX_ONLY - // Broadcast-only node: never scan. Also the isolation switch for the ESP32 fault - if the - // build is stable with this set and boot-loops without it, the fault is in the scan start. LOG_INFO("BLE mesh: TX-only build, not scanning"); return; #endif @@ -193,9 +186,8 @@ void ESP32BLEMesh::startScanning() uncodedParams.window = BLE_MESH_SCAN_WINDOW; uncodedParams.passive = 1; // never scan-request; the payload is all in the advertisement - // filter_duplicates MUST stay 0. The controller de-duplicates on advertiser address, not on - // payload, so enabling it would deliver one report per neighbour and then go silent - every - // subsequent mesh frame from that node filtered away as a "duplicate" advertisement. + // filter_duplicates MUST stay 0: the controller de-duplicates on advertiser address, not on + // payload, so every frame after a neighbour's first would be filtered away. int rc = ble_gap_ext_disc(BLE_OWN_ADDR_PUBLIC, 0 /* duration: forever */, 0 /* period */, 0 /* filter_duplicates */, BLE_HCI_SCAN_FILT_NO_WL, 0 /* limited */, &uncodedParams, NULL, onGapEvent, this); #else @@ -242,7 +234,6 @@ int ESP32BLEMesh::onGapEvent(struct ble_gap_event *event, void *arg) break; #endif case BLE_GAP_EVENT_DISC_COMPLETE: - // Scanning timed out or was stopped - restart if still running if (self->isRunning) { self->startScanning(); } @@ -268,7 +259,7 @@ void ESP32BLEMesh::handleExtendedAdvertisement(const struct ble_gap_ext_disc_des if (!isRunning || !desc) return; - // We never chain on send, so anything flagged INCOMPLETE is some other advertiser's. + // Nothing chains on send, so anything flagged INCOMPLETE is another advertiser's. if (desc->data_status != BLE_GAP_EXT_ADV_DATA_STATUS_COMPLETE || !desc->data) return; @@ -281,7 +272,7 @@ void ESP32BLEMesh::handleAdvertisementData(const ble_addr_t &addr, int8_t rssi, if (!isRunning || !data) return; - // Walk AD structures looking for ours; advertisements routinely carry several. + // An advertisement carries several AD structures, and the mesh one is not necessarily first. uint16_t offset = 0; while (offset + 1 < len) { uint8_t adLen = data[offset]; diff --git a/src/platform/esp32/ESP32BLEMesh.h b/src/platform/esp32/ESP32BLEMesh.h index fb90ab347f..f15c869bb0 100644 --- a/src/platform/esp32/ESP32BLEMesh.h +++ b/src/platform/esp32/ESP32BLEMesh.h @@ -4,18 +4,15 @@ #include "mesh/BLEMeshHandler.h" -// The tree's NimBLE comes from the ESP-IDF component, so the host headers are on the include path -// directly - the same form src/nimble/NimbleBluetooth.cpp uses. +// NimBLE comes from the ESP-IDF component, so the host headers are on the include path directly. #include "host/ble_gap.h" -// Max number of BLE mesh peers we can track #ifndef BLE_MESH_MAX_PEERS #define BLE_MESH_MAX_PEERS 8 #endif -// Scan interval and window in units of 0.625ms. -// Continuous scan (100% duty) to maximise packet capture: unlike LoRa there is no -// retransmit-until-heard, only the fixed BLE_MESH_ADV_EVENTS repeats the sender emits. +// Units of 0.625ms. Window equals interval: a sender emits only BLE_MESH_ADV_EVENTS repeats and +// nothing retransmits until heard, so a gap in the duty cycle can only lose frames. #ifndef BLE_MESH_SCAN_INTERVAL #define BLE_MESH_SCAN_INTERVAL 160 // 100ms #endif @@ -23,7 +20,7 @@ #define BLE_MESH_SCAN_WINDOW 160 // 100ms #endif -// Advertising interval for mesh data in units of 0.625ms +// Units of 0.625ms. #ifndef BLE_MESH_ADV_INTERVAL #define BLE_MESH_ADV_INTERVAL 48 // 30ms #endif @@ -33,7 +30,6 @@ #define BLE_MESH_ADV_INSTANCE 1 #endif -// How long before a peer is considered stale (ms) #ifndef BLE_MESH_PEER_TIMEOUT_MS #define BLE_MESH_PEER_TIMEOUT_MS 300000 // 5 minutes #endif diff --git a/src/platform/nrf52/NRF52BLEMesh.cpp b/src/platform/nrf52/NRF52BLEMesh.cpp index a040650f45..f391383cfd 100644 --- a/src/platform/nrf52/NRF52BLEMesh.cpp +++ b/src/platform/nrf52/NRF52BLEMesh.cpp @@ -72,8 +72,8 @@ bool NRF52BLEMesh::platformBeginAdvertising(const uint8_t *adv, size_t len) if (len > sizeof(advBuf)) return false; - // Copy into our own storage: sd_ble_gap_adv_set_configure retains the pointer rather than - // copying, so the caller's buffer must not be the one the SoftDevice reads from. + // sd_ble_gap_adv_set_configure retains this pointer rather than copying, so the payload lives in + // this object for the whole burst, not in the caller's buffer. memcpy(advBuf, adv, len); advBufLen = (uint8_t)len; @@ -94,7 +94,6 @@ bool NRF52BLEMesh::platformBeginAdvertising(const uint8_t *adv, size_t len) advParams.max_adv_evts = BLE_MESH_ADV_EVENTS; if (!ownsDedicatedSet && advHandle == BLE_GAP_ADV_SET_HANDLE_NOT_SET) { - // First attempt: ask the SoftDevice for a set of our own. uint8_t handle = BLE_GAP_ADV_SET_HANDLE_NOT_SET; uint32_t err = sd_ble_gap_adv_set_configure(&handle, &gapAdvData, &advParams); if (err == NRF_SUCCESS) { @@ -102,8 +101,8 @@ bool NRF52BLEMesh::platformBeginAdvertising(const uint8_t *adv, size_t len) ownsDedicatedSet = true; LOG_INFO("BLE mesh using dedicated adv set %u", advHandle); } else { - // No spare set. Share handle 0 with the phone advertisement, which means suspending it - // for the length of each burst and restoring it afterwards. + // Sharing handle 0 means suspending the phone advertisement for each burst and + // restoring it afterwards. LOG_WARN("BLE mesh: no spare adv set (0x%x), sharing the phone's", err); advHandle = 0; ownsDedicatedSet = false; @@ -175,8 +174,8 @@ void NRF52BLEMesh::startScanning() } else if (err == NRF_ERROR_INVALID_STATE) { LOG_DEBUG("BLE mesh scanning already active"); } else { - // NRF_ERROR_NOT_SUPPORTED / INVALID_STATE here usually means the central role is not - // enabled: Bluefruit.begin() defaults to zero central links, and scanning needs one. + // Scanning needs a central link and Bluefruit.begin() defaults to zero, which surfaces here + // as NRF_ERROR_NOT_SUPPORTED. LOG_WARN("BLE mesh scan start failed: 0x%x", err); } } @@ -277,7 +276,7 @@ void NRF52BLEMesh::updatePeer(const ble_gap_addr_t &addr, int8_t rssi) peers[peerCount].addr = addr; peers[peerCount].rssi = rssi; peers[peerCount].lastSeenMs = now; - peers[peerCount].nodeNum = 0; // unknown until we decode a packet from them + peers[peerCount].nodeNum = 0; // unknown until a packet from them decodes peerCount++; LOG_DEBUG("BLE mesh new peer (%u total)", peerCount); } diff --git a/src/platform/nrf52/NRF52BLEMesh.h b/src/platform/nrf52/NRF52BLEMesh.h index 2e830e8625..dbd91af023 100644 --- a/src/platform/nrf52/NRF52BLEMesh.h +++ b/src/platform/nrf52/NRF52BLEMesh.h @@ -9,7 +9,7 @@ #define BLE_MESH_MAX_PEERS 8 #endif -// Scan interval and window in units of 0.625ms. Continuous, for the same reason as ESP32. +// Units of 0.625ms. Window equals interval: continuous, as on ESP32. #ifndef BLE_MESH_SCAN_INTERVAL #define BLE_MESH_SCAN_INTERVAL 160 // 100ms #endif @@ -54,11 +54,9 @@ class NRF52BLEMesh : public BLEMeshHandler BLEMeshPeer peers[BLE_MESH_MAX_PEERS]; uint8_t peerCount = 0; - // The SoftDevice advertising set this handler owns. Allocated once by passing - // BLE_GAP_ADV_SET_HANDLE_NOT_SET, so mesh advertising gets its own set rather than reusing - // handle 0 - which is Bluefruit's, i.e. the phone's. Reusing it means tearing the phone - // advertisement down and restoring it around every single frame. If the SoftDevice has no spare - // set (Bluefruit's default configuration allows one), we fall back to exactly that. + // A SoftDevice advertising set of its own, allocated by passing BLE_GAP_ADV_SET_HANDLE_NOT_SET. + // Handle 0 is Bluefruit's phone advertisement; sharing it means tearing that down and restoring + // it around every frame, which is the fallback when the SoftDevice has no spare set. uint8_t advHandle = BLE_GAP_ADV_SET_HANDLE_NOT_SET; bool ownsDedicatedSet = false; bool advActive = false; diff --git a/test/test_ble_mesh/BleMesh.cpp b/test/test_ble_mesh/BleMesh.cpp index 388c7bfa4f..052699cc2c 100644 --- a/test/test_ble_mesh/BleMesh.cpp +++ b/test/test_ble_mesh/BleMesh.cpp @@ -13,13 +13,10 @@ namespace { -/** - * A BLEMeshHandler with the radio replaced by a record of what it was asked to send. - * - * Everything worth testing here is platform-independent - the advertisement the transport builds - * and the guards it applies to what it receives - so the platform hooks only need to be observable, - * not real. - */ +/// A BLEMeshHandler with the radio replaced by a record of what it was asked to send. +/// +/// What is under test is platform-independent - the advertisement the transport builds and the +/// guards it applies to what it receives - so the platform hooks need only be observable, not real. class FakeBLEMesh : public BLEMeshHandler { public: @@ -31,7 +28,6 @@ class FakeBLEMesh : public BLEMeshHandler void start() override { isRunning = true; } void stop() override { isRunning = false; } - // Exposed so tests can drive ingress without a BLE stack. void feed(const uint8_t *data, size_t len, int8_t rssi) { deliverToRouter(data, len, rssi); } bool cancel(meshtastic_MeshPacket_TransportMechanism medium, NodeNum from, PacketId id) { @@ -85,10 +81,9 @@ meshtastic_MeshPacket packetAt(meshtastic_MeshPacket_Priority priority, uint32_t /// What Router::send actually hands a transport, as opposed to the minimal fixture above. /// -/// fixPriority() runs before encryption and never leaves priority UNSET (Router.cpp:562), and -/// FloodingRouter::send stamps relay_node on everything we send (FloodingRouter.cpp:22). A relay -/// carries transport_mechanism and the reception metadata it was received with. Measuring the -/// ceiling against the minimal fixture measures the fixture, not the bearer. +/// fixPriority() never leaves priority UNSET and FloodingRouter::send stamps relay_node on +/// everything sent; a relay also carries transport_mechanism and the reception metadata it arrived +/// with. Measuring the ceiling against the minimal fixture measures the fixture, not the bearer. meshtastic_MeshPacket productionPacket(size_t payload = 32, bool relayed = false) { meshtastic_MeshPacket p = encryptedPacket(0x3061b02e, 0x04050b6e, payload); @@ -126,7 +121,7 @@ void test_advertisement_carries_the_packet(void) uint8_t len = h.build(&p, adv, sizeof(adv)); TEST_ASSERT_TRUE_MESSAGE(len > BLE_MESH_ADV_OVERHEAD, "built an advertisement"); - // Flags AD, then manufacturer-specific data with our company ID and protocol version. + // Flags AD, then manufacturer-specific data with the company ID and protocol version. TEST_ASSERT_EQUAL_UINT8(2, adv[0]); TEST_ASSERT_EQUAL_UINT8(0x01, adv[1]); TEST_ASSERT_EQUAL_UINT8(0xFF, adv[4]); @@ -145,8 +140,8 @@ void test_refuses_an_unencrypted_packet(void) p.which_payload_variant = meshtastic_MeshPacket_decoded_tag; uint8_t adv[BLE_MESH_ADV_TOTAL_MAX]; - // Router::send encrypts before any transport sees a packet, so plaintext here is a bug - // upstream - putting it on air would leak the message. + // Router::send encrypts before any transport sees a packet, so plaintext here would put a + // readable message on air. TEST_ASSERT_EQUAL_UINT8(0, h.build(&p, adv, sizeof(adv))); } @@ -164,20 +159,19 @@ void test_drops_a_packet_too_large_for_one_advertisement(void) { FakeBLEMesh h; h.start(); - // A single unfragmented extended advertisement holds 251 bytes and we never chain, so the - // largest packets cannot ride BLE. They still go out over LoRa - Router::send has already - // handed them to the radio by the time we refuse. + // One unfragmented extended advertisement holds 251 bytes and nothing chains, so the largest + // packets cannot ride BLE. They still go out over LoRa: Router::send has already handed them to + // the radio by the time the transport refuses. auto p = encryptedPacket(0x3061b02e, 0x04050b6e, MAX_ENCRYPTED_FOR_TEST); uint8_t adv[BLE_MESH_ADV_TOTAL_MAX]; TEST_ASSERT_EQUAL_UINT8(0, h.build(&p, adv, sizeof(adv))); } -/// The largest ciphertext that still fits one advertisement, found rather than assumed. +/// The largest ciphertext that still fits one advertisement, for a given packet shape. /// -/// The budget is BLE_MESH_MAX_PROTO_LEN for the *whole* encoded MeshPacket, so the answer is that -/// minus whatever envelope the packet happens to carry - which is why it is measured per packet -/// shape rather than written down once. +/// BLE_MESH_MAX_PROTO_LEN is the budget for the *whole* encoded MeshPacket, so the answer is that +/// minus whatever envelope the packet carries, and differs per shape. size_t largestCiphertextThatFits(meshtastic_MeshPacket shape) { FakeBLEMesh h; @@ -200,22 +194,19 @@ void test_the_advertisement_ceiling_is_below_the_lora_ceiling(void) { const size_t fits = largestCiphertextThatFits(productionPacket()); - // The bearer is not a full bearer and never has been: one unfragmented extended advertisement - // cannot hold what one LoRa frame holds, and nothing fragments - the nRF52 SoftDevice caps both - // the advertising data and the scan buffer at 255, so chaining is not available in either - // direction. Everything above this rides LoRa only, counted by txDroppedTooLarge. - // 216, not the 219 the minimal fixture reaches: relay_node costs 3 (field 19, so a two-byte - // tag). priority costs nothing because strippedForAir drops it, which is also why node-kmp's - // BleAdvertCeilingTest measures 214 for the same packet - it has no equivalent strip yet. + // One extended advertisement cannot hold what one LoRa frame holds, and nothing fragments: the + // nRF52 SoftDevice caps the advertising data and the scan buffer at 255 in either direction. + // Everything above this rides LoRa only, counted by txDroppedTooLarge. + // relay_node costs 3 of the budget (field 19, so a two-byte tag); priority costs nothing, + // because strippedForAir drops it. TEST_ASSERT_EQUAL_size_t(216, fits); TEST_ASSERT_LESS_THAN_size_t_MESSAGE(MAX_RADIO_PAYLOAD_LEN, fits, "BLE carries less than LoRa"); } void test_relaying_no_longer_costs_budget(void) { - // Before the egress strip a relay reached 21 bytes less far than the originator did, because - // the packet went out carrying the rx_rssi, rx_snr and rx_time it arrived with. It now reaches - // exactly as far: the receiver overwrites all three, so they were never worth sending. + // A relay reaches exactly as far as an originator: the rx_rssi, rx_snr and rx_time it arrived + // with are stripped on egress, since the receiver overwrites all three anyway. TEST_ASSERT_EQUAL_size_t(largestCiphertextThatFits(productionPacket()), largestCiphertextThatFits(productionPacket(32, true))); } @@ -241,8 +232,7 @@ void test_the_air_copy_drops_everything_the_receiver_overwrites(void) pb_decode_from_bytes(adv + BLE_MESH_ADV_OVERHEAD, len - BLE_MESH_ADV_OVERHEAD, &meshtastic_MeshPacket_msg, &air)); // Exactly the set deliverToRouter rewrites, plus rx_time which Router::handleReceived stamps. - // A sender that omits them loses nothing and stops publishing its own link quality; one that - // sends them is paying for bytes the far side discards. + // Sending them costs budget for bytes the far side discards. TEST_ASSERT_EQUAL(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_INTERNAL, air.transport_mechanism); TEST_ASSERT_FALSE(air.via_mqtt); TEST_ASSERT_EQUAL_UINT32(0, air.tx_after); @@ -284,8 +274,7 @@ void test_an_urgent_frame_overtakes_one_already_queued(void) TEST_ASSERT_TRUE(h.onSend(&ack)); // Extended advertising is set-and-repeat, so whatever leaves first holds the radio for a whole - // burst. Arrival order would put a position update ahead of an ack that a sender is timing out - // waiting for. + // burst: arrival order would put a position update ahead of an ack a sender is timing out on. uint8_t expected[BLE_MESH_ADV_TOTAL_MAX]; const uint8_t len = h.build(&ack, expected, sizeof(expected)); @@ -306,8 +295,7 @@ void test_equal_priorities_leave_in_arrival_order(void) uint8_t expected[BLE_MESH_ADV_TOTAL_MAX]; const uint8_t len = h.build(&first, expected, sizeof(expected)); - // Priority orders the queue; it does not reorder within a priority. Without this a burst of - // same-priority traffic would leave in an order nothing defines. + // Priority orders the queue; it does not reorder within a priority. h.pump(); TEST_ASSERT_EQUAL_UINT8_ARRAY(expected, h.sent[0].data(), len); } @@ -326,8 +314,8 @@ void test_a_full_queue_makes_room_only_for_something_more_important(void) TEST_ASSERT_TRUE_MESSAGE(h.onSend(&ack), "an ack gets in"); TEST_ASSERT_EQUAL_UINT32_MESSAGE(1, h.txDroppedQueueFull, "and the displacement is counted"); - // Now full again, and a second frame of the same priority as the rest has nothing to displace: - // shuffling equals would only change which packet is lost. + // Full again, and an equal has nothing to displace: shuffling equals only changes which packet + // is lost. auto peer = packetAt(meshtastic_MeshPacket_Priority_BACKGROUND, 0x33333333); TEST_ASSERT_FALSE_MESSAGE(h.onSend(&peer), "an equal is refused"); TEST_ASSERT_EQUAL_UINT32(2, h.txDroppedQueueFull); @@ -351,8 +339,8 @@ void test_the_frame_displaced_is_the_newest_of_the_least_important(void) auto ack = packetAt(meshtastic_MeshPacket_Priority_ACK, 0x33333333); TEST_ASSERT_TRUE(h.onSend(&ack)); - // A frame that has already waited its turn is not the one to throw away, so the displaced slot - // is the newest of the least important rather than the first one found. + // The displaced slot is the newest of the least important, not the first one found: a frame + // that has already waited its turn is not the one to throw away. uint8_t expectedAck[BLE_MESH_ADV_TOTAL_MAX]; uint8_t expectedOldest[BLE_MESH_ADV_TOTAL_MAX]; const uint8_t ackLen = h.build(&ack, expectedAck, sizeof(expectedAck)); @@ -373,8 +361,8 @@ void test_a_dupe_heard_on_ble_cancels_our_queued_copy(void) auto p = encryptedPacket(0x3061b02e, 0x04050b6e); TEST_ASSERT_TRUE(h.onSend(&p)); - // A neighbour relayed it before we got to. One advertisement reaches every neighbour at once, - // so their copy has already done our work. + // One advertisement reaches every neighbour at once, so a neighbour's relay has already done + // this node's work. TEST_ASSERT_TRUE(h.cancel(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_BLE_ADV, p.from, p.id)); h.pump(); @@ -388,8 +376,8 @@ void test_a_dupe_heard_on_lora_leaves_the_ble_queue_alone(void) auto p = encryptedPacket(0x3061b02e, 0x04050b6e); TEST_ASSERT_TRUE(h.onSend(&p)); - // Hearing a LoRa neighbour relay this says nothing about whether our BLE neighbours have it. - // Cancelling here would silently thin the BLE flood every time the two meshes overlap. + // A LoRa neighbour's relay says nothing about whether the BLE neighbours have it; cancelling + // here would thin the BLE flood wherever the two meshes overlap. TEST_ASSERT_FALSE(h.cancel(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_LORA, p.from, p.id)); h.pump(); @@ -409,8 +397,8 @@ void test_canceling_keeps_the_other_queued_frames_in_order(void) TEST_ASSERT_TRUE(h.cancel(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_BLE_ADV, doomed.from, doomed.id)); - // Compacting the ring must not drop or reorder its neighbours - the frames either side are - // unrelated packets that still have to go out, in the order they were queued. + // Compacting the ring must not drop or reorder the frames either side, which are unrelated + // packets that still have to go out in the order they were queued. uint8_t expectedFirst[BLE_MESH_ADV_TOTAL_MAX]; uint8_t expectedLast[BLE_MESH_ADV_TOTAL_MAX]; const uint8_t firstLen = h.build(&first, expectedFirst, sizeof(expectedFirst)); @@ -436,13 +424,12 @@ void test_canceling_cuts_a_burst_already_on_air(void) h.pump(); TEST_ASSERT_TRUE_MESSAGE(h.advertising, "on air"); - // The payload repeats for BLE_MESH_ADV_EVENTS events, so the copies still to come are exactly - // what the overhear says are unnecessary. runOnce pops before it advertises, so this frame is - // no longer in the ring and only the live-burst identity can find it. + // runOnce pops before it advertises, so the frame is no longer in the ring and only the + // live-burst identity can find the BLE_MESH_ADV_EVENTS repeats still to come. TEST_ASSERT_TRUE(h.cancel(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_BLE_ADV, p.from, p.id)); TEST_ASSERT_FALSE_MESSAGE(h.advertising, "burst ended"); - // And the state machine is not left half-advanced: the next pump finds an empty ring and idles + // The state machine is not left half-advanced: the next pump finds an empty ring and idles // rather than re-ending a burst that is already over. h.pump(); TEST_ASSERT_EQUAL_MESSAGE(1, h.sent.size(), "nothing re-sent"); @@ -455,9 +442,8 @@ void test_send_queues_rather_than_transmitting(void) auto p = encryptedPacket(); TEST_ASSERT_TRUE(h.onSend(&p)); - // onSend is reached from Router::send on the main task. An implementation that advertised - // inline would stall the router - and so LoRa timing and the whole main loop - for the length - // of every burst. + // onSend is reached from Router::send on the main task, so advertising inline would stall the + // router, and with it LoRa timing, for the length of every burst. TEST_ASSERT_EQUAL_MESSAGE(0, h.sent.size(), "nothing on air yet"); h.pump(); @@ -486,10 +472,8 @@ void test_a_relayed_packet_is_re_advertised(void) auto p = encryptedPacket(); p.transport_mechanism = meshtastic_MeshPacket_TransportMechanism_TRANSPORT_BLE_ADV; - // A packet already marked as BLE-sourced is what a *rebroadcast* looks like: - // perhapsRebroadcast allocCopy()s the received packet and nothing on the TX path rewrites - // transport_mechanism. Refusing it caps the mesh at a single hop - two nodes can talk and a - // three-node chain cannot form. + // A rebroadcast arrives marked BLE-sourced: perhapsRebroadcast allocCopy()s the received packet + // and nothing on the TX path rewrites transport_mechanism. Refusing it caps the mesh at one hop. TEST_ASSERT_TRUE_MESSAGE(h.onSend(&p), "relay must not be refused"); } @@ -508,9 +492,8 @@ void test_ingress_accepts_a_well_formed_frame(void) TEST_ASSERT_EQUAL_MESSAGE(1, h.received.size(), "delivered to the router"); const auto &got = h.received[0]; TEST_ASSERT_EQUAL_UINT32(0x3061b02e, got.from); - // Stamped so the router - and anything downstream - can tell how it arrived. TEST_ASSERT_EQUAL(meshtastic_MeshPacket_TransportMechanism_TRANSPORT_BLE_ADV, got.transport_mechanism); - // Unlike UDP there IS a real measurement of this hop, so it is reported rather than cleared. + // Unlike UDP there is a real measurement of this hop, so it is reported rather than cleared. TEST_ASSERT_TRUE(got.has_rx_rssi); TEST_ASSERT_EQUAL_INT(-42, got.rx_rssi); TEST_ASSERT_EQUAL_MESSAGE(0, got.rx_snr, "no SNR exists for a BLE arrival"); @@ -525,8 +508,8 @@ void test_ingress_drops_a_frame_with_no_sender(void) uint8_t body[meshtastic_MeshPacket_size]; size_t n = encodeForAir(p, body, sizeof(body)); - // Nothing legitimate advertises from=0, and a packet with no sender can reach remote admin - // without authorisation - the LoRa path refuses it for the same reason. + // A packet with no sender can reach remote admin without authorisation; the LoRa path refuses + // it for the same reason. h.feed(body, n, -50); TEST_ASSERT_EQUAL_MESSAGE(0, h.received.size(), "spoofed origin rejected"); } @@ -551,8 +534,8 @@ void test_ingress_clears_pki_metadata(void) FakeBLEMesh h; h.start(); auto p = encryptedPacket(); - // A sender must not be able to assert its own packet was PKI-authenticated: that flag is - // local state the Router sets after a successful decrypt, never something off the wire. + // pki_encrypted is local state the Router sets after a successful decrypt, never something off + // the wire: a sender must not be able to assert its own packet was PKI-authenticated. p.pki_encrypted = true; p.public_key.size = 32; @@ -574,7 +557,7 @@ void test_ingress_ignores_our_own_advertisement(void) uint8_t body[meshtastic_MeshPacket_size]; size_t n = encodeForAir(p, body, sizeof(body)); - // Our own advertisement echoing back into our own scanner would loop. + // A self-echo back into the node's own scanner would loop. h.feed(body, n, -50); TEST_ASSERT_EQUAL_MESSAGE(0, h.received.size(), "self-echo dropped"); } @@ -588,8 +571,8 @@ void test_pump_waits_for_the_platform(void) TEST_ASSERT_TRUE(h.onSend(&p)); h.pump(); - // Readiness is polled rather than pushed: the BLE stack comes up before main() constructs the - // handler about half the time, so a one-shot "ready" callback is a race that loses silently. + // Readiness is polled, not pushed: the order in which the BLE stack comes up and main() + // constructs the handler is not fixed, so a one-shot "ready" callback is a race. TEST_ASSERT_EQUAL_MESSAGE(0, h.sent.size(), "nothing transmitted before the stack is up"); h.ready = true; @@ -603,8 +586,7 @@ void tearDown(void) {} void setup() { initializeTestEnvironment(); - // deliverToRouter consults nodeDB to recognise - and drop - our own advertisement echoing back - // into our own scanner, so the ingress tests need a real one. + // deliverToRouter consults nodeDB to recognise and drop a self-echo, so ingress needs a real one. if (!nodeDB) nodeDB = new NodeDB(); UNITY_BEGIN(); diff --git a/variants/esp32/esp32-common.ini b/variants/esp32/esp32-common.ini index bb73e8618a..1b8a8e4638 100644 --- a/variants/esp32/esp32-common.ini +++ b/variants/esp32/esp32-common.ini @@ -372,22 +372,20 @@ custom_sdkconfig = ; build_flags = ${esp32_common.build_flags} ${ble_mesh_esp32.build_flags} ; custom_sdkconfig = ${esp32_common.custom_sdkconfig} ${ble_mesh_esp32.custom_sdkconfig} ; -; Order matters: esp32_common sets all three NimBLE options to n, so this must come after it. -; Every line here was needed to make the transport work on hardware, and each failed differently. -; The same ordering is what resolves the two values this appends for keys esp32_common already sets +; Order matters: esp32_common sets all three NimBLE options to n, so this must come after it. That +; ordering is also what resolves the two keys this appends that esp32_common already sets ; (CONFIG_BT_NIMBLE_MAX_CONNECTIONS 1 -> 2, CONFIG_BT_CTRL_BLE_MAX_ACT 2 -> 6): pioarduino keeps the ; last occurrence of a duplicated key, and the generated sdkconfig.defaults carries the values from -; this section. Read that file, or the built ELF, when in doubt - never the framework's shared -; sdkconfig, which reflects whichever env built last. Editing any option line inside a -; custom_sdkconfig value changes its hash and rebuilds the IDF libraries from source; comment lines -; do not - the parser strips them before the value is hashed. +; this section. Read that file, or the built ELF - never the framework's shared sdkconfig, which +; reflects whichever env built last. Editing any option line inside a custom_sdkconfig value changes +; its hash and rebuilds the IDF libraries from source; comment lines do not, being stripped before +; the value is hashed. [ble_mesh_esp32] build_flags = -DHAS_BLE_MESH=1 ; Gates the extended-advertising path. NOT MYNEWT_VAL(BLE_EXT_ADV): that macro resolves from the ; prebuilt esp_nimble_cfg.h, which does not reflect custom_sdkconfig, so it reads 0 even in a - ; build whose rebuilt NimBLE has ext-adv - silently compiling the feature out and dropping the - ; transport to the legacy 31-byte path, which cannot carry a mesh frame at all. + ; build whose rebuilt NimBLE has ext-adv, compiling the feature out silently. -DBLE_MESH_USE_EXT_ADV=1 ; The mesh-peer GATT service: phones connect to the node as mesh peers (the SIG Mesh "GATT proxy" ; role). Needs the second connection and the third advertising instance configured below. @@ -400,17 +398,14 @@ custom_sdkconfig = ; OBSERVER not set), so ble_gap_ext_disc returns BLE_HS_ENOTSUP and the transport can advertise ; but never receive. CONFIG_BT_NIMBLE_ROLE_OBSERVER=y - ; The default 1650 is the chained ceiling, reserved per instance - 3.3 KB across two instances, - ; for a transport capped at one 251-byte PDU. + ; The default 1650 is the chained ceiling and is reserved per instance, 3.3 KB across two, for a + ; transport capped at one 251-byte PDU. CONFIG_BT_NIMBLE_EXT_ADV_MAX_SIZE=257 - ; The controller counts advertising sets, scans and connections all as "activities", and the - ; default budget of 2 is exactly the stock build: one advertisement plus one connection. This - ; transport needs three - the PhoneAPI advertisement, the mesh advertisement, and a scan - and - ; without the extra headroom both the scan enable and the second ext_adv_configure come back - ; HCI 0x07, Memory Capacity Exceeded (NimBLE 519). The mesh-peer edge adds a third advertising - ; set (its own connectable advertisement, on instance 2 - the host already addresses instances - ; 0..MAX_EXT_ADV_INSTANCES) and a second peripheral connection (the PhoneAPI link plus one mesh - ; phone), so the budget is 3 + 1 + 2 = 6 and the NimBLE host is allowed two concurrent connections. + ; The controller counts advertising sets, scans and connections alike as "activities", and short + ; of the budget both the scan enable and the second ext_adv_configure fail with HCI 0x07, Memory + ; Capacity Exceeded (NimBLE 519). Three advertising sets (PhoneAPI, mesh, and the mesh-peer + ; connectable advertisement on instance 2, since the host addresses 0..MAX_EXT_ADV_INSTANCES), + ; one scan and two peripheral connections (the PhoneAPI link plus one mesh phone) make 6. ; Both peers are centrals connecting inward, so ROLE_CENTRAL stays off. CONFIG_BT_CTRL_BLE_MAX_ACT=6 CONFIG_BT_NIMBLE_MAX_CONNECTIONS=2