Arrival time fix perhaps (#11274)
* Add explicit presence for MeshPacket.rx_time (arrival time) rx_time is now proto3 optional with a has_rx_time presence bit, matching the rx_rssi treatment. A node with no GPS and no phone connected yet has no time source at all, so a bare 0 was indistinguishable from a genuine 1970-01-01 reading; downstream consumers (replay packets, JSON serialization) now check has_rx_time instead of the value. * Dedupe rx_time stamping into a shared helper; trim a debug log string Extract the repeated haveTime/rx_time/has_rx_time stamp logic (5 call sites across Router.cpp, MeshBeaconModule.cpp, MeshService.cpp) into Router::computeRxTimeStamp()/stampRxTime(). Also shorten the new RTC.cpp LOG_DEBUG string. Saves 48 bytes of flash on rak4631 (measured), no behavior change. * Fix has_rx_rssi presence carried unconditionally through StoreForward replay preparePayload() set has_rx_rssi = true unconditionally on replay, regardless of whether the packet's rx_rssi at store time was a genuine measurement (e.g. MQTT-relayed packets carry no real RSSI). Store the presence bit alongside rx_rssi in PacketHistoryStruct and restore it on replay instead. Flagged by Copilot on #11271 (same root cause the has_rx_time explicit presence work fixes) but never addressed before that PR merged. * Trim comment blocks to the repo's 1-2 line guideline .github/copilot-instructions.md:338 caps code comments at 1-2 lines; several blocks added across the rx_time explicit-presence work ran well past that. Also consolidates Time.cpp's file-level doc comment into Time.h, where the rest of the Time:: API contract already lives. No behavior change. * Add rx_time explicit-presence test coverage - test_meshpacket_serializer: has_rx_time=false fixture plus tests asserting JsonSerialize/JsonSerializeEncrypted emit 0 rather than leaking the millis() placeholder, alongside the has_rx_time=true baseline. - test_stream_api: two tests driving a real PhoneAPI handshake (want_config_id through STATE_SEND_PACKETS) that simulate a phone time-giving transaction arriving before vs. after a queued packet is drained - covering both the reconciled and the ships-with-placeholder-absent paths of MeshService::reconcilePendingRxTimes(). * Fix three correctness issues flagged in review - Time.h: drop the reserved-identifier include guard (_MT_TIME_H); pragma once already covers it, matching convention elsewhere (e.g. RTC.h). - Time.cpp: rebase getMillis64()'s wrap accumulator when the test seam swaps clock sources, so a real<->injected clock jump isn't miscounted as a genuine 32-bit wrap. - NodeInfoModule: the 12h reply-suppression window is a local dedup duration, not a wall-clock reading - switch it to Time::getMillis64() so RTC-quality jumps and replayed packets' stale rx_time can't perturb it. - StoreForwardModule: has_rx_time was derived from *current* RTC quality at replay time rather than stored at capture time, so a history entry saved while time-blind could be misreported as a valid epoch once the clock later improved. Persist the presence bit in PacketHistoryStruct instead. * tryfix CI * post review fixes * more test fixes
This commit is contained in:
@@ -1,7 +1,9 @@
|
||||
#include "MeshTypes.h"
|
||||
#include "SerialConsole.h"
|
||||
#include "TestUtil.h"
|
||||
#include "UptimeClock.h"
|
||||
#include "configuration.h"
|
||||
#include "gps/RTC.h"
|
||||
#include "mesh-pb-constants.h"
|
||||
#include "mesh/MeshService.h"
|
||||
#include "mesh/NodeDB.h"
|
||||
@@ -10,6 +12,7 @@
|
||||
#include <algorithm>
|
||||
#include <cstdarg>
|
||||
#include <cstdint>
|
||||
#include <ctime>
|
||||
#include <deque>
|
||||
#include <limits>
|
||||
#include <unity.h>
|
||||
@@ -522,6 +525,131 @@ static void test_want_config_includes_status_message_module_config(void)
|
||||
TEST_ASSERT_TRUE(foundStatusMessageConfig);
|
||||
}
|
||||
|
||||
/// Queue a packet as Router::dispatchReceived would have, before any time source existed.
|
||||
static void queuePendingTimePlaceholderPacket(NodeNum from, uint32_t placeholderMillis)
|
||||
{
|
||||
meshtastic_MeshPacket pending = meshtastic_MeshPacket_init_zero;
|
||||
pending.which_payload_variant = meshtastic_MeshPacket_decoded_tag;
|
||||
pending.decoded.portnum = meshtastic_PortNum_TEXT_MESSAGE_APP;
|
||||
pending.from = from;
|
||||
pending.to = NODENUM_BROADCAST;
|
||||
pending.rx_time = placeholderMillis;
|
||||
pending.has_rx_time = false;
|
||||
service->sendToPhone(packetPool.allocCopy(pending));
|
||||
}
|
||||
|
||||
static void startHandshake(PhoneAPITestShim &api)
|
||||
{
|
||||
meshtastic_ToRadio request = meshtastic_ToRadio_init_zero;
|
||||
request.which_payload_variant = meshtastic_ToRadio_want_config_id_tag;
|
||||
request.want_config_id = SPECIAL_NONCE_ONLY_CONFIG;
|
||||
uint8_t requestBytes[meshtastic_ToRadio_size];
|
||||
const size_t requestSize = pb_encode_to_bytes(requestBytes, sizeof(requestBytes), &meshtastic_ToRadio_msg, &request);
|
||||
api.handleToRadio(requestBytes, requestSize);
|
||||
}
|
||||
|
||||
/// Drain the config stream looking for the first packet from `from`; false if never delivered.
|
||||
static bool drainHandshakeForPacketFrom(PhoneAPITestShim &api, NodeNum from, meshtastic_MeshPacket &outPacket)
|
||||
{
|
||||
for (unsigned i = 0; i < 256; ++i) {
|
||||
uint8_t responseBytes[meshtastic_FromRadio_size];
|
||||
const size_t responseSize = api.getFromRadio(responseBytes);
|
||||
if (responseSize == 0)
|
||||
return false;
|
||||
meshtastic_FromRadio response = meshtastic_FromRadio_init_zero;
|
||||
TEST_ASSERT_TRUE(pb_decode_from_bytes(responseBytes, responseSize, &meshtastic_FromRadio_msg, &response));
|
||||
if (response.which_payload_variant == meshtastic_FromRadio_packet_tag && response.packet.from == from) {
|
||||
outPacket = response.packet;
|
||||
return true;
|
||||
}
|
||||
}
|
||||
return false;
|
||||
}
|
||||
|
||||
/// Swaps in a scratch NodeDB and the injected clock, restoring both plus the RTC on destruction.
|
||||
/// Unity's TEST_ASSERT longjmps out on failure, so cleanup must not live at the end of the test.
|
||||
class ScopedTimeFixture
|
||||
{
|
||||
public:
|
||||
ScopedTimeFixture(uint32_t startMillis) : previous(nodeDB)
|
||||
{
|
||||
resetRTCStateForTests();
|
||||
nodeDB = &instance;
|
||||
Time::setTestMillis(startMillis);
|
||||
}
|
||||
~ScopedTimeFixture()
|
||||
{
|
||||
nodeDB = previous;
|
||||
Time::useRealClock();
|
||||
resetRTCStateForTests();
|
||||
}
|
||||
|
||||
private:
|
||||
NodeDB instance;
|
||||
NodeDB *previous;
|
||||
};
|
||||
|
||||
// Time given at the start of the handshake, before the queued packet is drained: reconciliation
|
||||
// (fired by the RTC quality crossing hook in RTC.cpp) rewrites the placeholder in place.
|
||||
static void test_time_given_at_handshake_start_reconciles_queued_packet(void)
|
||||
{
|
||||
ScopedMeshService scopedService;
|
||||
ScopedTimeFixture timeFixture(5000);
|
||||
|
||||
const NodeNum sender = 0x12345678;
|
||||
queuePendingTimePlaceholderPacket(sender, 2000); // "received" 3s before the test's current millis()
|
||||
|
||||
PhoneAPITestShim api;
|
||||
startHandshake(api);
|
||||
|
||||
struct timeval networkTime;
|
||||
networkTime.tv_sec = time(NULL) + SEC_PER_DAY;
|
||||
networkTime.tv_usec = 0;
|
||||
TEST_ASSERT_EQUAL_INT(RTCSetResultSuccess, perhapsSetRTC(RTCQualityFromNet, &networkTime));
|
||||
|
||||
meshtastic_MeshPacket delivered;
|
||||
TEST_ASSERT_TRUE_MESSAGE(drainHandshakeForPacketFrom(api, sender, delivered),
|
||||
"queued packet was not delivered during the handshake");
|
||||
TEST_ASSERT_TRUE(delivered.has_rx_time);
|
||||
TEST_ASSERT_UINT32_WITHIN(2, (uint32_t)networkTime.tv_sec - 3, delivered.rx_time);
|
||||
|
||||
api.close();
|
||||
}
|
||||
|
||||
// Time given at the end - after the queued packet already left via the handshake: the delivered
|
||||
// copy keeps its unresolved placeholder, since reconciliation can only rewrite what's still queued.
|
||||
static void test_time_given_at_handshake_end_does_not_rewrite_already_sent_packet(void)
|
||||
{
|
||||
ScopedMeshService scopedService;
|
||||
ScopedTimeFixture timeFixture(5000);
|
||||
|
||||
const NodeNum sender = 0x12345678;
|
||||
queuePendingTimePlaceholderPacket(sender, 2000);
|
||||
|
||||
PhoneAPITestShim api;
|
||||
startHandshake(api);
|
||||
|
||||
// rx_time is proto3 optional, so has_rx_time false omits it from the wire entirely: the
|
||||
// decoded copy reads back 0 and the placeholder itself never left the device.
|
||||
meshtastic_MeshPacket delivered;
|
||||
TEST_ASSERT_TRUE_MESSAGE(drainHandshakeForPacketFrom(api, sender, delivered),
|
||||
"queued packet was not delivered during the handshake");
|
||||
TEST_ASSERT_FALSE(delivered.has_rx_time);
|
||||
TEST_ASSERT_EQUAL_UINT32(0u, delivered.rx_time);
|
||||
|
||||
// Time-giving transaction happens only now, at the end of the handshake.
|
||||
struct timeval networkTime;
|
||||
networkTime.tv_sec = time(NULL) + SEC_PER_DAY;
|
||||
networkTime.tv_usec = 0;
|
||||
TEST_ASSERT_EQUAL_INT(RTCSetResultSuccess, perhapsSetRTC(RTCQualityFromNet, &networkTime));
|
||||
|
||||
// The already-delivered copy is a value, not a queue reference - untouched either way.
|
||||
TEST_ASSERT_FALSE(delivered.has_rx_time);
|
||||
TEST_ASSERT_EQUAL_UINT32(0u, delivered.rx_time);
|
||||
|
||||
api.close();
|
||||
}
|
||||
|
||||
/// Unity per-test setup; fixtures are local to each test.
|
||||
void setUp(void) {}
|
||||
/// Unity per-test teardown; fixtures clean themselves up.
|
||||
@@ -544,6 +672,8 @@ void setup()
|
||||
RUN_TEST(test_lockdown_admin_gate_ignores_wire_from);
|
||||
RUN_TEST(test_lockdown_admin_gate_rejects_undecodable_admin);
|
||||
RUN_TEST(test_want_config_includes_status_message_module_config);
|
||||
RUN_TEST(test_time_given_at_handshake_start_reconciles_queued_packet);
|
||||
RUN_TEST(test_time_given_at_handshake_end_does_not_rewrite_already_sent_packet);
|
||||
// usingProtobufs intentionally has no reset path, so this must run last.
|
||||
RUN_TEST(test_serial_console_suppresses_raw_output_in_protobuf_mode);
|
||||
exit(UNITY_END());
|
||||
|
||||
Reference in New Issue
Block a user