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