diff --git a/bin/test-config-check.sh b/bin/test-config-check.sh index 3143a0db51..2d3d00c25c 100755 --- a/bin/test-config-check.sh +++ b/bin/test-config-check.sh @@ -188,10 +188,18 @@ assert "sensors written as a mapping are refused" 1 sensors-not-a-list.yaml chec "Sensors must be a list of entries with a Type and a Command" \ "Result: 1 error, 0 warnings" assert "bad Scale, unknown key and duplicate Type are warned" 0 sensors-warnings.yaml check \ - "Sensors Scale 'milli' is not a number" \ + "Sensors Scale 'milli' is not a finite number" \ + "Sensors Scale '.inf' is not a finite number" \ "unknown key 'Sensors.Interval'" \ "Sensors lists temperature 2 times" \ - "Result: 0 errors, 3 warnings" + "Result: 0 errors, 4 warnings" +# A non-finite Scale falls back to 1, so it is not written back out. +assert "a non-finite Scale is replaced by 1" 0 sensors-warnings.yaml yaml \ + "!Scale: .inf" \ + "!Scale: inf" +assert "an empty Sensors section is skipped" 0 sensors-empty.yaml check \ + "!Sensors must be a list" \ + "Result: 0 errors, 0 warnings" # Like admin keys, sensors append across files so one file per sensor works. --output-yaml reports # the merged list, with the Type normalised to the field name. assert "sensors append across config.d" 0 sensors-append/config.yaml yaml \ diff --git a/src/modules/Telemetry/EnvironmentTelemetry.cpp b/src/modules/Telemetry/EnvironmentTelemetry.cpp index 1b9ca10356..626ab605f5 100644 --- a/src/modules/Telemetry/EnvironmentTelemetry.cpp +++ b/src/modules/Telemetry/EnvironmentTelemetry.cpp @@ -161,6 +161,11 @@ extern void drawCommonHeader(OLEDDisplay *display, int16_t x, int16_t y, const c EnvironmentTelemetryModule *environmentTelemetryModule = nullptr; +#if ARCH_PORTDUINO +// Also in `sensors`, but read last in getEnvironmentTelemetry() so a configured command wins a shared field. +static TelemetrySensor *commandSensor = nullptr; +#endif + namespace { EnvironmentTelemetryModule::DisplaySource gDisplaySource = EnvironmentTelemetryModule::DisplaySource::Mesh; @@ -290,8 +295,14 @@ void EnvironmentTelemetryModule::i2cScanFinished(ScanI2C *i2cScanner) #if !MESHTASTIC_EXCLUDE_ENVIRONMENTAL_SENSOR #if ARCH_PORTDUINO - // Not an I2C device. Added first so it is read last, letting a configured command win a shared field. - addSensor(i2cScanner, ScanI2C::DeviceType::NONE); + // Not an I2C device. + commandSensor = new CommandSensor(); + if (commandSensor->initDevice(nullptr, nullptr)) { + sensors.push_front(commandSensor); + } else { + delete commandSensor; + commandSensor = nullptr; + } #endif #ifdef T1000X_SENSOR_EN // Not a real I2C device @@ -725,6 +736,10 @@ bool EnvironmentTelemetryModule::getEnvironmentTelemetry(meshtastic_Telemetry *m m->variant.environment_metrics = meshtastic_EnvironmentMetrics_init_zero; for (TelemetrySensor *sensor : sensors) { +#if ARCH_PORTDUINO + if (sensor == commandSensor) + continue; +#endif get_metrics = sensor->getMetrics(m); // avoid short-circuit evaluation rules valid = valid || get_metrics; hasSensor = true; @@ -766,6 +781,13 @@ bool EnvironmentTelemetryModule::getEnvironmentTelemetry(meshtastic_Telemetry *m valid = valid || get_metrics; hasSensor = true; } +#endif +#if ARCH_PORTDUINO + if (commandSensor) { + get_metrics = commandSensor->getMetrics(m); + valid = valid || get_metrics; + hasSensor = true; + } #endif return valid && hasSensor; } diff --git a/src/modules/Telemetry/Sensor/CommandSensor.cpp b/src/modules/Telemetry/Sensor/CommandSensor.cpp index 23567c1db6..90e21e5855 100644 --- a/src/modules/Telemetry/Sensor/CommandSensor.cpp +++ b/src/modules/Telemetry/Sensor/CommandSensor.cpp @@ -7,6 +7,7 @@ #include "PortduinoGlue.h" #include "UptimeClock.h" #include "mesh/Throttle.h" +#include // getMetrics() also runs for screen refreshes and phone requests; this keeps those from re-running every command. static constexpr uint32_t COMMAND_SENSOR_CACHE_MS = 30 * 1000; @@ -45,7 +46,12 @@ void CommandSensor::refresh(size_t index) LOG_WARN("Sensor %s: command output is not a number: '%s'", sensor.type.c_str(), output.c_str()); return; } - reading.value = value * sensor.scale; + const float scaled = value * sensor.scale; + if (!std::isfinite(scaled)) { + LOG_WARN("Sensor %s: %f scaled by %f is not a finite number", sensor.type.c_str(), value, sensor.scale); + return; + } + reading.value = scaled; reading.valid = true; LOG_DEBUG("Sensor %s: %f", sensor.type.c_str(), reading.value); } diff --git a/src/platform/portduino/ConfigCheck.cpp b/src/platform/portduino/ConfigCheck.cpp index 6066ca2f13..4f6ebfbd4d 100644 --- a/src/platform/portduino/ConfigCheck.cpp +++ b/src/platform/portduino/ConfigCheck.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -891,10 +892,10 @@ void checkCommandSensors(const std::string &file, const YAML::Node &node, std::v "Sensors entry has no Command, so meshtasticd refuses to start on this file"}); const YAML::Node scale = entry["Scale"]; - if (scale && !converts(scale, kFloat)) + if (scale && (!converts(scale, kFloat) || !std::isfinite(scale.as()))) findings.push_back( {kWarn, file, lineOf(scale), - "Sensors Scale '" + scale.as("") + "' is not a number, so it is silently replaced by 1"}); + "Sensors Scale '" + scale.as("") + "' is not a finite number, so it is silently replaced by 1"}); } } diff --git a/src/platform/portduino/PortduinoGlue.cpp b/src/platform/portduino/PortduinoGlue.cpp index 3c66b08f3c..eed56437d2 100644 --- a/src/platform/portduino/PortduinoGlue.cpp +++ b/src/platform/portduino/PortduinoGlue.cpp @@ -20,6 +20,7 @@ #include #include #include +#include #include #include #include @@ -1483,7 +1484,8 @@ bool loadConfig(const char *configPath) portduino_config.hostMetrics_user_command = (yamlConfig["HostMetrics"]["UserStringCommand"]).as(""); } - if (yamlConfig["Sensors"]) { + // An empty `Sensors:` is null; skip it like any other empty section. + if (yamlConfig["Sensors"] && !yamlConfig["Sensors"].IsNull()) { const YAML::Node sensors = yamlConfig["Sensors"]; if (!sensors.IsSequence()) { std::cout << "Sensors must be a list of entries with a Type and a Command!" << std::endl; @@ -1503,6 +1505,8 @@ bool loadConfig(const char *configPath) sensor.type = CommandSensorFields::canonicalType(type); sensor.command = entry["Command"].as(""); sensor.scale = entry["Scale"].as(1.0f); + if (!std::isfinite(sensor.scale)) + sensor.scale = 1.0f; if (sensor.type.empty() || sensor.command.empty()) { if (sensor.type.empty()) std::cout << "Sensors: '" << type diff --git a/test/fixtures/portduino-config/README.md b/test/fixtures/portduino-config/README.md index 333629366c..c79d31d478 100644 --- a/test/fixtures/portduino-config/README.md +++ b/test/fixtures/portduino-config/README.md @@ -190,12 +190,13 @@ Each entry runs a shell command and puts the number it prints into the named `EnvironmentMetrics` field. An entry meshtasticd cannot place stops it, rather than booting a node that silently never reports that reading. -| File | Expected | -| ------------------------- | ------------------------------------------------------------------------------------------- | -| `sensors.yaml` | Two sensors, one Type in mixed case. Clean apart from the info line about enabling them. | -| `sensors-bad-type.yaml` | `humidity` is not a field name, so the file is refused and the valid types are listed. | -| `sensors-not-a-list.yaml` | One sensor written as a mapping, not a one-item list: nothing would be read, so refused. | -| `sensors-warnings.yaml` | Non-numeric `Scale` (read as 1), an unknown key, and one Type listed twice. Three warnings. | +| File | Expected | +| ------------------------- | ------------------------------------------------------------------------------------------------------ | +| `sensors.yaml` | Two sensors, one Type in mixed case. Clean apart from the info line about enabling them. | +| `sensors-bad-type.yaml` | `humidity` is not a field name, so the file is refused and the valid types are listed. | +| `sensors-not-a-list.yaml` | One sensor written as a mapping, not a one-item list: nothing would be read, so refused. | +| `sensors-warnings.yaml` | Non-numeric and `.inf` `Scale` (both read as 1), an unknown key, one Type listed twice. Four warnings. | +| `sensors-empty.yaml` | A bare `Sensors:` with its entries commented out is skipped like any empty section. | `sensors-append/` mirrors `adminkeys-append/`: sensors append across files, so one file per sensor in `config.d/` works, and `--output-yaml` asserts both by value. diff --git a/test/fixtures/portduino-config/sensors-empty.yaml b/test/fixtures/portduino-config/sensors-empty.yaml new file mode 100644 index 0000000000..8e1583700e --- /dev/null +++ b/test/fixtures/portduino-config/sensors-empty.yaml @@ -0,0 +1,11 @@ +# A bare `Sensors:` with every entry commented out is null, like an empty section. It must be +# skipped: a normal boot used to reject it as "not a list" while --check called it clean. +Lora: + Module: sx1262 + CS: 21 + IRQ: 16 + Busy: 20 + Reset: 18 +Sensors: +# - Type: temperature +# Command: cat /sys/class/thermal/thermal_zone0/temp diff --git a/test/fixtures/portduino-config/sensors-warnings.yaml b/test/fixtures/portduino-config/sensors-warnings.yaml index f3e147a9e7..8082c70cdf 100644 --- a/test/fixtures/portduino-config/sensors-warnings.yaml +++ b/test/fixtures/portduino-config/sensors-warnings.yaml @@ -1,6 +1,6 @@ -# FAULT: three mistakes that still start meshtasticd. Scale is not a number (silently read as 1), an -# unknown key (ignored), and temperature listed twice (both run, one value is sent). Expect three -# WARNINGs and exit 0. +# FAULT: four mistakes that still start meshtasticd. Two Scales that are not finite numbers (both +# silently read as 1), an unknown key (ignored), and temperature listed twice (both run, one value is +# sent). Expect four WARNINGs and exit 0. Lora: Module: sx1262 CS: 21 @@ -14,3 +14,6 @@ Sensors: - Type: temperature Command: /usr/local/bin/outdoor-temp Interval: 60 + - Type: lux + Command: cat /sys/bus/iio/devices/iio:device1/in_illuminance_input + Scale: .inf diff --git a/test/test_command_sensor/test_main.cpp b/test/test_command_sensor/test_main.cpp index 2e57193f88..f92ed8d492 100644 --- a/test/test_command_sensor/test_main.cpp +++ b/test/test_command_sensor/test_main.cpp @@ -26,7 +26,8 @@ // // Regressions guarded: an unbounded wait on a hung or stdout-closing command; a timeout that kills // only /bin/sh and leaves the pipeline running; trailing text accepted as a number; NaN/inf sent; -// integer fields wrapping; the cache either missing or never expiring; Scale not applied. +// integer fields wrapping; a scaled value overflowing to inf and being sent; the cache either missing or +// never expiring; Scale not applied. #include "TestUtil.h" #include "configuration.h" #include @@ -250,6 +251,19 @@ void test_CommandSensor_applies_scale_and_skips_bad_output() TEST_ASSERT_FALSE(t.variant.environment_metrics.has_lux); } +void test_CommandSensor_rejects_scale_overflow() +{ + // Both factors are finite but the product is not; it must not be sent, nor reach an integer cast. + portduino_config.command_sensors.push_back({"temperature", "echo 3e38", 10.0f}); + portduino_config.command_sensors.push_back({"soil_moisture", "echo 3e38", 10.0f}); + CommandSensor sensor; + TEST_ASSERT_TRUE(sensor.initDevice(nullptr, nullptr)); + meshtastic_Telemetry t = emptyEnvironment(); + TEST_ASSERT_FALSE(sensor.getMetrics(&t)); + TEST_ASSERT_FALSE(t.variant.environment_metrics.has_temperature); + TEST_ASSERT_FALSE(t.variant.environment_metrics.has_soil_moisture); +} + void test_CommandSensor_all_failing_reports_no_metrics() { portduino_config.command_sensors.push_back({"temperature", "echo oops", 1.0f}); @@ -305,6 +319,7 @@ void setup() RUN_TEST(test_apply_unknown_type_changes_nothing); RUN_TEST(test_CommandSensor_not_added_without_config); RUN_TEST(test_CommandSensor_applies_scale_and_skips_bad_output); + RUN_TEST(test_CommandSensor_rejects_scale_overflow); RUN_TEST(test_CommandSensor_all_failing_reports_no_metrics); RUN_TEST(test_CommandSensor_caches_readings_then_refreshes); const int failures = UNITY_END();