mirror of
https://github.com/meshtastic/firmware.git
synced 2026-09-20 05:18:54 -04:00
thinknode-m9-v2
225
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Thomas Göttgens
|
c29bd00971 |
Rewrite the MQTT region root topic only on the default broker (#11899)
* fix(mqtt): rewrite the region root topic only on the default broker A region change rewrote any root starting with "msh", on any broker. Custom roots such as msh/home were clobbered, and private brokers had their topics moved even though a regional broker is regional already. An empty root, which MQTT treats as the default, was never updated. Region changes now go through MQTT::applyRegionRootTopic(), which rewrites the root only on the default broker and only when the root is empty, the default, or a msh/<region> the firmware wrote itself. * fix(mqtt): parse the broker address before the default-server check Persist module config only when the root actually changed. Replace a stray NUL byte in the test with the \0 escape. * fix(mqtt): count the regional roots as the default root topic |
||
|
Benjamin Faershtein
|
3aac179397 | fix(phoneapi): wake clients after config sync (#11818) | ||
|
Thomas GöttgensandBen Meadors
|
0dafcc90fe |
fix(api): retain the unwritten tail on a short TCP API write (#11890)
* fix(api): retain the unwritten tail on a short TCP API write ServerAPI closed the session whenever stream->write() returned fewer bytes than requested. A short write is transmit-buffer backpressure, not a dead socket, and it is most likely during the back-to-back frames of the initial NodeDB dump, so a node at its node cap dropped clients on effectively every connect. Route TCP frames through StreamFrameWriter, the retained-tail path the USB CDC console already uses: the remainder is re-offered on the next pass and the session is closed only when the link itself is gone. Poll at 25ms while output is still undelivered, since nothing wakes the thread when the socket frees transmit space. Fixes #11822 * fix(api): block log re-encoding while a TCP frame is retained emitLogRecord() writes into txBufLog and StreamFrameWriter can now hold that buffer as a retained tail, so a second log record would overwrite bytes the transport has not sent yet. Gate encoding on the retained-frame state, matching SerialConsole. No caller reaches this today (emitLogRecord() is only used by SerialConsole), but retaining the buffer at all is new here. --------- Co-authored-by: Ben Meadors <benmmeadors@gmail.com> |
||
|
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. |
||
|
Jonathan BennettandClaude Opus 5
|
6f3f0bd7c2 |
Prove explicit acks with Routing.ack_proof (#11877)
* Prove explicit acks with Routing.ack_proof
Explicit acks are ROUTING_APP packets, and ROUTING_APP is excluded from PKC, so
an ack travels under channel encryption alone - and the default channel key is
public. Anyone in range can forge one, and the client grants its strongest
delivery claim on the strength of the ack's unauthenticated `from`.
Where the acknowledged packet was PKI encrypted the endpoints already share a
Curve25519 secret, so the recipient can prove receipt in ~10 encoded bytes:
ack_proof = HMAC-SHA256(shared_key,
"ack" | LE32(from) | LE32(to) | LE32(request_id)
| routing)[0..8)
where `routing` is the encoded Routing message without the ack_proof field,
taken as received with that byte range removed rather than re-encoded. Excising
keeps the value a function of the received bytes alone, so it does not depend on
two implementations' encoders agreeing and does not drop fields this build has
never heard of. For the same reason the sender appends the field rather than
setting it on a decoded struct and re-encoding.
What this does and does not buy. It buys an authenticated delivery receipt from
the actual recipient, which is the property a forged ack costs a user and which
matters where people act on a delivery confirmation. It does NOT protect the
retransmission loop, and must not be described as if it does:
perhapsGenerateImplicitAckForOwnOverheard clears a pending retransmission on any
overheard rebroadcast of our own (from, id), header-only and keyless, so
replaying the originator's own ciphertext stops their retries more cheaply than
forging an ack. No ack authentication of any kind closes that path.
Advisory only, and deliberately not a step toward enforcement. A rule requiring
a proof once a peer has sent one would make a missing proof destroy the only
delivery signal we have, on state the user cannot see: the proof needs the peer
to hold our key, and peer-side eviction, a downgrade or a factory reset are all
invisible to us. A valid proof marks the ack verified; anything else behaves
exactly as today. The client renders the difference.
Verification costs one X25519 and nothing caches the shared secret, so
ackProofPermitsAction gates on findPendingPacket first - otherwise a forged ack
naming any packet id, which is visible in the cleartext header, would force a DH.
Depends on meshtastic/protobufs#1094 for the generated field.
* test: correct a comment that predates ack_proof being generated
The wire-roundtrip test described ack_proof as an unknown field, which was true
while the prototype hand-encoded it. The field is generated now, so this build
understands it - but older firmware does not, which is the case the assertion
actually covers.
* Fix the EXCLUDE_PKI test build, and format
Two copies of test_proof_binds_error_reason and test_proof_binds_direction were
sitting in the #else branch of test_ack_proof, where Identity, makeIdentity,
makeAck, becomeNode, crypto and ACK_PROOF_SIZE do not exist. They were also
unregistered, so they were dead code that only served to break the build. The
native suite never compiles that branch, so a local run could not see it.
Also apply clang-format to a declaration that had been wrapped by hand.
* Exempt test_ack_proof from trufflehog's Lob false positive
Same detector and same shape as the three suites already listed here: it
stitches nearby hex literals into one candidate string, and this suite's node
numbers and request ids (0x0A0A0A0A, 0x0B0B0B0B, 0xABCD1234) happen to match a
Lob API key. The suite holds no literal key material - every key it uses comes
from crypto->generateKeyPair at runtime.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012hLcVif8GDEmA2k77hmFG8
---------
Co-authored-by: Claude <noreply@anthropic.com>
|
||
|
Thomas Göttgens
|
f36a1ea821 |
nRF52: reclaim flash to bring rak4631 back under its size budget (#11873)
* build(nrf52): drop unused TinyUSB classes and assert function-name strings Only the CDC class is used on nRF52. Disable the MSC, HID, MIDI, vendor and video class drivers in the Adafruit TinyUSB config, and pass an empty __ASSERT_FUNC so assert() no longer embeds __PRETTY_FUNCTION__ strings. File and line are still reported. rak4631 estimate: ~7.4 KB flash, ~2.3 KB RAM. * fix(nrf52): link only the secp256r1 cc310 curve domain CRYS_ECPKI_GetEcDomain indexes ecDomainsFuncP, which references the parameter tables of all eleven cc310 curves. Bluefruit LESC pairing only requests secp256r1, so override the lookup to return that domain alone. rak4631 estimate: ~7.4 KB flash. * fix(airtime): replace powf in the channel-utilization EMA fold foldChannelUtil was the only powf caller on nRF52. The exponent is an integer step count, so raise the EMA factor by squaring instead; a multi-day sleep still folds in at most 32 multiplications. rak4631 estimate: ~1.9 KB flash. * fix(graphics): use double sin/cos in the compass renderers The compass renderers were the only sinf/cosf callers on nRF52 screen builds, pulling in the float trig kernels next to the double ones GeoCoord already links. Call the double variants instead. rak4631 estimate: ~3.2 KB flash. * fix(motion): use double atan2 for magnetometer heading fallbacks MMC5983MA, QMC6309 and the InkHUD map centre were the remaining application atan2f callers. The double atan2 is already linked, so the float variant only added atan2f, __ieee754_atan2f and atanf. The saving lands once meshtastic/Fusion#1 removes the library's atan2f as well. rak4631 estimate: ~0.8 KB flash with Fusion#1. * fix(hopscale): trim diagnostic logging to state changes and anomalies Drop the save/restore confirmations, the hourly histogram and trend dumps, the denominator step logs and the per-packet hop_limit log (printPacket already reports HopLim). Keep the save-failure and histogram-full warnings, the congestion on/off transition and a single periodic status line, and remove lastScaledPerHop, which only fed the logs. * fix(hopscale): silence cppcheck uselessAssignmentArg on restored count * perf(crypto): use full-schedule AES128/AES256 for AES-CCM aesSetKey used AESSmall128/AESSmall256, which re-derive round keys for every block. AES128/AES256 precompute the schedule, encrypt faster and are already linked by encryptAESCtr, so the AESSmall*/AESTiny* code drops out. No change on ESP32, where AESSmall* already aliases AES128/AES256. rak4631 estimate: ~3.9 KB flash; cipher object up to 184 bytes larger. * perf(nrf52): use the shared software CTR for AES-256 and remove tiny-aes CryptoCell only accelerates AES-128, which stays on hardware. AES-256 CTR now calls CryptoEngine::encryptAESCtr (rweather CTR<AES256>, already linked) instead of the in-tree tiny-aes copy, whose sources were removed in the previous commit. Output is identical. rak4631 estimate: ~0.8 KB flash. * perf(mesh): use std::map for pending retransmissions and API port timestamps NextHopRouter::pending and PhoneAPI::lastPortNumToRadio were the only unordered_map instances linked on nRF52. Switching them to std::map, which is already linked, drops the libstdc++ hashtable, rehash policy and prime table. GlobalPacketId gains operator<; the unused hash functor is removed. rak4631 estimate: ~2.1 KB flash. * perf: parse sensor decimals without strtod The WS85 serial parser (strtof) and DFRobotLarkSensor (String::toFloat) were the only callers of newlib's strtod. Add parseDecimalFloat to meshUtils for plain [+-]digits[.digits] fields and use it at both sites. Covered by test_type_conversions against strtof. rak4631 estimate: ~4.5 KB flash. * perf(gps): compute tan from sin/cos in UTM and OSGR conversion latLongToUTM and latLongToOSGR were the only tan callers. sin and cos are already linked, so deriving tan from them drops tan and __kernel_tan. rak4631 estimate: ~1.1 KB flash. * fix(graphics): only dispatch the theme menu when TFT coloring is enabled The Theme option is only offered with GRAPHICS_TFT_COLORING_ENABLED, but handleMenuSwitch dispatched ThemeMenu unconditionally, linking kThemes and the theme accessors into monochrome builds where the menu is unreachable. rak4631 estimate: ~1 KB flash. * fix(senxx): trim diagnostic logging to errors and user-visible actions Keep all errors and warnings and a single version line; shorten the admin action messages; drop progress chatter, state save/restore confirmations and the per-reading and VOC-state debug dumps. The nested VOC restore branch collapses to one condition with the same behaviour. rak4631 estimate: ~2 KB flash. * build(nrf52): define CRYPTO_AES_NO_DECRYPT CTR and CCM only encrypt, so the AES inverse tables and round helpers are dead code on nRF52. Takes effect once the Crypto dependency includes meshtastic/Crypto#5. rak4631 estimate: ~1.0 KB flash. |
||
|
|
ae8dee9582 |
Fix: MQTT topic not updated when LoRa region changes (#10565)
* Initial plan
* Fix: update MQTT topics when LoRa region changes
When the LoRa region is changed via AdminModule::handleSetConfig,
moduleConfig.mqtt.root is updated (e.g. from msh/US to msh/EU_868)
but the running MQTT instance kept using the stale topic strings
(cryptTopic / jsonTopic / mapTopic) that were set at construction time.
Introduce MQTT::reinitTopics() which:
- resets the topic strings to their base values and prepends the
current moduleConfig.mqtt.root, and
- disconnects from the broker so the next reconnect re-subscribes
under the new topic prefix.
Call reinitTopics() from MQTT's constructor (replacing the inline
block) so the logic lives in one place, and call it from
AdminModule::handleSetConfig right after moduleConfig.mqtt.root is
rewritten on a region change.
Add a unit test (test_reinitTopicsUpdatesOnRegionChange) that verifies
both the updated subscriptions and the updated publish topic after a
simulated region change.
* Fix: call mqtt->reinitTopics() on region change via menuhandler
* Format MQTT test with trunk style
* fix(mqtt): drop undeclared jsonTopic refs in reinitTopics()
reinitTopics() assigned to a jsonTopic member that does not exist on this
branch (the MQTT class only has cryptTopic and mapTopic), so MQTT.cpp failed
to compile ("'jsonTopic' was not declared in this scope") and broke every
build that compiles it. Remove the jsonTopic lines so reinitTopics() rebuilds
exactly the topics the original constructor did.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* refactor(mqtt): simplify reinitTopics and its call sites
* fix(mqtt): rebuild topics in runOnce when the root changes
Replaces the per-call-site reinitTopics() calls. Also keep the device state and node database segments when the EU clamp swaps the region.
* fix(mqtt): refresh topics in onSend when the root changed
Rename the region change test to underscore-separated segments.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Ben Meadors <benmmeadors@gmail.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com>
|
||
|
Tom
|
ee76117835 |
fix(router): relay opaque packets in CORE_PORTNUMS_ONLY (#11844)
* fix(router): relay opaque packets per rebroadcast_mode, not only in ALL |
||
|
Tom
|
2d6dad9ee9 |
Portduino: Fix LR2021 switch tables, power ceilings and IRQ handling (#11382)
* fix(portduino): recognise the LR2021 power ceilings in --check loadConfig() has read Lora.LR2021_MAX_POWER and Lora.LR2021_MAX_POWER_HF since LR2021 support landed, but neither was listed in the config checker's schema. --check therefore reported both as "unknown key ... ignored by meshtasticd" -- false, and actively misleading: it tells the user to delete a key that is doing exactly what they wanted. This breaks the contract stated above schema(), that a key taught to loadConfig() is added there too. CI enforces that by running --check over bin/config.d/**, but no shipped config sets either key -- or mentions lr2021 at all -- so nothing ever tripped over the omission. It could only surface for someone hand-writing an LR2021 config. LR20x0 is the only module with two power ceilings, one per band, selected at runtime by region; every other family expresses the split as separate module names and needs a single key. That is the likely reason the pair was missed while every other *_MAX_POWER key was added. Also adds both to valueSpecs(), so a wrong-typed value is reported rather than silently replaced by the default. * feat(portduino): configurable IRQ DIO and a chip-neutral RF switch table for LR20x0 Two gaps found bringing an LR2021 up under meshtasticd on a Luckfox Lyra Zero W. Both sit in the LR20x0 support added in #11252, and they interact: the switch table has to be written slightly wrong to pass validation, and the interrupt lands on a pin that table is driving. Every symptom is silent, because begin() only exercises SPI and BUSY -- the radio reports init success and then receives nothing. IRQ DIO could not be set on Portduino ------------------------------------- LR20x0Interface picked the IRQ DIO purely at compile time, and neither LR2021_IRQ_DIO_NUM nor IRQ_DIO_NUM exists for a Portduino target, so meshtasticd always fell through to RadioLib's default of DIO5 -- which is also the first RF switch line on carriers using the DIO5-DIO8 table. A variant says this with a #define (the pro-micro DIY board uses DIO9); a carrier has only the YAML, and had no way to say it. Adds Lora.IRQ_DIO_NUM, and an ARCH_PORTDUINO branch after the two existing #define branches, so a variant that already sets one still wins. The switch table was parsed as LR11xx-only ------------------------------------------ Pin names resolved to RADIOLIB_LR11X0_DIOn whatever the radio, and the mode set was the LR11xx's, so MODE_RX_HF -- a mode the LR20x0 really has -- was rejected as an unknown key and had to be omitted. The two families are not interchangeable: an LR11xx has no DIO9, so its fifth switch slot is DIO10, while an LR20x0's fifth slot is DIO9 and DIO10 is its sixth. A table naming DIO10 was therefore driving the wrong pin on an LR20x0. The YAML layer now stores what was written -- a DIO number and a neutral mode id -- and each interface supplies its own DIO constants and OpMode_t map to a shared builder. Neither family's constants are assumed to coincide with the other's. This also fixes a round trip in the config writer, which decoded pins by comparing against RADIOLIB_LR11X0_* and always emitted five values per mode row: for a four-pin table it produced YAML that --check would reject for mismatched row lengths. --check ------- Findings are now judged against the resolved module rather than a fixed list, so a mode or pin the part does have can no longer be rejected, and one it does not have is named instead of silently accepted. The claim that the table "is only applied to LR11xx radios" was stale and is corrected, and the missing-table warning now covers both families. "auto" is excluded throughout: the module has not been probed yet, so absence cannot be judged. The IRQ/switch-pin collision is reported in both directions, including the harder case where no key is set and the radio default collides -- nothing in the file looks wrong. Note that listing a pin is what breaks it, not driving it: setRfSwitchTable() reassigns the DIO function for every pin in the list whatever the levels say, so an all-LOW column is still a collision. Seven fixtures cover these, including a false-positive guard: DIO5 as the interrupt is normal, and must stay silent when the table is elsewhere. * feat(portduino): let the YAML ask for a TCXO probe, across every family that has one A variant declares "a TCXO may or may not be fitted" at compile time with TCXO_OPTIONAL, because the board is known when the image is built. A Portduino carrier cannot: the same meshtasticd binary runs on hardware populated either way, so the statement has to arrive as YAML and be answered at runtime. Adds Lora.TCXO_OPTIONAL, and TCXO_OPTIONAL_ENABLED in RadioLibInterface.h to unify the two, so each driver asks the question once rather than growing a second, Portduino-shaped code path. On an embedded target it stays a compile-time constant, so `if (TCXO_OPTIONAL_ENABLED)` folds away exactly as the old `#if` did: the nrf52_promicro_diy_tcxo image, which defines TCXO_OPTIONAL and so exercises the converted branches, still ends at 0xDF1D0 -- the same address as before this change. It is defined there rather than in a header of its own because InterfacesTemplates.cpp includes all three interface .cpp files into one translation unit, where a per-file definition would collide. Covers every family that has a TCXO reference to probe for: SX126x (sx1262/sx1268/LLCC68), LR11xx and LR20x0. With no DIO3_TCXO_VOLTAGE given, the TCXO attempt uses RadioLib's own 1.6 V default rather than being skipped -- otherwise there is nothing to fall back FROM and the flag would silently do nothing. This is also the FIXME that sat on the Portduino branch in LR20x0Interface: an unset voltage now means "no TCXO" explicitly. Two things are deliberately left alone: Each family keeps its own probe order. LR11xx tries XTAL first, because a TCXO-first attempt hangs RadioLib's unbounded calibration wait on a module with no TCXO fitted, whereas XTAL fails fast and cleanly on a module that has one; LR20x0 and SX126x try the TCXO first. A carrier therefore behaves the same way in a Portduino build as in an embedded one, and changing an order stays a hardware-behaviour decision rather than a tidying-up one. The SX126x retry is Portduino-only. An embedded TCXO_OPTIONAL board already gets this from initLoRa(), which constructs a second SX126x interface with no Vref when the first fails; retrying inside init() as well would leave that ladder step unreachable and change how every existing t-echo-class board reports its oscillator. A Portduino build has no ladder to fall through, because the module is named in YAML rather than probed. --check learns the key, reports which Vref will actually be tried, and warns when it is set on a radio with no TCXO reference, where it is read, stored and inert. * docs(portduino): condense the comments on this branch The repo asks for one or two lines and no multi-paragraph blocks, on the grounds that the diff and the commit message carry the rationale while the code carries the behaviour. What landed here was well past that: 163 added comment lines, including a 26-line block above a single macro. Removes the rhetoric, the issue numbers and the before-and-after asides, and the notes on where a thing used to live. No added block is longer than three lines now. Two facts needed stating and are stated once each rather than repeated at every use: the slot/DIO divergence between the families, in PortduinoGlue.h, and the per-family TCXO probe order, in RadioLibInterface.h. The longest surviving explanation is why an all-LOW switch column still collides with the interrupt, which sits in the fixtures README because without it that pair of fixtures reads as contradictory. Comments only; no functional change. * address CodeRabbit review on #11382 - SX126xInterface: distinguish an explicit DIO3_TCXO_VOLTAGE from the TCXO_OPTIONAL probing default in the debug log instead of always claiming the config field was set. - ConfigCheck: modesFor() now reports an unresolved use_autoconf against the union of both radio families' modes, not the LR11xx subset - fixes a false "not a mode this part has" warning for valid LR2021-only modes (e.g. RFSW_RX_HF) before autodetection resolves the module. - PortduinoGlue loadConfig: build rfswitch_mode_high[m] as a fresh per-row bitmask instead of OR-accumulating onto a stale value, so a config re-parse can clear a slot back to LOW. - PortduinoGlue YAML serialization: gate rfswitch_table emission on has_rfswitch_table rather than rfswitch_dio_num[0] >= 0 (missed sparse pin lists), and track each emitted pin's original slot so row values line up correctly instead of shifting when a low slot is absent. - config-dist.yaml: document the per-family TCXO/XTAL probe order (SX126x/LR20x0 TCXO-first, LR11x0 XTAL-first). - Trim three overlong comments per the coding-guideline nitpicks. Left the SX126x XTAL-retry-on-oscillator-failure nitpick alone - RadioLib's begin() already does its own XOSC_START_ERR recovery internally, and narrowing our wrapper's retry condition on top of that needs hardware to verify it doesn't regress a real failure path. Verified: bin/test-config-check.sh GREEN 69/69 against an isolated native build; pio test -e native -f test_rtc PASSED. * fix CI: cppcheck duplicateValueTernary, harden kRfSwitchModes init LR11x0Interface::init(): work around cppcheck's duplicateValueTernary on `TCXO_OPTIONAL_ENABLED ? 0 : tcxoVoltage` (both branches fold to 0 on a board with no ARCH_PORTDUINO, no TCXO_OPTIONAL, and no explicit Vref, since tcxoVoltage already reduces to 0 via the same macro chain) by splitting it into a plain assignment + if, rather than suppressing the warning. The other TCXO_OPTIONAL_ENABLED ternaries in this PR (LR11x0Interface.cpp:75, SX126xInterface.cpp:76, LR20x0Interface.cpp: 86,240) pick between TCXO_OPTIONAL_DEFAULT_VOLTAGE (1.6f) and 0, which can never coincide, so they're unaffected and left as-is. ConfigCheck.cpp: kRfSwitchModes was a namespace-scope global with dynamic initialization (a lambda IIFE) reading kRfSwitchModeNames, which is defined in a different translation unit (PortduinoGlue.cpp). Currently safe only because kRfSwitchModeNames's initializer is constant-expression-only (string literals + enum constants), which the standard guarantees completes before any TU's dynamic initializers - but that safety is silent and would break if PortduinoGlue.cpp's array initializer ever stopped being a constant expression, with nothing to warn a future editor. Converted to a function-local static (Meyers' singleton), which is correct by construction regardless of the other TU's initializer, updating all 4 call sites (definition + 3 uses) from kRfSwitchModes to kRfSwitchModes(). Verified: pio test -e native -f test_radio PASSED; bin/test-config- check.sh GREEN 69/69 against an isolated native build. * refactor: simplify TCXO voltage handling across interfaces and improve comments * fix rfswitch_table cross-file merge; drop now-stale checker warning Three CodeRabbit findings on |
||
|
Thomas Göttgens
|
3468af94aa |
fix(hopscale): gate hop scaling on measured channel congestion (#11826)
* fix(hopscale): gate hop scaling on measured channel congestion * fix(hopscale): rename test tripping trufflehog and release congestion at the threshold * test(hopscale): pin the unscaled precondition in the busy-channel gate test * fix(hopscale): floor infrastructure roles and engage below the polite gate * test(hopscale): name the symbol under test and reset the gate in clear() * fix(hopscale): drop the unneeded congestion reset in clear() * refactor(hopscale): drop the unused utilization accessor and duplicated log fields * refactor(hopscale): drive the politeness extension from measured congestion (#11831) * refactor(hopscale): drive the politeness extension from measured congestion * refactor(airtime): own the smoothed channel utilization (#11832) * refactor(airtime): own the smoothed channel utilization * refactor(airtime): cut comments to the two-line limit * fix(airtime): fold the smoothed utilization once per crossed bucket * fix(hopscale): compare the congestion thresholds at whole-percent resolution |
||
|
|
ee15508494 |
time: arm the remaining 0-means-unset stamps through the helpers (#11830)
* 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>
* time: arm the remaining 0-means-unset stamps through the helpers
The follow-up sweep to the sixteen fields the previous commits covered. A field
here uses 0 to mean "unset" - some read spells `if (f)`, `f != 0`, `f == 0 ||` or
`f > 0`, or a site disarms it with `f = 0` - but it was armed from a raw clock read,
so once per ~49.7-day wrap it stores the value its own readers treat as never-set.
34 fields, 58 arm sites.
The rule could not have found most of them first. It treated `field = <variable>`
as inheriting whatever that variable did, which made the commonest shape in the tree
invisible: one `now = millis()` at the top of a runOnce(), then several
`xStartTime = now` below it. Listing those names would have bought no protection at
all, so the scanner now tracks a local assigned from a clock and treats a write from
it as the raw arm it is. One hop, one function, name-based, and it forgets a local
reassigned from anything else; taint is dropped at each function boundary. Twelve
fixtures pin it, including the negative cases - no leak across functions, `now` does
not match `nowMs`, and neither `==` nor `+=` records anything.
That pass immediately found a site the previous commits missed: setTransmitDelay()
recomputes p->tx_after from a tainted `now`, two lines under the `if (p->tx_after)`
read that takes 0 as "no delay wanted".
Three of the fields are worth naming because the consequence is not cosmetic:
* UpDownInterruptBase press/up/downStartTime - xDetected is only cleared INSIDE
the block guarded by `xDetected && xStartTime > 0`, so a stored 0 makes both the
entry and the exit condition unreachable and that button is dead for the rest of
the boot, not for one tick.
* PhoneAPI lastContactMsec - ServerAPI reads `lastContactMsec > 0` before the TCP
idle close, and the field stays 0 until the next inbound packet, so a client that
never speaks again leaks the socket for the life of the connection.
* EInkDisplay lastDrawMsec - `if (lastDrawMsec)` gates every plain display() call
on a keyframe having been shown, so a stored 0 stops the screen updating until
something calls Screen::forceDisplay() again.
TransmitHistory needed more than its arm sites. getLastSentToMeshMillis() returns 0
to mean "module has never sent", and besides the two stores, both reconstruction
helpers end in `millis() - msAgo`, which can produce a 0 of their own. All three
computed returns are guarded; the deliberate `return 0;` sentinels are untouched.
Judged and deliberately not changed:
* nRF54L15 connect_time_ms is armed from k_uptime_get_32(), not millis(). It is
guarded with skipZero() but keeps its own clock - swapping in Time::getMillis()
would have it compared against a k_uptime now at the watchdog read. The rule now
recognises that clock too, so listing the field is not an empty gesture.
* RotaryEncoderInterruptBase pressStartTime shares a name with the UpDown field and
has a different contract: no read here tests the stamp against 0, pressDetected
is the only armed flag. Opted out at the write. Its lastPressLongEventTime
sibling IS a `== 0` latch and is fixed.
* PositionModule line 38 copies a value the enclosing `if (restored != 0)` has
already proven non-zero. Opted out.
* pmMeasureStarted, adminKeyFallbackRefillMs, the two autosave stamps and
scrollStartDelay are lazy initialisations whose wrap behaviour costs at most one
interval and drops nothing. Left alone, and not listed.
47 lint fixtures green, the rule silent across src/, full native suite 1421/1421.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* time: dodge the wrap at the clock read, not only at the store
Review on #11830 made a point that was right and that this branch had wrong.
Applying skipZero() at the STORE while a reader measures elapsed time against a
raw clock splits the two sides apart for one tick per ~49.7-day wrap: the stamp
becomes 1 while `now` is still 0, so `now - stamp` is UINT32_MAX and a brand new
stamp reads as about 49.7 days old. Every elapsed-since guard then fires when it
must not. Concretely, UpDownInterruptBase computed `now - pressStartTime` and
emitted a long press for a fresh press, and TraceRouteModule read
`now - lastTraceRouteTime < cooldownMs` as false and bypassed its cooldown.
So the dodge moves to the read. Time::stampMillis() is getMillis() with the one 0
tick called 1; a site that both stores a stamp and measures against stamps reads
the clock once through it and stores that value directly. Nine files, and the 1 ms
skew is the same one skipZero() already documents.
Where the clock arrives as a PARAMETER the store keeps its own skipZero() as well,
because the function cannot assume the caller dodged anything. Removing that was a
real regression and test_nexthop_routing caught it: noteRouteLearned() and
noteRouteSuccess() are called with a literal 0 by
test_health_learn_never_stores_zero_sentinel and
test_health_success_never_stores_zero_sentinel, which assert the store normalises
it - 0 is the empty-slot marker getOrAllocRouteHealth() evicts on. The two guards
compose without shifting twice, since skipZero() of a non-zero value is itself.
trySmartBroadcast() and directResponseAllowed() have the same parameter shape and
keep their store-side guard for the same reason. Only stores fed by a stampMillis()
local in the same function are bare.
EInkParallelDisplay was missed the first time: the third class in the family, still
storing skipZero(getMillis()) while rate-limiting against a raw millis() local.
Normalised like its siblings.
The lint rule gained three false positives with the class-scope tracking, all of
them shapes that are not class bodies at all:
template <class T> void f(T x) { uint32_t lastSort = millis(); }
class Foo { void tick() { uint32_t lastSort = millis(); } };
void g(struct Bar *b) { uint32_t lastSort = millis(); }
Two causes. pending_class matched class/struct anywhere on the line, so a template
parameter list and an elaborated type in a parameter list both marked the following
FUNCTION body as class scope; it is anchored to the start of the line now. And
update_scope() runs at the end of a line, so a body opened earlier on the same line
had not been counted when the statement was judged; is_declaration() now also
counts unmatched braces earlier in the statement. The rule is blocking and
`template <class T>` is ordinary C++, so these would have reddened files nobody
touched.
note_taint() also never received the per-write `;` cut the judging path was given
earlier in review, so on a line holding two statements it learned taint from the
neighbour. Same cut applied.
Five tests in test_uptime_clock pin the contract, including one that asserts the
old store-only shape really does produce UINT32_MAX, and one that pins the 1 ms
skew at 399 rather than 400 so nobody "corrects" it back into a raw read. Lint
fixtures 53 -> 65. Full native suite 1426/1426, rule silent across src/.
Known residual, deliberately not changed: a store that dodges zero while its reader
measures through a Throttle:: helper still splits for that one tick, because those
helpers read the clock internally and raw. About eight sites tree-wide, including
PositionModule trySmartBroadcast and the lastContactMsec TCP idle check. Closing it
means making Throttle read through the dodge, which was proposed on #11692 and
declined there pending a caller audit, so fixing one site here would only make the
tree inconsistent. The direction is also the same one the un-dodged code already
took: a fresh stamp reads as old, and the guards involved were already passing on a
0 stamp.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* lint: a dodged value is safe to copy, not to do arithmetic on
Two more review findings on the rule, both real, both false negatives.
Arithmetic on an already-dodged value was excused. stampMillis() guarantees only
its own result, so `now + 5000` can carry a non-zero stamp straight back onto the
sentinel - 0xFFFFEC78 + 5000 is exactly 0. That sum is precisely what
Time::timerEndsAtMillis() exists to dodge, and the rule was waving it through
because a helper name appeared somewhere in the expression. Worse, a fixture
asserted that behaviour was correct, so the self-test was pinning the hole open.
A local holding a dodged value is now tracked separately from a tainted one: it may
be stored or copied straight through, but + or - applied at the OUTERMOST level is
reported and the message points at timerEndsAtMillis(). Depth-aware, so the operator
inside Time::skipZero(getMillis() - msAgo) is still fine, and so is the
`(d == 0) ? 0 : timerEndsAtMillis(d)` arming form, which has no top-level operator
at all. The wrong fixture is replaced by four: store-through, copy one more hop,
arithmetic on a dodged local, and arithmetic on a direct helper call.
A class body that opens and closes on one line was never recognised. The header
check rejected it because the line ends in a semicolon, which a one-liner body
always does, and even once armed the class brace counted as a function body and
excused the member. Both halves fixed: the header arms on the brace rather than on
the absence of a semicolon, and when the body opened on the statement being judged,
one unmatched brace is class scope while two is a method body inside it. Getting
that wrong first broke every multi-line class, because setting the per-statement
flag without also arming pending_class meant update_scope() never registered the
body - the three existing class fixtures caught it.
72 fixtures, green under bash 3.2, shellcheck clean, rule silent across src/.
No src/ or test/ file changes, so the native suite is untouched by this commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* style(time): trim comments to the house limit
---------
Co-authored-by: Tom <116762865+Nestpebble@users.noreply.github.com>
Co-authored-by: nomdetom <nomdetom@protonmail.com>
Co-authored-by: Tom <116762865+NomDeTom@users.noreply.github.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
|
bef289ef42 |
fix(extnotif): make isNagging the only armed flag for the nag cycle (#11828)
* fix(extnotif): make isNagging the only armed flag for the nag cycle
ExternalNotificationModule kept the nag cycle's armed state in two places that
could disagree: the isNagging bool, and nagCycleCutoff reserving UINT32_MAX for
"not armed". handleInputEvent() read only the second one:
if (nagCycleCutoff != UINT32_MAX) { stopNow(); return 1; }
The field is declared `= 1`, while isNagging starts false, so at boot that test
said "armed" when nothing was nagging. The first input event of every boot was
therefore answered with stopNow() and a non-zero return - and a non-zero return
ends the observer chain (Observable::notifyObservers in src/Observer.h returns on
the first one), so that event was swallowed from every later observer. The handler
is registered whenever external_notification.enabled, and InputBroker only
short-circuits while nagging() is true, so the event does reach it.
The same read had a second failure mode once per ~49.7-day wrap: armNagCycle()
computes `millis() + durationMs`, which can land exactly on UINT32_MAX. When it
does, a real nag is running with isNagging true, but this read says "not armed" and
the module's own handler never stops it. Time::skipZero() cannot help here - it
lifts 0 to 1 and leaves UINT32_MAX alone, which src/UptimeClock.h static_asserts.
So the fix is not a zero guard, it is removing the second opinion. isNagging is
the armed flag - which is what the comment above the expiry check already claimed,
and what the other four reads already use - and nagCycleCutoff is now only ever a
deadline, read after isNagging has been checked. Nothing reserves a value, which
matters because an arm site spelled `millis() + interval` can produce any value
there is, so no value is safe to reserve. That is the shape the TODO(deadline-type)
note in src/mesh/Throttle.h is aiming at, and that note is updated to match rather
than keep describing the sentinel this removes.
Worth knowing for review, though not changed here: InputBroker::handleInputEvent
already calls stopNow() itself when nagging() is true, and returns without
notifying observers. Every path that starts a notification calls armNagCycle()
first, so isNagging is true for the whole life of any real nag. That makes this
handler reachable only when there is nothing to stop - its stopNow() was never
doing useful work. Gated rather than deleted, because removing a public handler
and its observer registration is a bigger call than fixing the defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* style(extnotif): trim comments to the house limit
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: nomdetom <nomdetom@protonmail.com>
|
||
|
Matias DendaandThomas Göttgens
|
d05fbec64c |
Add AEAD (AES-CCM) authenticated encryption for PSK channels (#9749)
* Add AEAD (AES-CCM) authenticated encryption for PSK channels Extend PSK channel encryption with optional AES-CCM authenticated encryption (use_aead flag in ChannelSettings). When enabled, messages include a 12-byte authentication tag that prevents forgery, bit-flipping, and injection attacks by anyone with the channel PSK. Changes: - Add encryptPacketCCM/decryptPacketCCM to CryptoEngine with key promotion (16-byte keys zero-padded to 32 for AESSmall256 compat) - Move AES-CCM primitives (aes-ccm.h/cpp, aesSetKey, aesEncrypt) outside PKI guard so they're available unconditionally - Add isAEADEnabled() to Channels with hash differentiation (XOR 0xAE) - Add AEAD encrypt/decrypt branches in Router perhapsEncode/perhapsDecode with no CTR fallback on AEAD channels - Add use_aead field to channel.pb.h (bool, tag 8) - Add MESHTASTIC_AEAD_OVERHEAD constant to RadioInterface.h - Add comprehensive test suite: round-trip (AES-128/256), tamper detection (ciphertext, tag, sweep), wrong PSK, wrong sender, packet-too-small, deterministic output verification Addresses firmware#4030. * Apply clang-format to match project style * Guard AEAD path against empty PSK and check encrypt return value - Add early return in encryptPacketCCM/decryptPacketCCM when psk.length == 0, preventing null dereference in aesSetKey - Check encryptPacketCCM return value in Router::perhapsEncode (both PKI and non-PKI paths), returning BAD_REQUEST on failure instead of silently transmitting corrupt packets - Add unit test for empty PSK (encrypt and decrypt must return false without crashing) * Use true AES-128 for 16-byte PSKs instead of promoting to AES-256 aesSetKey now dispatches based on key length: 16 bytes creates AESSmall128, 32 bytes creates AESSmall256. The aes member type changes from AESSmall256 to BlockCipher (polymorphic base class). This removes the unnecessary key promotion that added two extra AES rounds (14 vs 12) with no security benefit since the entropy stays at 128 bits for 16-byte keys. encryptPacketCCM/decryptPacketCCM now pass psk.length directly to aes_ccm_ae/aes_ccm_ad instead of promoting to 32. New tests: ECB AES-128 with NIST vectors, AEAD test verifying AES-128 and AES-256 produce different ciphertexts with same key material and cross-key decryption fails. * Reject the invalid-key sentinel in the AEAD paths CryptoKey documents length == -1 as "invalid key - do not use", but the AEAD guards only tested for 0. Since length is int8_t and the aes_ccm_* key length parameter is size_t, a -1 would widen into a huge unsigned length and be handed to the cipher instead of being rejected. Both callers in Router.cpp are gated on a non-negative channel hash, and generateHash() already returns -1 exactly when getKey() yields an invalid key, so the sentinel cannot reach these functions today. Guard against it anyway rather than relying on callers to keep that invariant. * Tie MESHTASTIC_AEAD_OVERHEAD to CryptoEngine::AEAD_TAG_SIZE The packet-size boundary checks in perhapsEncode/perhapsDecode budget for MESHTASTIC_AEAD_OVERHEAD, but the tag actually written is AEAD_TAG_SIZE. Nothing tied the two together, so changing one would have silently produced oversized packets or truncated payloads. Assert they match instead of coupling RadioInterface.h to CryptoEngine. Also trims the sentinel comment to the two-line limit in AGENTS.md. * Add RFC 3610 known-answer vectors and widen the tamper sweep Packet Vectors #1, #2 and #7 pin aes_ccm_ae()/aes_ccm_ad() to published data rather than to their own output, covering M=8 and M=10, a trailing partial block in every case, and rejection of a modified AAD. Test 1 in test_AES_CCM_AEAD is relabelled as the smoke test it actually is. The per-byte tamper loop now walks the whole buffer including the tag, instead of only the first four ciphertext bytes. * Cover the second nonce input and tighten the AEAD test buffers Test 10 only ever varied fromNode, leaving packetId — the other half of the nonce — unexercised. It now checks each one wrong on its own, both wrong, and both right, so the negative assertions cannot pass vacuously. The undersized-packet test wrote into a one-byte buffer and only survived because decryptPacketCCM() returns before touching it; size it for the whole input so a regressed length guard fails an assertion instead of the stack. Also assert makePsk() cannot overrun CryptoKey::bytes. * Rewrite Unicode dashes to ASCII in AEAD comments The ascii-dash formatter that landed in develop rewrites U+2014/U+2013 to an ASCII hyphen. Three files on this branch still carried em dashes in comments, so Trunk Check went red once develop was merged in. Comments only, no code change. * Authenticate sender and destination IDs as AEAD associated data The nonce binds the sender and the packet id, but nothing bound the destination, so `to` could be rewritten in flight and the tag would still validate. Pass `from || to` as associated data to aes_ccm_ae/aes_ccm_ad so a redirected packet fails authentication. The hop fields stay out of the AAD on purpose: relays legitimately rewrite hop_limit, hop_start, relay_node and next_hop. Adds a sub-test covering redirection to another node and promotion of a unicast to a broadcast; both must be rejected, and the unmodified destination must still round-trip. This changes the on-the-wire format for AEAD packets. Nothing ships with use_aead yet, so there is no deployed traffic to stay compatible with. * fix(crypto): repair EXCLUDE_PKI builds and guard AEAD channel config aes-ccm.cpp is compiled in every build now and calls CryptoEngine::aesSetKey and CryptoEngine::aesEncrypt, whose definitions were still inside the !(MESHTASTIC_EXCLUDE_PKI) block in CryptoEngine.cpp, so MESHTASTIC_EXCLUDE_PKI=1 failed at the link step. Move both definitions outside the guard, and move the pending-public-key declarations back inside it next to the fields they read. fixupChannel() clears use_aead on a channel that resolves to no key material. That combination kept a valid-looking channel hash while every encode returned BAD_REQUEST and every decode dropped, with nothing in the config to show why. encryptPacketCCM/decryptPacketCCM are virtual, so a platform engine can back them with hardware CCM the way it already overrides encryptAESCtr. perhapsEncode() carries one copy of the AEAD/CTR branch instead of an identical copy in each arm of the MESHTASTIC_EXCLUDE_PKI ifdef. Tests: three use_aead cases in test_channel_keys covering the hash split, the no-key clear, and a secondary that borrows the primary's key. * fix(crypto): move CryptoEngine::hash out of the PKI guard hash() is plain SHA256, and PortduinoGlue calls it unguarded to derive a MAC address from the CH341 serial, so MESHTASTIC_EXCLUDE_PKI=1 failed to compile. With this and the previous commit that build links clean. * fix(channels): resolve primaryIndex before hashing in onConfigChanged A keyless secondary resolves its key through primaryIndex, so fixing up channels in the same pass that finds the primary hashed the early slots against the previous one and cleared their use_aead against a key they do in fact inherit. Split the pass, and re-run the fixups in the no-primary restore path, which moves the primary after the fact. Also splits the thirteen AES-CCM AEAD scenarios into separate test functions so a Unity failure names the one that broke. * chore(crypto): trim the AEAD maintainer commits Shortens three comments that outgrew the one-to-two line house rule, drops a truncated sentence and the braces around a single return in perhapsEncode(), and removes a channel test that the moved-primary regression test already covers. No behaviour change. --------- Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> |
||
|
|
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>
|
||
|
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öttgensandClaude Opus 5
|
8a9e10d120 |
fix(power): stop a battery-less board deep-sleeping itself forever (#11821)
* fix(power): stop a battery-less board deep-sleeping itself forever The low-battery counter only reset inside its `hasBattery && !hasUSB` guard, so a board with no battery - whose floating divider drifts in and out of the battery-present window - ratcheted the count up across the gaps until it tripped `sds_secs`, which defaults to a ~24.8-day deep sleep. The button could not rescue it either, because `doDeepSleep()` force-holds `BUTTON_PIN` and a held pad ignores `ext1_wakeup_prepare()`'s re-route to RTC; `rtc_gpio_isolate()`'s pin list has the same effect on boards whose button is GPIO 2 or 34. Separately the cutoff now scales by `NUM_CELLS`, without which no multi-cell pack can ever read low enough to shut down at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(power): satisfy trunk check Apply the `ascii-dash` autoformat that `trunk fmt` wants on the comments this PR's file already carries, and rename the no-battery test so its `test_` prefix plus exactly 35 characters stops matching trufflehog's Lob API-key shape. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Thomas Göttgens
|
d4482a28a1 |
Fail the build when Telemetry no longer fits the packet payload (#11810)
* Fail the build when Telemetry no longer fits the packet payload * Trim the comments to the house limit |
||
|
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 |
||
|
Benjamin Faershtein
|
29a65aa13d | fix(mesh): use valid default packet history size (#11786) | ||
|
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> |
||
|
Thomas Göttgens
|
14eaa5587d |
Honor mute when waking the screen for a received message (#11688)
* fix(ui): honor mute when waking the screen for a received message TextMessageModule fired powerFSM.trigger(EVENT_RECEIVED_MSG) for every text packet, gated only by shouldWakeOnReceivedMessage(), which checks external notification, device role and battery level but never the mute flags. A muted channel therefore suppressed the banner and still lit the screen. MessageRenderer::handleNewMessage() only computed mute for MessageType::BROADCAST, so a DM from a muted node produced a banner and a wake. Add isMutedForPacket() in Channels: a DM addressed to us reads the sender's NodeInfoLite mute bit, every other packet reads the mute bit of the channel it arrived on. This is the predicate ExternalNotificationModule already applied to the buzzer, vibra and LED outputs, hoisted so all three call sites share it. Bell and alert messages still break through mute on both paths, unchanged. No protobuf or config change: ChannelSettings.module_settings.is_muted and the NodeInfoLite mute bit already exist and are already settable from the device menu and via AdminMessage.toggle_muted_node. Closes #11674 * fix(ui): let an alert break through mute on the screen wake path In COLOR display mode TextMessageModule skips handleNewMessage(), so powerFSM.trigger(EVENT_RECEIVED_MSG) is the only wake an alert gets. Gating it on mute alone dropped that wake for a bell on a muted channel. Add MeshService::isAlertPayload(): an ASCII BEL in the payload while at least one alert_bell_* output is enabled. The wake gate is now "not muted, or an alert". MessageRenderer uses the same predicate instead of its own inline bell scan, which also lifts that scan's arbitrary 100 byte cap. Rename three test cases. Their names carried exactly 35 characters after the test_ prefix, which matches the Lob API key format and tripped trufflehog in the trunk check gate. |
||
|
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
|
7e9525ad83 |
feat(baseui): default US to LongTurbo on first region selection (#11637)
Selecting US in the BaseUI region chooser now installs LongTurbo instead of LongFast, but only for out-of-box setup: the outgoing region must be UNSET, so a later switch to US leaves whatever preset the node is running alone. Scoped to the menu on purpose. The US entry in regions[] keeps LongFast as its default preset, so preset repair, admin/phone writes and every other route onto US are unchanged. A build pinning USERPREFS_LORACONFIG_MODEM_PRESET, a preset already moved off the install default, or use_preset=false all outrank it. The decision is lifted into menuHandler::presetForRegionSelection() so it is reachable without a Screen, following toggleNodeMuted(). |
||
|
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. |
||
|
Ben Meadors
|
122ec0e9f4 |
Revert "feat(baseui): default US to LongTurbo on first region selection"
This reverts commit
|
||
|
Ben Meadors
|
dbba2b3f6c |
feat(baseui): default US to LongTurbo on first region selection
Selecting US in the BaseUI region chooser now installs LongTurbo instead of LongFast, but only for out-of-box setup: the outgoing region must be UNSET, so a later switch to US leaves whatever preset the node is running alone. Scoped to the menu on purpose. The US entry in regions[] keeps LongFast as its default preset, so preset repair, admin/phone writes and every other route onto US are unchanged. A build pinning USERPREFS_LORACONFIG_MODEM_PRESET, a preset already moved off the install default, or use_preset=false all outrank it. The decision is lifted into menuHandler::presetForRegionSelection() so it is reachable without a Screen, following toggleNodeMuted(). |
||
|
Ben Meadors
|
9a59e9088d |
fix(test): restore the sendAckNak overrides broken by #10767 (#11626)
#10767 added a relaySource parameter to the RoutingModule::sendAckNak virtual, but the five test mocks that derive from RoutingModule still declared the six-parameter signature with `override`. Nothing overrides the new virtual, so all five suites fail to compile and the native test job has been red on develop since the merge: test/test_reliable_ack_matrix/test_main.cpp:167:10: error: 'void MockRoutingModule::sendAckNak(meshtastic_Routing_Error, NodeNum, PacketId, ChannelIndex, uint8_t, bool)' marked 'override', but does not override Widen the five mocks to the new signature. Also carry has_rx_rssi with rx_rssi in allocAckNak(). rx_rssi has explicit presence, so copying only the value left has_rx_rssi false and nanopb dropped the field at encode time - the phone never saw the relayer's RSSI that #10767 set out to deliver. Cover both: test_reliable_ack_matrix asserts the overheard rebroadcast is handed through as the relay source on the decodable path and the opaque #11502 ingress path, and that no other ACK/NAK claims a relayer; test_mesh_module drives a real RoutingModule and asserts the relay fields, has_rx_rssi included, survive all the way to the phone. |
||
|
Thomas Göttgens
|
a8934a16d4 |
Route waypoint expiry through waypointIsActive instead of a raw getTime compare (#11621)
* Route waypoint expiry through waypointIsActive instead of a raw getTime compare * Let isExpired own the zero-clock policy for purgeExpired too * Resolve the clock in isExpired when a packet carries no valid rx_time |
||
|
Thomas Göttgens
|
a89f1920e1 |
fix(traffic): don't re-stamp dropped duplicate positions, which slid the dedup window indefinitely (#11620)
* fix(traffic): don't re-stamp dropped duplicate positions, which slid the dedup window indefinitely * test(traffic): trim the regression test comment and derive its counts |
||
|
AustinandClaude Opus 5
|
514b476189 |
feat(admin): append the optional ham long_name to the call sign (#11612)
* feat(admin): append the optional ham long_name to the call sign HamParameters gained a long_name field (meshtastic/protobufs#941) that handleSetHamMode never read, so a client that sent one still ended up with a node named after the bare call sign. Join it behind the call sign with the "//" separator hams already use on the air: call_sign "N0CALL" plus long_name "Attic Heltec" becomes "N0CALL//Attic Heltec". An absent long_name keeps the previous call-sign-only name, which is what the on-device region picker still sends. Being cosmetic, long_name stays out of the whitespace-only rejection that guards call_sign and short_name: a blank one is dropped rather than costing the operator the whole licensing request over a stray space, which that path would report only as a LOG_WARN and so would be invisible from the app. The composed name is finished with clampLongName() rather than a bare sanitizeUtf8(), matching handleSetOwner and NodeDB: the proto caps the parts at 7 + 2 + 14 bytes, inside the 24-byte local budget, and clampLongName is the backstop if either cap moves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(admin): enhance handleSetHamMode to return status for request validation --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Thomas Göttgens
|
576a1bb008 |
Fix trackball dropping short presses and losing the click when tilted (#11599)
* Fix trackball dropping short presses and losing the click when tilted * Do not let a direction counter overwrite an emitted press event * Accept the first press interrupt when the clock still reads zero * Classify a press released before the first poll by its latched time |
||
|
|
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> |
||
|
TomandBen Meadors
|
7e11bde8c8 |
fix(beacon): repair the MeshBeacon radio switch/restore regression from #11573 (#11596)
* fix(radio): put the beacon restore back inside completeSending's if (p) Reverts the RadioLibInterface and RadioInterface changes from #11573 ( |
||
|
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> |
||
|
Thomas Göttgens
|
56ce743f75 | Show waypoints sent with no expiry and stop expiring on an unset clock (#11600) | ||
|
Thomas Göttgens
|
8a15d9258f |
fix(test): unbreak test_radio under ASan (#11589)
* test_radio: prove the rejected packet was released via pool accounting, not pointer identity * test-state.sh: silence the shell's own open failure when scanning /proc for survivors * Trim the comments added with the test_radio and test-state fixes |
||
|
Clive BlackledgeandClaude Opus 5
|
0271be9369 |
fix(SafeFile): remove a stale .tmp before opening it for write (#11428)
* fix(SafeFile): remove a stale .tmp before opening it for write SafeFile writes to <filename>.tmp, verifies it by readback, then renames it over the real file. openFile() never removed a pre-existing .tmp - an unfinished FIXME - and FILE_O_WRITE appends rather than truncates on Adafruit_LittleFS (nRF52) and STM32 LittleFS. So a .tmp left behind by a reset in the window between close() and renameFile() is appended to on the next save. The readback hash covers only the bytes just written, so it mismatches, close() returns false, and the tmp is left behind again - the failure latches and every subsequent save of that file fails. Today saveProto() discards close()'s result, so this is silent and permanent. Guard the remove with exists(): a bare remove() of a missing file logs on Portduino. The same guarded pattern is already used for this exact append trap in xmodem.cpp. Note the FIXME's commented-out body named the wrong path - it removed 'filename', the real file, not 'filenameTmp' - so it would have destroyed the good copy had it ever been enabled. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(SafeFile): cover the stale-.tmp path, and trim the fix comment Adds test_safefile, the first coverage of SafeFile's write-tmp / verify-by-readback / rename-over path that every saveProto() caller goes through. Five cases pin the contract the fix restores: whatever the backend does on open, a completed save leaves the real file holding exactly the bytes written and nothing else, on both the fullAtomic and the !fullAtomic construction, with no .tmp left behind. These tests cannot go red on the native host, and that gap cannot be closed here. FILE_O_WRITE is an append-then-seek-to-end open only on Adafruit_LittleFS (nRF52) and on the in-repo STM32 port (STM32_LittleFS_File.cpp: LFS_O_RDWR | LFS_O_CREAT followed by lfs_file_seek to LFS_SEEK_END). On Portduino FILE_O_WRITE is the string "w" (FSCommon.h:13), which reaches fopen() and truncates. Reverting the source fix and re-running leaves all five green, verified rather than assumed. test_write_open_truncates _on_this_host asserts that premise out loud, so if the host ever gains the append behaviour the suite starts discriminating instead of quietly agreeing. Why the original FIXME stayed commented out, since that is the real history here. It read "if (fullAtomic) FSCom.remove(filename)" and named the real file, not the tmp. Running it would delete the last good copy before the replacement had been written and verified, which is precisely the guarantee fullAtomic exists to provide. Disabling it was correct. The fix under test removes filenameTmp instead, which is the file that actually carries the stale bytes, and is safe to drop at any point because nothing has been promised about it yet. Scoping the remove to fullAtomic would be wrong for the same reason. Both paths open the same filenameTmp with the same FILE_O_WRITE; fullAtomic only decides whether the real file is nuked up front to free space. The !fullAtomic path is the space-constrained one, so it is if anything the more likely to be interrupted mid-write and inherit a stale tmp. Test 2 pins that. On the cost of the added exists(). Every saveProto() already ends in SafeFile::close(), which calls testReadback(): it reopens the tmp and reads the whole proto back one byte at a time through f2.read() to XOR a verification hash, then renames. So the per-save cost is already an open, a full write, a close, a full byte-wise reread, and a rename. One exists() is a single path lookup with no erase, no program and no data read, and on the common path there is no remove() at all. Next to the readback loop it is noise. Happy to put a number on it if wanted. Scoping it to fullAtomic would also not do what it looks like it does. SafeFile's constructor defaults fullAtomic to false (SafeFile.h:28), and of the saveProto call sites only saveDeviceStateToDisk passes true. Config, moduleconfig, channels, nodedatabase and backup all take the default, so scoping would leave the stale tmp live on almost every save path, including the space-constrained one most likely to be interrupted mid-write. Also trims the fix's comment to two lines per AGENTS.md, and drops the stale-tmp removal log from LOG_WARN to LOG_DEBUG: an interrupted write is recoverable and self-healing, so it does not warrant a warning on every boot after one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(SafeFile): fail the write when a stale tmp cannot be removed openFile() ignored the result of FSCom.remove(). If the removal failed on an append-on-write backend, the open that follows appended to the stale bytes, and the readback hash is an 8 bit XOR over the whole tmp, so polluted content has a real chance of verifying and being renamed over the good file. It now logs and returns an invalid File. SafeFile::write() already no-ops on !f and close() already returns false, so the caller sees the save fail rather than silently getting a corrupt one. This is the only checked FSCom.remove() in the tree; the other call sites are all best-effort cleanups where failure does not compromise anything. Also gates test_write_open_truncates_on_this_host to ARCH_PORTDUINO. It asserts that this host truncates on FILE_O_WRITE, which is false by design on the Adafruit_LittleFS and STM32 backends the fix exists for, so running the suite there would fail on a premise that is only meant to describe the test host. Both reported by CodeRabbit on #11428. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tadayoshi MIURA
|
ac330e6a6b |
fix(radio): MeshBeacon heap leak and runtime packet payload size check (#11573)
* Fix for MeshBeacon packet leakage * fix: add runtime payload size check against radiobuffer * review fix for PR#11573: clear target radio settings before MeshBeacon packet release * add unit test for radio buffer capacity check, removing related assert for the test * review fix for PR#11573: add explicit verifaction against rejected packets |
||
|
Tom
|
bc035bb812 |
feat(lora): state a pinned userPrefs preset as the unset region's intent (#11507)
* feat(lora): state a pinned userPrefs preset as the unset region's intent A vendor build can pin USERPREFS_LORACONFIG_MODEM_PRESET while leaving the region unset, so a fresh flash comes up as region UNSET plus a deliberate preset. Stock installs come up as region UNSET plus the LONG_FAST placeholder, and nothing in FromRadio told the two apart - so clients treat every unset-region node as factory-fresh and replace its preset with the region default as soon as the user picks a region. A mesh pinned to SHORT_TURBO loses every new node to LONG_FAST or LONG_TURBO, silently. getRegionPresetMap() now emits an UNSET entry when, and only when, the build pins a preset, stating that preset as both the group's sole entry and its default. Stock builds are unchanged on the wire: no UNSET entry, which clients already read as unconstrained. This is intent, not enforcement. supportsPreset() still accepts any known preset while the region is unset (#11496) and the radio is held silent either way, so the device continues to honour whatever the user or an admin sets. Costs one group slot and one region slot on pinned builds only (6->7 of 8, 34->35 of 38); exhaustion is logged and degrades to the existing unconstrained behaviour. * Trim comments to the project's one-to-two-line limit |
||
|
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> |
||
|
Andrew YongandThomas Göttgens
|
93d15a5368 |
Add AS3935 lightning sensor support (#10931)
* Add AS3935 lightning sensor support Implements meshtastic/firmware#10774: an AS3935Sensor (TelemetrySensor subclass) that reports lightning_strike_count_1h and lightning_distance_km on the normal environment telemetry interval, like a rain gauge - strikes are counted over a fixed rolling ~1h window and read non-destructively, so replying to a peer's telemetry request in between broadcasts can't silently drop counted strikes. The AS3935's IRQ pin (opt-in per board via AS3935_IRQ) is polled with a plain digitalRead() in runOnce(), deliberately not attachInterrupt(): the IRQ line is a level that stays asserted until its interrupt register is read, so polling can't miss an event regardless of timing, matching the SparkFun library's own reference examples. An interrupt would also buy nothing here even setting that aside - classification requires an I2C read (readInterruptReg(), which itself calls delay(2) per the datasheet's settle-time requirement), and blocking I2C/delay() calls aren't safe from ISR context on any of this codebase's target platforms, so the ISR could only ever set a flag for later draining - no less work than just polling the pin directly on the next tick. A genuine lightning classification also requests an immediate out-of-cycle send via a new EnvironmentTelemetryModule:: requestImmediateSend() hook. There's no fixed debounce on the request itself - EnvironmentTelemetryModule's existing airtime/duty-cycle gate already paces every send, so it sends as often as airtime allows rather than an arbitrary fixed rate. The request does expire after 5 minutes unfulfilled, so it can't fire an arbitrarily stale broadcast if airtime was blocked for a long stretch. The AS3935's I2C addresses (0x01-0x03) fall inside the range this codebase's I2C scanner otherwise skips as reserved, so detection is a small dedicated probe gated behind AS3935_IRQ and respecting the caller's address filter, rather than a change to the general scan loop. Presence is confirmed via a register write/readback round-trip rather than a fixed expected value, since the AS3935 has no WHOAMI register and a power-on-reset-only check can't survive a warm reboot that doesn't power-cycle the sensor (initDevice() permanently rewrites that register on first configuration). Generated files under src/mesh/generated/ are intentionally excluded from this commit - they're regenerated from the protobufs submodule by update_protobufs.yml, and hand edits get overwritten and conflict once the companion protobufs PR merges and the submodule pointer updates. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * fix(as3935): calibration and telemetry logging initDevice() never called the library's calibrateOsc(). The AS3935's internal oscillators are calibrated against the antenna's resonance, which the AFE/watchdog/spike-rejection thresholds depend on; without it, only a directly-driven IRQ pin (bypassing detection entirely) reacted during testing. The sensor could already have a historical detection event latching the IRQ pin high before our initialization. Added an explicit drain read after the IRQ pin is configured, so the sensor doesn't start out stuck asserting IRQ. EnvironmentTelemetryModule::sendTelemetry() logs every other environment metric category on send but was missing lightning; added a matching log line. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg> * Support AS3935 without an IRQ line, make the antenna trim configurable Detection no longer requires AS3935_IRQ. The probe is gated like the other environmental sensors, so an I2C-only breakout is found on any board. Where AS3935_IRQ is defined the pin still gates the I2C read, otherwise runOnce() polls the interrupt register, which latches until read. Antenna tuning capacitance moves to AdminMessage.sensor_config.as3935_config, persisted to /prefs/as3935.dat and defaulting to 96pF. The chip does not retain it across power loss. Disturbers are masked in the chip, since runOnce() now polls every second. The lightning telemetry log is guarded so nodes without the sensor no longer log it on every send. Requires meshtastic/protobufs#981. * Revert protobufs pointer to the develop baseline The submodule bump conflicts on merge and the generated headers come from an out of band CI job, so the pointer moves with that job rather than in this branch. * Report lightning strikes over a true rolling hour strikeCountWindow was zeroed on a fixed interval, so lightning_strike_count_1h reported strikes since the last reset rather than over the preceding hour. RollingCounter is a fixed memory sliding window: one counter per bucket, nothing stored per event, so a storm cannot grow it. The ring holds one bucket more than the window needs so none is recycled while part of it is still inside, and the oldest bucket contributes only the fraction still in range. Both are needed to hold the span at exactly the window length rather than letting it drift by a bucket either way. Expiry is exact to one bucket rather than to the event, which is below the 5 minute floor on mesh telemetry sends. The distance expires with the last strike in the window instead of on the interval reset. Covered by test/test_rolling_counter. * Widen the RollingCounter edge weighting to 64 bit counts * inWindow is a 32 bit product, so a bucket holding more than 2^32 / BucketMs events wraps. At a 5 minute width that is about 14k: a bucket of 50000 reported 11367 instead of 40000 once it reached the window edge. Below the threshold nothing changes, so lightning was unaffected, but the helper is meant to be reused by counters with far higher rates. test_large_burst_at_window_edge covers it. The existing burst test sampled only inside the window, where the bucket is whole and never weighted. * Trim RollingCounter comments to the house limit --------- Signed-off-by: Andrew Yong <me@ndoo.sg> Co-authored-by: Thomas Göttgens <tgoettgens@gmail.com> |
||
|
Ben MeadorsandClaude Fable 5
|
a5fc95f774 |
fix(mesh): coerce coordinate traffic to the position channel on event builds (#11545)
Under USERPREFS_BLOCK_POSITION_ON_EVENT_CHANNEL every coordinate packet a client aimed at the event channel was rejected with the "Location sharing is disabled on this channel" notification - including the phone's own location feed. Both apps hand a GPS-less node its fix as a POSITION_APP packet addressed to the node itself on channel 0; that packet never leaves the device (Router::sendLocal delivers it locally) but resolved to the event channel and was dropped before PositionModule saw it. Result: the toast on every location tick, and nodes without a GPS never learned a position to share on their private channel. Position traffic now converges on the position channel - findPositionChannel(), the first channel with non-zero on-wire precision, which is never the event channel: - From-us-to-us coordinate packets are exempt from the event block. - Local coordinate sends aimed at the event channel (phone share-location, request-position, waypoints, any module/UI originator) are moved onto the position channel in Router::sendLocal and PhoneAPI instead of rejected. The client notification is only sent when no channel carries positions at all. - A position request DM'd to us on the event channel is answered on the position channel at that channel's precision (request_id preserved, same reply throttle); the requester's coordinates are still not stored, forwarded, relayed or published. want_response from the bitfield is merged before the event-channel decode short-circuit so such requests are seen. - PositionModule::sendOurPosition, positionUnchangedSinceLastSend and MeshService::trySendPosition use the shared helper instead of three copies of the same walk. Non-event builds are unaffected: the coercion compiles out and the helper matches the previous walk. Tests: coverage-event-policy (test_event_channel_phone_api, test_event_channel_router, test_position_precision, test_mqtt, test_nexthop_routing) and the same suites with the policy off. Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
Ixitxachitl
|
8fe246e250 |
fix(mesh): relay foreign packets whose channel hash collides with a local channel (#11544)
* fix(mesh): relay foreign packets whose channel hash collides with a local channel
The channel hash is one byte, so a foreign channel's name/PSK can fold to the
same hash as a local channel (~1/256 per local channel held). Since
|
||
|
Ben MeadorsandClaude Opus 5
|
83fd62b756 |
test(native): add 14 suites for routing, persistence, parsing and identity gaps (#11515)
* test(native): add 14 suites for routing, persistence, parsing and identity gaps Coverage audit of the native test tree; adds the highest-value untested logic as 11 new suites and extends 3 existing ones (200 test functions). New: test_stream_framing, test_nodedb_boot_recovery, test_nodedb_legacy_migration, test_nodedb_v25_roundtrip, test_nodedb_identity_hygiene, test_channel_keys, test_reliable_ack_matrix, test_hop_start_policy, test_routing_response_hops, test_phone_api_config_dump, test_observer. Extended: test_rtc, test_mqtt, test_xmodem. Two source changes the audit produced: - StreamAPI::handleRecStream copied stream->read()'s `cInt < 0` EOF check into the buffer-fed path, where there is no EOF sentinel; with signed char any byte >= 0x80 (START1 is 0x94) aborted the parse. Read the byte as uint8_t directly. Latent on develop (no callers), pinned by test_stream_framing. - Extract the post-decode pre-hop predicate from Router::handleReceived into shouldSkipHandleForPostDecodeHop() (NodeDB.h) so test_hop_start_policy drives the exact expression the router calls. No behavior change. test/state-manifest.tsv declares the suites that construct a NodeDB. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): address review - harden observer dispatch, trim comments Review follow-ups on the coverage-audit suites: - Observable::notifyObservers() erased list nodes while holding an iterator into them, so an observer that unobserves itself from onNotify corrupted the dispatch. Today the only self-detacher (PhoneAPI::onNotify -> checkConnectionTimeout -> close -> unobserve) survives solely because it returns -1 and aborts the chain before the increment; that unwritten contract is now gone. Removal during a dispatch nulls the entry and the outermost notify sweeps afterwards, which keeps self-detach, next-detach and destruction-during-notify all safe without an allocation. Hoisting the next iterator instead would have inverted the hazard and broken the existing next-detach case. Two regression tests added. - Correct the documented caller of shouldSkipHandleForPostDecodeHop: the call is in Router::dispatchReceived, not handleReceived. - Cast hop fields to unsigned at the %u call site in test_hop_start_policy. - Trim the new suites' file headers to the one-or-two-line rule in AGENTS.md. - Rename eight test functions whose names were exactly `test_` + 35 chars: that is the shape of a Lob API key, so trufflehog flagged them as secrets and failed the Trunk CI check. Full 68-suite Docker coverage run matches the pre-change baseline. * test(native): revert the observer dispatch change, keep the contract test Backs out the notifyObservers() deferred-removal hardening from the previous commit. It was reviewer-driven scope creep: nothing in the coverage audit needed it, no test required it, and it changes dispatch semantics in a header with ~76 observe() call sites on native verification alone. The hazard it addressed is not reachable today. The only observer that unobserves itself from onNotify is PhoneAPI (onNotify -> checkConnectionTimeout -> close -> unobserve), and it returns -1, which aborts the chain before the iterator is advanced past the erased node. test_self_detach_with_abort_during_notify stays: it passes against the unmodified dispatch and pins that the -1 is load-bearing, so a later cleanup that "simplifies" it away goes red. The unsafe variant (self-detach returning 0) is documented in a comment rather than tested, since asserting it would be asserting UB. * fix(serial): recover the frame behind a stray framing marker A byte that failed the START2 check was discarded rather than re-tested as a possible START1, so 0x94 0x94 0xc3 ... lost the real frame: one corrupted byte on a noisy UART silently dropped the frame behind it. Re-test the byte in place instead. Applied to both copies of the receive state machine. readStream() is the one that matters in the field - it is the serial path every phone client uses - while handleRecStream() still has no callers on develop. Strictly widens what the parser accepts; no frame that parsed before parses differently. test_stream_framing covers it on both receive paths, plus a run of stray markers and a START1-then-unrelated-byte resync. This was originally documented as a known gap in the framing suite. Fixing it instead was NomDeTom's call on review: a passing test asserting the bad behavior is what makes it hard to change later, and it is the same defect shape as the signedness fix three functions away. Also: use Throttle::deadlinePassed() in test_reliable_ack_matrix rather than a bare millis() compare, matching the house deadline rule. * test(native): cover the stray-marker resync on the buffer path too The stray-marker fix went into both copies of the receive state machine, but only test_stray_start1_before_frame_still_delivers drove both. The repeated- marker and unrelated-byte cases drove readStream() alone, so a regression in handleRecStream() would have gone unnoticed by two of the three. Verified load-bearing: reverting only the handleRecStream() half of the fix turns test_repeated_stray_start1_before_frame_still_delivers red on the new assertion. test_start1_then_unrelated_byte_resyncs stays green under that mutation by design - its failing byte is 0x00, where both branches reset to 0 - and covers the other half of the ternary. Also drops the stale header on test_stray_start1_before_frame_still_delivers, which still described the gap as pinned-as-is after the fix landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(native): make the hop-start truth table assert the rows it prints test_truth_table_summary was six TEST_MESSAGE lines and no assertion, so it reported as a case that could not fail - the anti-pattern #11517 names in its unfinished assertion-presence lint, and the one exception to NomDeTom's "no RUN_TEST without an assertion" pass over this PR. The printed row and the checked expectation now come from one struct, so the summary cannot narrate a table the predicates no longer implement. It also covers the consequence columns the per-row tests do not assert together: classifyHopStart, shouldDropPacketForPreHop and shouldSkipHandleForPostDecodeHop for the same packet, with the expectations gated on MESHTASTIC_PREHOP_DROP. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Thomas Göttgensandcoderabbitai[bot]
|
ef2be877a5 |
Stop breaking TestUtil.cpp on Windows, dangit! (#11529)
* Stop breaking TestUtil.cpp on Windows, dangit! * Update test/TestUtil.cpp Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> |
||
|
Tom
|
bca7c0b480 |
Tom fiddles with the test suite - again (#11517)
* test: make every suite run its own binary, and fail the run when it does not PlatformIO links every native test program to the one $BUILD_DIR/$PROGNAME path and attributes Unity output by text alone, never checking that the source file a case came from belongs to the suite it thinks it ran. Both harnesses had been split into a build pass (--without-testing) and a run pass (--without-building), and for a non-embedded platform the run pass never relinks - so all 57 suites executed whichever suite was linked last, each reporting PASSED under its own name. Introduced for CI in |
||
|
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. |