mirror of
https://github.com/meshtastic/firmware.git
synced 2026-11-07 06:44:33 -05:00
34cb8d5828be71fc7d55498e415953eea8a27821
642
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jonathan BennettandClaude Opus 5.5
|
34cb8d5828 |
Rework XEdDSA signing policy: Compatible stops signing, AES-CCM overrides, Strict exemptions (#12191)
* Rework XEdDSA signing policy: Compatible stops signing, AES-CCM overrides Signing costs 66 bytes per packet. On a crowded mesh that matters, and packets on an AES-CCM channel were paying it on top of the CCM tag. Sending (perhapsEncode): - Compatible, the default, no longer signs. Balanced and Strict sign broadcasts as before; licensed (ham) nodes also sign unicasts, but now only under Balanced or Strict. - Packets on an AES-CCM (use_aead) channel are never signed. This also removes the case where a signed CCM packet passed signedDataFits() but then failed the CCM size check with TOO_LARGE. Receiving (checkXeddsaReceivePolicy): - Only Strict drops a packet for lacking a signature. Balanced no longer drops unsigned broadcasts from known signers; that downgrade rule and its canonical-sizing helper are removed. - An AES-CCM channel overrides Strict: its unsigned or unverifiable packets are accepted, since the CCM tag authenticates them. - A malformed or failing signature still drops the packet in every mode. Identity (new NodeDB::refusesUnsignedIdentity, used by updateUser, NodeInfoModule and TrafficManagementModule): - Balanced and Strict still refuse an unsigned NodeInfo that would change a known signer's identity, except on an AES-CCM channel. - Compatible accepts it, since firmware before 2.8 strips signatures when relaying. Channels::isAEADEnabled() now returns false for an out-of-range index instead of logging, since receive paths pass a packet's index unchecked. XEDDSA_SIGNATURE_FIELD_BYTES lost its only user and is removed. Tests: test_packet_signing drops the cases that pinned the removed downgrade sizing (A7-A9, A18-A19, E5, E10-E13, D1), flips A4 and E1 to acceptance, and adds AES-CCM, Compatible and ham cases (A18-A20, B18-B20, N12-N13). test_mqtt and test_traffic_management pin the same rules on the plaintext-MQTT and NodeInfo-cache paths. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Sign NodeInfo unicasts, forward signed requests, check signed NodeInfo keys A NodeInfo request or reply carries the key a peer will use for PKI direct messages, but off ham mode only broadcasts were signed. A Strict node therefore dropped every NodeInfo reply and could only learn a new peer's key from a signed broadcast, hours apart. - perhapsEncode: under Balanced and Strict, NodeInfo is signed even when unicast (still never on an AES-CCM channel, never in Compatible). - TrafficManagementModule: a NodeInfo request that carries a signature is forwarded instead of answered from the cache. Its sender may need a signed reply, and a reply sent on the target's behalf can never be signed. - checkXeddsaReceivePolicy: a signed NodeInfo verified against a stored key must advertise that same key, and the key must hash to the sender's node number, as first contact already requires. Otherwise it is dropped. This stops a valid signature vouching for another key, and stops a key planted on trust under the wrong node number proving itself with fresh signatures. Tests: B7 now expects NodeInfo unicasts signed; B21 pins signing per policy; A21-A23 pin the key-consistency check; a traffic-management case pins that a signed request is forwarded while the same request unsigned is answered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Strict: accept unsigned acks and traceroutes; never sign traceroutes Strict dropped every unsigned packet that was not PKI-encrypted. Acks and traceroutes are never signed, so a Strict node saw no acks, retransmitted every reliable packet to exhaustion, and could not take part in traceroutes. - checkXeddsaReceivePolicy: Strict no longer drops an unsigned ROUTING_APP packet; an ack carries its own proof (AckProof.h). Traceroutes are accepted in every mode, and any signature on one is ignored rather than verified. - perhapsEncode: a traceroute is never signed, including in ham mode, which signs other unicasts. Every relay rewrites the route, so a signature fails verification from the second hop on, and a bad signature drops the packet in every mode. - TraceRouteModule::alterReceivedProtobuf: strip any signature when rewriting the route, so a traceroute signed by older firmware is not dropped by older receivers further along. Tests: A24-A26 (Strict accepts unsigned acks and traceroutes; a traceroute signature is ignored), B22 (traceroutes never signed), B7 updated, and test_traceroute_nexthop pins the signature strip on rewrite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Drop the traceroute signature strip Only 2.8.0 and 2.8.1 pre-release nodes ever signed a traceroute: ham mode, which signs every unicast, and client-sent broadcast traceroutes. Stripping the signature on rewrite only helped those pre-releases interoperate, which does not justify the code. Receivers still ignore a traceroute signature, and traceroutes are still never signed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Sign unicast positions under Balanced and Strict A position request or reply is PKI-excluded, so a signature is its only authentication, but off ham mode only broadcasts were signed. Strict dropped every position request and reply, and under Balanced any channel member could fake another node's location with an unsigned reply. perhapsEncode now signs POSITION_APP unicasts under Balanced and Strict, as it does NodeInfo. Nothing rewrites a position in flight - precision is applied by the originator before signing - so the signature survives relaying, and it binds request_id so a reply cannot be retargeted. Compatible and AES-CCM channels still sign nothing. Tests: B23 pins position unicast signing per policy and Strict acceptance; B7 expects position unicasts signed; B2 and B5 use PRIVATE_APP as their unsigned unicast now that position is signed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Strict: accept unsigned key verification messages Key verification's bootstrap messages (M1, M2) go out channel-encrypted because the peer's key is not known yet, so they cannot carry a signature the receiver could verify. Strict dropped them, so two Strict nodes that did not already hold each other's keys could never run key verification. The handshake does not rely on the transport: hash1 binds both public keys to the security number the users exchange, and a key is only stored after the numbers match and the user accepts. A forged request can at most raise a verification prompt, bounded by the single session slot and the 60 s remote cooldown - as it already can under Balanced. Test: A27 pins that Strict accepts an unsigned key verification message. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * test_nodedb_identity_hygiene: pin the identity refusal to Balanced Two cases expected updateUser() to refuse an unsigned update from a known signer under a zeroed config. Zeroed is Compatible, which now accepts it, so they failed in CI. Run them under Balanced, and add the Compatible counterpart so both halves of the rule are pinned at the updateUser level. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Ham mode: sign in place of PKI, authenticate acks with the ack proof (#12200) Ham mode signed every unicast under Balanced and Strict, acks included. A signed ack from a node whose key a Strict receiver lacks is dropped, so the sender retransmits to exhaustion, and the signature costs 66 bytes where the ack proof costs about 10. Ham mode now signs the unicasts PKI would cover in regular mode, plus broadcasts, NodeInfo and position as before. Acks are no longer signed; they carry the ack proof, which needs only the two nodes' keys, not PKI. Strict already accepts unsigned acks. Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK Co-authored-by: Claude <noreply@anthropic.com> * Notify the client when an unsigned identity update is refused Balanced and Strict refuse an unsigned NodeInfo that would change a known signer's name or role, but the refusal only reached the device log, so the user saw a stale name with no reason. Send a ClientNotification naming the node (by the name already held, never the refused claim) and how to accept its updates again: remove the node. Rate limited to once per node per boot, over the last 8 nodes, and to one notice per minute overall, so a signer relayed through pre-2.8 firmware or a burst of them cannot flood the client. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Refused identity notice: only for a name or role change, drop the warnings Notify only when the refused NodeInfo would have changed the stored long name, short name or role, compared through CopyUserToNodeInfoLite so truncation and sanitizing cannot read as a change. A signer held only in the warm tier has no stored name to compare and is not reported. Remove the two LOG_WARNs at the refusal sites; the notice replaces them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Refused identity notice: name the security setting that refused it Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK * Refused identity notice: one global hourly throttle, no per-node tracking Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DjtCrF8NLMGs9DtEVQkUK --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
c7df2e85bc |
fix(traffic): round the default position dedup window to 6h, derive the floor from it (#11765)
The 5 h dedup window and the 6 h stationary broadcast floor were paired by a hand-picked one-hour margin. Round the window to 6 h and derive the floor from it: one dedup tick (6 min) above the window, which is the exact margin a stationary node's refresh needs to clear its neighbours' tick-quantised window regardless of tick phase. A static_assert enforces the pairing. Role caps (tracker 1 h, lost-and-found 15 min) are unchanged. Saved configs keep their value, as with the 11 h -> 5 h change in #11606; the new default applies when traffic management config is first installed. Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com> |
||
|
|
40a0178157 |
fix(nodedb): keep deferred node saves pending instead of dropping them (#11930)
* fix(nodedb): persist newly discovered nodes saveNodeDatabaseToDisk() is reached from the deep sleep path (sleep.cpp), the shutdown path (Power.cpp) and the classic screen's menu handler. A node that runs with power.is_power_saving disabled and a UI other than the classic screen reaches none of them, and a graceful reboot does not save either: AdminModule::reboot() only arms rebootAtMsec, and Power::reboot() notifies observers (only InkHUD listens) before ESP.restart(). nodes.proto therefore keeps whatever was written on first boot, usually just the local node, and every peer learned since is dropped on restart. Because the peer's public key lives in its node entry, PKI direct messages to that peer then fail until it is heard again: Loaded saved nodedatabase v25: 1 nodes Received a NAK, error reason: PKI_SEND_FAIL_PUBLIC_KEY Save when a node is first added, rate limited to once a minute. * fix(nodedb): keep deferred node saves pending instead of dropping them The previous commit misread the save paths: updateUser() already saves the node DB when a peer's user info changes. But it saves at most once a minute, and a change inside that window is dropped, not deferred: it logs "Defer NodeDB saveToDisk" and nothing retries it. lastNodeDbSave starts at 0, so every change in the first minute after boot is dropped, and a graceful reboot has no save point. A later save writes the whole DB and catches up, so the loss shows when nothing else changes before the restart. This removes the save on admission (nodes without user info are dropped by cleanupMeshDB() at boot anyway) and instead: - marks the node DB dirty when updateUser() records a change; - clears the flag only after nodes.proto is actually written, so a save that was skipped or deferred (no keypair yet, xmodem busy, unsafe power) stays pending; - retries a pending save from the main loop as a single attempt that shares updateUser()'s lastNodeDbSave budget, so no new write trigger is added and the worst case stays one write a minute; - saves a pending change from a notifyReboot observer on graceful reboot. Addresses both CodeRabbit findings on #11930. * fix(nodedb): skip the deferred node save while encrypted storage is locked; test it saveNodeDatabaseIfDirty() and the reboot observer call saveNodeDatabaseToDisk() directly, so they missed the locked-storage skip in saveToDiskNoRetry() and logged a failed encrypted write every minute on a locked MESHTASTIC_ENCRYPTED_STORAGE build. Both now apply the same check. test_nodedb_save_retry pins the deferred save: a change held back by the one-minute throttle is written once the minute passes, and a reboot writes it at once. --------- Co-authored-by: marshallumsted <bot@marshallumsted.dev> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
|
6362e05424 |
Stop NodeInfo relays rewriting the User payload (#12088)
* Stop NodeInfo relays rewriting the User payload NodeInfoModule::alterReceivedProtobuf rewrote user.id and re-encoded the User payload of every NodeInfo it saw, including ones it relays. The XEdDSA signature covers Data.payload, so a rewritten NodeInfo failed verification at receivers that know the sender's key. Relays now forward the payload unmodified. The user.id coercion moves to the local copies that consumed it: phone delivery (MeshService::sendToPhone) and the unencrypted MQTT uplink. It only re-encodes when the id does not match the sender. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmhtcXBe3B3NvS9RttzwHW * Keep signed NodeInfo unmodified on MQTT uplink A coerced copy keeps the sender's XEdDSA signature, which no longer covers the payload, so downlink gateways would drop it. Signed NodeInfo is now published as sent. Also assert that a re-encoded User always fits Data.payload. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NmhtcXBe3B3NvS9RttzwHW --------- Co-authored-by: Jonathan Bennett <jonathan@meshtastic.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> |
||
|
Jonathan BennettandClaude Opus 5
|
c7826791d4 |
feat(portduino): admin keys in config.yaml, beyond the protobuf's three (#12056)
* feat(portduino): admin keys in config.yaml, beyond the protobuf's three meshtasticd can now list the public keys authorized to administer the node in its own config, as Security.AdminKeys, and list more than the three config.security.admin_key holds. The host's file is the authority: it is root-owned, so unlike the persisted protobuf an authorized remote cannot rewrite it, and applyHostKeys() re-asserts it at boot and after every remote write of the security config. A key removed from the file loses access on the next boot instead of lingering in flash. The protobuf array stays at three. It is not an arbitrary cap: a security set_config already encodes to ~207 bytes against the 221 a PKC admin packet can carry (233 payload bytes less 12 of PKC overhead), so a fourth key would not fit in one admin message on any platform. Four sites decided who may administer a node, each unrolling admin_key[0..2] by hand - the PKC gate in AdminModule, the same gate in DMShell, the is_managed guard, and the admin-key trial decrypt in Router::perhapsDecode. They now share AdminKeys::isAuthorized(), so a key source reaches all four at once, and enumeration skips empty slots rather than trusting admin_key_count. Each configured key costs one trial decryption per PKI packet from an unknown sender, so the host list is capped at 16 and duplicates are dropped. Files in config.d append to the list, one per operator. A key that is not exactly 32 canonical base64 bytes stops meshtasticd rather than silently authorizing something else, and --check reports it with its line instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SP5hpFwdTBC2K9ggGFLpcs * test(portduino): config-check fixtures for Security.AdminKeys bin/test-config-check.sh drives a built meshtasticd against the fixtures, which is the only place the two process-level behaviours here are reachable: what --check reports, and that a normal boot refuses the file. A clean two-key list, a key three characters short of 32 bytes, and one key written as a scalar instead of a list - the last two as errors under --check and as a non-zero exit on a normal boot, since a truncated admin key would authorize something other than what was written and a file whose only key is unreadable would otherwise boot a node nobody can administer. adminkeys-append/ pins the append-across-config.d rule by value through --output-yaml, with the first key in config.yaml so the effective list does not depend on the filesystem's order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SP5hpFwdTBC2K9ggGFLpcs --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Jonathan BennettandClaude Opus 5
|
8c0abbd522 |
feat(portduino): BLE peripheral support via BlueZ for meshtasticd (Raspberry Pi) (#11396)
* feat(portduino): BLE peripheral support via BlueZ for meshtasticd on Linux Adds the standard Meshtastic BLE service (toRadio/fromRadio/fromNum/logRadio) to the Linux native target, so a Raspberry Pi running meshtasticd can be paired and used over BLE like any other Meshtastic device. Implementation: a new LinuxBluetooth backend registers a GATT application, LE advertisement and pairing agent with bluetoothd over the org.bluez D-Bus APIs, using sdbus-c++ (both the 1.x and 2.x major versions, via a small compat shim - Debian bookworm/Ubuntu 24.04 ship 1.x, trixie/Fedora ship 2.x). When the sdbus-c++ dev package is absent the whole backend compiles out via __has_include, the same optional-dependency idiom as the ulfius webserver. Threading follows the NimbleBluetooth model, simplified: the sdbus event loop runs its own thread, and all PhoneAPI calls happen on the main thread. Writes queue to the main loop; reads park the D-Bus reply and are completed from the main thread after queued writes, so write-then-read clients see their answer without any busy-waiting. Enablement is a double opt-in: a new `Bluetooth:` config.yaml section (Enabled, default false; AdapterId, default hci0) must turn BLE on for the host, and the regular device config bluetooth.enabled must be on. The config-check schema and fixtures cover the new section. Pairing honors config.bluetooth.mode: NO_PIN maps to a NoInputNoOutput just-works agent; RANDOM_PIN to DisplayOnly with the kernel-generated passkey shown on screen/log via the existing BluetoothStatus plumbing. FIXED_PIN falls back to random-passkey semantics with a warning - BlueZ does not support forcing a passkey. PIN modes enforce encrypt-authenticated-read/write on all mesh characteristics. Packaging: install a D-Bus system policy so the meshtasticd user may talk to org.bluez, add it to the bluetooth group, order the unit after bluetooth.service, and add libsdbus-c++-dev to debian/rpm/docker/CI deps. Verified in-container against a mock bluetoothd: registration flow, GATT tree enumeration, advertisement properties, and a full config download (ToRadio wantConfig -> 47 FromRadio packets) through the D-Bus bridge. Real-hardware pairing/notify testing on a Pi still pending. Known limitations (v1): meshtasticd must be restarted if bluetoothd restarts; FIXED_PIN degrades to a random passkey; getRssi() returns 0 (same as nRF52). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): BLE fixes from first real-hardware pass (Pi CM5 + RAK6421) Findings from testing PR #11396 on a Raspberry Pi CM5 (Pi OS trixie, BlueZ/sdbus-c++ 2.1 - the v2 compat path) with a RAK6421 HAT and an Android phone: - getMacAddr() leaked its HCI socket on every call and never closed it, and on failure returned without touching the caller's buffer - which getDeviceName() passed in uninitialized. Close the socket on all paths and cache the MAC after the first successful read; it cannot change at runtime and this now runs on every bluetoothd property read. - getDeviceName() zero-initializes its MAC buffer, and LinuxBluetooth snapshots the name once at setup() on the main thread: the advertisement's LocalName getter runs on the D-Bus event-loop thread and getDeviceName()'s static buffer is not thread-safe. - Restore NimBLE-style config-phase packet prefetch (depth 3). The initial port answered every FromRadio read with a D-Bus -> main-loop round trip, which made the config download noticeably slow; ReadValue now answers straight from the prefetch queue on the event-loop thread, with NimBLE's safety rules (never in STATE_SEND_PACKETS, writes always observed before reads, queue cleared on disconnect). - Set advertising MinInterval/MaxInterval to 20-100ms (BlueZ >= 5.71; older versions ignore the properties). btmon showed the kernel default of 1.28s otherwise, and Android's background-connect scan windows are sparse enough that tap-to-connect took 8-14s; 20ms is the same floor NimBLE uses on ESP32. Verified on hardware: scan, passkey pairing, connect, config download, reconnect after bond wipe. Also diagnosed (no code change): the node identity MAC comes from the RAK HAT EEPROM by design, so the BLE name suffix follows the HAT rather than the BT adapter. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): address CodeRabbit review on BLE support - setBluetoothEnable: handle disable before the config gate, so a running BLE stack is always stoppable even after the device config turns Bluetooth off underneath it - getMacAddr: read the adapter configured as Bluetooth.AdapterId instead of hardcoding hci0, falling back to hci0 for unparseable names - systemd unit: Wants=bluetooth.service so bluetoothd is pulled up when present (After= only orders, it does not start it) - debian/rpm: Recommends: bluez as the runtime contract for BLE - dbus policy: document why the org.bluez rule is destination-wide rather than a per-interface allowlist Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): show the BLE pairing code on BaseUI screens onDisplayPasskey published the passkey to bluetoothStatus and triggered PowerFSM, but never called screen->startAlert(), so on BaseUI the code only ever reached the log. BluetoothStatus has no BaseUI consumer -- only InkHUD's PairingApplet and StatusLEDModule read it -- so a Pi driving a HUB75/OLED panel showed nothing while BlueZ sat waiting for the user to type a code they could not see. NimBLE and nRF52 draw it via startAlert(); this adds the missing half for Linux. The agent callbacks run on the sdbus event-loop thread while the screen is owned by the main thread, so the passkey is handed over as a pending flag and drawn from runOnce(), matching the existing disconnectCleanupPending pattern rather than reaching into the screen from the event loop. Dismissed on all four exits, so a stale code cannot stick on an always-on panel: Paired -> true (newly watched in PropertiesChanged, which previously only looked at Connected), agent Cancel, peer disconnect (moved out of the lastGone branch so a peer leaving mid-pairing clears the code even when another device is still connected), and doDeinit() -- applied inline there because runOnce() may never be scheduled again after teardown. Verified on a Pi 5 + BlueZ 5.66 in RANDOM_PIN mode: the code renders on a HUB75 panel and clears once the phone completes pairing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: install libsdbus-c++-dev for the native test build setup-native-test landed on develop while this branch was adding libsdbus-c++-dev to setup-native, so the new action's "full setup-native list" of C libraries is missing it. Without the package the test job builds with HAS_BLUETOOTH 0 and never compiles LinuxBluetooth.cpp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): warn when a factory reset cannot clear BLE bonds factoryReset(eraseBleBonds) silently did nothing on Linux when the BLE backend was not running, so the reset reported success while the host's pairings stayed. Removing them needs a live connection to bluetoothd that a disabled backend never opened, so say so rather than imply they went. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): gate the factory-reset bond clear on an enabled backend setup() leaves linuxBluetooth allocated with its bus torn down when it throws, so a pointer check alone let factoryReset log "Clear bluetooth bonds" for a clear that clearBonds() then declined to perform. isEnabled() is only true after setup() completes, which routes that case to the warning that says the bonds were left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * style: reformat under clang-format 20 #11909 moved trunk from clang-format 16 to 20, which spaces C-style casts differently and reindents the comment above the HAS_WIFI block. Both files are ones this branch already touches, and trunk's fmt linter grades whole files, so its check fails until they are reformatted. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0184v7MyCLuJHW2ebZ9r8NmQ * fix(portduino): three BLE config and lifecycle fixes from review Bluetooth config keys are now assigned individually rather than per section. loadConfig() runs once for every file in config.d, so reading an absent key as its default let a later file that named only one of them silently reset the other: `AdapterId: hci1` alone turned Bluetooth off, and `Enabled: true` alone dragged the adapter back to hci0. Only what a file actually states should override what an earlier one set. A backend that failed to come up is now retried. setup() can leave linuxBluetooth non-null but disabled - bluetoothd not ready, adapter missing, policy refusing - and every later enable then called resumeAdvertising(), which returns immediately while disabled. A transient failure at boot kept BLE off until the process restarted. doSetup() already opens with `if (enabled) return` and tears the bus down on every failure path, so calling it again is safe. Bluetooth.AdapterId is now checked for the hci<digits> form. LinuxBluetooth uses the value verbatim as the BlueZ object path while the MAC fallback reads only the leading hciN, so "hci1junk" looks plausible, yields a MAC, then finds no adapter and BLE never comes up. Covered by a new fixture and suite case, which is the kind of silent no-op that directory exists to catalogue. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
|
f5940ab5dd |
Meshtasticd notifications (#10216)
* First attempt at sending message notifications on Linux
* Add files via upload
* Notifications: Use libnotify
Use libnotify for meshtasticd desktop notifications.
Add "HAS_LIBNOTIFY" macro to guard for builds where libnotify is expected.
This is not included in buildroot/openwrt builds. (no libnotify except when extra repos are added).
Install desktop icon in the correct location on Debian and Fedora packages.
Update dependencies in packaging and dockerfiles.
* Add libnotify to setup-native (GitHub Actions)
* Address review feedback on the meshtasticd notification path
- platformio.ini: only define HAS_LIBNOTIFY when pkg-config actually finds
libnotify. The probe previously ran unguarded and the macro was defined
unconditionally, so a native build without libnotify-dev both hard-failed at
config time and claimed the feature was available.
- Fix the sender lookup for develop's flattened NodeInfoLite: has_user/user.*
are gone, replaced by nodeInfoLiteHasUser() and direct long_name/short_name.
This is a silent semantic conflict - it merges cleanly but does not compile.
- Move the desktop notification off the packet path. notify_notification_show()
is a synchronous DBus round trip and meshtasticd's packet handling is
single-threaded, so a slow or wedged notification daemon could stall the
radio. handleReceived() now resolves the strings and queues them (bounded at
16); a dedicated worker owns every libnotify call.
- Stop retrying forever: a failed notify_init(), or three consecutive failed
shows, latches desktop notifications off instead of re-logging per message.
- NodeDB: the ARCH_PORTDUINO default-enable block was nested inside
#ifdef HAS_I2S, which Portduino never defines, so it never ran. Hoist it out.
* fix(portduino): close input-broker guard before the libnotify block
The libnotify implementation was inserted ahead of the #endif that closed
#if !MESHTASTIC_EXCLUDE_INPUTBROKER, so the file's trailing #endif closed
#if HAS_LIBNOTIFY instead and the input-broker conditional was never closed.
Every build target failed to preprocess:
src/modules/ExternalNotificationModule.cpp:630: error: unterminated #if
Close the guard immediately after handleInputEvent(), as develop does, so the
two conditionals stay independent and HAS_LIBNOTIFY still resolves when the
input broker is excluded.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013EZstXBJtzRyBmUD1h2FLs
* fix(portduino): back off desktop notifications instead of latching them off
notifyDisabled was doing two jobs: "libnotify looks unusable" and "the
destructor wants the worker to exit". Because the worker's only exit path was
also its only failure path, three failed shows turned notifications off for the
process lifetime with no way back. The packaged daemon is exactly that case:
bin/meshtasticd.service runs as User=meshtasticd and the rpm spec creates that
account with /sbin/nologin, so there is no session bus and every show() fails.
A stock install logged three warnings and then went silent forever, while
NodeDB now enables the module by default on Portduino.
Split the flag. notifyShutdown is destructor-only and remains the worker's one
exit; a retry window replaces the latch. After maxNotifyFailures the worker
arms a backoff (30s, doubling to a 15min cap), drops the queue rather than
holding stale popups, and keeps looping. The producer refuses to queue while
the window is open, so the retry is driven by the next message after it expires
rather than by a timer - no idle wakeups, no probe notifications. Any success
resets the backoff. notify_init() moved inside the loop so a retry can pick up
a session bus that was absent at startup.
The worker also no longer calls the LOG_ macros. RedirectablePrint formats into
a shared static buffer that nothing guards and every other writer to it is on
the main thread, so the worker now records the state change under the mutex it
already holds and reportNotifyStatus() emits it from portduinoNotify() and
runOnce(). Per-message failure spam becomes one line per transition: the reason
and retry interval when it goes down, and a line when it comes back.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013EZstXBJtzRyBmUD1h2FLs
* fix(portduino): sanitize mesh text before it reaches libnotify
The notification body was the raw decoded payload and the summary was a node
name, both attacker-controlled, and neither was checked before being handed to
libnotify. g_variant_new_string() rejects invalid UTF-8: a GLib CRITICAL and an
"[Invalid UTF-8]" body by default, and a hard abort under G_DEBUG=fatal-criticals,
so an unauthenticated mesh packet could terminate meshtasticd on any install
running with that flag. An embedded NUL separately truncated the body at the
first one, hiding the rest of the message.
Route both strings through sanitizedMeshText(), which replaces embedded NULs and
then applies the existing sanitizeUtf8() helper. TypeConversions already
sanitizes names on the way into NodeDB, but an abort is too sharp an edge to
leave resting on an invariant owned by another file.
Escape the body for Pango markup as well. Servers advertising "body-markup"
parse a markup subset there, so a message can inject formatting to dress itself
up as trusted UI, and where the server also advertises body-images or
body-hyperlinks it can inject tags that make the notification daemon fetch a
remote URL. The escaping is unconditional rather than gated on
notify_get_server_caps(): a caps query that fails, or goes stale across a daemon
restart, fails open, while on a server without body-markup the only cost is
entities rendering literally, and mesh text carries no markup worth preserving.
The summary is not escaped - the spec gives it no markup, so escaping it would
only ever show entities.
Also call notify_uninit() in the destructor, after the join: the worker owns
every libnotify call, so tearing down while it is still running would be a
use-after-uninit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013EZstXBJtzRyBmUD1h2FLs
* fix(portduino): gate the notification default-on behind HAS_LIBNOTIFY
The default-enable block was gated on ARCH_PORTDUINO, so a native build where
pkg-config could not find libnotify still shipped external_notification enabled.
portduinoNotify() is not compiled into that build, and native defines
EXT_NOTIFICATION_MODULE_OUTPUT as 0 with setup() guarding the pin writes behind
output > 0, so nothing was driven - it only exposed a config surface that can
do nothing. Gate it on the same macro that governs the code it exists to feed.
HAS_LIBNOTIFY is a build flag for the whole env and is simply absent when the
probe fails, so this reads as 0 on every other target, matching how
ExternalNotificationModule.cpp already tests it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013EZstXBJtzRyBmUD1h2FLs
* fix(portduino): fail closed when markup escaping returns NULL
escapedNotificationBody() returned its input unescaped if g_markup_escape_text()
gave back NULL, which would hand libnotify the exact attacker-controlled string
the function exists to neutralize. The branch is unreachable for the input we
pass - already sanitized to valid UTF-8, and GLib documents no NULL return for
it - but an error path whose fallback is the unsafe action is the wrong shape
for a helper on this boundary. Drop the body instead.
Also note in the doc comment why running this on the caller's thread does not
weaken the rule that the worker owns every libnotify call: it is a pure GLib
string function that touches no libnotify or DBus state.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013EZstXBJtzRyBmUD1h2FLs
* style: format ExternalNotificationModule.h per the pinned clang-format
Trunk Check Runner failed on
|
||
|
Tom
|
332c4d7c6f |
Narrow the ad-hoc NodeInfo greeting (#11897)
* feat(nodedb): greet only while the node store is under half full The ad-hoc greeting in MeshService::handleFromRadio() was gated on !isFull(), so a node kept sending unsolicited NodeInfo right up to the last free slot - on a dense mesh that is the regime where the store is already churning and the greeting is least likely to buy a lasting entry. Add NodeDB::isHalfEmpty(), true only when strictly more than half the slots are free, and gate the greeting on it instead. The comparison is written as 2 * numMeshNodes < cap so a half-full store reads false with no integer rounding, and MAX_NUM_NODES is read into a local because portduino resolves it through a runtime call. The helper keeps the MINIMUM_SAFE_FREE_HEAP term that !isFull() used to contribute: low heap disqualifies the store regardless of occupancy, so a sparse database on a memory-starved device still does not transmit. Admission is untouched - updateFrom() and getOrCreateMeshNode() still fill to capacity. Only greeting stops early. * fix(nodeinfo): raise the minimum greeting window to 30 minutes The !shorterTimeout branch of NodeInfoModule::allocReply() used a 10-minute base, so a node that had just greeted one neighbour could greet the next ten minutes later. Raise the base to 30 minutes. This is the floor, not the window: getConfiguredOrDefaultMsScaled() still multiplies by the congestion coefficient for the roles that scale, so a busy mesh stretches it further. ROUTER/ROUTER_LATE and the tracker/sensor roles bypass the scaling and get a flat 30 minutes. The interactive paths are unaffected - they pass shorterTimeout and keep their own 60-second gate. The periodic broadcast is unaffected too: default_node_info_broadcast_secs is 3 hours with a 1-hour minimum, both clear of the new floor, so the timer is not swallowed by the throttle. * fix(nodeinfo): a send restarts the routine broadcast countdown sendOurNodeInfo() left the OSThread schedule alone, so an ad-hoc send had no effect on the periodic broadcast: run() anchors the next run at runned() + interval, and nothing re-anchored it when the send came from a greeting, a PKI decrypt failure or a completed key verification. The routine copy could follow minutes behind an ad-hoc one, putting two NodeInfos on the air for no gain. Call setIntervalFromNow() with the configured broadcast interval once the packet is queued, so the next periodic copy is a full interval from the send rather than from the last tick. It sits on the return-true path only: a send vetoed by allocReply() - throttle, airtime ceiling, reply suppression - must not be able to silence the routine broadcast. Calling it from inside runOnce() is harmless, since run() then applies the same interval from a last_run of effectively now. * test(nodeinfo): cover the send window, the countdown reset and the greeting gate Three behaviours from this branch had no coverage: isHalfEmpty()'s exclusive boundary, the 30-minute send floor, and the countdown reset on a send. isHalfEmpty() goes to test_nodedb_blocked, which already owns the full-store cases and clears the hot store per test. Three tests sweep the cap over the sizes real deployments have - portduino resolves MAX_NUM_NODES from General.MaxNodes on every read, so a predicate that cached it would greet at the wrong occupancy - and pin the band where admission outlives greeting. That suite had no tearDown; it has one now, restoring the cap so an assertion firing mid-sweep cannot leak a 2-node cap into the tests after it. test_nodeinfo_send_window is new because nothing in the tree stands up NodeInfoModule's send path. Six tests: the floor at 30 minutes with 10 refused, the interactive 60-second gate staying separate, the countdown re-armed by a broadcast and by an ad-hoc unicast, left alone by a refused send, and a preset change consumed only by a send that goes out. The scaling above 40 online nodes is deliberately not retested here - getConfiguredOrDefaultMsScaled() is test_default's contract, per preset and per role. These tests pin the base and leave the multiplier alone. NodeInfoModule gains two PIO_UNIT_TESTING accessors for the countdown: concurrency::OSThread is a private base, so a test shim cannot reach it and only the class itself can. They compile out of a shipping build. The heap term in isHalfEmpty()/isFull() stays uncovered: memGet.getFreeHeap() returns UINT32_MAX on portduino, so a native test could only pin a stub. * chore(trunk): exempt test_nodedb_blocked from the trufflehog Lob detector test_removeNodeByNum_presentNodeOnFullDb is exactly 35 characters after the test_ prefix, which is the length of a Lob API key, and trufflehog's detector matches the bare identifier. The name is years old; it surfaces now only because this branch touches the file, and the pre-push gate reports a finding in a changed file as new. Added to the ignore block that already carries the same detector's hex-literal false positives, with the reason stated alongside them. Nothing in that file is a credential. * fix(nodeinfo): exempt a licensed station from the floor, delay only on a real send Two review findings on the 30-minute window. Ham mode sets node_info_broadcast_secs to 600 s for the FCC minimum call-sign announcement (AdminModule.cpp). The new floor refused every one of those sends until 30 minutes had passed, so a licensed station's call sign went out three times less often than the regulation asks - a regression the old 10-minute base did not have. A licensed station now keeps its own interval whenever that is shorter than the floor. The exemption is exactly the licensed case because nothing else can get under the floor: a set-config clamps the field to an hour, and the userprefs path clamps identically. sendOurNodeInfo() ignored what sendToMesh() returned, so a packet the router declined - no interface, queue full - still re-armed the routine broadcast and still reported success, which let runOnce() consume a pending channel change for a send that never reached the air. Only ERRNO_OK and ERRNO_SHOULD_RELEASE now count; sendToMesh() has already released the packet in both cases. Both are pinned by tests that fail without them, measured: the licensed case fails at "11 min is past it, and the floor must not override it", the declined send at "a declined send is not a send". The licensed test carries an unlicensed control on the same configuration, so deleting the floor outright would not satisfy it. * test(nodeinfo): assert the deadline the scheduler reads, from an aged last_run The countdown cases asserted Thread::interval, which is not what schedules the next run: shouldRun() keys off _cached_next_run, and the two ways of writing it differ. setIntervalFromNow() recomputes it from now; Thread::setInterval() recomputes it from last_run. Swap the call in sendOurNodeInfo() for the latter and the period still reads three hours while the deadline lands wherever the last tick was - firing the routine copy right behind an ad-hoc send, the exact thing the reset exists to prevent. Every test passed. Assert the deadline instead, from a fixture where the two answers are distinguishable: ageLastRunForTests() calls Thread::runned() with an hour-old timestamp, the state a periodic thread is genuinely in between runs, so a deadline off last_run lands an hour early against a five second tolerance. Measured: with setInterval() in place of setIntervalFromNow(), the new case fails by 3600004 ms and the eight others pass, including the one asserting the period - which is what says the old assertion could not see this. runned() and _cached_next_run are protected in Thread and OSThread is a private base, so the hooks live on NodeInfoModule, with the two already there. Raised by Copilot on #11897. * fix(nodeinfo): a declined send must not start the throttle window either allocReply() stamped TransmitHistory when it built the packet, before anything had been sent. The previous commit made sendOurNodeInfo() report a router rejection instead of swallowing it, but the stamp was already written by then, so a packet that never reached the air still started the window - and with the floor now at 30 minutes, that silences the node for half an hour over a send that failed. allocReply() has two callers and only one of them can see the outcome: the module framework sends its own reply through currentReply, with no post-send hook a module can reach (MeshModule::sendResponse is not virtual). So the stamp stays there for that path, and sendOurNodeInfo() defers it across its own allocReply() call and stamps once the router has accepted the packet. deferHistoryStamp mirrors the shorterTimeout member alongside it - same call-scoped signal, same lifetime. test_sendWindow_aRejectedSendDoesNotStartTheWindow asserts both halves: no stamp after the rejection, and the retry immediately after goes out. The existing rejected-send case checked the first failure and the countdown only, which is how this survived it. 248/248 across every suite that touches NodeInfoModule (admin_session_repro, admin_radio, nodeinfo_send_window, traffic_management, fuzz_packets) plus transmit_history, whose subject this is. Raised by CodeRabbit on #11897. |
||
|
Ben Meadors
|
585ce17f59 |
fix(nrf52): stop concurrent flash writers corrupting LittleFS, and stop a failed save formatting it (#11872)
* fix(nrf52): serialise the warm-node ring against LittleFS on the shared flash cache On nRF52840 the warm-node store writes its 3-page record ring straight through flash_nrf5x_write/erase/flush, holding only spiLock. Every LittleFS writer instead holds Adafruit_LittleFS's own mutex, and two of them run on other tasks entirely: Bluefruit's bond saves on the callback task, and - since phone config writes moved into BLE context - a whole saveToDisk on the BLE task. Neither takes spiLock. Both writers share one 4 KB page cache, one SoftDevice flash semaphore and one result word. flash_cache_write repoints that cache when the requested page differs from the cached one, so a second writer arriving mid-write flushes the first writer's page and re-points the buffer; the first writer's remaining memcpy then lands in the wrong page's image. Ring records end up inside LittleFS metadata, or the reverse. The collision also exhausts the flash layer's 20 x 1 ms busy-retry budget against an 85 ms page erase, and flash_cache_flush discards the failure, so 32 LittleFS blocks vanish with no error reaching the filesystem. What the user sees is a torn directory pair on the next mount, a format, and critical error 13. Take the filesystem mutex in the five ring entry points that reach flash, after spiLock and never before - the order every existing path already uses. The ring touches no LittleFS call itself, so the non-recursive mutex is never re-entered. Longest new hold is a page rotation at roughly half a second, against a 2 s supervision timeout and a 90 s watchdog. Non-nRF52840 backends are untouched. * fix(nodedb): make saveProto report a failed readback or rename SafeFile::close() already verifies the .tmp by hash and renames it over the live file, and saveProto captured that result, logged it, and then returned the pb_encode status alone. A torn or half-programmed page therefore counted as a successful save: for the fullAtomic files the old contents silently survived, for nodes.proto (written in place) the file was simply gone, and saveToDisk's recovery path never fired for the one failure it exists for. * fix(nodedb): retry a failed save before formatting, and never format on a low rail saveToDisk answered any failed write with an immediate fsFormat(), which is where most "critical error 12/13" reports and the total config wipe behind them come from. A write that fails once is far more often a busy SoftDevice or a VDD dip mid-save than a corrupt filesystem, so: - retry twice, 150 ms apart, re-checking powerHAL_isPowerLevelSafe() before each attempt and before the format; on a low rail return false and leave the filesystem alone (the next save lands once the rail recovers, and boot already waits for a safe level) - check fsFormat()'s result instead of assuming it worked - after a successful format rewrite every segment, not only the ones this call asked for: the format took config.proto and the node identity with it, so a nodes-only save that ended in a format used to come back up as a new node - with encrypted storage a format also destroys the DEK; skip the resave rather than land the private key and PSKs on flash in plaintext RP2040 feeds its watchdog across the delays, as the neighbouring code does. * fix(nrf52): quiesce flash before every software reset and power-off The Adafruit flash layer keeps one 4 KB page image and one SoftDevice flash semaphore for the whole chip. Every reset path we own - Power::reboot(), enterDfuMode() (admin enter_dfu_mode_request, which arrives on the BLE task since #10967), cpuDeepSleep()'s reset and system-off arms, and the wio-t1000-s secure DFU handler - went straight to NVIC_SystemReset or sd_power_system_off while another task could be half-way through a page program or erase. A reset in that window leaves the page erased or partly programmed; LittleFS finds the torn metadata on the next mount and the corruption handler formats the filesystem. nrf52FlashQuiesce() takes spiLock and the LittleFS mutex, waits out whatever write is in flight, flushes the page cache, and keeps both locks because the caller resets next. The corruption-reboot handler and __assert_func are left alone: they run inside the filesystem call stack or a fault, where taking the mutex would deadlock. nRF54L is a second copy of these paths since #11867 and still defines ARCH_NRF52, so it gets the same function on the same core flash layer. * fix(nrf52): quiesce flash before the library BLE DFU handler jumps to the bootloader On every board except wio-t1000-s the Nordic DFU service is the framework's BLEDfu, whose START_DFU handler runs on the callback task and jumps to the bootloader with no regard for a flash write in progress on the loop task. That is the OTA path the Apple app and nRF Connect use (Android sends enter_dfu_mode_request instead, which the previous commit covers). QuiescingBLEDfu re-installs the control-point write callback after BLEDfu::begin() and wraps the library's: flush under both locks, then drop the LittleFS mutex before handing over, because the library reloads the bond keys through LittleFS on its way to the jump and the mutex is not recursive. spiLock stays held across the handler: every LittleFS writer on the BLE task takes it first, the loop task cannot preempt the callback task, and the handler never blocks after the flush, so nothing can dirty flash before bootloader_util_app_start(). If the handler returns, nothing jumped, and the lock is released. The library callback is a file-static, so it is read back out of the characteristic through a pointer-to-member obtained via a using-declaration; that is well-formed C++ and compiles under the pinned GCC 9.3 with LTO. * test(nodedb): pin the save-failure contract of saveProto and saveToDisk A failed rename must come back as false from saveProto, a one-off unsafe rail reading during a write must be retried and land, and a rail still unsafe at the retry gate must make saveToDisk return false with the filesystem untouched. The rail is scripted through a strong powerHAL_isPowerLevelSafe() over the weak native default; on Windows the default is strong, so only the rename case runs there. The format branch itself is unreachable natively (a FLASH_CORRUPTION critical error exits the portduino process), which is what the survival assertions pin. * fix(nodedb): only format when the filesystem itself is unreadable Making saveProto honest about write failures gave the recovery path a new way in: any persistent write failure now reached fsFormat(), which takes every file with it. A busy or lock-protected nRF52 flash fails every write for as long as it lasts, so two retries are not enough to tell that apart from a corrupt filesystem, and guessing wrong costs the node its config, keys and bonds. Reads settle it. They never touch the SoftDevice write path that a busy flash fails on, so if /prefs still walks and a stored proto still opens and reads, the metadata chain is intact and the write failure was transient - return false and let the caller try again later. Genuine corruption is not silently tolerated: lfs asserts on it, and the nRF52 handler reboots and formats on the way back up. Covered by a test that fails without this: a save whose rename cannot succeed, against an otherwise healthy filesystem, must leave devicestate untouched. * trunk: exempt Unity test entry points from trufflehog trufflehog's Lob detector matches "test_" followed by alphanumerics, which describes every Unity test function name. It fired on a new test in test_nodedb_save_retry and will fire again on the next suite added. Scoped to test/**/test_main.cpp, alongside the existing gitleaks exemption for the synthetic node-DB fixtures. * fix(nodedb): feed the RP2040 watchdog around the format and the resave saveToDisk() only feeds the watchdog at the top of each retry. The last retry, the readable probe, fsFormat() and the five-segment resave then share one 8 s budget (watchdog_enable in main-rp2xx0.cpp) with no loop left to feed it. A timeout during the resave leaves the filesystem empty and the node boots on defaults with a new identity - the exact outcome this PR exists to prevent, reached by a different road. Feed once before the probe and again before the resave. Both feeds sit outside any lock: filesystemStillReadable() takes spiLock itself, and the format has already released it. ARCH_RP2040 covers rp2040 and rp2350 alike, and the blocks compile out everywhere else, so no other platform and no native test changes. Raised by @caveman99 in review. * fix(nodedb): narrow the save-probe comment and name the full-filesystem case The comment on filesystemStillReadable() claimed "real corruption asserts in lfs and formats on reboot". That does hold on nRF52 - nrf52.ini builds with -DLFS_NO_ASSERT and force-includes cpp_overrides/lfs_util.h, whose LFS_NO_ASSERT arm routes LFS_ASSERT to the lfs_assert() in main-nrf52.cpp, which stamps NRF52_MAGIC_LFS_IS_CORRUPT and resets into the format - but NodeDB.cpp compiles for ESP32, RP2040 and portduino too, where nothing of the sort is wired up. It is also not true on nRF52 under POFWARN, where lfs_assert() deliberately skips the stamp. Drop the claim rather than qualify it three ways. The log line now names what a field log actually needs to tell apart: a filesystem that still reads but cannot be written is either busy or full. Raised by @caveman99 in review. * fix(sx128x): quiesce flash before the 2.4GHz region reset reinitChip() saves the region, waits 2 s and resets. On nRF52 that was the last software reset still going straight to NVIC_SystemReset with a page program possibly in flight, so "every software reset" in the earlier commit did not quite hold. The quiesce stays inside the ARCH_NRF52 arm on purpose. The #else arm logs and falls through to lora.setCRC() further down, which re-enters spiBeginTransaction(); a quiesce hoisted above the #if would take spiLock and never give it back, self-deadlocking portduino and stm32wl. Routing this through Power::reboot() is wrong for the same class of reason: setupModules() runs before initLoRa, so its notifyReboot observers and waypointStore.saveToFlash() are live and would add a flash write to an aborted radio init. Raised by @caveman99 in review. * fix(nrf52): only quiesce on the DFU control write that actually resets QuiescingBLEDfu wrapped every control-point write, so a write that was never going to reset still blocked the Bluefruit callback task on spiLock, forced an early page-cache commit and held back the GATT authorize reply. Only START_DFU resets; gate on that. Deliberately no "request->len &&" term. The library's own test is `request->data[0] == START_DFU` with no length check (BLEDfu.cpp:110 in both the nRF52 and nRF54 cores), and Bluefruit hands the callback a copy of a reused event buffer, so a zero-length write carrying a stale 0x01 still resets inside the library. A len term here would let exactly that reset run unquiesced, which is the case this wrapper exists for. Reading data[0] is always in bounds: ble_gatts_evt_write_t declares uint8_t data[1] and the copy covers it. Raised by @caveman99 in review. |
||
|
|
80cfa52665 |
Add zero guards on time calculations where they were missing (#11692)
* time: add skipZero/safeMillis/timerEndsAtMillis helpers
skipZero() steps a millis value past 0, since stored stamps and deadlines
conventionally use 0 for "unset" and the one tick per ~49.7-day wrap that
lands on 0 would otherwise read as never-set.
safeMillis() covers a bare stamp; timerEndsAtMillis(delayMs) covers a
deadline, where the sum is what has to dodge 0 - a non-zero read plus a
delay lands there once per wrap - so it is not safeMillis() + delayMs.
* time: replace hand-rolled zero-dodging with the UptimeClock helpers
PacketHistory rxTimeMsec, EncryptedStorage s_lastFailMillis (stamps), and
SGM41562 lastRefreshMs_ / NextHopRouter learnedAtMsec (ternary stamps) each
hand-rolled skipZero() in place; swap in safeMillis()/skipZero() directly.
HapticFeedback pulseOffAt/delayedPulseAt and GPS fixHoldEnds hand-rolled the
deadline form - millis() + delay, then remap a 0 result to 1 - swap in
timerEndsAtMillis(delay).
No behavior change; each site keeps the value it already computed.
* time: guard the remaining 0-means-unset deadline/stamp writes
rebootAtMsec, shutdownAtMsec, and NotificationRenderer::alertBannerUntil are
all read back with a bare == 0 / != 0 check for 'not scheduled', but every
write site computed millis() + delay (or a bare millis() stamp) with no
guard against landing exactly on 0 - the same wrap hazard skipZero() exists
for, just never applied here.
Route every rebootAtMsec/shutdownAtMsec/alertBannerUntil write through
timerEndsAtMillis()/safeMillis(); RadioLibInterface's reboot-on-stuck-tx
sums an already-captured stamp rather than "now", so it goes through
skipZero() directly instead.
No behavior change outside the ~1-in-2^32 wrap window each site was
already exposed to.
* time: guard three more 0-means-unset deadline writes
ntp_renew (ethClient.cpp), suppressTouchTapUntilMs (Events.cpp), and tx_after
(RadioLibInterface.cpp) all read back 0 as a real state - forced NTP renewal,
no suppress window active, no TX delay armed, respectively - but each arm
site wrote a bare millis()/getMillis() + delay with no guard against the sum
landing exactly on 0.
Route each through Time::timerEndsAtMillis(). No behavior change outside the
wrap window each site was already exposed to.
Refresh the Throttle.h TODO list to note ntp_renew is converted too.
* motion: guard the calibration deadline and use Throttle::deadlinePassed
endCalibrationAt's arm site wrote millis() + calibrateFor with no guard
against landing on 0, the same value finishCalibrationIfExpired()/
drawFrameCalibration() treat as "not calibrating". Route it through
Time::timerEndsAtMillis().
Also swap finishCalibrationIfExpired()'s hand-rolled (int32_t)(now - deadline)
< 0 for Throttle::deadlinePassed(): same wrap-safe comparison the codebase
already provides, without the signed-cast pattern Throttle.h documents as
implementation-defined past INT32_MAX, and it drops the file's last direct
millis() call in favor of the Time:: wrapper the rest of it already uses.
* time: fix Throttle::execute()'s own zero-dodging
Both places execute() writes *lastExecutionMs - the first-ever-run branch
and the regular update - used bare Time::getMillis() with no guard against
landing on 0, which is the exact sentinel this function reads back as
"never run" one line above. A hit there makes the next call re-fire
immediately instead of respecting minumumIntervalMs.
Capture now via Time::safeMillis() once; every use downstream (the elapsed
comparison, the stored value) is then safe by construction instead of
needing the guard reapplied at each write.
* revert some safeMillis cases where overflow is a bad thing
* test(uptime): pin skipZero/safeMillis/timerEndsAtMillis at the wrap boundary
Covers the zero case, an ordinary nonzero value, and a sum that lands
exactly on 0 from a nonzero start - the case timerEndsAtMillis() exists
for, and the one the prior suite had no direct coverage of.
* time: restore the route-health write normalization and put it on one clock
noteRouteLearned()/noteRouteSuccess() lost their `now ? now : 1` normalization,
leaving learnedAtMsec able to store 0 - which getOrAllocRouteHealth() reads as an
ever-growing age, making the slot the first eviction candidate and permanently
stale. Normalize at the write, where the block comment already says it happens,
so every caller is covered rather than just today's two.
Both callers, the two isRouteStale() sites and doRetransmissions() now read
Time::getMillis(), so the stamp and every comparison against it share a clock.
doRetransmissions() goes back to getMillis(): its `now` feeds only comparisons,
never a 0-sentinel field, so skipping zero there only cost accuracy.
* time: read the haptic, InkHUD and calibration deadlines on the write's clock
These three deadlines were converted to Time::timerEndsAtMillis() on the write
side while their reads stayed on millis(), so each spanned two clocks and would
fire immediately or never under an injected test clock. Convert the reads to
match: HapticFeedback::scheduleNext()/runOnce(), the InkHUD tap-suppression
window, and the calibration countdown's read-back of screen->getEndCalibration().
MotionSensor's sampledAtMs is left alone - its write and read are both millis()
and consistent already.
* time: correct the sentinel notes to match what the code actually does
The Throttle.h enumeration claimed the remaining timerEndsAtMillis() callers
"already dodge the sentinel", which reads as a completeness claim the same branch
contradicts: RadioLibInterface's tx_after and activeReceiveStart are both 0=unarmed
and both still arm from bare millis(). Name them instead, so the deadline-type
conversion has the real list. The ntp_renew entry now separates a deliberate 0
("due now", forced at link-up) from a computed one, which is what changed there.
The three TODO(elapsed-stamp) blocks ran four and five lines against the repo's
one-or-two rule, and two of them argued their case wrongly. Throttle.cpp implied
safeMillis() simply doesn't help; in fact neither store is safe on the wrap tick -
the 1 underflows a same-instant read, the 0 re-takes the never-run branch - which
is the symmetry worth recording. PacketHistory.cpp called its dodge "reflecting
the previous pattern" when it is load-bearing: rxTimeMsec 0 means "empty slot"
(PacketHistory.h:21) and insert() drops a record stamped 0 outright, so without it
a packet arriving on the wrap tick is never stored and loses its dedup.
Also picks up trunk fmt's trailing-whitespace fix in Throttle.cpp and the comment
realignment in SGM41562.cpp that this branch's added comment knocked out.
* test(nexthop): pin the route-health stamp against the 0 sentinel
The uptime suite covers skipZero/safeMillis/timerEndsAtMillis themselves, but
nothing covered a call site, so the branch deleted noteRouteLearned()'s
normalization and stayed green. None of the existing route-health tests pass 0 as
`now` - they use 1000, learnAt, or millis() - (TTL + 5000) - which is exactly the
gap the regression went through.
Both new tests fail with "Expected 0 to be not equal to 0" when the skipZero() is
backed out of NextHopRouter, and pass with it. noteRouteSuccess() only refreshes
an existing record, so its twin learns a route first to reach the write.
Also drops a self-referential assertion in the uptime suite: comparing
getMillis() against safeMillis() passes even if safeMillis() does no dodge at
all, so it now asserts the literal.
* discard safemillis for skipzero (better semantics and therefore maintainability) and make consistent use of getmillis where it is called (to permit testing)
* more wrapzero safety
* STM gets some too
* time: stop the next 0-means-unset deadline being armed from raw millis()
The fields this branch armed through Time::timerEndsAtMillis() / Time::skipZero()
are the kind that get added by copy-paste: `rebootAtMsec = millis() + N` appears
at twenty-odd sites across six files, and the next module to defer a reboot will
be written from one of them. Nothing catches the mistake afterwards - the sum
lands on 0 for one tick per ~49.7-day wrap, so a test run, a soak and a bench
session all pass while a pending reboot, shutdown, DFU jump or banner expiry is
silently dropped.
Two guards, at the two places it can go wrong.
The helpers themselves: skipZero() is constexpr, so its contract is now pinned by
static_assert in the header rather than only by test_uptime_clock. The asserts are
chosen against the two plausible rewrites - `ms | 1` perturbs every even value and
`ms + 1` turns the last tick of the wrap into the 0 the function exists to avoid.
Both compile, and both pass a test that only checks skipZero(0); each trips a
distinct assert here, naming the failure mode.
The call sites: bin/lint-unset-sentinel-millis.sh flags a sentinel field in src/
assigned from a raw millis()/getMillis() read, and names the helper to use. It is
name-driven because the 0 contract is declared in src/main.h and enforced in six
other files, so no single-file scan can infer it; every one of the thirteen fields
was checked to actually test against 0 before being listed. nagCycleCutoff and
LinuxJoystick's nextRepeatX/nextRepeatY are deliberately absent - their unset state
is a separate bool - and the nine remaining `millis() + x` sites in src/ are locals
that never store 0 for anything to misread.
Blocking, unlike its note-level neighbours: there is no run-time enforcer to pair
with, and the tree has zero violations today, so gating costs nothing. Scoped to
src/ so test_uptime_clock can keep building raw wrap values on purpose.
bin/test-lint-unset-sentinel-millis.sh pins the scanner against 23 fixtures -
reads, disarms, shadowing locals, comments, string literals and the already-fixed
forms all have to stay quiet.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* time: guard the four 0-means-unset stamps this branch had missed
Sweeping src/ for the `if (stamp && <deadline check>)` idiom - the shape that
makes 0 mean "unset" - turned up four stamps still armed from a raw clock read,
so the new lint rule would have had to either ignore them or go red on checkout.
Each is the same one-tick-per-wrap hole the rest of the branch closes:
* TrackballInterruptBase lastInterruptTime, armed in all four ISR handlers and
explicitly disarmed to 0 at the threshold reset. getMillis() is the ISR-safe
read by construction - it compiles to millis() outside PIO_UNIT_TESTING - and
skipZero() is pure, so neither adds anything to interrupt context.
* NeighborInfoModule lastSentReply, read as `if (lastSentReply && ...)` before
the 3-minute reply throttle. Needed the UptimeClock.h include.
* PositionModule lastSentReply, same throttle; already on the injectable clock
but still missing the guard.
* NodeDB lastSort, whose own read spells the sentinel out as `lastSort == 0 ||`.
On the wrap tick each would read as never-stamped: a trackball debounce window
lost, a neighbour or position reply sent inside the throttle it was meant to
respect, one extra NodeDB sort. Cheap individually, which is why they were missed.
All four are now listed in bin/lint-unset-sentinel-millis.sh, so the rule covers
every field in the tree that actually tests against 0 rather than a subset, and
the header records the eight stamps left off for the opposite reason - their unset
state is a separate flag (isNagging, busyTx, heldX/heldY, formatted_this_boot,
heartbeat, gotwind, haveSample, lastIaqValid), so 0 is a value they may legally
hold. The rule is silent across src/ on this tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* lint: let a site opt out of the sentinel rule, with its reason on the record
The rule is blocking, so it needs an escape hatch for the site where 0 genuinely
is a legal timestamp - and the hatch should cost something, or it becomes the
first thing anyone reaches for. `unset-sentinel-ok: <reason>` in a comment on the
write, or on a comment line above it, suppresses that one statement:
// unset-sentinel-ok: busyTx carries the armed state, so 0 is a legal stamp here
lastTxStart = Time::getMillis();
The reason is mandatory. A bare `unset-sentinel-ok`, or a colon with nothing
after it, is reported instead of honoured - with a message saying so - so the
only way to silence a site is to write down why it is safe. trunk-ignore still
works, but this states the justification at the write and also applies when the
script runs outside trunk.
The marker is read from comment text collected during the same character-level
pass that strips comments and literals, not by re-scanning the raw line. That is
what keeps it out of reach of data: LOG_DEBUG("unset-sentinel-ok: ...") mutes
nothing, because a string literal is not a comment. It is also consumed by the
statement it was written for, so it cannot leak onto the next write - while still
carrying across any number of intervening comment lines to the statement below,
which is where a real justification wants to be written.
Twelve fixtures added for the new behaviour: both comment styles, block and
multi-line block comments, the bare form, the marker-in-a-string cases, and three
leak cases. 35 total, all green, under bash 3.2 as well.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* lint: watch the separate-flag stamps too, with their exemption stated at the write
The nine stamps whose armed state lives in a companion boolean were previously
just absent from the rule's list, which meant the reasoning for leaving them out
existed only as prose in a shell script. They are now listed and individually
opted out at the write, naming the flag that actually carries the armed state:
// unset-sentinel-ok: haveSample carries the armed state, so 0 is a legal stamp
lastSampleMs = Time::getMillis();
The point is what happens later. If someone rewrites `if (haveSample && ...)` as
`if (lastSampleMs && ...)`, the field has silently acquired the 0 contract; with
the opt-out sitting at the write, the claim to re-examine is in front of whoever
makes that edit instead of buried in bin/.
Every exemption was checked against its real read sites before being written, and
three candidates did not survive that check. They stay off the list, because
listing one would mean stamping an opt-out over a claim that does not hold:
* nagCycleCutoff. handleInputEvent reads `if (nagCycleCutoff != UINT32_MAX)`
without consulting isNagging, so at that read the field is its own armed flag
with UINT32_MAX as the sentinel - and the arm at ExternalNotificationModule
.cpp:521 can land exactly there. skipZero() cannot help: it lifts 0 to 1 and
leaves UINT32_MAX alone, which UptimeClock.h's own static_assert pins. There
is also a live boot-state bug behind this - the in-class initializer is 1
while isNagging starts false - and fixing the read is a behaviour change that
belongs in its own PR.
* TouchScreenBase::_start. Overloaded as an event stamp AND a `+ 30000`
suppression deadline compared by signed subtraction, so a near-zero value
reads as "long ago" rather than "armed 30s out" and LONG_PRESS re-fires.
skipZero() does not fix this one either: 1 reads as long-ago exactly as 0
does. It needs the stamp and the deadline held separately.
* StoreForwardModule::retry_delay. No reads at all today, so nothing misbehaves
yet; exempting it now would pre-approve the raw arm for whoever implements the
retry its own comment promises.
The rule is silent across src/ on this tree, and the header records all three
rejections so the next person does not have to re-derive them. The self-test's
negative fixture no longer uses nagCycleCutoff as its example of a safely
unlisted field - that would have encoded the opposite of what the header says.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(time,lint): guard the recomputed tx_after, and judge one write at a time
Two review findings, both real.
setTransmitDelay() recomputes p->tx_after from a clamp of three candidates, and
that recomputation was still raw. Two lines above it, `if (p->tx_after)` is the read
that takes 0 as "no delay wanted", so a clamp landing on 0 drops the CSMA backoff
and the packet goes out immediately instead of after its computed delay. The first
arm site in this function was already guarded; this one was missed because the
value is not a plain `now + delay` and so does not fit timerEndsAtMillis() - it
takes skipZero() instead.
The narrowing order matters here and is spelled out at the site: add_delay is
unsigned long, 64-bit on the portduino host, so the clamp can exceed UINT32_MAX
there. skipZero() on the wide value would pass 0x100000000 through as non-zero and
the store to this uint32_t field would then truncate it back to the 0 being
avoided, so the cast comes first.
The lint rule judged each write by the wrong text. rhs was taken from the write to
the end of the accumulated statement, so a neighbour on the same line decided the
verdict - and it was wrong in both directions:
rebootAtMsec = millis() + 5; shutdownAtMsec = Time::timerEndsAtMillis(10);
the later helper call suppressed a genuine raw arm
rebootAtMsec = otherDeadline; shutdownAtMsec = millis();
the later millis() reported a safe copy
rhs is now cut at its own semicolon. Six fixtures cover it, including both cases
above, two raw writes on one line, two helper writes on one line, and a statement
split across lines, which must still see its whole right-hand side. 41 fixtures
total, green under bash 3.2.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Tom <116762865+Nestpebble@users.noreply.github.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Ben Meadors
|
df47f95ff3 |
fix(nodedb): build the fixed-GPS userprefs path again (#11825)
* fix(nodedb): build the fixed-GPS userprefs path again
The USERPREFS_FIXED_GPS block in the NodeDB constructor carries two defects
that only surface on vendor builds setting USERPREFS_FIXED_GPS_LAT and
USERPREFS_FIXED_GPS_LON. userPrefs.jsonc ships those keys commented out and no
CI target defines them, so the block is never compiled here and neither defect
was caught.
info has not existed in this scope since
|
||
|
Thomas Göttgens
|
2a01676227 |
fix(nodedb): track whether each node was heard on the current LoRa config (#11811)
* fix(nodedb): track whether each node was heard on the current LoRa config Set NODEINFO_BITFIELD_HEARD_ON_CURRENT_LORA on a genuine RF hear and clear it for every node when the LoRa slot config moves, so clients can tell which nodes went unreachable after a preset, region, slot or primary-channel-name change. Hooked into MeshService::reloadConfig(), the single funnel for the device menu, admin/CLI and scanned-URL paths, plus NodeDB::restorePreferences(), which reboots without passing through it. Fixes #11745 * fix(nodedb): store the slot each node was heard on instead of sweeping a bit A client scanning for traffic rolls through presets with live set_config writes, so every hop reached reloadConfig and the sweep cleared the marks on the way out and again on the way home. Each node now carries a 12-bit fingerprint of the slot it was heard on in spare bitfield bits, and heard_on_current_lora is derived by comparing that against the slot the radio is committed to. Config changes no longer touch the node database at all. * fix(nodedb): keep comments inside the two-line limit, rename a test Trunk read test_fingerprint_channelNumIsASlotChange as a Lob API key, since it is test_ followed by exactly 35 alphanumerics, so the tail is now shorter. The comments added under src/ are back within the one-or-two-line limit in AGENTS.md. * fix(nodedb): drop legacy bitfield bits above 10 during v24 migration v24 assigned bits 0..10, so a legacy record carrying anything higher would arrive claiming an RF hear with a stray slot fingerprint, and a never-heard node would read as reachable whenever that stray value matched ours. The migration now masks those bits off, and a new case in test_nodedb_legacy_migration pins it. |
||
|
Thomas Göttgens
|
34190aac07 |
fix(nodedb): drop satellite entries that no hot node owns (#11808)
* fix(nodedb): drop satellite entries that no hot node owns * test(nodedb): assert every persisted satellite key is owned |
||
|
|
382980637b |
perf(nodedb): bind encode-loop entries by const reference (#11780)
cppcheck (iterateByValue) flagged five range-for loops in the NodeDatabase pb_callbacks that copy a whole protobuf entry off the vector only to pass its address to pb_encode_submessage(), which takes a const void *. Bind by const reference instead. Drops one meshtastic_NodePositionEntry, NodeTelemetryEntry, NodeStatusEntry, NodeEnvironmentEntry and NodeInfoLite_Legacy copy per node per encode pass; these run on every nodes.proto save. The nodes_tag loop in NodeDB.cpp is intentionally left as a by-value copy: it mutates item.snr_q4/item.snr to the on-disk quantized form before encoding, so it is a working copy rather than a redundant one. cppcheck does not flag it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
Thomas Göttgens
|
73c4110528 |
fix(phoneapi): resend my_info when the node num moves mid-session (#11732)
* fix(phoneapi): resend my_info when the node num moves mid-session The first region set mints the PKI key and moves my_node_num to crc32(public_key) live. my_info only went out during the want_config_id handshake, so an already-connected client kept addressing the old number and its admin packets NAKed PKI_SEND_FAIL_PUBLIC_KEY until it reconnected. PhoneAPI tracks the number it last reported and re-sends my_info from STATE_SEND_PACKETS when it no longer matches. createNewIdentity() nudges fromNum so clients poll. Fixes #11718 * fix(phoneapi): key the MyInfo re-announce off a one-shot state Review follow-up. The per-connection reportedNodeNum field is gone: adding per-instance members to PhoneAPI is documented as breaking USB-CDC enumeration on the nRF52 Adafruit framework, and the baseline was never set for SPECIAL_NONCE_ONLY_NODES, which skips STATE_SEND_MY_INFO and so emitted an unexpected my_info after config_complete_id. MeshService::identityMoved is set with the nudge and cleared once the notify pass has reached every observer, so PhoneAPI::onNotify arms STATE_RESEND_MY_INFO on each connected client in that single pass and stores nothing per connection. The test now drives NodeDB::createNewIdentity() and MeshService::loop() instead of writing my_node_num directly, and asserts the transport wake-up. Nodes-only sync asserts no trailing my_info. drainToIdle() honours its read cap. * fix(phoneapi): restart the dump when the node num moves mid-sync Review follow-up. A client still in its config dump has already been sent the old my_info and has no steady state for the one-shot to fall back from, so the notify pass cleared identityMoved without covering it and the client finished syncing on the obsolete number. PhoneAPI::onNotify now restarts such a client's dump, which is the existing re-handshake path. Skipped for a client that has not reached my_info yet and for SPECIAL_NONCE_ONLY_NODES, which never sends one. test_node_num_change_mid_dump_restarts_sync renumbers mid-dump and asserts the restart, the new number, and that no part of the config is lost. Verified to fail without the fix. * fix(phoneapi): make the identity-move signal survive a concurrent notify pass Review follow-up. The identity move can run off the loop task: a local admin set_config reaches AdminModule through Router::sendLocal() on whichever task delivered it. A bool cleared by MeshService::loop() could therefore be set and cleared without any client being armed, losing the re-announce. A generation counter replaces the bool. loop() snapshots it with fromNum before notifying and only advances the seen counter afterwards, so anything bumped during the pass is still pending. The same snapshot fixes a notify for a fromNum bump that arrived mid-pass being marked delivered. test_node_num_change_mid_dump_restarts_sync now asserts the whole restarted dump: header order, channels, both config sections, our node record, nonce. Also trims the MyInfo redaction comment to the two-line cap. * fix(nodedb): keep self at index 0 after a live renumber, restart nodes-only syncs Review follow-up. createNewIdentity() removed our old row and appended the new one, leaving index 0 pointing at some other node. PhoneAPI's own-nodeinfo read and the demote/evict scans that skip index 0 to protect us both rely on that slot being self, so a renumbered node handed every client a stranger's record as its own. Pinned the way nodeDBSelfCare() does it. onNotify no longer exempts SPECIAL_NONCE_ONLY_NODES from the mid-sync restart. That dump carries no my_info, but it does carry the self record, which the move invalidates the same way. Such a client also gets the re-announce once its sync lands in STATE_SEND_PACKETS, which it previously never did. The generation counters are atomic. Every interleaving was already safe, since observers read the live counter and the seen counter only advances to a pre-pass snapshot, but the concurrent plain accesses were a data race on paper. * fix(meshservice): make fromNum atomic Review follow-up. The counter is bumped from whichever task queued the packet and read by loop(). It is private to MeshService, so the type change covers every access. |
||
|
Garth Vander HouwenandThomas Göttgens
|
83198c1cbb |
fix(pki): reject a restored pre-2.8 low-entropy key at set time, explain the swap (#11686)
* fix(pki): reject a restored pre-2.8 low-entropy key at set time, explain the swap Restoring/setting a private key is a private-key change: the public key is *generated* from it. The low-entropy blacklist check in generateCryptoKeyPair runs against the stored public_key at entry, which is empty on a bare key restore — so a known pre-2.8 weak key derived from the provided private key was never caught at set time. It was only detected on the next boot (once the weak public key had been persisted and re-checked), which looks to the user like their saved key silently "did not stick", and their node number (== crc32(public_key)) had quietly changed too. - NodeDB::generateCryptoKeyPair: in the provided-private-key branch, re-check the *derived* public key against LOW_ENTROPY_HASHES. If it matches, replace it with a fresh secure keypair and set keyIsLowEntropy so the reason is surfaced. - AdminModule set-config(security): when the restore path regenerated a rejected low-entropy key, send a client warning at set time explaining the key can't be restored and the node number changed. Scoped to that branch so a stale flag from a boot-time regeneration can't fire on unrelated security sets. No protobuf changes; reuses the existing ClientNotification warning path. Signed-off-by: Garth Vander Houwen <garthvh@yahoo.com> * fix(pki): gate low-entropy restore warning on successful keygen generateCryptoKeyPair returns false on an unset LoRa region before resetting keyIsLowEntropy, so the set-time warning could fire on a stale flag. Capture the return value and require both. Shorten the rationale comments to two lines each. * fix(pki): clear key sizes when a restored private key derives nothing The provided-private-key branch sets private_key.size and public_key.size to 32 before regeneratePublicKey() runs. On failure it returned false with both sizes still set, and AdminModule persisted that pair; every later keygen then re-derived from the same dead key. Clear both on the failure path so the next keygen mints a fresh identity. Add test_admin_radio coverage for the set-time restore path: a derived low-entropy key warns and rotates, a stale keyIsLowEntropy flag with keygen blocked does not warn, and a failed derivation clears both sizes. * fix(pki): validate a restored public key that is itself blacklisted A restore supplying both private_key and public_key reached neither keygen branch, so a whole pre-2.8 low-entropy pair was accepted and persisted at set time and only caught on the next boot. Re-derive when the supplied public key is blacklisted, which routes it through the same rejection and warning as the bare-private-key restore. A non-blacklisted keypair import is unaffected. Install the test crypto stub through a helper and drop it in restoreAdminRadioGlobals(), so a failed assertion's longjmp cannot leak a freed engine into later tests. * fix(pki): only warn about a swapped key when one was actually swapped keyIsLowEntropy is set from the stored public key at function entry, so a restore whose supplied public key is blacklisted set it even when keygen merely re-derived the public key from a private key that was kept. The warning then claimed a new key had been generated and the node number changed, which was only half true. Gate it on the private key actually being replaced. * fix(pki): re-check a freshly minted keypair against the blacklist Both mint sites called crypto->generateKeyPair() once and trusted the result, so an entropy source still producing known-weak keys could persist another blacklisted identity. Route both through a helper that re-checks and retries a bounded number of times, then logs if it cannot do better. Pass the caller's own copy of the private key to generateCryptoKeyPair() instead of config.security.private_key.bytes, which aliased the memcpy destination inside it. * fix(pki): fail keygen when every replacement stays blacklisted generateBlacklistCheckedKeyPair() logged an error after exhausting its retries but left the compromised keypair in place and its callers marked the keygen successful, persisting exactly the identity the check exists to reject. Return a flag, clear both key sizes on exhaustion, and abort both callers so the next keygen starts clean. Match the declaration guard to the definition's, and derive the expected mint count in the retry test from the configured one. * refactor(pki): drop the keygen retry loop, fail on the first weak mint Retrying cannot help: an entropy source that lands on one of the twelve blacklisted keys is broken, and a second call to it produces the same result. With real entropy the odds are ~2^-250, so the loop never runs twice in practice either. Check once and fail, which is the same guarantee in a third of the code. * fix(pki): check the derived key on the stored-private-key path too factory_reset_config keeps the private key and clears the public one, so the entry check sees no stored key, reports "not low entropy" and takes the regenerate branch, which adopted whatever it derived. A preserved pre-2.8 key was therefore accepted for a whole boot cycle before the next boot caught it - the same silent revert this PR exists to remove. Hoist the post-derive blacklist check into a helper and use it on both derive paths. * fix(pki): clear key sizes when stored-private derivation fails too The stored-private-key path set public_key.size to 32 up front and left it there when regeneratePublicKey() failed, so config claimed a pair the node never got - the same defect already fixed on the provided-key path. Both paths now derive through one helper that clears on failure and vets the derived key, replacing the separate blacklist-replace helper. --------- Signed-off-by: Garth Vander Houwen <garthvh@yahoo.com> Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> |
||
|
Manuelandcoderabbitai[bot]
|
36c89fa3a7 |
feat: Support Seeed Wio Tracker L2 (#10909)
* initial commit * enable power save * implement mesh LED * add ADS1115+AW35615 for wio tracker L2 * add ES8311, GT911, AW35615, LP5814 to I2C scanner * update commit references * move variant.cpp to extras * update hw_model * update lovyanGFX * point to device-ui commit * trunk fmt * fix IO expander (have to take from SensorLib for now as long as AudioThread has the limitation to only support SensorLib and the previous IO expander clashes with duplicate names in arduino-audio-driver) * workaround duplicate defined symbol * remove SensorLib; add lightweight Pca9555 class and use unified USE_PCA95X5; add wake button detection * keep TP_INT disabled(OUTPUT) as we use wake button for wakeup * PA off by default, enabled when playing sound; add some delay because typical class-D amps (NS4150 family) spec 20–50ms for the output stage to reach full swing after power-on * refactored AW35615 into new external library * local revert of PR10571 as this PR completely breaks the alert sound * fix detection of ADS1115 * update device-ui commit reference * add synchronisation to IO expander and call toggleDisplay() on wake button press * add battery curve, fix io expander sync * add SPILock, simplify macro usage * update device-ui * fix wakeup from sleep * revert because of #11604 * use new AUDIO_AMP_SETTLE_MS * remove test logs * enable BaseUI * use touch screen * refactor wakekey thread * fix wake button toggle screen on/off * fix battery percentage and plugIn state * Update src/graphics/TFTDisplay.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * consider to return I2C errors to make coderabbi happy * fix warnings * use Throttle for millis comparison * fix endTransmission in write * make the rabbit happy * spli targets -tft / non-tft * fix compile * revert forced use of Throttle * remove MeshLED * add HW_MODEL --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> |
||
|
Ben Meadors
|
7afd270f39 |
Gut beacon send-as-node and consolidate TX onto broadcast_targets (#11646)
* Gut beacon send-as-node and consolidate TX onto broadcast_targets Two MeshBeaconConfig changes, both against fields that never reached a tagged release, so there is no migration for existing nodes. broadcast_send_as_node let a client name a node ID to send beacons AS, rewriting the packet's `from`. Firmware never applied it - the assignment was commented out, so `from` was always the local node and the field was a settable, persisted no-op. It was also unsound as designed: rewriting `from` forges no signature, it only makes isFromUs() false, so perhapsEncode() skips XEdDSA signing and receivers get an unsigned packet attributed to another node. broadcast_on_channel / broadcast_on_region / broadcast_on_preset were a second way to name a beacon destination alongside broadcast_targets, chosen silently on whether broadcast_targets was empty. The comments claimed the two were equivalent; they were not. An inline ChannelSettings carries name and PSK, so broadcast_on_channel could transmit on a channel absent from the node's channel table, which channel_index cannot express. That is dropped deliberately - the channel must exist on the node. Empty broadcast_targets now synthesises one target on the running preset and region over the primary channel, matching what the scalar path produced when left unset, so an otherwise unconfigured node still beacons. The USERPREFS_MESH_BEACON_ON_* keys go with the fields. A preconfigured build that still defines one now fails at compile time with a pointer to the USERPREFS_MESH_BEACON_TARGET_0_* equivalents, rather than silently losing its beacon channel. The replacement names a channel-table slot, so such a build must also provision that channel. MeshBeaconConfig shrinks 324 -> 240 bytes and ModuleConfig 328 -> 244, against the 512-byte MAX_TO_FROM_RADIO_SIZE ceiling that FromRadio sits 2 bytes under. The protobufs submodule points at a branch carrying both proto changes; it needs re-pointing to master once meshtastic/protobufs#1047 and #1048 merge. * Point protobufs submodule at master now that the beacon protos are merged meshtastic/protobufs#1047 and #1048 are in master, so drop the temporary beacon-proto-integration pin. MeshBeaconConfig stays 240 bytes and ModuleConfig 244, unchanged from the integration branch. The bump also picks up master's unrelated additions: the MESHNOLOGY_W12 and MESHPAGER_X2 hardware models, and a ground-speed unit correction in Position. |
||
|
Ben Meadors
|
63f0f1edd0 |
fix(nodedb): clear the whole LocalModuleConfig when installing defaults (#11627)
installDefaultModuleConfig() memset sizeof(meshtastic_ModuleConfig) - the
368-byte union-backed wire oneof - over `moduleConfig`, which is a
meshtastic_LocalModuleConfig: 1092 bytes with every submessage inlined. The
function assigns only the fields it cares about and relies on that memset to
zero the rest, so every byte past offset 368 that it never assigns kept its
previous value across what is supposed to be a full reset.
installDefaultConfig() directly above already used the correct
sizeof(meshtastic_LocalConfig); only the module variant was wrong.
statusmessage is the field this shows up on. It sits at offset 609 and is
never assigned by the defaults installer, so it survives both routes into
installDefaultModuleConfig():
- moduleConfig.version < DEVICESTATE_MIN_VER -> "old, discard". The decode
succeeded, so the complete old config is in RAM and its statusmessage
survives the discard verbatim.
- loadProto() failure -> whatever a partial decode wrote there survives
(loadProto itself clears correctly, using the caller's objSize).
node_status is char[80]. When the surviving bytes carry no NUL, nanopb
refuses the field ("unterminated string"), pb_encode_to_bytes() returns 0 and
PhoneAPI::getFromRadio() returns 0. config_state has already advanced, so the
frame is never retried - and 0 is the client's end-of-data sentinel, so the
rest of the config dump goes with it and the client never receives
StatusMessageConfig.
traffic_management is not affected: installDefaultModuleConfig() calls
installTrafficManagementDefaults(), which reassigns the whole submessage and
its has_ flag regardless of the memset size.
Also add has_traffic_management to the has_* list in saveToDiskNoRetry() for
consistency - it was the only module config missing from it.
|
||
|
Thomas Göttgens
|
9fbc176e91 |
Extend userPrefs coverage to the whole channel table and the missing config fields (#11624)
* Extend userPrefs coverage to the whole channel table and the missing config fields initDefaultChannel() handled only indices 0-2, so USERPREFS_CHANNELS_TO_WRITE above 3 produced live secondary channels carrying the public default PSK; it now covers all eight slots, with bin/platformio-custom.py completing every field of a configured index so indices 0-2 stay byte-identical. Adds USERPREFS_CHANNEL_<n>_IS_MUTED, USERPREFS_CONFIG_DEVICE_REBROADCAST_MODE, USERPREFS_CONFIG_DEVICE_NODE_INFO_BROADCAST_SECS, USERPREFS_CONFIG_LORA_CONFIG_OK_TO_MQTT, USERPREFS_CONFIG_SECURITY_IS_MANAGED and USERPREFS_CANNED_MESSAGES, applied after installRoleDefaults() and validated the way AdminModule validates a set-config. Adds test_userprefs_channels, covering the configured table under coverage-channel-table and the stock defaults under every other env. * Address review: hex channel count, PSK width assert, canned-message termination USERPREFS_CHANNELS_TO_WRITE now parses 0x-prefixed hex, matching the format userPrefs.jsonc documents, without int(x, 0)'s rejection of a leading-zero decimal such as "03". A static_assert rejects a USERPREFS_CHANNEL_<n>_PSK literal wider than psk.bytes, which memcpy would otherwise write over the fields after it. The USERPREFS_CANNED_MESSAGES copy keeps strncpy's zero-padding and terminates explicitly, rather than shortening the length, which would have left the last byte unwritten. |
||
|
|
cd6ac90f7e |
Add waypoint & geofence support with notifications for BaseUI and InkHUD (#10920)
* Implement GeofenceModule for waypoint crossing notifications and integrate with existing modules * Waypoint Applet Initial Support on InkHUD * undo tile change * Update screen when Waypoint shows or dissapears * Merge branch 'develop' into waypoint-geofence * Geofence on InkHUD * Update MapTile.h * Update WaypointStore.cpp * Notifications * remove GF from waypoint screen * Prevent Focus from closing the notifiaction banner * Trunk fix * cleanup * undo merge conflix mistake * Waypoint screen on BaseUI * Focus preserve fix * UI bugs * Allow Inkhud to remove waypoint * Respect Locked Waypoints * Trunk fix * Update WaypointStore.cpp * Use 8-digit hex formatting for waypoint IDs. 0x%x was inconsistent with the repo's own convention (0x%08x for 32-bit IDs, used elsewhere in this file). Fixed here and in two other spots I found with the same issue (WaypointModule.cpp, GeofenceModule.cpp). * Update ExternalNotificationModule.cpp * Reject invalid surrogate codepoints in waypoint icon rendering * Update WaypointModule.cpp * Update WaypointStore.cpp * Update WaypointStore.cpp * Update WaypointStore.cpp * trunk fix * fix warnings * power.h rename to Power.h * Update Power.h * Fix executable bit on bin/lint-ifdef-complexity.sh Lost during a prior merge from develop (Windows checkout doesn't preserve file mode), causing "execve failed: Permission denied" in the Trunk Check Runner CI job. develop has this file at 100755; restoring that here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Update README.md * Clean up waypoint and geofence integration * Minimize waypoint and geofence implementation * removed unnecessary gating * Geofence alert * trunk fix * Update test_main.cpp * Update WaypointStore.cpp --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Ben MeadorsandClaude Opus 5
|
98c88d7e19 |
fix(position): halve the stationary/fixed-position broadcast floor to 6h (#11606)
The 12h floor introduced with traffic management was too aggressive: a fixed_position or stationary node goes quiet for half a day after its boot broadcast, so anything that missed that one packet - a node that joined later, or one that restarted - shows it with no position until the next refresh. Drop the floor to 6h, and drop the traffic-management identical-position dedup window from 11h to 5h with it. The two are a pair: the dedup window was deliberately sized just under the broadcast floor so a stationary node's periodic refresh clears its neighbours' window instead of being dropped as a duplicate. Leaving it at 11h would have made the extra broadcast pure airtime - aired, then discarded by every receiver - so the mesh would still have seen a 12h refresh. Role caps are unchanged and still bind: tracker 1h, lost-and-found 15m. Both remain shorter than the new 5h default, so those exceptions apply exactly as before. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Ben MeadorsandQuency-D
|
bfd1e1a231 |
Add Heltec RC32, RC52 and RCC6 boards, and LC760CA GNSS support (#11572)
* refactor(graphics): select Arduino_GFX panels with a capability flag TFTDisplay tested `defined(HACKADAY_COMMUNICATOR)` in a dozen places to mean "this panel is driven by Arduino_GFX rather than LovyanGFX". Every new Arduino_GFX board had to be appended to all of them. Move the decision into the variant as USE_ARDUINO_GFX so the display code stops naming individual boards. No behaviour change: the Hackaday Communicator is still the only board that sets it. * feat(boards): add Heltec RC32, RC52 and RCC6 Three boards around the same 128x220 NV3001B panel: RC32 (ESP32-S3), RCC6 (ESP32-C6) and RC52 (nRF52840). They differ only in how the panel bus is wired, so they share one branch in TFTDisplay behind TFT_NV3001B. RC32 and RC52 also carry a rotary encoder on a TCA6408 I2C expander. That lands as its own input source rather than as board conditionals inside i2cButton, which is the M5Stack UnitC6L button driver and stays untouched. On RC52 and RCC6 the panel is an add-on module, so probe it before reporting a screen. The probe reuses the bit-banged SPI helper that already backs the T114 ST7789 check. Arduino_GFX is pinned to the upstream commit that added the NV3001B driver; it has not shipped in a tagged release yet. Co-Authored-By: Quency-D <55523105+Quency-D@users.noreply.github.com> * feat(gps): detect and configure the LC760CA GNSS module The LC760CA is another Unicore part, so it joins the $PDTINFO probe family and reuses the CM121 message-rate setup. It answers with CC1161W. GNSS_MODEL_LC760CA goes immediately before GNSS_MODEL_GENERIC_NMEA: the sentinel has to stay last because isValidGnssModel() uses it as the exclusive upper bound on values the probe cache may hold. Placing the new model after it would leave LC760CA permanently uncacheable. Co-Authored-By: Quency-D <55523105+Quency-D@users.noreply.github.com> * fix(graphics): re-init the NV3001B after the panel rail comes back DISPLAYOFF de-asserts VTFT_CTRL, which cuts power to the panel, so the controller loses MADCTL, COLMOD and gamma. displayOn() only sends sleep-out and cannot restore them, leaving the panel dark or in the wrong format after wake. Re-run begin() once the rail has settled, and repaint in full since the re-init leaves display RAM undefined. Also stop the TCA6408 rotary polling from two threads at once. Registering as an InputPollable meant InputBroker's pollSoon task could call pollOnce() while runOnce() was mid-transfer on the main thread, with nothing serialising Wire or the decoder state. Drop InputPollable and have the interrupt wake the thread instead, the way ButtonThread does, so the bus and the decode stay on one thread. * fix(graphics): skip the NV3001B wake when re-init fails begin() reports whether the bus came up. Ignoring it meant a failed re-init still lit the backlight and drove a full-screen repaint at a panel that was never initialised. * chore(boards): ship the Heltec RC boards at release level release is the normal level for a variant; the matrix generator still builds each of these in this PR because they add a new platformio.ini. --------- Co-authored-by: Quency-D <55523105+Quency-D@users.noreply.github.com> |
||
|
|
0b906b4d15 |
T-Watch Ultra support (#8171)
* feat: T-Watch Ultra support * fix init touch controller * add framebuffer * update to device-ui * trunk fmt * update amoled driver reference * PMU cosmetics * power off lora * fix NodeDB defaults * trySetRTC when fixedPosition * haptic touch (only BaseUI) * init lora RF switch * update LovyanGFX 1.2.19 * earlyInitVariant() adaptations acc. #9438 * update device-ui / touch handling * Set NFC_CS disabled on boot * Get t-watch-ultra working better on BaseUI * Fix compilation * Fix flash reads on t-watch-ultra * Get baseui drawing to the screen correctly again on t-watch and add touch IRQ handling * Add PMU IRQ handling * Add IMU support * Change define to avoid collision * BaseUI changes to support t-watch-s3 rounded screen (#10786) * BaseUI changes to support t-watch-s3 rounded screen * Extend margin work to CannedMessages * Finish merge * Get audio working on watch-ultra * trunk fmt * added custom_meshtastic boilerplate * T-Echo-Plus: disable BHI260AP while assumingly not implemented * Drop the duplicate origBold declaration from the merge * Inset incoming message bubbles on rounded screens * Fix RTTTL tempo, WiFi screen margins, PMU guard and a duplicate define * fix compile errror (the 2nd time) * fix SDcard * fix/workaround CO5300 pixel flush to SPI * trunk fmt --------- Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> |
||
|
Clive BlackledgeandClaude Opus 5
|
389559bddb |
fix(NodeDB): re-derive my_node_num when ensurePkiKeys() mints the identity keypair (#11426)
* fix(pki): re-derive NodeNum when setting a region mints the identity key
A node's mesh address is derived from its identity key:
my_node_num == crc32Buffer(config.security.public_key.bytes, 32)
NodeDB::createNewIdentity() is what establishes that, and NodeDB::
generateCryptoKeyPair() is the only thing that called it.
CryptoEngine::ensurePkiKeys() generates or re-derives the keypair and writes
security.public_key, security.private_key and user.public_key - but never
re-derives my_node_num. Boot-time keygen is suppressed while the LoRa region is
UNSET (generateCryptoKeyPair()'s regionBlocksKeygen guard), so on a fresh device
my_node_num is still the MAC-derived value from pickNewNodeNum(). The user then
sets the region - the stock onboarding flow - ensurePkiKeys() mints a key, and
the invariant is broken.
The node then signs its broadcasts (Router.cpp signs when !pki_encrypted &&
(owner.is_licensed || isBroadcast(p->to))). Every receiver runs
verifyFirstContactNodeInfo, fails crc32Buffer(user.public_key) != p->from, and
drops the NodeInfo. The node's identity beacons are invisible to the mesh.
Nothing reboots to repair it: AdminModule sets requiresReboot = false for LoRa
changes ("All LoRa radio changes apply live via configChanged observer") and
MenuHandler ends at service->reloadConfig(changes).
Four call sites reached ensurePkiKeys():
1. AdminModule set_config LORA, region first set (phone app - the common path)
2. MenuHandler applyLoraRegion (on-device region picker)
3. InkHUD MenuApplet applyLoRaRegion (schedules a reboot, so it
self-healed at next boot)
4. portduino wasm wasm_set_region
The reference implementation was already in the tree: the *licensed* branch of
call site 1, thirteen lines below the broken unlicensed one, calls
nodeDB->generateCryptoKeyPair() (which reaches createNewIdentity()) and widens
the persisted mask with SEGMENT_DEVICESTATE | SEGMENT_NODEDATABASE.
Rather than repeat that at four call sites, the key-mint is routed through one
chokepoint that owns both halves of the identity: NodeDB::ensurePkiIdentity()
calls crypto->ensurePkiKeys() and then createNewIdentity(). It lives in NodeDB
because createNewIdentity() operates on the devicestate/node-DB globals, which
CryptoEngine deliberately does not touch - ensurePkiKeys() takes the security
config and user by reference precisely so it stays free of that dependency, and
it is unit-tested against a standalone CryptoEngine.
ensurePkiIdentity() returns true only when my_node_num actually moved
(createNewIdentity() early-returns when the key is unchanged, so a repeat region
change does not disturb the self entry or force a needless flash write). Callers
use that to widen their save mask; my_node_num lives in devicestate and the self
row moves in the node DB, so both segments must be persisted or the fix would
revert at the next boot. SEGMENT_CONFIG, which carries the key itself, is
already unconditional on all four paths.
The InkHUD reboot is left as-is. It is now redundant for this invariant, but it
covers the rest of that menu's behaviour and a redundant reboot is not a bug.
Adds test_handleSetConfig_persistsUnlicensedFirstRegionIdentity, the unlicensed
twin of the existing licensed test, asserting both the segment mask and
my_node_num == crc32(public_key).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* style(NodeDB): trim identity-recovery comments and guard the WASM nodeDB deref
Two review asks, no behaviour change on any built target.
Copilot flagged the unguarded nodeDB deref in the WASM region setter; it is the
only ensurePkiIdentity() call site that did not check the pointer first.
The rest is comment length. AGENTS.md:83 caps code comments at two lines, and the
identity-recovery comments across the four call sites plus the NodeDB.h doc block
ran to four and six lines. The rationale they carried is in the commit messages
and the PR body, which is where AGENTS.md says it belongs.
The PR's own fix in AdminModule.cpp is deliberately untouched.
* fix(NodeDB): keep the identity move authoritative when the self record cannot be created
createNewIdentity() removes the old node entry and assigns myNodeInfo.my_node_num
before it tries to create the row for the new number. If getOrCreateMeshNode()
came back null it returned false, so the first-region callers left
SEGMENT_DEVICESTATE and SEGMENT_NODEDATABASE out of the save mask.
The number had already moved in RAM at that point, and the freshly minted key
goes to flash under SEGMENT_CONFIG regardless. The next boot therefore reloads
the old number alongside the new key, which is exactly the
crc32(public_key) != my_node_num break this path exists to prevent, reached
through the error branch instead of the happy one.
Rolling the number back is not an option either, since the key has already been
replaced by the time this runs. So the move is now reported as the fact it is and
the missing self record is logged separately; getOrCreateMeshNode() will recreate
that row on the next contact. Reachable when the self record is absent and the
table is full of protected nodes.
Reported by CodeRabbit on #11426.
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
|
90a6dec3f3 |
fix(NodeDB): require a full 32-byte key when demoting to the warm tier (#11431)
* fix(NodeDB): require a full 32-byte key when demoting to the warm tier meshtastic_User.public_key is a wire `bytes` field with max_size 32, so any size in 0..32 decodes off the air, and nothing validates it on ingress: NodeInfoModule hands the decoded User straight to NodeDB::updateUser, whose PKI gates are all `== 32` and so fall through for a partial key, and TypeConversions::CopyUserToNodeInfoLite then stores it with the short size. demoteOldestHotNodesToWarm() admitted that partial key into the warm tier on a `size > 0` gate. WarmNodeEntry has no length field - it distinguishes "has a key" from "no key" purely by all-zero - so N real bytes plus 32-N zeros become indistinguishable from a genuine key. copyPublicKeyAuthoritative() then hands that fabricated key back with size = 32 and reports it AUTHORITATIVE, and re-admission writes size = 32 into the hot store. From then on updateUser's key pin permanently rejects the node's real NodeInfo, and DMs to it are encrypted to a key nobody holds. Require a full 32-byte key, so a partial one is absorbed as "no key" (nullptr) rather than as a truncated one. WarmNodeStore::place() already treats a null key as keyless and clears the slot's stale key when repurposing it. This aligns the site with its two siblings, which both already gate on `size == 32` (the purge path in cleanupMeshDB and the runtime eviction in getOrCreateMeshNode). The ingress gap - updateUser accepting a 1..31-byte key at all - is a separate, larger change and is left for its own review. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(NodeDB): shorten warm-demotion comment to two lines Repo guideline (AGENTS.md): keep code comments to one or two lines. Retains the non-obvious invariant - warm entries have no key length field - and drops the restated detail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(NodeDB): cover short-key demotion into the warm tier A warm record stores 32 raw key bytes with no length field, so a partial hot-store key is indistinguishable from a real one once demoted. The public_key.size == 32 gate in demoteOldestHotNodesToWarm() is what keeps a truncated key from being laundered into a full-looking warm key, but nothing exercised it. test_migration_dropsShortKeyOnDemotion overflows the hot store with one node carrying a 31-byte key and asserts it lands as a keyless placeholder while a genuine 32-byte key still survives. push() grows a keySize parameter to seed the partial key, and clearWarm() gives the test an empty warm tier, which it needs because the warm store outlives setUp() and a prior run's warm.dat. Verified to discriminate: with the size gate reverted to size > 0 the new test fails on "a 31-byte key must not be demoted as if it were a full key", and passes again once restored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(NodeDB): assert the keyless placeholder carries last_heard The test only proved a warm metadata row survived the demotion, not that the placeholder does the job the nullptr is there for, which is preserving last_heard when the key is dropped. Asserting the value needed the seeds fixing first. Warm entries pack role, protected category and the xeddsa flag into the low 7 bits of last_heard (WARM_TIME_MASK is 0xFFFFFF80), so warm time has 128 second granularity and the old seeds of 1, 2, 3 all quantised to 0. They are now multiples of 128, which keeps the demotion ordering identical and makes the values survive the round trip. Real last_heard is epoch seconds, so this is closer to production than the old counter was. Reads the entry through WarmNodeStore::take() rather than getOrCreateMeshNode(), which does not restore last_heard from the warm tier and would have been asserting a path that does not exist. Reported by CodeRabbit on #11431. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
|
c308d0aca4 |
feat: Support Elecrow ThinkNode M9 (#10908)
* thinknode-m9 variant * move lora to SPI1 device * enable SDcard * use HSPI * BaseUI tft -> HSPI * buzzer, webdav lib * fix build issues * M9 default to MUI, no BT, short ringtone * add keyboard long-press config * update variant * add ThingNode-M9 GPS string * GPS 115200 baud * Basic BaseUI support * Fixup power detection * Compass and KB fixes for M9 * add timed Lock::lock() * add SD card * point device-ui to thinknode m9 draft branch * trunk fmt * fix FusionCompass * Fix t-deck-tft linker arg list overflow in CI * SDcard/lora fix: SPI1 must not be declared twice in arduino 3.x -> reuse SPI1 defined in FSCommon.cpp * update battery parameters * reinit SD card when updating; fix PSRAM size * update lib versions * fix wakeup on key press (KB_INT) * fix default nag_timeout for TFT/MUI devices with buzzer * increase PSRAM and SD freq * trunk fmt * update lovyanGFX 1.2.26 * update device-ui commit reference * fix screen definition * remove DONE; maybe a keyword or other used identifier * fixed CI error nag_timeout * fix prepareSleep initialization * trunk fmt * reduce SD SPI frequency * update device-ui * fix SDcard issue * stage * fix device-ui commit reference * fix device-ui commit * update device-ui commit (fixed keyboard lag) * fix QMI8658 * trunk fmt * update .ini meta information, align SD freq * fix device-ui reference to target (ready to merge) * device-ui for all other targets * make the rabbit happy * trunk fmt * fixed lock screen * fix compile error * SPI lock timeout * apply device-ui fix * revert bad RadioLib commit hash in platformio.ini Co-authored-by: mverch67 <71137295+mverch67@users.noreply.github.com> * fix wrong commit hash change * fix fix commit fix * I love changing random numbers in random files --------- Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> |
||
|
James Rich
|
51eadb77d4 |
fix(NodeDB): reset a persisted event firmware_edition on vanilla builds (#11504)
* fix(NodeDB): reset a persisted event firmware_edition on vanilla builds myNodeInfo lives in devicestate, which survives a firmware reinstall, and the boot-time edition stamp was compiled out entirely on builds without USERPREFS_FIRMWARE_EDITION. A device flashed from an event build back to vanilla therefore kept reporting the event edition forever, and clients kept its branding until a factory reset. Stamp VANILLA in the else branch so the running build is always the source of truth. * Stamp the edition before the boot save decision, and assert the on-disk value Review follow-up: the stamp sat after the devicestate CRC compare, so an edition-only change stayed RAM-only and the persisted event edition survived on disk. Move it next to the other running-build-wins fixups (device_id, min_app_version), which run inside the CRC window, and extend the test to read device.proto back so the persisted value is asserted too. |
||
|
Tom
|
f5314148c2 |
Serialise AirTime behind a lock, and stop handing out its buckets (#11362)
* Copy airtime reports into a caller buffer instead of exposing the array
airtimeReport() returned a pointer into the rotating bucket arrays, so the
caller held a handle to state that logAirtime() and every accessor mutate
underneath it. Copy into a caller-supplied buffer instead, and report failure
for a null buffer, a count past the log depth, or an unknown report type.
ContentHandler owns its buffer and hoists getPeriodsToLog() out of the three
calls that repeated it.
* Cover the AirTime report API and log-dispatch contract
Half of AirTime's surface had no tests: which store each report type feeds,
what airtimeReport() does when misused, how the first sync seeds itself, and
whether calling several entry points in one interval compounds the rotation.
Eighteen tests, asserted through the public API rather than the public bucket
arrays - those arrays are meant to become private, and a test that reads them
would have to be rewritten rather than pinning a contract.
Two of them state a convention that was never written down: the report arrays
are shift-ordered with slot 0 newest, and slot 0 covers only the time since the
last rotation. channelUtilization and utilizationTX use the opposite convention
- a modular ring indexed by uptime phase - and reading one as if it were the
other is a defect that has already happened once.
* Characterise AirTime window decay, TX gates, and sleep behaviour
Thirty-three tests in three kinds. Invariants must hold forever; boundaries pin
off-by-ones a refactor would move; five characterisations encode today's wrong
numbers, each tagged with the phase that will flip it.
Readings are asserted against an event-log oracle - airtime physically on air
inside (now - window, now], computed from a list of completed packets - rather
than against hand-worked constants, so a test states "this matches the
definition" instead of "this looked right when I wrote it".
The characterisations, all measured rather than assumed:
- the window covers (N-1)p + phase but divides by Np, so a steady 10% load
reads 8.33% right after a bucket boundary -> phase 5
- the same load sweeps across bucket phase instead of holding -> phase 5
- the hour window carries the same defect, 10x smaller -> phase 5
- a packet longer than its bucket is credited whole to the bucket
it completed in, so a saturated LONG_SLOW channel reads >100% -> phase 4b
- getSilentMinutes() reads a modular ring as if the index were an
age, so identical airtime gives different answers by phase -> phase 6
Two tests needed correcting during the write, both my expectations rather than
the code: a six-bucket ring sheds whole buckets, so a 30s gap drops three of
five survivors and not "half"; and the oracle sees 59 completions in a 60s
window, not 60, because the one on the lower edge is outside it.
Not written: the planned RX_LOG/RX_ALL_LOG disjointness test. That is a
property of the two radio drivers, which choose one or the other per packet -
it is not observable from AirTime, which records what it is told. The
AirTime-side half is already covered by the routing tests.
* Drop write-only and undefined AirTime members
None of this was reachable:
air_period_tx / air_period_rx file-scope mirrors of airtimes.periodTX/RX,
accumulated, rotated and memset in lockstep
with them but never read out or serialised.
Orphaned when #2552 re-pointed the writes at
bare globals instead of deleting them.
lastUtilPeriod, lastUtilPeriodTX written on every sync, read nowhere
airtimes.lastPeriodIndex written on every rotation, read nowhere
currentPeriodIndex() computes (secs / 3600) % 8 - a modular-ring
index for the one array that is shift-ordered
rather than a ring. Its only two uses were the
dead field above and a log line. It is the
fossil of the same confusion that makes
getSilentMinutes() wrong.
UtilizationPercentTX() declared, never defined
free logAirtime()/airtimeReport() declared, never defined; the latter still
carried the array-returning signature the
previous commit removed, so it actively misled
Also fixes the rotation log line, which read currentPeriodIndex() from inside
the loop although the index is advanced before it - on a multi-hour wake it
printed the same final value once per rotation. It now reports which of the
crossed hours is being rotated.
airtimeRotatePeriod() is kept: it has no caller in the tree either, but unlike
the above it is a defined public method, so out-of-tree callers are plausible.
Measured, not estimated: sizeof(AirTime) 464 -> 456 B, plus 64 B of globals, so
-72 B of static RAM. Padding accounts for the difference from the 66 B the plan
predicted by counting declared bytes.
The whole point of writing the tests first: the suite is green here with zero
test changes.
* Document what the AirTime figures measure and how they are stored
Comments only, but four of the things they replace were false.
The header's example analytics claimed RX_ALL_LOG was "all received lora
packets" and offered "RX_ALL_LOG - RX_LOG = other lora radios". Both radio
drivers pick exactly one of the two per packet, so they are disjoint: RX_ALL_LOG
is airtime we could not parse, the subtraction can go negative, and the total is
TX + RX + RX_ALL. Replaced with the actual contract - four inputs, eight
outputs, the window each spans, and the fact that the three thresholds are
hard-coded members rather than the settings they look like.
Names the two storage conventions on their declarations, because mixing them up
is what makes getSilentMinutes() wrong: channelUtilization and utilizationTX are
modular rings indexed by uptime phase, where the oldest bucket is (current + 1)
% N; airtimes.period* is shift-ordered with slot 0 newest, where the index IS an
age and slot 0 is a partial hour.
Defines the measurement as wall time rather than awake time, and says why: a
sleeping node still hears traffic, and per-node redefinition would make two
broadcast readings incomparable. Records that the 60s figure is published to the
mesh at >= 1h cadence, so what other nodes see is a snapshot - at LONG_FAST and
1% occupancy it reads exactly 0 in about 44% of reports - and that the contention
window it feeds moves in 20-percentage-point steps, so small errors never reach
the backoff.
Finally, states that rotation happens on access rather than on the scheduler
tick, names the test that enforces it, and leaves a TODO pointing at the plan
phases that fix the characterised accuracy defects.
* Serialise AirTime behind a lock proven by a private token
Two mechanisms solving different halves. A lock-free inner core (Windows) holds
all state and all logic; it has no lock and no way to reach one, so nesting is
impossible by construction. A private Held token takes the lock in its own
constructor and is the only thing that can be passed where a core method demands
one, so the lock cannot be forgotten either.
The rule is now uniform with no exceptions to remember: every public method
takes the lock once and delegates. In particular isTxAllowed*() lock like
everything else - before the split they could not, because they called the
public accessors and the lock is not recursive. That asymmetry was the foot-gun
the previous design documented in prose and hoped nobody would trip.
getPeriodsToLog()/getSecondsPerPeriod() still take no lock; they return
compile-time constants and touch no state.
channelUtilization[] and utilizationTX[] were public, so the lock was bypassable
at compile time. They move into the private core. Four test sites reached in;
all four now use logAirtime() plus the virtual clock, and no new test seam was
needed. Nothing in src/ was affected.
The re-entry assert is guarded on PIO_UNIT_TESTING, so it exists in test builds
only. The design sketched #ifdef DEBUG, but nothing in this tree defines DEBUG
or NDEBUG, so either spelling ships the assert to every board - and
nrf52_promicro_diy_tcxo has ~128 bytes of headroom under its 0xEA000 warm-store
cap, which the assert's strings and abort path overrun. It would have worked on
hardware, since the check runs in Held's owner initialiser and so precedes the
blocking take; the objection is that abort()ing a live mesh node is a poor trade
for a bug never seen in the field. Native tests are where it earns its keep
anyway: Portduino compiles Lock::lock() to an empty body, so a nested take there
succeeds silently and nothing else would notice.
Also comments out ScopedBusyAirTime in test_traffic_management. It is inert
twice over: the module holds no reference to airTime at all since hop exhaustion
was shelved, and the fixture never worked anyway - writing the buckets on a
fresh AirTime is undone by the first accessor call, which takes the firstTime
branch and memsets them. It reported 0%, not the 100% it claimed. Left in place,
commented, with both reasons recorded.
Cost on the tightest board in the tree, nrf52_promicro_diy_tcxo: the six phases
together add 96 bytes of flash, leaving it 32 bytes clear of the warm-store
guard. RAM is 72 bytes lower from the dead-state removal. Suite green at 47/47,
with test_airtime unedited apart from the added nesting test.
* Count rotations with the loop variable, not a separate tally
LOG_DEBUG compiles to nothing under DEBUG_MUTE, so the counter's only read
disappeared with it and the tally became write-only. It does not warn today -
this build has -Wunused-but-set-variable on, and it fires for other locals, but
not for one that is only initialised and never read - so it was latent rather
than broken: a stricter flag or -Werror would have failed muted builds only.
Using the loop variable removes the class of problem, since the loop condition
reads it, and drops the elapsedAirtimePeriods-- mutation as a side benefit.
Same iteration count, same output.
Found by compiling nrf52_promicro_diy_tcxo with -D DEBUG_MUTE, which is worth
recording for its own sake: muting logs takes that image from 802 784 to
673 416 bytes, 98.5% to 82.6% of flash. Logging is 16% of the largest nrf52
image, and its 32 bytes of warm-store headroom are a logging-verbosity question
rather than a code-size one.
* Tighten the comments added by this branch
Comment-only: with comments stripped, all five files are byte-identical to the
previous commit.
Removed the references to the planning notes. Those documents are working
material and will go stale; the code should not depend on them. The five
CHARACTERISATION tags now describe the defect they pin and stop there, and the
accuracy TODO names the four defects and points at the tests instead of a plan
file.
Also removed, as noise rather than information:
- comparisons against pre-#11291 behaviour, which nobody reading this needs
- a comment describing the lock restructure as future work, written before it
landed
- speculation ("plausible", "worth pinning so a future...")
- an aside arguing with an arithmetic slip made while writing the test
Kept the mechanical facts that are slow to re-derive: the two storage orderings
and which array uses which, RX_LOG/RX_ALL_LOG disjointness, the locking rule and
the addSpanned() constraint that protects it, why the re-entry assert is
test-only, and the concrete numbers - (N-1)p + phase, 14 164 ms, the 20 pp
contention-window steps.
Net 16 comment lines out of src/, 33 out of test/.
* Gate the AirTime re-entry check on the host, not on testing
PIO_UNIT_TESTING is injected by PlatformIO purely on BUILD_TYPE, with no
platform check, so it is defined on an on-target `pio test` run too. The
check arms before the lock is taken - a nested take blocks forever, so a
later check would never run - which under preemption false-positives on
legitimate contention and races on its own write.
Derive AIRTIME_REENTRY_CHECK once from PIO_UNIT_TESTING && !HAS_FREE_RTOS
and use it at all three sites. Had the three conditions ever diverged, an
on-target test build would fail to compile on a member the header no
longer declares.
* Log AirTime outside the lock it serialises
DEBUG_PORT.log() blocks on a UART write, and `lock` is a plain binary
semaphore with no priority inheritance, so holding it across a log call
lets the main thread stall the radio thread in getTxDelayMsec().
Move logAirtime()'s LOG_DEBUG into the shell, after the Held scope
closes; the shell already has both arguments, so nothing has to be
passed back out of the core. isTxAllowed{ChannelUtil,AirUtil} read into
a local under the lock and warn after it. The log bodies are braced
because LOG_DEBUG compiles away under DEBUG_MUTE and a bare `if (x) ;`
trips -Wempty-body.
Fold the two doubled index calls into `+=` while touching the lines.
* Give each airtime report its own buffer
handleReport() reused one array across the three airtimeReport() calls
and ignored the bool. A failed report would have left the previous
type's data in place and emitted it under the next type's key. Build
each through a lambda whose buffer is zeroed per call, so a failure
emits zeros.
Unreachable today - the count is always PERIODS_TO_LOG and the type is
always valid - but the old shape only read as correct by accident.
* Drop a stray semicolon from the inert-guard comment
* Address external review: name the race, tighten the claims and the tests
The header sold the lock as mechanism without naming a second thread, which
invites the reasonable objection that this is a cooperative OSThread codebase.
There is a real race and it is nRF52-only: NRF52Bluetooth registers its ToRadio
write callback with defer == false, so a phone's packet runs handleToRadio ->
sendToMesh -> Router::send on the Bluefruit BLE task, reading
utilizationTXPercent() and getSilentMinutes() while loopTask may be inside
logAirtime(). ESP32 hands BLE work to the main task and does not have it.
Three claims in the header were wrong or overstated:
- "nesting is impossible by construction" - Windows is a nested class with an
enclosing class's access rights, and `extern AirTime *airTime` is in the
same header, so airTime->anyPublicMethod() from inside it is well-formed
and would hang. Nothing does it; the assert is the backstop. Say that
instead, because the comment below instructs contributors to add helpers
to Windows on the strength of the guarantee.
- "every public method takes the lock exactly once" - two constant accessors
take none and isTxAllowedAirUtil() takes it zero or one times. State the
exceptions where the invariant is stated, not only at the definitions.
- "both radio drivers pick exactly one per packet" - five drop paths log
neither. At most one. Recorded against plan4 rather than fixed here: it
changes a telemetry value.
getPeriodsToLog()/getSecondsPerPeriod() become static constexpr, which removes
them from the locking claim structurally and lets ContentHandler size its
buffer and its count from one constant.
Tests:
- C14's saturated AirTime is installed by a helper and restored in tearDown.
Unity's TEST_ABORT() is longjmp and does not run destructors of automatic
objects, so the scoped guard it replaces would leave airTime dangling into
an abandoned frame on any assertion failure - and the same commit that
added it removed the tearDown reset that did cover that.
- test_getSilentMinutes_counts_minutes_until_enough_ages_out asserted only
`mins <= 60`, which neither return path can violate. The answer is 59.
- test_backwards_uptime_degrades_safely stepped 600s -> 60s, which leaves
elapsedAirtimePeriods at 0, so it never reached the hourly-report branch
its own comment describes. Step by the wrap instead and assert the exact
figures.
- test_airtime leaked EU_868 out of the duty-cycle case into every later one,
and the reentry test's isTxAllowedAirUtil() coverage depended on it.
Restore the region in tearDown and set it explicitly where it is wanted.
- Rename that test to what it can actually check: no single method takes the
lock twice. The calls are sequential, so it cannot catch two methods
nesting.
* trunk: suppress trufflehog/Lob false positives in test_airtime
* Address CodeRabbit review: the rotate trace, the cap warn, the backoff
Four findings from the CodeRabbit pass. Two were introduced by this branch,
one is a real inconsistency it inherited, one is a naming slip.
The rotate trace was the one that mattered. "Log AirTime outside the lock it
serialises" moved the per-packet lines and the two TX-gate warnings out to the
shell, but missed LOG_DEBUG("Rotate airtimes, crossed hour %u") because it does
not sit in the shell at all: it is inside Windows::syncNow(), the lock-free
core, which by construction only ever runs under Held. Nothing at that line
looks like a lock, which is why it survived.
The exposure is smaller than the review suggests - runOnce() syncs at 1 Hz, so
in steady state this is one line an hour, and the PERIODS_TO_LOG - 1 burst
needs an hour of light sleep with no intervening sync - but a UART write under
a plain binary semaphore with no priority inheritance is exactly what the
comment above logAirtime() says this code does not do. syncNow() now
accumulates crossings in rotationsPendingLog and runOnce() drains it inside the
Held scope, then logs after release. Any caller can cross an hour; only that
thread reports it, so a crossing raised elsewhere is traced at most one tick
late. The `if (rotations > 0)` guard keeps the drained value read under
DEBUG_MUTE, where LOG_DEBUG expands to nothing - the write-only tally that
"Count rotations with the loop variable" removed.
addFromContact()'s favorite fallback stamped silently when the protected cap
refused it. The stamp is new on this branch; the two sibling refusals (ignore,
verify) both emit PROTECTED_CAP_WARN_FMT, so the operator lost the only signal
that the cap was hit on the one path that has a fallback.
lfs_assert() mixed clocks: Throttle read Time::getMillis(), the remainder was
computed from a second, bare millis(). The review's stated failure mode - a
native test overriding the clock - cannot happen, since the hook is behind
PIO_UNIT_TESTING and this file is nRF52-only. The real defect is the second
read: a tick landing on the 20-minute boundary between the check and the
subtraction underflows the remainder into delay(~50 days), on a device that has
just found its flash corrupt. One read, clamped, and preFSBegin() stores from
the same clock.
The eviction test is renamed to
test_eviction_prefersCurrentBootStampOverPost2038Epoch. The finding is right
that it was snake_case, but the suggested testEvictionPrefers... does not match
this file either, which is test_<area>_<camelCase> throughout.
Not taken, both pre-existing and out of scope for a rollover branch:
- t5s3_epaper's touchResumeAtMs/suppressFromMs read an active suppression as
inactive if the wake lands in the 1 ms where millis() is 0. Consequence is
one skipped 150 ms touch-settle window per 49.7-day wrap.
- NRF52Bluetooth::onPairingPasskey() busy-waits 30 s in a BLE callback. Worth
saying plainly that this branch makes it more visible: the old
`millis() < start_time + 30000` overflowed at the wrap and cut the wait
short, so the correct Throttle form is what lets it run the full 30 s.
Reworking it into an OSThread is its own change.
Native suite GREEN, 48/48, 672 cases.
|
||
|
|
fdb644e0b7 |
Fix millis() rollover in deadline, interval, and timestamp handling (#11291)
* Add native test coverage for the UptimeClock monotonic seam
src/UptimeClock.{h,cpp} shipped without a dedicated test suite. Port the six
tests from the monotonic-time branch (test/test_time), retargeted to the
renamed header.
The wrap test crosses 0xFFFFFFFF via advanceTestMillis() rather than a second
setTestMillis(): setTestMillis() sets clockSourceChanged, which makes
getMillis64() rebase its accumulator and swallow the wrap.
* NextHopRouter: fix 49.7-day millis() rollover in retransmission timing
Resolves the "FIXME, handle 51 day rolloever here!!!" in
NextHopRouter::doRetransmissions() by switching the retransmission-due
comparison from plain unsigned <= to a signed-difference cast.
The previous p.nextTxMsec <= now comparison silently breaks across the
~49.7 day millis() wraparound: pending retransmissions either stall
for the remainder of the wrap window, or all fire simultaneously at
the rollover boundary. Long-running router/infrastructure nodes do hit
this in practice.
The replacement (int32_t)(p.nextTxMsec - now) <= 0 is the standard
Arduino/embedded idiom for rollover-safe deadline checks and behaves
identically to the original for any non-wrap timing.
* Address Copilot review: use unsigned half-range for rollover-safe retransmit check
Review feedback from @Copilot on PR #10227: casting a uint32_t
subtraction to int32_t is implementation-defined in C++ when the
unsigned value exceeds INT32_MAX (even though it works on typical
two's-complement targets).
Switch to the fully well-defined unsigned half-range form:
nextTxMsec is in the past-or-equal iff (now - nextTxMsec) has not
wrapped past 2^31 ms. Future offsets < 2^31 ms wrap into the top
half and read as 'not yet'.
Same semantics as the signed-cast version on every two's-complement
platform we care about, but portable to any conforming C++ impl.
* Use monotonic time for airtime windows
* Document monotonic airtime windows
* Fix test_packet_signing sentinel that #10227's rollover fix inverts
test_C3_invalid_repeated_packet_cannot_ack_or_change_retry_state parked a
pending packet at nextTxMsec = UINT32_MAX to mean "never retransmit", then
asserted that a rejected repeated packet leaves the retry state untouched.
NextHopRouter::doRetransmissions() now tests whether a retransmit is due with
an unsigned half-range compare, (uint32_t)(now - nextTxMsec) < 0x80000000u,
so that retransmission timing survives the ~49.7 day millis() wrap. Under it
now - 0xFFFFFFFF == now + 1, a small positive delta, so UINT32_MAX reads as
~1ms in the past: the retransmit fires and rewrites nextTxMsec, and the test
failed with "Expected 4294967295 Was 6247".
Use a representable future time instead. Production is unaffected either way -
nextTxMsec is only ever written as millis() + d, and UINT32_MAX came from the
test harness alone - so the sentinel is what needs to go, not the comparison.
Special-casing UINT32_MAX in the retransmit path would keep a value that reads
as "expired" under any wrap-correct compare.
The value is held in a local because millis() advances across
runPipelineIngress(), so recomputing it at the assertion would compare against
a different number.
Reported upstream on meshtastic/firmware#10227, whose branch predates this test.
* Make Throttle time-injectable and add hasElapsed()
Throttle backs ~94 call sites, which makes it the highest-leverage place in
the tree to put the clock seam: reading Time::getMillis() instead of millis()
in its three call sites turns all of them into time-injectable code at once,
without touching any of them. The 32-bit millis() wrap is not otherwise
reachable from a native test.
The read is behaviour-preserving - Time::getMillis() returns millis() unless a
test injects a clock - and the full native suite passes with it live.
Also add hasElapsed(), the complement of isWithinTimespanMs(), because 51 of
the 94 call sites are spelled !isWithinTimespanMs and read poorly. Its
boundary is inclusive (>=) since isWithinTimespanMs uses <; both are
documented. It deliberately does not treat lastExecutionMs == 0 as "never
run": call sites pair that test with the interval check themselves, and
absorbing a sentinel into the one helper every module depends on is exactly
the value-overloading hazard being removed elsewhere.
Migrating the existing !isWithinTimespanMs sites is cosmetic and deliberately
left out of this commit.
test/test_throttle/ covers window semantics, both boundaries, the complement
identity, execute()'s first-run and throttled paths, and - the point of the
exercise - a window opened before the wrap closing correctly after it,
including at the 24h interval that is the longest in the tree.
* Stop disarmed deadline sentinels reaching the comparison
Two deadline variables encoded "inactive" as a magic value that only reads as
"never" because the comparison against it is a naive millis() compare. Under
any rollover-correct comparison both invert to "expired ~49 days ago", so they
have to be untangled before those comparisons can be fixed.
Power::reboot() set rebootAtMsec = -1 on platforms with no reboot
implementation, intending "never fire". Every reader already treats 0 as the
disarm value - powerCommandsCheck() tests `if (rebootAtMsec && ...)`, and
AdminModule writes 0 to cancel - so -1 was both wrong and unnecessary. Use 0.
Left as UINT32_MAX it would reboot-loop the moment the comparison is corrected.
ExternalNotificationModule's nag window compared against nagCycleCutoff, which
holds UINT32_MAX once stopped and 1 at boot. isNagging is the real armed flag,
so test it first and short-circuit: a disarmed cutoff can no longer reach the
arithmetic, while an idle module still takes the same sleep path that the
boot-time value of 1 was relying on.
Note this fixes the sentinel only. The comparison itself is still a naive
`nagCycleCutoff < millis()` and remains on the list to convert.
* Fix millis() rollover in every deadline and interval comparison
Roughly 20 sites compared against millis() directly - `millis() > deadline`,
`deadline < millis()`, `last + interval < millis()`. All of them break for
about 24 days after the 32-bit millis() wrap: depending on which side of the
wrap each value sits, the action either stalls for weeks or fires immediately
and repeatedly. The longest affected interval is the 12 hour NTP renewal, a
~50x margin against the wrap, so none of these needed the range - only the
correct comparison.
Add Throttle::deadlinePassed(deadlineMs) for sites that store an absolute
deadline they cannot re-express as "interval since an event". It uses the same
unsigned half-range test as NextHopRouter::doRetransmissions() rather than
introducing a competing signed-cast idiom, and unlike the signed cast it is
defined for every input. Sites that do store an event use the existing
isWithinTimespanMs / hasElapsed. Nothing gained new state.
Because both helpers read Time::getMillis(), every converted site is now
reachable from a native test that drives the clock across the wrap; the
comparison itself is covered directly in test/test_throttle/.
Sentinel handling is the reason this could not be a mechanical rewrite. The
disarm convention is not uniform: 0 means "inactive" for rebootAtMsec,
shutdownAtMsec, alertBannerUntil, fixHoldEnds, suppressUntilMs and
touchResumeBlockUntilMs; 0 means "due now" for ntp_renew, which is forced to 0
at link-up; UINT32_MAX means "inactive" for nagCycleCutoff; and
alertBannerUntil == 0 in isOverlayBannerShowing() means "show indefinitely".
Every inactive marker is arithmetically far in the past, so a correct
comparison fires on it - each site tests its sentinel before the arithmetic,
and keeps the meaning it had.
Two sites carried a second bug found on the way:
BME680Sensor tested (stateUpdateCounter * STATE_SAVE_PERIOD) < millis(). With
a 6 hour period and a uint16_t counter that product overflows uint32_t after
about 198 saves, independently of the millis() wrap. It now measures the
interval since the last save.
EInkDynamicDisplay had `if (previousRunMs > millis()) return;` as a millis()
overflow guard, which skipped rate limiting entirely for the whole post-wrap
period - the bug it meant to prevent. Every check below it already goes
through Throttle, so the guard is removed rather than fixed.
MotionSensor's calibration countdown is converted to a signed delta rather
than deadlinePassed, because it needs the remaining magnitude and not a
boolean; that matches the already-correct check in the same file.
* Remove getMillis64() and use Throttle for the NodeInfo reply window
getMillis64() had exactly one caller and no callers in tests. It also carried
obligations that made it the wrong shape for this firmware: a wrap accumulator
in mutable statics, which is not ISR-safe, and which must be polled at least
once every ~49.7 days or it silently misses a wrap and returns a time ~49 days
short.
Its one caller only wanted to know whether a 12 hour suppression window had
elapsed - which Throttle answers correctly across the wrap without any
accumulator. NodeInfoModule now stores Time::getMillis() in lastNodeInfoSeen
and tests the window with Throttle::isWithinTimespanMs, so the map holds
milliseconds rather than seconds derived from a 64-bit read.
USERPREFS_NODEINFO_REPLY_SUPPRESS_SECS is user-overridable and now feeds a
multiply by 1000, so a static_assert rejects any value too large to express in
milliseconds instead of letting it wrap.
clockSourceChanged goes too. It existed solely to rebase getMillis64()'s
accumulator when a test swapped clock sources, and it made the wrap untestable
through the injection API: setTestMillis() set the flag, so a wrap crossed by
two setTestMillis() calls was swallowed. With the accumulator gone the flag has
nothing to rebase, and the injection API is a plain settable clock.
The three getMillis64 tests are dropped as they no longer describe anything.
One test replaces them, pinning that advanceTestMillis() wraps past
0xFFFFFFFF rather than saturating, since the Throttle wrap tests rely on it.
Also fix eviction in pruneLastNodeInfoCache(): it picked the entry with the
smallest stored stamp, which is the wrong victim once some stamps sit on the
far side of the wrap. It now evicts the largest elapsed time.
* Add CI guard and docs rule against naive millis() comparisons
Fixing the existing sites does not stop the next one being added. The
millis-deadline-check job rejects millis() placed directly next to a comparison
operator, in either order, anywhere in src/. It lives in test_native.yml
alongside suite-count-check, which sets the precedent for a repo-hygiene guard
that CI enforces and bin/run-tests.sh does not.
The correct idioms all subtract before comparing, so none of them match the
pattern. Line comments are stripped first, so documentation is free to name the
broken form - as the guard's own comment and the coding conventions both do.
Writing the check before finishing the sweep turned out to be worth it: it
found roughly 14 sites that a by-hand audit of deadline variables had missed,
including two extra nagCycleCutoff compares, both boot-screen timeouts, and a
6 hour sensor save interval that was also overflowing a uint32_t multiply.
.github/millis-deadline-allowlist.txt covers the cases that are genuinely not
deadline tests. Both current entries are uptime thresholds - "has the device
been up N ms" - with no stored deadline and no event to measure from: a 30s
button holdoff against phantom shutdown from floating pins, and a 10s window
for the OEM boot logo. Each re-crosses its threshold once per wrap, which is
harmless for boot-holdoff logic and not worth new state to avoid. Entries are
keyed on file plus exact source text, without line numbers, so an edit above an
entry does not silently invalidate it.
Locally the guard reports 19 matches before the sweep and 2 after, both
allowlisted.
The Throttle bullet in the coding conventions is rewritten from "prefer
Throttle for rate limiting" to "never compare against millis() directly", lists
all four helpers with when to use which, names the CI guard, and documents the
sentinel hazard with the rebootAtMsec = -1 case that would have become a reboot
loop. Mirrored into AGENTS.md; CLAUDE.md gets a pointer row.
* Trim rollover comments to what the code needs
The comments added with the millis() rollover fixes carried too much of the
investigation that produced them: how many sites were found, which document
recorded them, what the old code used to do. That belongs in the commit history,
not in the source, and some of it was already stale - Power::reboot() still
described the check it disarms as "a naive millis() > deadline" when that
comparison had been fixed in the same series.
What stays is the non-obvious part at each site: which sentinel value the
variable overloads and what it means there, since that differs between call
sites and is what a correct comparison gets wrong. 0 means "not scheduled" for
rebootAtMsec, "renew now" for ntp_renew, and "show indefinitely" in
isOverlayBannerShowing().
Exposition is kept where it earns its place: the Throttle helpers, the uptime
clock's note on why there is no 64-bit variant, and the tests. The Throttle
docs lose only the site count and the "longest interval in the firmware"
statistic, both of which would age badly; the range trade-off between the two
forms is what a caller actually needs.
Comments only - no code changed, verified by diff.
* possible fixes
* Address review feedback on the rollover fixes
- BME680Sensor: checkpoint lastStateSaveMs after a successful write instead of
at the interval test. The first save (IAQ accuracy >= 2) left it at 0, timing
the next save from boot, and stamping before the write deferred the retry a
full period when the write failed. Reads Time::getMillis(), the same clock
Throttle compares against.
- Throttle: add deadlinePassedAt(now, deadline) for loops that snapshot the
clock once and test many deadlines; deadlinePassed() now delegates to it.
NextHopRouter::doRetransmissions() uses it, replacing the inline half-range
compare adopted from #10227 (nightjoker7) - same arithmetic, credited at the
call site - and takes its snapshot from Time::getMillis() so setNextTx()
deadlines and the due test cannot diverge under an injected test clock.
- test_native.yml: set -euo pipefail in the millis-deadline guard, matching the
sibling suite-count job. Without -e a partially failed scan could report "no
violations" from truncated output.
- test_packet_signing: build the not-due deadline from Time::getMillis() rather
than millis(), so the test and the router read one clock.
- test_throttle: cover deadlinePassedAt(), and correct a wrapped-value comment
(0xFFFFFF00 + 400 is 0x00000090, not 0x00000094).
Two review comments were declined: the AirTime mutex (every airTime-> caller
runs in the single cooperative loop, WebServerThread included) and the
MotionSensor 0-sentinel countdown (the calibration frame is only installed
while a window is open).
clod helped out here
* Correct the described failure window of a naive millis() compare
The comments and agent docs said a bare `millis() > deadline` "breaks for ~24
days after the wrap". That figure belongs to the fix, not the bug: it is the
half-range limit of deadlinePassed(), which reads deadlines more than 2^31 ms
ahead as already passed, and the range over which a UINT32_MAX sentinel reads
as passed.
The naive compare's actual failure is an inversion lasting only while the
deadline sits on the far side of the wrap, so it is bounded by the interval:
the action fires immediately and loses its wait, or blocks for about the wait
it should have performed - days for the nRF52 flash-corruption backoff,
one skipped cycle for a seconds-long retransmit timer.
Comments and docs only; the ~24.8 day statements that correctly describe
deadlinePassed()'s own range are left as they were.
clod helped out here
* Restore a monotonic uptime clock and consolidate the wrap counters
Time::getMillisMonotonic() is the getMillis64() shape - a 32-bit wrap
counter carried across reads - promoted to the shared timebase, with
Time::getUptimeSecs() as the derived whole-seconds view. This deliberately
reverses the earlier removal of getMillis64(), and the distinction matters:
removal was right for a lazily-read accumulator with one rare caller, where
a 49.7-day gap between reads silently swallowed a wrap. Here every read is
the poll and AirTime::runOnce() guarantees one per second; the missed-wrap
contract is pinned by a test rather than left as a footnote.
Three private wrap counters collapse into it:
- AirTime::syncNow() takes its seconds from Time::getUptimeSecs() and drops
its lastSyncMsec checkpoint; window rotation is unchanged.
- DeviceTelemetryModule loses refreshUptime()/uptimeWrapCount/uptimeLastMs;
uptime_seconds comes from Time::getUptimeSecs(), which also removes the
0.296s-per-wrap truncation of (0xFFFFFFFF / 1000) * wraps. Its two
interval checks move to Throttle::hasElapsed().
- HostMetricsModule's copies of those members were never read (its uptime
comes from /proc/uptime) - deleted.
Not ISR-safe (unguarded mutable carry): ISRs keep using getMillis(), which
stays a pure read. Audited: no interrupt-context file reads getTime(),
getValidTime(), or the new accessors.
test/native-suite-count 44 -> 45: the bump for test_uptime_clock was lost
in a branch history rewrite, leaving every later value off by one -
run-tests.sh reports AMBER and CI's suite-count-check fails on the current
push until this correction.
* Anchor the wall clock in monotonic milliseconds
getTime() computed elapsed-since-time-set as a 32-bit millis() delta, so a
node that took time once and stayed up past 49.7 days reported a wall clock
one full cycle in the past - and last_heard, rx_time, message and position
stamps all inherited it. The anchor is now the 64-bit monotonic count
(timeStartMsec -> timeStartMs64) and the elapsed term is computed in 64-bit,
so the wall clock is exact at any uptime.
All six anchor writers follow: the five hardware-RTC read branches and
perhapsSetRTC(), which keeps a truncated 32-bit copy of the same instant for
its Throttle-checked rate-limit stamps. The test seams anchor the same way.
Two native regression tests drive getTime() across the wrap through the
Time seam - one anchored before the wrap and read after it, one anchored
after a counted wrap - with the test epoch derived from BUILD_EPOCH so the
plausibility window cannot rot as the build date advances.
* Stamp the rx_time placeholder in monotonic uptime seconds
computeRxTimeStamp() stamped Time::getMillis() when the clock was untrusted,
and reconcilePendingRxTimes() back-calculated with a 32-bit millis() delta -
correct within one wrap, but a placeholder older than 49.7 days aliased to a
small elapsed value and reconciled to a plausible-but-wrong recent epoch:
the exact failure has_rx_time exists to prevent, reachable by an ordinary
unattended router whose phone connects two months in.
The placeholder is now Time::getUptimeSecs(). Both stamps come off the
monotonic counter, so the elapsed term is exact at any age and the aliasing
window is gone outright rather than widened. If elapsed somehow exceeds the
epoch itself, the packet stays un-dated (absent, never wrong) instead of
clamping to a pre-1970 value. Defence in depth: a placeholder that leaks
needs ~50 years of uptime to cross MIN_PLAUSIBLE_EPOCH, where milliseconds
took 18.3 days.
The stream-API reconciliation tests keep their scenarios with the placeholder
unit switched, and ScopedTimeFixture resets the monotonic carry so uptime
seconds are deterministic per case.
* Date nodes heard before the clock arrives, without polluting last_heard
A node first heard while the wall clock was untrusted got no last_heard at
all, and nothing backfilled it once time arrived - the phone showed "Last
heard: unknown" for a node it had just announced. The arrival instant now
waits in a RAM-only sidecar (NodeNum -> uptime seconds, 32 slots,
reuse-oldest - the RouteHealth shape) and is converted to a real epoch on
the clock-becoming-trusted transition, beside the existing rx_time
reconciliation. last_heard itself never holds anything but a real epoch or
0: it persists to flash and the warm tier, where an uptime-relative value
would be meaningless after reboot.
The sidecar's write sites are updateFrom()'s no-trusted-clock path (the
rx_time placeholder already carries the arrival instant, so this is a store,
not a second clock read) and addFromContact's anti-eviction stamps, which
previously wrote a bare getTime() - boot-relative seconds on a clockless
node, the exact value lastHeardIsWallClock() exists to catch. Eviction
ranking honours the stamps: heard-this-boot outranks every stored epoch,
ordered among themselves, so a stamped contact is not the first victim.
PhoneAPI re-reads last_heard at nodeinfo send time: a record prefetched
before the clock became trusted can carry 0 while the store has since been
backfilled, and re-reading at the pop makes handshake ordering (time-set vs
node-list download) irrelevant. Backfill never moves last_heard backwards
and skips the pathological elapsed-exceeds-epoch case. A node evicted to
the warm tier before time arrives is still absorbed with last_heard 0 -
same as before, bounded to the untrusted window.
* Update the agent docs for the monotonic timebase
The conventions bullet asserted there is deliberately no 64-bit millis; the
monotonic uptime clock restored for timestamps changes that contract. State
the split explicitly: Throttle for deadlines and intervals (no carry state),
Time::getMillisMonotonic()/getUptimeSecs() for timestamps, polled by
construction and not ISR-safe.
* Publish the monotonic wrap carry from a single writer
getMillisMonotonic() was a read-modify-write on two unguarded statics, and it
is reached off the main loop: the nRF52 Bluefruit task via
onFromRadioAuthorize() -> PhoneAPI::getFromRadio -> getValidTime(), and the
portduino civetweb workers via the same path. Two readers interleaving inside
the wrap window could each increment the carry, putting every uptime and
wall-clock reading 2^32 ms ahead for the rest of the boot - a permanent ~49.7
day jump in rx_time, last_heard and ClientNotification.time.
Readers no longer write. serviceMonotonic() publishes a snapshot behind a
seqlock and is the only writer; a reader adds its own unsigned elapsed time to
that snapshot, which is exact across the wrap, so it never inspects the
boundary and cannot miscount it. The main loop publishes every iteration, so
the once-per-49.7-days obligation now has the whole window of margin instead of
resting on an instruction-wide race.
AirTime was the guaranteed poller and is now a pure reader, so the two airtime
wrap tests step the clock the way loop() does. The test clock itself is atomic
so a suite can drive it from one thread while others read.
* Re-arm the GPS ephemeris hold when none is in force
The rollover sweep guarded the hold re-arm with `fixHoldEnds != 0 &&`, which
reads like the sentinel rule but inverts this site. The comparison it replaced,
`(fixHoldEnds + GPS_THREAD_INTERVAL) < millis()`, was always true when nothing
was armed - that was the point, since 0 means "not holding" and so is a reason
to arm. With the guard, a publish that cleared the hold without sleeping (the
`shouldPublish && !tooLong && !holdExpired` path, which does not call down())
left hasValidLocation set and prev_fixQual non-zero, so no disjunct held:
nothing re-armed, nothing published, and the receiver stayed powered at the
200ms poll until searchedTooLong() fired.
State the question positively instead. fixHoldInForce() is the only place the
sentinel is interpreted, and both of runOnce()'s decisions derive from it - the
asymmetry is now visible rather than implied, since arming does not require a
prior hold but expiring does. Its `!= 0` test is not redundant with the
arithmetic: deadlinePassed() is an unsigned half-range test, so past 2^31 ms of
uptime the sentinel reads as a deadline ~24.9 days in the future.
Kept beside its caller rather than in a header; the native test build compiles
GPS.cpp, so the suite declares the prototypes.
Also converts the getACK() wait to isWithinTimespanMs(start, interval): it has
both the start instant and the interval in hand, which gives the full 49.7-day
range instead of 24.8 days ahead, and takes its anchor from Time::getMillis()
so the wait is injectable.
* Date the NodeInfo reply window in uptime seconds
The 12h reply-suppression stamp regressed from wrap-immune 64-bit seconds to
raw 32-bit milliseconds, and pruneLastNodeInfoCache() evicts only by node count
and DB membership - never by age. A stable mesh under the node cap therefore
keeps every stamp indefinitely, and once uptime passes 49.7 days an old one
aliases back into the window: `now - stamp` computes as ~0 and a legitimate
NodeInfo request goes unanswered for up to 12h. It self-heals and repeats once
per wrap cycle.
Store Time::getUptimeSecs() instead, which does not wrap for 136 years, and
drop the millisecond conversion the previous shape needed. Entries past the
window are now evicted too: they can only ever decide "don't suppress".
N8-N11 cover the window from both sides, and N10 pins the regression - it needs
a full 2^32 ms of uptime to elapse, not merely a crossing of the boundary,
because that is when a millisecond stamp reads as "answered this instant".
tearDown() now restores the injected clock and C14's region and TX bucket. A
failing assertion aborts the test body, so restoring at the end of it leaked
that state into every later case.
* Update the agent docs for the single-writer clock and sentinel direction
Two rules the preceding three commits changed.
The monotonic clock is no longer maintained by whoever happens to read it:
serviceMonotonic() is the only writer, readers are pure, and calling it from
anywhere but the main loop reintroduces the double-count.
The sentinel guidance gained the half it was missing. It named UINT32_MAX as a
sentinel while prescribing an idiom that only covers 0, and it assumed the
sentinel always means "suppress" - at the GPS fix-hold site it meant "fire",
which is how that regression passed review looking like the rule.
* Name the fix-hold expiry predicate and arm it from the injected clock
holdJustExpired() gives the second reading of the fixHoldEnds sentinel a
name beside the first, so both are pinned by test/test_gps_fix_hold/ and
neither can be respelled at the call site. The old inline form could not
be tested: written as a literal, its guard folds at compile time and the
assertion asserts nothing.
The arm site used bare millis() while the evaluation reads the Throttle
clock; same value in production, but it kept that write out of reach of
Time::setTestMillis(). Remap a deadline that lands on 0, which would
otherwise read as no hold at all.
* Share the extend formula between the clock's reader and writer
getMillisMonotonic() and serviceMonotonic() carried byte-identical wrap
arithmetic. A one-sided edit to either would drift the published carry
from what readers report, so keep one copy.
* Trim the NodeInfo dedup comment to the house limit
* todo note for potential future imrpovments
* fix some simple deadlines
* Trim the hold-expiry test comment to the house limit
* Fix non-blocking uptime publication and pre-clock recency edges (#29)
* fix(time): avoid blocking monotonic readers
* test(time): make paused-publisher check deterministic
* fix(time): address review portability gaps
* Init the eviction sentinel to the newest possible recency
EvictionRecency{} is {0, false}, which evictionRecencyOlder() ranks as older than
every candidate: without the oldestIndex/oldestBoringIndex guards nothing would
ever be selected and a full node DB would stop evicting entirely.
Init to the genuine maximum instead, so the sentinel is correct on its own. The
index guards stay: two independent reasons the scan is right beats one.
* Keep the deadline-guard check name branch protection matches
The guard was widened to cover Time::getMillis() and unqualified getMillis(),
and renamed to suit. Upstream branch protection matches required checks by name,
so a rename means the old name never reports and merges block on a check that
will never arrive.
Widen the guard, keep the name; the descriptive text carries the broader scope.
* Correct native-suite-count to 47 after the develop merge
Upstream #11293 added test_nmea_wpl and took develop's count to 43; this branch
had independently reached 46. Merging develop resolved the counter textually,
keeping 46, while the directory set became the union of both sides at 47.
The suite-count CI gate fails on the mismatch, and it gates the native test jobs,
so the tests themselves were being skipped.
* test(uptime): make the wrap fall where the comment says it does
The concurrent-reader case started at 0xFFFFF000, leaving 0x1000 to the wrap, so
the 0x800 advance annotated "cross the wrap" fell short and the wrap actually
happened during the following 60s advance.
Start at 0xFFFFF800 instead, so the first advance lands exactly on the wrap while
the readers are running and the second is the ordinary time after it - the shape
both comments already described. Total elapsed is unchanged, so the closing
assertion still holds.
* Respond to human comments
* Did I ever tell you about the time I went to Shelbyville? I wore an onion on my belt, which was the style at the time.
* Convert the I2S nag deadline develop dragged in
The HAS_I2S_SPEAKER_NRF52 RTTTL block arrived from develop with a raw
nagCycleCutoff >= millis(), which the deadline guard rejects. Use the same
Throttle::deadlinePassed() form as the two sibling paths in this function.
* Arm the LittleFS format guard with a flag, not a zero timestamp
preFSBegin() runs in the first millisecond of boot, so millis() can legitimately
return 0 there. Both readers of last_format_ms treated 0 as "nothing formatted
this boot", which would skip the repeat-corruption escalation and let a dead
flash reformat-loop instead of reporting FLASH_CORRUPTION_UNRECOVERABLE.
* Note the single-thread contract on AirTime
* Note the AirTime locking TODO, and tighten the thread note
The two constant getters are not constrained, and getSilentMinutes() reads the
buckets without rotating them, so "the accessors mutate" was not accurate.
* trunk: ignore trufflehog false positives on millis-wrap test constants
test_throttle and test_uptime_clock pin dense clusters of hex boundary
constants (0xFFFFFF00u and neighbors) to exercise 32-bit millis()
rollover. trufflehog's Lob detector stitches nearby hex literals into
one candidate string, and the result happens to match a Lob API key
shape - not a secret, just test fixtures.
Same pattern already used for the gitleaks/nodedb-fixture false
positive in this file.
---------
Co-authored-by: nightjoker7 <mattdeering7@gmail.com>
Co-authored-by: Clive Blackledge <clive@ansible.org>
Co-authored-by: Benjamin Faershtein <119711889+RCGV1@users.noreply.github.com>
Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
|
||
|
Clive BlackledgeandClaude Opus 5
|
db3eb91015 |
fix(security): never log the X25519 identity private key (#11435)
installDefaultConfig() restores a preserved identity key when the config
is reset with preserveKey=true. On that path it called:
printBytes("Restored key", config.security.private_key.bytes,
config.security.private_key.size);
printBytes() hex-dumps the buffer to LOG_DEBUG, so this emitted all 32
bytes of the raw X25519 identity private key to the serial/BLE debug log.
Debug logs are not a private channel. They are routinely captured over
serial or BLE and pasted verbatim into GitHub issues, Discord threads and
support requests. Anyone who reads such a log recovers the node's identity
private key, and can then impersonate the node and decrypt every PKI direct
message addressed to it -- past messages included, since the key is
long-lived and the DH shared secret is static per node pair. There is no
revocation story short of generating a new identity.
Replaced with a LOG_DEBUG that records that a key was restored and contains
no key-derived bytes. The restore/no-restore signal is the genuinely useful
diagnostic here ("did my key survive the reset?"), and it costs nothing to
keep; the bytes were never what made the line useful. Log level is unchanged
-- printBytes() already logged at LOG_DEBUG.
Deliberately NOT a truncated prefix or a hash. A prefix is still key
material: it hands an attacker free bytes and shrinks the search space.
A hash is a confirmation oracle -- it lets anyone holding a candidate key
verify it against the log, which is exactly the check an attacker needs.
Neither is a compromise; both leak. If a log line survives at all it must
carry zero key-derived bytes.
Sites changed:
- src/mesh/NodeDB.cpp:988 -- the only full private-key dump in src/.
Audited and deliberately left alone:
- NodeDB.cpp:3552,3604 ("Incoming Pubkey", "Saved Pubkey") -- public keys,
published to the mesh by design; not secret.
- CryptoEngine.cpp:245,285 -- nonces, not key material.
- CryptoEngine.cpp:246,286 -- first 8 bytes of the derived shared_key, and
AdminModule.cpp:2006,2013,2014 -- the 8-byte admin session passkey.
Both are secrets rather than public values, but neither is the identity
private key and both are out of scope for this fix; noted for follow-up.
No unused-variable fallout: private_key_temp is still read by the memcpy
above, and printBytes() is still used by the two pubkey sites, so the
meshUtils.h include is still required.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Clive BlackledgeandClaude Opus 5
|
579d26e1b2 |
fix(NodeDB): don't let an empty contact key erase a stored public key (#11432)
Clients send `add_contact` before every text-message DM, because a phone often holds a larger contact database (with public keys) than the radio can keep. That makes `addFromContact()` the highest-volume key-write path on the device - and it had no protection against key erasure. Its only key guard covered the manually-verified case: if the local entry was marked manually verified and the incoming contact was not, a key mismatch aborted the update. Every ordinary entry fell straight through to `CopyUserToNodeInfoLite()`, which assigns `public_key` unconditionally. So a SharedContact with `has_user` set and an empty `public_key` overwrote a peer's stored, XEdDSA-proven key with zeros - and `addFromContact()` calls `saveNodeDatabaseToDisk()`, so the erasure survived a reboot. Subsequent DMs to that peer then failed with PKI_SEND_FAIL_PUBLIC_KEY, with no way to recover until the peer's NodeInfo was re-exchanged. `public_key` is a singular (non-optional) bytes field, so "absent" and "empty" both decode to size 0; a client that simply has no key for a contact is indistinguishable on the wire from one asking to clear it. The fix is deliberately narrow: keep the stored key when the entry already holds a full 32-byte key and the incoming contact does not. A well-formed 32-byte contact key still updates the entry exactly as before. Deliberately NOT changed here: - `updateUser()`'s first-key-wins pin is not applied to this path. Clients legitimately use add_contact to supply keys the radio never had and to update them (QR-code contact sharing); a blanket pin would break that documented flow. Only erasure is refused. - `CopyUserToNodeInfoLite()` itself is untouched - it has many other callers (self-record refresh, updateUser, warm-tier rehydration), so the guard lives at this call site. - The manually-verified branch is unchanged. - Node-number validation (reserved/broadcast/self) on this path remains open and is tracked separately. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Jonathan BennettandClaude Fable 5
|
5baad2e2a8 |
logging: compile out LOG_TRACE by default, demote chatty DEBUG lines, drop redundant logs (#11391)
* logging: gate LOG_TRACE behind MESHTASTIC_TRACE_LOGGING, drop redundant reclock logs LOG_TRACE now compiles out by default so trace-level diagnostics cost no flash; enable with -DMESHTASTIC_TRACE_LOGGING. Portduino keeps it on for the traceFilename packet-trace feature. Remove the 66 caller-side I2C reclock/restore log lines in the telemetry sensors: ReClockI2C::setClock/restoreClock already log both frequencies internally (now at trace level, since they fire every sensor read). Also unify near-duplicate literals (colon/case/punctuation variants) so linker string dedup applies, and drop an information-free bare 'done'. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: demote chatty per-packet/per-poll DEBUG lines to trace level With LOG_TRACE compiled out by default, per-iteration chatter (packet bookkeeping, sensor poll values, e-ink refresh reasons, GPS pin states, UI runState traces) now costs no flash on device builds while remaining one -DMESHTASTIC_TRACE_LOGGING away. 108 lines demoted, 4 information- free lines removed; failure paths, drop reasons, and one-time init logs all stay at debug level. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: address CodeRabbit review on trace-gate PR - GPS: pass serial-derived buffers as %s args, never as format strings (untrusted bytes could contain % directives) - 0x%08x for packet id / NodeNum per convention (Router, CannedMessage, NeighborInfo); unsigned casts for size_t args; %u for uint32_t delta - EInk: async full-refresh begin/complete back to DEBUG (rare state transitions); per-frame SKIPPED lines stay trace Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: gate trace on the flag's value, not its presence -DMESHTASTIC_TRACE_LOGGING=0 previously *enabled* trace logging because the gate tested definedness. The flag now defaults per-platform (portduino 1, else 0) and both backends test the value, so =0 disables, =1 or a bare -D enables. Also cast tx_after-millis() to uint32_t for %u (millis() is unsigned long on native). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: clang-format rewrap after specifier widening Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * Even fewer bytes! * logging: keep compile-gated debug lines at debug level; fix native-suite-count Lines already inside default-off #ifdef blocks (GPS_DEBUG, DEBUG_LOOP_TIMING) cost no flash and should stay visible at debug level when their gate is enabled, rather than also requiring MESHTASTIC_TRACE_LOGGING. test/native-suite-count lags the two test_event_channel_* suites added by #11045 (develop's Native Suite Count check has the same mismatch); bump 46 -> 47. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * gps: route GPS_DEBUG diagnostics through a LOG_DEBUG_GPS() macro (#11414) Replaces 27 log-only #ifdef GPS_DEBUG blocks across GPS.cpp, PositionModule, MeshService, and GPSStatus.h with a single-line LOG_DEBUG_GPS() call (src/gps/GPSLog.h, modeled on LOG_MIGRATION: value-gated, ((void)0) when off). Blocks containing declarations, control flow, hexDump, or nested conditionals keep an explicit '#if GPS_DEBUG' guard. RTC.cpp's per-reading raw time dumps and per-candidate rejection chatter fold under the same gate; quality transitions and boot-time seeding stay at debug. Also fixes the '// define GPS_DEBUG' missing-# typo in two variant headers and updates all seven commented examples to the value form ('#define GPS_DEBUG 1') required by the value-based gate. Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 Co-authored-by: Claude <noreply@anthropic.com> * gps: declare RTC gmtime result as pointer to const (cppcheck) With the setTime debug dump gated behind GPS_DEBUG, all remaining uses of t are reads; cppcheck (constVariablePointer) now flags it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Ben MeadorsandJonathan Bennett
|
87a009da63 |
fix(nrf52): LTO was dropping the board variant's weak hook overrides (#11415)
* fix(nrf52): LTO was dropping the board variant's weak hook overrides Whole-image LTO (enabled arch-wide for nrf52840 in #10655) inlines the empty weak body of earlyInitVariant()/lateInitVariant()/variant_shutdown()/ variant_nrf52LoopHook()/variantDefault*Config() at the call site, because the weak default and the call site live in the SAME translation unit. The strong override in variants/<arch>/<board>/variant.cpp is then never linked, and the board's hardware setup silently does not run. nrf52_lto.py's -fno-lto variant recompile does not help here: the caller is the problem, not the variant object. Needs both ingredients, so this only affects 2.8: the earlyInitVariant() indirection landed in #9438 and is present in v2.7.26 too, but v2.7.26 has no -flto, so the override linked normally. Found on the muzi R1 Neo, whose earlyInitVariant() drives DCDC_EN_HOLD (P0.13, the DC-DC hold after the user button) and NRF_ON (P0.29, "tells IO controller device is on"). Both were dropped from the image, so the companion MCU never saw the nRF application come up and stayed in its DFU indication (purple LED). Verified in the ELF: pre-fix setup() runs straight from waitUntilPowerLevelSafe() to the LED_NOTIFICATION block with no earlyInitVariant symbol in the binary and no pinMode/digitalWrite on P0.13 or P0.29 anywhere; post-fix it calls the real override. HW-confirmed on an R1 Neo. Also affected on nrf52840: earlyInitVariant() on 10 variants (incl. t-echo-card, which sequences its RT9080 3V3 rail there), variant_shutdown() on 18 variants (t114, t-echo, ThinkNode M1-M8, meshlink, wio-tracker-L1 ... - sleep pin parking, so deep-sleep leakage), variant_nrf52LoopHook() on 3 RAK variants. Confirmed dropped on heltec-mesh-node-t114 by build, not just by inspection. Fix is __attribute__((noinline)) on both the weak declaration and definition - the same guard already carried by loopCanSleep(), preFSBegin(), PowerHAL and variant_enableBatteryLpcompWake(), whose comment in main-nrf52.cpp already documents this exact failure mode. Also extend _VARIANT_OVERRIDES in extra_scripts/nrf52_lto.py from just _Z11initVariantv to all eight hooks. That post-link guard already had the right logic and would have caught this on every PR - it simply was not listing Meshtastic's own weak variant hooks, only the core's. With the list extended it goes red on both r1-neo and heltec-mesh-node-t114 when the noinline is reverted, and green with it. Its failure message now names both possible causes. * review: trim the noinline rationale comments to two lines Per AGENTS.md ("keep code comments minimal - one or two lines, max"), the incident detail and extended background belong in the PR description, not the source. Keeps the LTO/noinline rationale and the pointer to the guard. --------- Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> |
||
|
|
cd716fe384 |
logging: audit log strings for terseness, reclaiming ~6.8 KB of string data (#11374)
* logging: strip redundant punctuation, level prefixes, and 'successfully' from log strings The logger already appends a newline and prints the level tag, so trailing '.', '!', '...', literal \n, and 'Error:'/'Warning:' prefixes inside format strings are wasted flash bytes. Same for 'successfully' (the affirmative form already implies it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: tighten verbose log strings in modules, radio, platform, and system code Rewrite wordy log messages to terser equivalents - drop filler words (articles, 'attempting', 'due to', 'please'), use 'Can't X'/'X failed' phrasing, and abbreviate where the codebase already does (config, init, msg, BT). Format specifiers and argument lists are unchanged; distinctive greppable tokens are preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: tighten verbose log strings in telemetry sensors and GPS Same terseness pass: drop filler, 'Can't X'/'X failed' phrasing, common abbreviations (temp, msg). Specifiers, arguments, and sensor-name prefixes unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: tighten verbose log strings in mesh core Same terseness pass over NodeDB, Router, MeshService, PhoneAPI, RadioInterface, NextHopRouter, and PacketHistory: 'X failed'/'Can't X' phrasing, imperative verbs, dropped filler. Specifiers and arguments unchanged; duplicate literals kept identical to preserve linker string dedup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: 'Unable to/Could not/Cannot' -> "Can't" in log strings Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: clang-format rewrap after string shortening Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * logging: restore boot-logo trailing newline and progress-dot strings The terseness pass over-trimmed: the Meshtastic ASCII boot logo kept its blank line via a trailing \n, and three bare "." progress ticks were reduced to empty strings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LBiZc9sfPrH1MZ2L3Fxgt1 * Update src/mesh/wifi/WiFiAPClient.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * logging: address review feedback on the terseness audit - Node/packet IDs use the repo's 0x%08x convention in NextHopRouter, NodeDB, AdminModule and CannedMessageModule. The sibling log in each if/else pair is converted too, so a pair isn't split across two formats. next_hop stays 0x%x - it's the last-byte relay hint, not a NodeNum. - RTC: the read-path and set-path "not found" warnings were byte-identical, so the linker deduped them and the log couldn't say which one fired. Split into "RTC read:" / "RTC set:". (The four sites live in mutually exclusive #ifdef branches, so the RTC family was never ambiguous.) - SCD4X getAmbientPressure()/setAmbientPressure() logged "altitude", and SCD30 getASC() logged "Can't send command" for a read. Both now name the operation they actually perform. - LOG_ERROR already carries the level: ". Error: %u" -> ", rc=%u" (matching the existing rc=%d house style) and "Error executing X()" -> "X() failed". - Typos and wording: "OTA partiton. (Reason" -> "OTA partition (reason", "CST3530 not response ~" -> "CST3530 no response", "Packet received with to: of 0" -> "to=0", HostMetrics "Error decoding" -> "Can't decode", and the dangling ": " on the NextHopRouter retransmission line. Printf specifier sequences are byte-identical on all 37 touched lines apart from the 6 deliberate %x/%u -> %08x node-ID widenings, all on uint32_t args. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
HarukiToredaandJason P
|
39e7aa6a6c |
Full T-echo card support + Compact UI (#11342)
* T-echo card * Update NRF52I2SOutput.cpp * Update NRF52I2SOutput.h * cleanup * Update buzz.cpp * use consistent runtime compact-panel check instead of mixing with compile-time macro * Update NodeDB.cpp * Update ExternalNotificationModule.cpp * switched to Throttle::isWithinTimespanMs * Update SharedUIDisplay.h * trunk fix * last cleanup * ClockRenderer.cpp for OLED_COMPACT_UI and setup Unit C6L for new UI. * Fixed regressions in standard OLED and TFT --------- Co-authored-by: Jason P <applewiz@mac.com> |
||
|
Austin
|
478206ea72 |
NodeDB: Change pointer to const for mesh node retrieval in loadFromDisk (#11315)
Addresses cppcheck src/mesh/NodeDB.cpp:2431: [low:style] Variable 'us' can be declared as pointer to const [constVariablePointer] |
||
|
TomandClaude Opus 4.8
|
e26b6bfc5a |
TMM & Warmstore key source naming (#11119)
* one becomes two * warmstore clarify * Address PR review: key-provenance terminology consistency - Log line now says "not key-proven" (gate is XEdDSA OR manual, not just signer) - Rename markKeySignerProvenForTest -> markKeyXeddsaSignedForTest (sets only the XEdDSA bit) - Docs + test comments: "signer bit" -> "XEdDSA-signed bit" clod helped too * Rename signer-proven -> key-proven for broadened provenance predicate Address PR #11119 review: the copyPublicKey()/copyUser() out-parameter and the cache-path replay gate now report entry->keyProven() (XEdDSA-signed OR manually verified), so the "signerProven" name and "signer-proven" comments were misleading. Rename the public out-param to keyProven, the local cachedKeySignerProven to cachedKeyProven, and update coupled callers, log strings, docs headings, and comments to say "key-proven". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * nitpicks --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
|
597f6767b5 |
Add Elecrow ThinkNode M8 board support (thinknode_m8) (#11226)
* Add Elecrow ThinkNode M8 variant scaffold (thinknode_m8) nRF52840 + SX1262 + 2.4" e-paper + ATGM336H-5NR32 GPS. All pins resolved from ThinkNode_M8_V0.3.sch; cross-checked against meshtastic/firmware#9181 (Elecrow V0.1 reference). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add Elecrow ThinkNode M8 board support (nRF52840/SX1262, 1.54in e-ink, ATGM336H GNSS, SC7A20, EC04 encoder) * Address review: keep the stored backlight level out of blanking, match only the SC7A20 WHO_AM_I byte, and transfer detents atomically * Use std::atomic for the press-and-turn detent counter so native builds compile * Drop the ThinkNode M8 LED_BUILTIN redefinition that warned on every translation unit --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Thomas Göttgens
|
fc67590317 |
Lockdown PIN redaction, a build guard, and favorite compaction (#11285)
* Redact the pairing PIN from unauthorized lockdown clients * Fail the build when PacketAPI would bypass the lockdown gate * Compact kept favorites when resetting the node database * Make the compaction loop reference const |
||
|
Thomas Göttgens
|
daf1213580 |
Licensed channel defaults, phone map growth, and payload read bounds (#11286)
* Strip the default PSK when licensed defaults are installed * Only record rate-limited portnums from the phone * Bound payload reads by the received size |
||
|
Thomas Göttgens
|
df6e67f70b |
Signing and ingress hardening (#11282)
* Include warm-tier signers in the identity update gate * Fail the send when PKI encryption fails * Require signatures on licensed unicasts * Include warm-tier signers in the NodeInfo downgrade drop * Clamp hop fields on UDP multicast ingress * Address review comments on signing hardening Condense the updateUser rationale to two lines and stop calling the Balanced-mode drop a broadcast now that licensed unicasts reach it. |
||
|
Tom
|
2024bb8384 |
Arrival time fix perhaps (#11274)
* Add explicit presence for MeshPacket.rx_time (arrival time) rx_time is now proto3 optional with a has_rx_time presence bit, matching the rx_rssi treatment. A node with no GPS and no phone connected yet has no time source at all, so a bare 0 was indistinguishable from a genuine 1970-01-01 reading; downstream consumers (replay packets, JSON serialization) now check has_rx_time instead of the value. * Dedupe rx_time stamping into a shared helper; trim a debug log string Extract the repeated haveTime/rx_time/has_rx_time stamp logic (5 call sites across Router.cpp, MeshBeaconModule.cpp, MeshService.cpp) into Router::computeRxTimeStamp()/stampRxTime(). Also shorten the new RTC.cpp LOG_DEBUG string. Saves 48 bytes of flash on rak4631 (measured), no behavior change. * Fix has_rx_rssi presence carried unconditionally through StoreForward replay preparePayload() set has_rx_rssi = true unconditionally on replay, regardless of whether the packet's rx_rssi at store time was a genuine measurement (e.g. MQTT-relayed packets carry no real RSSI). Store the presence bit alongside rx_rssi in PacketHistoryStruct and restore it on replay instead. Flagged by Copilot on #11271 (same root cause the has_rx_time explicit presence work fixes) but never addressed before that PR merged. * Trim comment blocks to the repo's 1-2 line guideline .github/copilot-instructions.md:338 caps code comments at 1-2 lines; several blocks added across the rx_time explicit-presence work ran well past that. Also consolidates Time.cpp's file-level doc comment into Time.h, where the rest of the Time:: API contract already lives. No behavior change. * Add rx_time explicit-presence test coverage - test_meshpacket_serializer: has_rx_time=false fixture plus tests asserting JsonSerialize/JsonSerializeEncrypted emit 0 rather than leaking the millis() placeholder, alongside the has_rx_time=true baseline. - test_stream_api: two tests driving a real PhoneAPI handshake (want_config_id through STATE_SEND_PACKETS) that simulate a phone time-giving transaction arriving before vs. after a queued packet is drained - covering both the reconciled and the ships-with-placeholder-absent paths of MeshService::reconcilePendingRxTimes(). * Fix three correctness issues flagged in review - Time.h: drop the reserved-identifier include guard (_MT_TIME_H); pragma once already covers it, matching convention elsewhere (e.g. RTC.h). - Time.cpp: rebase getMillis64()'s wrap accumulator when the test seam swaps clock sources, so a real<->injected clock jump isn't miscounted as a genuine 32-bit wrap. - NodeInfoModule: the 12h reply-suppression window is a local dedup duration, not a wall-clock reading - switch it to Time::getMillis64() so RTC-quality jumps and replayed packets' stale rx_time can't perturb it. - StoreForwardModule: has_rx_time was derived from *current* RTC quality at replay time rather than stored at capture time, so a history entry saved while time-blind could be misreported as a valid epoch once the clock later improved. Persist the presence bit in PacketHistoryStruct instead. * tryfix CI * post review fixes * more test fixes |
||
|
Benjamin Faershtein
|
0fef83d434 |
Add configurable event mode hop limit (#11275)
* feat: resolve event mode hop limit * feat: bake event mode hop limit * fix: honor event mode hop cap in routing * docs: expose event mode hop limit preference * fix: enforce event hop defaults across routing * docs: clarify event hop override behavior * refactor: simplify event mode hop preference * fix: cap equal event hop limit |
||
|
Tom
|
2c57a17124 |
Phantom node fix perhaps (#11271)
* trying to fix phantom nodes * fix comment spam * oops - missed one |
||
|
|
a640d44b5c |
Add T-Beam BPF - 144-148 Mhz LoRa @ ~37 dBm (5 Watts) (#10558)
* Add T-Beam BPF (144-148 Mhz LoRa) * minor correction to fix compiler warnings * Add T-Beam BPF (144-148 Mhz LoRa) * minor correction to fix compiler warnings * Add ITU regions for this device and make GPS work. * Switch pin after defining it as output Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Lora CS is indeed 1, SD Card CS is 10 * Include the back option. * Fix compilation with pioarduino (USB_MODE) * Default ham to narrow_fast * Default PROFILE_HAM to slot 17 This is an appropriate default in the USA but not the EU. The slot override really should follow the region itself, not the regionprofile. * Fix for ITU 2/3 split * Add ITU region options to MenuAction enum * Add HAS_HAM_2M definition to variant headers for 2M support * Re-add PROFILE_HAM regionprofile Accidentally removed in last merge * Trunk fmt * Initial default slots * Switch to TinyFast Still need to flesh out the default channels * Adjust slotOverrides for TinyFast * RadioLib doesn't accept 15.625 kHz Use 15.6 instead * Set RF95 pins for T-Beam Supreme May cause regressions!! * Remove other-variant changes (BPF-only) These have been moved to other PRs * Remove mismatch guarding (we need a more comprehensive approach) * Add comment back * Add template for PA curve * This is a 5-6W radio!! Add TX_GAIN_LORA * Trunk fmt because NomDeTom hates emdashes --------- Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> |
||
|
|
1e982fa78c |
Sign plaintext packets in licensed mode (#10969)
* feat: sign licensed plaintext packets Preserve and publish identity keys in licensed mode so existing XEdDSA signatures can authenticate plaintext traffic. Sign licensed broadcasts and direct messages when they fit, while keeping PKI encryption disabled and normal routing behavior unchanged. * fix(security): persist licensed channel sanitation * fix(security): close licensed migration lifecycle gaps * fix(security): address licensed signing review feedback * test: restore signing globals from Unity teardown * fix(baseui): allow confirmed ham region selection * fix(baseui): guard region picker validation * style: trunk fmt --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Austin <vidplace7@gmail.com> |
||
|
|
45cb8e7500 |
feat: preserve normal radio profiles across event firmware (#11110)
* feat: isolate event radio profiles * fix: preserve identity on degraded config boot * fix: guard event profile storage * style: align event profile storage names * fix: discard event profile on normal boot * refactor: simplify event profile cleanup * fix: limit inactive profile migration to event firmware * fix(event): make event-profile capacity check portable across filesystems The USERPREFS_EVENT_MODE storage preflight called FSCom.totalBytes() and FSCom.usedBytes(), which only exist on ESP32's LittleFS wrapper. Every other backend failed to compile once event mode was enabled: error: 'class InternalFileSystem' has no member named 'totalBytes' (nRF52) error: no member named 'totalBytes' in 'fs::FS' (Portduino) This was not caught earlier because the event block is behind #if USERPREFS_EVENT_MODE, and the existing unit tests only cover the pure helpers, which compile identically either way. Add fsTotalBytes()/fsUsedBytes() to FSCommon and implement them per backend: littlefs v1 traversal for nRF52 (Adafruit_LittleFS) and STM32 (STM32_LittleFS), FSInfo for RP2040, statvfs for Portduino, and the native methods for ESP32 and nRF54L15. The nRF52/STM32 path reports "full" if the traversal errors so the capacity check fails safe. Verified with USERPREFS_EVENT_MODE=1: native-macos test_event_profile_storage passes 5/5, and heltec-v3, rak4631, rak11310 and rak3172 all build clean. * fix(event): use std::filesystem for Portduino capacity, bump native-suite-count Two CI fixes: - native-windows has no <sys/statvfs.h>, so the Portduino branch of fsTotalBytes()/fsUsedBytes() broke the Windows build. Switch to std::filesystem::space(), which is already used under ARCH_PORTDUINO in HostMetrics.cpp and works on both POSIX and MinGW. Errors report "full" so the capacity check still fails safe. - This PR adds test/test_event_profile_storage, taking test/ from 40 to 41 suite directories, which trips the native-suite-count reconciliation check. * fix(event): scope boot-write deferral to the radio profile, address review Review feedback: - Copilot: saveProto()'s boot-write deferral applied to every file, so loadFromDisk()'s recovery writes (e.g. restoring owner fields into devicestate) were silently dropped and never retried, because the ctor CRC baselines are computed after loadFromDisk(). Deferral now applies only to the radio-profile files, via isRadioProfileFile(). - jp-bennett: eventConfigFromStandard() wrapped a struct copy plus one field assignment in a header helper with its own unit test. Inlined at its single call site and removed; the behaviour is covered end-to-end by hardware validation instead (RAK4631 normal -> event -> normal preserves NodeNum and public key while swapping LoRa, and restores the original channel byte-identically). - jp-bennett: the active-backup encrypted-storage migration ran for normal builds too, which changed non-event behaviour and contradicted the PR's stated event-only scope. Scoped to USERPREFS_EVENT_MODE; the adjacent block already covers the inactive standard backup in event builds. New tests (all verified to fail under mutation, not vacuous): - test_event_paths_never_collide_with_standard_or_shared_files: the core safety property. A path-table slip would make event firmware overwrite the user's real config, channels or backup, or capture a shared file like devicestate/nodedb. Nothing asserted this before. - test_only_radio_profile_files_defer_boot_writes: guards the deferral fix above so re-widening it fails loudly. - test_event_reservation_fits_smallest_supported_filesystem: the reservation is compile-time but must fit filesystems as small as 14 KiB (STM32WL) and 28 KiB (nRF52840). Protobuf growth pushing it past those would silently stop event profiles persisting - the exact failure mode confirmed by fault injection on a RAK4631. Verified with USERPREFS_EVENT_MODE both on and off: 7/7 tests pass, and rak4631, heltec-v3, rak11310 and rak3172 all build clean. * fix: preserve event profile storage recovery --------- Co-authored-by: Jonathan Bennett <jbennett@incomsystems.biz> Co-authored-by: Ben Meadors <benmmeadors@gmail.com> Co-authored-by: Benjamin Faershtein <benjaminfaershtein@Benjamins-MacBook-Pro-2.local> |
||
|
James RichandClaude Fable 5
|
8e104a909b |
fix(admin): persist TAK module config (team color / member role) (#11216)
Setting TAK team/role ACKed and rebooted but stored nothing: handleSetModuleConfig had no tak case, saveToDisk never set has_tak, and handleGetModuleConfig had no TAK_CONFIG case. Add all three, plus a native suite sweeping every ModuleConfig submessage through set -> save -> load -> get and a TAK value-fidelity suite. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Ben MeadorsandAustin
|
efc9708236 |
Add userpref for the packet signature policy default (#11144)
Co-authored-by: Austin <vidplace7@gmail.com> |