From b969d3cfd62f41542009d8f389e8325d4038f302 Mon Sep 17 00:00:00 2001 From: mmmorks Date: Mon, 7 Sep 2026 14:20:35 -0700 Subject: [PATCH] Fix airtime fallback underflow, LLCC68 wrapper build, gps CLI crash, host tests Five small independent fixes. calcMaxPacketMillis()'s fallback said "4 secs" and used 4000 - preamble_us, i.e. 4 ms minus a preamble that exceeds it at every setting there is. The unsigned subtraction underflows, and the result is not a mis-sized deadline but the absence of one: a ~49-day payload watchdog leaves CustomSX1262::isReceiving() holding a latched HEADER_VALID true forever, so the channel never again reads idle. The fallback is now a flat 4 s of payload (MAX_PACKET_FALLBACK_PAYLOAD_US, overridable), with no subtraction left to underflow. It is only reached when getTimeOnAir() returns nothing usable, which it does on an unconfigured modem. CustomLLCC68Wrapper::doResetAGC() calls sx126xResetAGC() with one argument; the only overload takes (SX126x*, bool rx_boost_gain). Nothing in the tree includes this wrapper yet, which is why no build has failed, but the first board to use it will. Pass getRxBoostedGainMode() like the SX1262, SX1268 and STM32WLx wrappers do. The bare `gps` CLI command crashes when no GPS was detected: the status branch does strcmp(_sensors->getSettingByKey("gps"), "1") and getSettingByKey() returns NULL when no "gps" setting is registered, which is the case whenever gps_detected was false at boot. Treat a missing setting as "deactivated". recvRaw()'s readData() error line now also prints the packet length, RSSI and SNR (quarter-dB, the same convention as Packet::_snr). The bare error code says nothing about the cause -- a packet at the edge of the demodulator and one lost to a collision both produce -7 -- and both existing packet loggers sit inside the success branch, so a failed receive is otherwise invisible to `log start` and MESH_PACKET_LOGGING alike. The modem's packet-status registers are written whether or not the CRC passed, so this costs no extra SPI traffic. Measured on a live repeater at SF7/62.5 kHz (490 failures against 961 successes over 25 min), the failure rate resolves into a clean waterfall against SNR, with 20% of failures at positive SNR where marginality cannot be the explanation. ConfigSerializer.cpp calls atoi/atol/atof and reaches them through Arduino.h on MCU targets, but not in the host `native` googletest build: on macOS the whole suite fails to compile at ConfigSerializer.o, taking every unit test down with it. Include . Likewise LocationProvider::sendSentence() was declared virtual but never defined; every current subclass overrides it so MCU links get away with it, but a translation unit that emits the base vtable gets an undefined reference. Give it an empty default body, matching the no-op override in EnvironmentSensorManager.cpp. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01EXSCjgNEbJfHwLjD2WSHW4 --- src/helpers/CommonCLI.cpp | 6 +++- src/helpers/ConfigSerializer.cpp | 3 ++ src/helpers/radiolib/CustomLLCC68Wrapper.h | 2 +- src/helpers/radiolib/RadioLibWrappers.cpp | 41 ++++++++++++++++++++-- src/helpers/sensors/LocationProvider.h | 2 +- 5 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 4930e81e9a..e73af5b77a 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -396,7 +396,11 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re bool enabled = l->isEnabled(); // is EN pin on ? bool fix = l->isValid(); // has fix ? int sats = l->satellitesCount(); - bool active = !strcmp(_sensors->getSettingByKey("gps"), "1"); + // getSettingByKey() returns NULL when no "gps" setting is registered + // (no GPS detected at boot) -- treat that as "deactivated" rather than + // handing strcmp() a NULL, which crashes. + const char* gps_setting = _sensors->getSettingByKey("gps"); + bool active = gps_setting != NULL && strcmp(gps_setting, "1") == 0; if (enabled) { sprintf(reply, "on, %s, %s, %d sats", active?"active":"deactivated", diff --git a/src/helpers/ConfigSerializer.cpp b/src/helpers/ConfigSerializer.cpp index adff147f47..a0911303cd 100644 --- a/src/helpers/ConfigSerializer.cpp +++ b/src/helpers/ConfigSerializer.cpp @@ -1,5 +1,8 @@ #include "ConfigSerializer.h" +#include // atoi/atol/atof -- reaches these via Arduino.h on MCU + // targets, but not in the host `native` test build + bool ConfigSerializer::saveSerial(Stream& s) { Context context(&s, OP::WRITE); _context = &context; // set the context for structure() call diff --git a/src/helpers/radiolib/CustomLLCC68Wrapper.h b/src/helpers/radiolib/CustomLLCC68Wrapper.h index ae0fe0a253..d5f97c65fd 100644 --- a/src/helpers/radiolib/CustomLLCC68Wrapper.h +++ b/src/helpers/radiolib/CustomLLCC68Wrapper.h @@ -35,7 +35,7 @@ class CustomLLCC68Wrapper : public RadioLibWrapper { } uint8_t getSpreadingFactor() const override { return ((CustomLLCC68 *)_radio)->spreadingFactor; } - void doResetAGC() override { sx126xResetAGC((SX126x *)_radio); } + void doResetAGC() override { sx126xResetAGC((SX126x *)_radio, getRxBoostedGainMode()); } bool setRxBoostedGainMode(bool en) override { return ((CustomLLCC68 *)_radio)->setRxBoostedGainMode(en) == RADIOLIB_ERR_NONE; diff --git a/src/helpers/radiolib/RadioLibWrappers.cpp b/src/helpers/radiolib/RadioLibWrappers.cpp index e4d2ba1c27..27d0cd71c2 100644 --- a/src/helpers/radiolib/RadioLibWrappers.cpp +++ b/src/helpers/radiolib/RadioLibWrappers.cpp @@ -11,6 +11,14 @@ #define NUM_NOISE_FLOOR_SAMPLES 64 #define SAMPLING_THRESHOLD 14 +// Payload budget calcMaxPacketMillis() assumes when the modem cannot tell it +// how long a packet takes. Long on purpose: this deadline exists to break a +// stuck header IRQ, and one that expires early would clear the flags of a +// packet still arriving. +#ifndef MAX_PACKET_FALLBACK_PAYLOAD_US + #define MAX_PACKET_FALLBACK_PAYLOAD_US 4000000UL +#endif + static volatile uint8_t state = STATE_IDLE; // this function is called when a complete packet @@ -133,7 +141,21 @@ int RadioLibWrapper::recvRaw(uint8_t* bytes, int sz) { if (len > sz) { len = sz; } int err = _radio->readData(bytes, len); if (err != RADIOLIB_ERR_NONE) { - MESH_DEBUG_PRINTLN("RadioLibWrapper: error: readData(%d)", err); + // Signal quality of the packet that just failed. A CRC mismatch (-7) is + // the common case and says nothing on its own about *why*: a packet at + // the edge of the demodulator and one lost to a collision both land + // here. The modem's packet-status registers are written whether or not + // the CRC passed, so this reads the same values a successful receive + // would report, at no extra SPI cost -- enough to tell a failure + // distribution sitting on the SF's SNR floor apart from one spread + // across strong signals. + // + // SNR is scaled by 4 rather than truncated because the threshold this + // is meant to resolve is a fraction of a dB wide, and %f is not + // portable across every platform this file builds for. Same quarter-dB + // convention as Packet::_snr. + MESH_DEBUG_PRINTLN("RadioLibWrapper: error: readData(%d) len=%d rssi=%d snr4=%d", + err, len, (int)getLastRSSI(), (int)(getLastSNR() * 4)); len = 0; n_recv_errors++; } else { @@ -252,7 +274,22 @@ PacketMillis RadioLibWrapper::calcMaxPacketMillis(uint8_t sf, float bw, uint8_t // airtime for max packet at current radio settings uint32_t total_us = _radio->getTimeOnAir(MAX_TRANS_UNIT); // airtime for payload only (no preamble, header or SOF) - uint32_t payload_us = total_us > preamble_us ? total_us - preamble_us : 4000 - preamble_us; // fallback to 4 secs at worst case + uint32_t payload_us; + if (total_us > preamble_us) { + payload_us = total_us - preamble_us; + } else { + // getTimeOnAir() gave nothing usable (it returns 0 on an unconfigured + // modem). Fall back to the 4 s this has always claimed -- as 4 s of + // *payload*, not as 4 s of total airtime minus the preamble. + // + // The value used to be 4000, i.e. 4 ms, and the subtraction underflowed for + // any setting whose preamble exceeds that: every one of them. An underflow + // here is not a mis-sized deadline but the absence of one, because the + // result becomes a ~49-day payload watchdog, so CustomSX1262::isReceiving() + // would hold a latched HEADER_VALID true forever and isReceiving() would + // never again report the channel idle. + payload_us = MAX_PACKET_FALLBACK_PAYLOAD_US; + } // rescale payload_us for max possible CR if (cr >= 5 && cr < 8) { payload_us = (payload_us * 8) / cr; } diff --git a/src/helpers/sensors/LocationProvider.h b/src/helpers/sensors/LocationProvider.h index 81d08652ed..488a755115 100644 --- a/src/helpers/sensors/LocationProvider.h +++ b/src/helpers/sensors/LocationProvider.h @@ -16,7 +16,7 @@ class LocationProvider { virtual long satellitesCount() = 0; virtual bool isValid() = 0; virtual long getTimestamp() = 0; - virtual void sendSentence(const char * sentence); + virtual void sendSentence(const char * sentence) {} virtual void reset() = 0; virtual void begin() = 0; virtual void stop() = 0;