mirror of
https://github.com/meshtastic/firmware.git
synced 2026-11-02 12:25:51 -05:00
renovate/python-3.x
4
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Jonathan BennettandClaude Opus 5
|
e691bd3790 |
Claude/dmshell lock (#12024)
* Lock: give Portduino a real mutex instead of the empty fallback Lock.cpp has a FreeRTOS implementation and an empty one, and Portduino takes the empty one: every lock() and unlock() on a Linux build is a no-op, so concurrency::Lock protects nothing there. TrafficManagementModule's cacheLock and SPILock are both built on it, and native meshtasticd runs the radio and the API on separate threads. Add a pthread implementation under ARCH_PORTDUINO. The timed lock(uint32_t) blocks rather than returning early, because there is no portable timed pthread_mutex_lock across Linux and macOS and returning true without acquiring would leave a caller such as SPILock unlocking a mutex it never took. Targets that have neither FreeRTOS nor pthreads, such as STM32WL, keep the existing empty implementation byte for byte. Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9 * test: create spiLock in the shared setup, which NodeDB needs and no test had Making Portduino's Lock real turns a latent null dereference into a crash. spiLock is a bare pointer that initSPI() fills in, and only main.cpp calls that, so in a test binary it stays null. NodeDB's constructor reaches it through loadFromDisk(), and while Lock::lock() was an empty function the call never touched `this`, so 23 suites have been calling a method on a null pointer and getting away with it. With a pthread mutex behind it the same call reads through the null pointer and takes SIGSEGV at offset 0x10, which is what test_phone_api_config_dump, test_muted_source, test_nodeinfo_send_window and test_module_config hit. Create it once in initializeTestEnvironment(), which every affected suite calls as the first statement of setup(), before any of them constructs a NodeDB. The guard is the idiom test_xmodem and test_nodedb_identity_hygiene already use; theirs stay correct and become no-ops. test_safefile called initSPI() bare right after the harness, which would now trip its assert, so that call goes away. No firmware behaviour changes: main.cpp still calls initSPI() exactly once, and nothing outside the test harness is touched. Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9 * test: create cryptLock where a suite reaches it without building a Router Second instance of the same latent null dereference the previous commit fixed for spiLock. cryptLock is a bare pointer that Router's constructor creates (Router.cpp:246); AdminModule::setPassKey takes a LockGuard on it, and a suite that exercises an admin path without standing up a Router leaves it null. While Lock::lock() was empty on Portduino the guard never touched `this`; with a pthread mutex it reads through null, which is test_tak_config's SIGSEGV in handleGetModuleConfig. It cannot go in initializeTestEnvironment() the way spiLock did, because Router asserts cryptLock is unset before allocating its own, so creating it for every suite would break the ones that do build a Router. It is a named helper instead, testEnsureCryptLock(), called by the six suites that reach a cryptLock path with no Router: test_ack_proof, test_admin_session_repro, test_fuzz_packets, test_hop_scaling, test_module_config and test_tak_config. The three that define setup() twice behind a PKI #if get the call only in the branch that compiles the tests in. No firmware behaviour changes; nothing outside test/ is touched. Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9 * test: create cryptLock in the harness instead of chasing suites Router's constructor asserted cryptLock was unset, then allocated it. That assert is why ten suites carry a mock-router destructor whose only job is to delete the global and null it so the next router can be built. While Lock::lock() was an empty function on Portduino a null cryptLock cost nothing, so those null windows were invisible; with a real mutex, anything reaching perhapsDecode() or the ack-proof paths after one of those destructors runs dereferences null. The fix is the idiom already on the next line of the same constructor, which routingAuthCacheLock has used all along: reuse the lock if one exists. Nothing in src/ ever deleted cryptLock, so a Router that finds one is finding the process's only one. initializeTestEnvironment() can then create it for every suite, the way it now does for spiLock, and the ten teardowns and the per-suite helper from the previous commit all go away. Replaces the six testEnsureCryptLock() call sites with one creation point, and removes the null windows in test_admin_radio, test_mesh_beacon, test_mesh_module, test_mqtt, test_nexthop_routing, test_nodeinfo_send_window, test_traffic_management, test_event_channel_phone_api, test_event_channel_router and test_phone_api_config_dump. Co-Authored-By: Jonathan Bennett <jbennett@incomsystems.biz> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01REkPVFh6kvG4AZJ5A8AtM9 --------- Co-authored-by: Claude <noreply@anthropic.com> |
||
|
Benjamin Faershtein
|
3aac179397 | fix(phoneapi): wake clients after config sync (#11818) | ||
|
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. |
||
|
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> |