From ee0b2efa368c4736f0c835f34547fa09ea19dd64 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Wed, 16 Sep 2026 13:34:05 -0700 Subject: [PATCH] Make GPS status reporting say what was actually checked Two GPS diagnostics assert more than the code verified, and a third problem surfaced in the same expression while fixing them. The serial detection branch logged "Serial GPS init correctly and is turned on" whenever Serial1.available() was non-zero. Nothing there parses the bytes, checks them for NMEA framing, or confirms a fix, so the message claimed an init that never happened. Report the byte count and the IO pin instead, and say the data is unparsed. The count is captured by the condition itself so the number logged is the number that was tested. The gps CLI status led with "on", taken from isEnabled(), which reports the GPS enable pin. A provider whose board has no usable enable pin returns a constant true, so the field carried no information and contradicted the next one: the real reply was "on, deactivated, no fix, 0 sats". Drop the leading field so the reply starts with the state that is actually known, and name the reason in the off case. While there, the same statement dereferenced NULL. SensorManager::getSettingByKey() returns NULL when the key is absent, and the "gps" setting is only published once a receiver is detected, since getNumSettings() returns 0 while gps_detected is false. The guard above does not prevent that: on a GPS-enabled build _location is assigned in the sensor manager's constructor, so getLocationProvider() is never NULL whether or not hardware was found. Running "gps" on such a build with no receiver attached therefore reached strcmp(NULL, "1"). Test the setting for NULL and use its absence as the real "no receiver detected" signal, which this block previously had no way to express. docs/cli_commands.md documented the old reply format and is updated. Both it and the code comment describe "no usable enable pin" as covering PIN_GPS_EN defined as -1 as well as left undefined, since the GPS_EN fallback resolves both to -1. Detection, enable-pin handling and the sensor settings are otherwise untouched; only what gets reported changes. Verified on RAK4631: 'gps' reports "active, no fix, 0 sats", 'gps off' then 'gps' reports "deactivated, no fix, 0 sats". Build-checked on RAK_4631_repeater (nRF52840) and Tbeam_SX1262_repeater (ESP32, GPS enabled). --- docs/cli_commands.md | 11 +++++- src/helpers/CommonCLI.cpp | 38 ++++++++++++++----- .../sensors/EnvironmentSensorManager.cpp | 10 ++++- 3 files changed, 45 insertions(+), 14 deletions(-) diff --git a/docs/cli_commands.md b/docs/cli_commands.md index f4a43fad3c..51a2442ce0 100644 --- a/docs/cli_commands.md +++ b/docs/cli_commands.md @@ -1010,8 +1010,15 @@ region save **Default:** `off` **Note:** Output format: -- `off` when the GPS hardware is disabled -- `on, {active|deactivated}, {fix|no fix}, {sat count} sats` when the GPS hardware is enabled +- `not detected` when no GPS receiver was found +- `off (enable pin low)` when the GPS enable pin is present and low +- `{active|deactivated}, {fix|no fix}, {sat count} sats` otherwise + +`active` reflects the `gps` setting, i.e. whether the receiver is being polled. +A board with no GPS enable pin cannot report the power state, so the reply begins with +`active`/`deactivated` rather than claiming the hardware is on. That covers boards +which leave `PIN_GPS_EN` undefined as well as those defining it as `-1`; both end up +with no usable enable pin. --- diff --git a/src/helpers/CommonCLI.cpp b/src/helpers/CommonCLI.cpp index 4930e81e9a..a01b0c9f6b 100644 --- a/src/helpers/CommonCLI.cpp +++ b/src/helpers/CommonCLI.cpp @@ -393,17 +393,35 @@ void CommonCLI::handleCommand(uint32_t sender_timestamp, char* command, char* re } else if (memcmp(command, "gps", 3) == 0) { LocationProvider * l = _sensors->getLocationProvider(); if (l != NULL) { - 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"); - if (enabled) { - sprintf(reply, "on, %s, %s, %d sats", - active?"active":"deactivated", - fix?"fix":"no fix", - sats); + // isEnabled() reports the GPS enable pin. A provider whose board has + // no usable enable pin returns a constant true, so it is not a + // statement that the receiver is running: `active` is. Leading with it + // produced replies like "on, deactivated, no fix, 0 sats". + // + // "No usable enable pin" covers both spellings: PIN_GPS_EN defined as + // -1, and PIN_GPS_EN left undefined, which MicroNMEALocationProvider's + // GPS_EN fallback also resolves to -1. + // A GPS-enabled build always has a provider, since _location is set in + // the sensor manager's constructor, so l != NULL does not mean a + // receiver was found. The "gps" setting is only published once one is + // detected, so its absence is what tells us, and getSettingByKey() + // returns NULL in that case. + const char* gps_setting = _sensors->getSettingByKey("gps"); + if (gps_setting == NULL) { + strcpy(reply, "not detected"); } else { - strcpy(reply, "off"); + bool enabled = l->isEnabled(); // is EN pin on ? + bool fix = l->isValid(); // has fix ? + int sats = l->satellitesCount(); + bool active = strcmp(gps_setting, "1") == 0; + if (enabled) { + sprintf(reply, "%s, %s, %d sats", + active?"active":"deactivated", + fix?"fix":"no fix", + sats); + } else { + strcpy(reply, "off (enable pin low)"); + } } } else { strcpy(reply, "Can't find GPS"); diff --git a/src/helpers/sensors/EnvironmentSensorManager.cpp b/src/helpers/sensors/EnvironmentSensorManager.cpp index 6f4607751c..df5c23c69d 100644 --- a/src/helpers/sensors/EnvironmentSensorManager.cpp +++ b/src/helpers/sensors/EnvironmentSensorManager.cpp @@ -823,6 +823,7 @@ void EnvironmentSensorManager::rakGPSInit(){ } bool EnvironmentSensorManager::gpsIsAwake(uint8_t ioPin){ + int serial_avail = 0; #if defined(ETHERNET_ENABLED) && defined(RAK_BOARD) if (ioPin == WB_IO2) { @@ -862,8 +863,13 @@ bool EnvironmentSensorManager::gpsIsAwake(uint8_t ioPin){ _location = &RAK12500_provider; return true; - } else if (Serial1.available()) { - MESH_DEBUG_PRINTLN("Serial GPS init correctly and is turned on"); + } else if ((serial_avail = Serial1.available()) > 0) { + // This only observes that bytes arrived on Serial1. Nothing here parses + // them, checks them for NMEA, or confirms a fix, so say that rather than + // reporting a successful init. The count is captured by the condition + // itself, so the number logged is the number that was tested. + MESH_DEBUG_PRINTLN("Serial GPS: %d byte(s) on Serial1 with IO pin %i (not yet parsed)", + serial_avail, ioPin); #ifdef PIN_GPS_EN if(PIN_GPS_EN){ gpsResetPin = PIN_GPS_EN;