* 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>
#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.
* fixes#11466
* Keep locally-addressed routing feedback out of the phone echo filter
allocForSending stamps ACK/NAK packets with from == our nodenum and sendLocal
defaults to RX_SRC_RADIO, so the loopback gate never applies. Filtering on
isFromUs alone dropped implicit rebroadcast ACKs, duty-cycle and NO_INTERFACE
NAKs, and PhoneAPI rate-limit errors on their way to the client.
Add coverage through the real RoutingModule, which the mocked one used by the
rest of the suite cannot exercise, and correct the test seam comment.
* Clean up the temporary RoutingModule in tearDown()
A failed Unity assertion longjmps out of the test, so the in-test delete never
ran and the module stayed registered in MeshModule::modules for every later
test. Track it at file scope, as realNeighborInfoModule already is.
* Deliver locally-generated replies addressed to us to the phone
Config get/set from the phone times out on every device: the client sends an
admin request, the node handles it, and the response is silently dropped
before it reaches the phone queue.
#10967 changed Router::sendLocal's isToUs branch from enqueueReceivedMessage()
to handleReceived(p, src), so a local packet keeps its RxSource instead of
being relabeled RX_SRC_RADIO by the queue round-trip. That is the right call
for the new policy gates, but module replies go out through
MeshService::sendToMesh() with the default RX_SRC_LOCAL, and a reply to a
phone-originated request is addressed to our own node (setReplyTo resolves
from == 0 to ourNodeNum). Those replies now re-enter callModules as
RX_SRC_LOCAL, where the loopback gate skips every module whose loopbackOk is
false - including RoutingModule, whose promiscuous sniff is the only path that
moves a received packet into toPhoneQueue. The reply is released, never sent.
Requests still work, because the phone's own packets arrive as RX_SRC_USER and
pass the gate, so a set_config is applied and only its acknowledgement is lost.
That is why a client can connect and download config but times out on every
config screen and every setter.
Deliver the phone's copy from sendToMesh() instead: for a local packet
addressed to us, the loopback gate is doing its job in keeping the packet away
from module re-dispatch, and the phone copy is exactly what is missing. Setting
loopbackOk on RoutingModule would instead echo every locally-generated
broadcast back to the phone, and relabeling replies RX_SRC_RADIO would undo the
origin separation #10967 added.
Also stop reporting ERRNO_SHOULD_RELEASE (35) to the phone in the QueueStatus
for these packets. It means "caller frees", not a send failure, and the same
hunk changed it from the 0 the phone used to see.
* Address review: trim comments, assert the QueueStatus count
Condense the added comments to the one-or-two-line house style; the rationale
lives in the commit message and PR.
The reply test drained QueueStatus records in a while loop, which would have
passed just as happily on an empty queue. Count them and require both the
request's and the reply's.
* Align telemetry broadcast want_response behavior with traceroute
* Fixes
* Reduce side-effects by making the telemetry modules handle the ignorerequest
* Remove unnecessary ignoreRequest flag
* Try inheriting from MeshModule
* Add exclusion for sensor/router roles and add base telem module
