From 773c3a69a2d6be1fddddbbf509627acf272c3107 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 21:23:53 +0000 Subject: [PATCH] portduino Sensors: run commands on a worker thread CommandSensor ran its commands from getMetrics(), on the main loop that also drives the radio. Each expired sensor could stall the loop for up to its 2 s timeout, and on Windows, where popen() has no timeout, a command that never exits froze the node. - A worker thread re-runs each command every 30 s and stores the result. getMetrics() only reads the latest results under a mutex, so it never waits on a command. - The worker never calls LOG_* (portduino's log buffer is not thread-safe). It records why a read failed, and getMetrics() logs each new result on the main thread, as NativeDiscovery does. - execCommand() returns an ExecResult instead of logging. exec() logs timeouts and start failures for its main-thread callers (HostMetrics). - The sensor copies its config at init and joins the worker on destruction. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011C7D9dE4aUYBrGdpPyHs25 --- .../Telemetry/Sensor/CommandSensor.cpp | 105 +++++++++++++---- src/modules/Telemetry/Sensor/CommandSensor.h | 27 ++++- src/platform/portduino/PortduinoGlue.cpp | 28 ++--- src/platform/portduino/PortduinoGlue.h | 7 +- test/test_command_sensor/test_main.cpp | 109 ++++++++++++++---- 5 files changed, 210 insertions(+), 66 deletions(-) diff --git a/src/modules/Telemetry/Sensor/CommandSensor.cpp b/src/modules/Telemetry/Sensor/CommandSensor.cpp index 90e21e5855..30190d5b6c 100644 --- a/src/modules/Telemetry/Sensor/CommandSensor.cpp +++ b/src/modules/Telemetry/Sensor/CommandSensor.cpp @@ -4,69 +4,130 @@ #include "CommandSensor.h" #include "CommandSensorFields.h" -#include "PortduinoGlue.h" #include "UptimeClock.h" #include "mesh/Throttle.h" +#include #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; +// How often each command is re-run. +static constexpr uint32_t COMMAND_SENSOR_REFRESH_MS = 30 * 1000; +// How often the worker wakes to check for due commands and for shutdown. +static constexpr uint32_t COMMAND_SENSOR_POLL_MS = 100; +static constexpr size_t COMMAND_SENSOR_MAX_QUOTED_OUTPUT = 64; CommandSensor::CommandSensor() : TelemetrySensor(meshtastic_TelemetrySensorType_SENSOR_UNSET, "Command") {} +CommandSensor::~CommandSensor() +{ + stopping = true; + if (worker.joinable()) + worker.join(); +} + bool CommandSensor::initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) { if (portduino_config.command_sensors.empty()) return false; - readings.assign(portduino_config.command_sensors.size(), Reading()); - LOG_INFO("Init sensor: %s, %u from config.yaml Sensors", sensorName, - static_cast(portduino_config.command_sensors.size())); + configs = portduino_config.command_sensors; + readings.assign(configs.size(), Reading()); + loggedGenerations.assign(configs.size(), 0); + LOG_INFO("Init sensor: %s, %u from config.yaml Sensors", sensorName, static_cast(configs.size())); status = 1; initialized = true; + worker = std::thread(&CommandSensor::run, this); return true; } -void CommandSensor::refresh(size_t index) +// Worker thread. Must not call LOG_*: portduino leaves RedirectablePrint's buffer unlocked, so results +// and problems are handed to getMetrics() to log on the main thread. +void CommandSensor::run() { - const commandSensorConfig &sensor = portduino_config.command_sensors[index]; - Reading &reading = readings[index]; + while (!stopping) { + for (size_t i = 0; i < configs.size() && !stopping; i++) { + uint32_t readAtMs; + { + std::lock_guard lock(mu); + readAtMs = readings[i].readAtMs; + } + if (readAtMs != 0 && !Throttle::hasElapsed(readAtMs, COMMAND_SENSOR_REFRESH_MS)) + continue; + + Reading fresh = read(configs[i]); + std::lock_guard lock(mu); + fresh.generation = readings[i].generation + 1; + readings[i] = fresh; + } + std::this_thread::sleep_for(std::chrono::milliseconds(COMMAND_SENSOR_POLL_MS)); + } +} + +CommandSensor::Reading CommandSensor::read(const commandSensorConfig &sensor) +{ + Reading reading; reading.readAtMs = Time::stampMillis(); - reading.valid = false; std::string output; - if (!execCommand(sensor.command.c_str(), COMMAND_SENSOR_TIMEOUT_MS, output)) { - LOG_WARN("Sensor %s: command failed or timed out: %s", sensor.type.c_str(), sensor.command.c_str()); - return; + switch (execCommand(sensor.command.c_str(), COMMAND_SENSOR_TIMEOUT_MS, output)) { + case ExecResult::Ok: + break; + case ExecResult::TimedOut: + reading.problem = "timed out after " + std::to_string(COMMAND_SENSOR_TIMEOUT_MS) + "ms and was killed"; + return reading; + case ExecResult::NotStarted: + reading.problem = "could not be started"; + return reading; + default: + reading.problem = "exited with an error"; + return reading; } + float value; if (!CommandSensorFields::parseValue(output, value)) { while (!output.empty() && (output.back() == '\n' || output.back() == '\r')) output.pop_back(); - LOG_WARN("Sensor %s: command output is not a number: '%s'", sensor.type.c_str(), output.c_str()); - return; + if (output.size() > COMMAND_SENSOR_MAX_QUOTED_OUTPUT) + output = output.substr(0, COMMAND_SENSOR_MAX_QUOTED_OUTPUT) + "..."; + reading.problem = "printed '" + output + "', which is not a number"; + return reading; } 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.problem = "printed a value that is not a finite number once scaled"; + return reading; } reading.value = scaled; reading.valid = true; - LOG_DEBUG("Sensor %s: %f", sensor.type.c_str(), reading.value); + return reading; } bool CommandSensor::getMetrics(meshtastic_Telemetry *measurement) { bool any = false; + std::lock_guard lock(mu); for (size_t i = 0; i < readings.size(); i++) { - if (readings[i].readAtMs == 0 || Throttle::hasElapsed(readings[i].readAtMs, COMMAND_SENSOR_CACHE_MS)) - refresh(i); - if (readings[i].valid && CommandSensorFields::apply(measurement->variant.environment_metrics, - portduino_config.command_sensors[i].type, readings[i].value)) + const Reading &reading = readings[i]; + const commandSensorConfig &sensor = configs[i]; + if (reading.generation != loggedGenerations[i]) { + loggedGenerations[i] = reading.generation; + if (reading.valid) + LOG_DEBUG("Sensor %s: %f", sensor.type.c_str(), reading.value); + else + LOG_WARN("Sensor %s: command %s: %s", sensor.type.c_str(), reading.problem.c_str(), sensor.command.c_str()); + } + if (reading.valid && CommandSensorFields::apply(measurement->variant.environment_metrics, sensor.type, reading.value)) any = true; } return any; } +uint32_t CommandSensor::finishedReads() +{ + std::lock_guard lock(mu); + uint32_t total = 0; + for (const auto &reading : readings) + total += reading.generation; + return total; +} + #endif diff --git a/src/modules/Telemetry/Sensor/CommandSensor.h b/src/modules/Telemetry/Sensor/CommandSensor.h index 3380023681..036b431b2e 100644 --- a/src/modules/Telemetry/Sensor/CommandSensor.h +++ b/src/modules/Telemetry/Sensor/CommandSensor.h @@ -5,27 +5,46 @@ #if ARCH_PORTDUINO && !MESHTASTIC_EXCLUDE_ENVIRONMENTAL_SENSOR #include "../mesh/generated/meshtastic/telemetry.pb.h" +#include "PortduinoGlue.h" #include "TelemetrySensor.h" +#include +#include +#include +#include #include // Fills EnvironmentMetrics fields from the shell commands listed under Sensors: in config.yaml. +// Commands run on a worker thread, so a slow or hung one never stalls the main loop; getMetrics() only +// reads the latest results. class CommandSensor : public TelemetrySensor { public: CommandSensor(); + ~CommandSensor() override; virtual bool getMetrics(meshtastic_Telemetry *measurement) override; virtual bool initDevice(TwoWire *bus, ScanI2C::FoundDevice *dev) override; + // Commands the worker has finished running, successful or not. For tests and diagnostics. + uint32_t finishedReads(); + private: struct Reading { bool valid = false; float value = 0; - uint32_t readAtMs = 0; // 0 = never read + uint32_t readAtMs = 0; // 0 = never read + uint32_t generation = 0; // bumped on every finished read + std::string problem; // why the last read gave no value; empty when valid }; - // Parallel to portduino_config.command_sensors. - std::vector readings; - void refresh(size_t index); + std::vector configs; // copied at init; read-only after + std::mutex mu; + std::vector readings; // guarded by mu + std::vector loggedGenerations; // main thread only + std::atomic stopping{false}; + std::thread worker; + + void run(); + Reading read(const commandSensorConfig &sensor); }; #endif diff --git a/src/platform/portduino/PortduinoGlue.cpp b/src/platform/portduino/PortduinoGlue.cpp index eed56437d2..eb4501e96f 100644 --- a/src/platform/portduino/PortduinoGlue.cpp +++ b/src/platform/portduino/PortduinoGlue.cpp @@ -1723,30 +1723,28 @@ bool MAC_from_string(std::string mac_str, uint8_t *dmac) // Captured output beyond this is drained and dropped; nothing that reads it wants more. static constexpr size_t EXEC_MAX_OUTPUT = 4096; -bool execCommand(const char *cmd, uint32_t timeoutMs, std::string &output) +ExecResult execCommand(const char *cmd, uint32_t timeoutMs, std::string &output) { output.clear(); #if defined(ARCH_PORTDUINO_WASM) (void)cmd; // no shell/popen in the browser - shell-outs degrade to empty (void)timeoutMs; - return false; + return ExecResult::NotStarted; #elif defined(_WIN32) // No process group to kill here, so the timeout is not enforced on Windows. (void)timeoutMs; FILE *pipe = popen(cmd, "r"); if (!pipe) - return false; + return ExecResult::NotStarted; std::array buffer; while (fgets(buffer.data(), static_cast(buffer.size()), pipe) != nullptr) if (output.size() < EXEC_MAX_OUTPUT) output += buffer.data(); - return pclose(pipe) == 0; + return pclose(pipe) == 0 ? ExecResult::Ok : ExecResult::Failed; #else int fds[2]; - if (pipe(fds) != 0) { - LOG_WARN("Can't run '%s': pipe: %s", cmd, strerror(errno)); - return false; - } + if (pipe(fds) != 0) + return ExecResult::NotStarted; fcntl(fds[0], F_SETFD, FD_CLOEXEC); fcntl(fds[1], F_SETFD, FD_CLOEXEC); @@ -1771,8 +1769,7 @@ bool execCommand(const char *cmd, uint32_t timeoutMs, std::string &output) close(fds[1]); if (spawnError != 0) { close(fds[0]); - LOG_WARN("Can't run '%s': %s", cmd, strerror(spawnError)); - return false; + return ExecResult::NotStarted; } const auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(timeoutMs); @@ -1823,21 +1820,24 @@ bool execCommand(const char *cmd, uint32_t timeoutMs, std::string &output) usleep(5000); } if (timedOut) { - LOG_WARN("Command timed out after %ums, killed: %s", static_cast(timeoutMs), cmd); kill(-pid, SIGKILL); do { waited = waitpid(pid, &status, 0); } while (waited < 0 && errno == EINTR); - return false; + return ExecResult::TimedOut; } - return waited == pid && WIFEXITED(status) && WEXITSTATUS(status) == 0; + return waited == pid && WIFEXITED(status) && WEXITSTATUS(status) == 0 ? ExecResult::Ok : ExecResult::Failed; #endif } std::string exec(const char *cmd, uint32_t timeoutMs) { std::string output; - execCommand(cmd, timeoutMs, output); + const ExecResult result = execCommand(cmd, timeoutMs, output); + if (result == ExecResult::TimedOut) + LOG_WARN("Command timed out after %ums, killed: %s", static_cast(timeoutMs), cmd); + else if (result == ExecResult::NotStarted) + LOG_WARN("Command could not be started: %s", cmd); return output; } diff --git a/src/platform/portduino/PortduinoGlue.h b/src/platform/portduino/PortduinoGlue.h index f672557e3b..f6421058ff 100644 --- a/src/platform/portduino/PortduinoGlue.h +++ b/src/platform/portduino/PortduinoGlue.h @@ -144,9 +144,12 @@ void readGPIOFromYaml(YAML::Node sourceNode, pinMapping &destPin, int pinDefault // stalls the main loop for at most this long. constexpr uint32_t EXEC_DEFAULT_TIMEOUT_MS = 10000; constexpr uint32_t COMMAND_SENSOR_TIMEOUT_MS = 2000; +// Logs a timeout or a failure to start; returns whatever the command printed, whatever its exit status. std::string exec(const char *cmd, uint32_t timeoutMs = EXEC_DEFAULT_TIMEOUT_MS); -// Runs cmd under /bin/sh and captures stdout. True only if it exited 0 within timeoutMs. -bool execCommand(const char *cmd, uint32_t timeoutMs, std::string &output); +enum class ExecResult { Ok, Failed, TimedOut, NotStarted }; +// Runs cmd under /bin/sh and captures stdout. Ok only if it exited 0 within timeoutMs. Never logs, so it +// is safe off the main thread. +ExecResult execCommand(const char *cmd, uint32_t timeoutMs, std::string &output); // One Sensors: entry. type is an EnvironmentMetrics field name, see CommandSensorFields.h. struct commandSensorConfig { diff --git a/test/test_command_sensor/test_main.cpp b/test/test_command_sensor/test_main.cpp index f92ed8d492..185911ada6 100644 --- a/test/test_command_sensor/test_main.cpp +++ b/test/test_command_sensor/test_main.cpp @@ -3,15 +3,16 @@ // - execCommand() / exec() in src/platform/portduino/PortduinoGlue.cpp // - CommandSensorFields::{canonicalType, apply, parseValue} in // src/modules/Telemetry/Sensor/CommandSensorFields.cpp -// - CommandSensor::getMetrics() in src/modules/Telemetry/Sensor/CommandSensor.cpp +// - CommandSensor (worker thread and getMetrics()) in src/modules/Telemetry/Sensor/CommandSensor.cpp // // Why these contracts matter: // -// execCommand() runs on the main loop, the same thread that services the radio. A sensor script that -// hangs - a stuck I2C read in Python, a network call with no timeout, a command that closes stdout and -// keeps running - must cost at most its timeout, and the whole pipeline it started must be killed so it -// cannot keep holding the pipe. exec() keeps its old contract for HostMetrics.UserStringCommand: it -// returns whatever the command printed, whether or not it exited 0. +// A sensor script that hangs - a stuck I2C read in Python, a network call with no timeout, a command +// that closes stdout and keeps running - must cost at most its timeout, and the whole pipeline it +// started must be killed so it cannot keep holding the pipe. exec() still runs on the main loop for +// HostMetrics, and keeps its old contract there: it returns whatever the command printed, whether or +// not it exited 0. execCommand() reports the outcome instead of logging, because CommandSensor calls +// it from a worker thread and portduino's log buffer is not thread-safe. // // parseValue() is the only thing between arbitrary command output and a field broadcast over the mesh. // It must take exactly one finite number: "21.5 C", an empty line, an error message or "nan" must be @@ -20,14 +21,17 @@ // apply() writes into nanopb fields of several widths. A value outside a narrow integer field // (soil_moisture is uint8_t) must clamp, not wrap: 300 wrapping to 44% moisture is a silent lie. // -// CommandSensor caches readings because getMetrics() is also called for screen refreshes and phone -// requests. Without the cache every command reruns on each of those calls, multiplying the main-loop -// stall; with a cache that never expires the node broadcasts the first reading forever. +// CommandSensor runs commands on a worker thread and getMetrics() only reads the latest results. +// getMetrics() is called on the main loop, which also drives the radio, so it must return at once even +// while a command is hanging. On Windows, where the timeout cannot be enforced, that is the only thing +// that keeps a command that never exits from freezing the node. Each command is re-run every 30 s: +// re-running on every getMetrics() call (screen refreshes, phone requests) would multiply the work, and +// never re-running would broadcast the first reading forever. // -// 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; a scaled value overflowing to inf and being sent; the cache either missing or -// never expiring; Scale not applied. +// Regressions guarded: getMetrics() waiting on a command; 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; a scaled value overflowing to inf +// and being sent; the refresh either missing or never happening; Scale not applied. #include "TestUtil.h" #include "configuration.h" #include @@ -72,6 +76,18 @@ int countLines(const std::string &path) return lines; } +// Waits, in real time, for the worker to finish `reads` command runs. +bool waitForReads(CommandSensor &sensor, uint32_t reads, long timeoutMs = 5000) +{ + const auto start = std::chrono::steady_clock::now(); + while (sensor.finishedReads() < reads) { + if (elapsedMs(start) > timeoutMs) + return false; + usleep(10 * 1000); + } + return true; +} + meshtastic_Telemetry emptyEnvironment() { meshtastic_Telemetry t = meshtastic_Telemetry_init_zero; @@ -99,14 +115,14 @@ void tearDown(void) void test_execCommand_captures_stdout_and_reports_success() { std::string output; - TEST_ASSERT_TRUE(execCommand("echo 21.5", 2000, output)); + TEST_ASSERT_TRUE(execCommand("echo 21.5", 2000, output) == ExecResult::Ok); TEST_ASSERT_EQUAL_STRING("21.5\n", output.c_str()); } void test_execCommand_nonzero_exit_is_failure() { std::string output; - TEST_ASSERT_FALSE(execCommand("echo 3; exit 3", 2000, output)); + TEST_ASSERT_TRUE(execCommand("echo 3; exit 3", 2000, output) == ExecResult::Failed); TEST_ASSERT_EQUAL_STRING("3\n", output.c_str()); } @@ -114,7 +130,7 @@ void test_execCommand_stdin_is_not_inherited() { // A command that reads stdin must see EOF at once, not block on meshtasticd's terminal or socket. std::string output; - TEST_ASSERT_TRUE(execCommand("cat; echo done", 2000, output)); + TEST_ASSERT_TRUE(execCommand("cat; echo done", 2000, output) == ExecResult::Ok); TEST_ASSERT_EQUAL_STRING("done\n", output.c_str()); } @@ -122,7 +138,7 @@ void test_execCommand_hung_command_is_killed_at_timeout() { const auto start = std::chrono::steady_clock::now(); std::string output; - TEST_ASSERT_FALSE(execCommand("sleep 10", 300, output)); + TEST_ASSERT_TRUE(execCommand("sleep 10", 300, output) == ExecResult::TimedOut); TEST_ASSERT_LESS_THAN(2000, elapsedMs(start)); } @@ -133,7 +149,7 @@ void test_execCommand_timeout_kills_the_whole_pipeline() const std::string marker = scratchFile("pipeline-survived"); const std::string cmd = "sleep 2 | cat; touch " + marker; std::string output; - TEST_ASSERT_FALSE(execCommand(cmd.c_str(), 300, output)); + TEST_ASSERT_TRUE(execCommand(cmd.c_str(), 300, output) == ExecResult::TimedOut); usleep(2500 * 1000); TEST_ASSERT_EQUAL(-1, access(marker.c_str(), F_OK)); } @@ -143,7 +159,7 @@ void test_execCommand_closed_stdout_still_bounded_by_timeout() // EOF on stdout is not exit: the wait for the process must also respect the deadline. const auto start = std::chrono::steady_clock::now(); std::string output; - TEST_ASSERT_FALSE(execCommand("echo early; exec >&-; sleep 10", 300, output)); + TEST_ASSERT_TRUE(execCommand("echo early; exec >&-; sleep 10", 300, output) == ExecResult::TimedOut); TEST_ASSERT_LESS_THAN(2000, elapsedMs(start)); TEST_ASSERT_EQUAL_STRING("early\n", output.c_str()); } @@ -151,7 +167,7 @@ void test_execCommand_closed_stdout_still_bounded_by_timeout() void test_execCommand_output_is_capped() { std::string output; - TEST_ASSERT_TRUE(execCommand("head -c 20000 /dev/zero | tr '\\0' a", 2000, output)); + TEST_ASSERT_TRUE(execCommand("head -c 20000 /dev/zero | tr '\\0' a", 2000, output) == ExecResult::Ok); TEST_ASSERT_EQUAL(4096, output.size()); } @@ -242,6 +258,7 @@ void test_CommandSensor_applies_scale_and_skips_bad_output() portduino_config.command_sensors.push_back({"lux", "exit 1", 1.0f}); CommandSensor sensor; TEST_ASSERT_TRUE(sensor.initDevice(nullptr, nullptr)); + TEST_ASSERT_TRUE(waitForReads(sensor, 3)); meshtastic_Telemetry t = emptyEnvironment(); TEST_ASSERT_TRUE(sensor.getMetrics(&t)); @@ -258,6 +275,7 @@ void test_CommandSensor_rejects_scale_overflow() portduino_config.command_sensors.push_back({"soil_moisture", "echo 3e38", 10.0f}); CommandSensor sensor; TEST_ASSERT_TRUE(sensor.initDevice(nullptr, nullptr)); + TEST_ASSERT_TRUE(waitForReads(sensor, 2)); meshtastic_Telemetry t = emptyEnvironment(); TEST_ASSERT_FALSE(sensor.getMetrics(&t)); TEST_ASSERT_FALSE(t.variant.environment_metrics.has_temperature); @@ -269,6 +287,7 @@ void test_CommandSensor_all_failing_reports_no_metrics() portduino_config.command_sensors.push_back({"temperature", "echo oops", 1.0f}); CommandSensor sensor; TEST_ASSERT_TRUE(sensor.initDevice(nullptr, nullptr)); + TEST_ASSERT_TRUE(waitForReads(sensor, 1)); meshtastic_Telemetry t = emptyEnvironment(); TEST_ASSERT_FALSE(sensor.getMetrics(&t)); TEST_ASSERT_FALSE(t.variant.environment_metrics.has_temperature); @@ -282,20 +301,60 @@ void test_CommandSensor_caches_readings_then_refreshes() portduino_config.command_sensors.push_back({"weight", cmd, 1.0f}); CommandSensor sensor; TEST_ASSERT_TRUE(sensor.initDevice(nullptr, nullptr)); + TEST_ASSERT_TRUE(waitForReads(sensor, 1)); + // Inside the refresh interval: several worker polls go by (it wakes every 100ms) without a re-run, + // and the last value is still reported. + Time::setTestMillis(1000 + 29 * 1000); + usleep(500 * 1000); meshtastic_Telemetry t = emptyEnvironment(); TEST_ASSERT_TRUE(sensor.getMetrics(&t)); - Time::setTestMillis(1000 + 29 * 1000); - t = emptyEnvironment(); - TEST_ASSERT_TRUE(sensor.getMetrics(&t)); // cached value is still reported TEST_ASSERT_TRUE(t.variant.environment_metrics.has_weight); + TEST_ASSERT_EQUAL(1, sensor.finishedReads()); TEST_ASSERT_EQUAL(1, countLines(runs)); Time::setTestMillis(1000 + 31 * 1000); - TEST_ASSERT_TRUE(sensor.getMetrics(&t)); + TEST_ASSERT_TRUE(waitForReads(sensor, 2)); TEST_ASSERT_EQUAL(2, countLines(runs)); } +void test_CommandSensor_getMetrics_does_not_wait_for_a_hung_command() +{ + // The worker is stuck in this command for its full 2 s timeout; getMetrics() must not be. + portduino_config.command_sensors.push_back({"lux", "sleep 10", 1.0f}); + CommandSensor sensor; + TEST_ASSERT_TRUE(sensor.initDevice(nullptr, nullptr)); + usleep(200 * 1000); + + const auto start = std::chrono::steady_clock::now(); + meshtastic_Telemetry t = emptyEnvironment(); + TEST_ASSERT_FALSE(sensor.getMetrics(&t)); + TEST_ASSERT_LESS_THAN(100, elapsedMs(start)); + TEST_ASSERT_EQUAL(0, sensor.finishedReads()); + + // ...and the hung command is still bounded: the worker gets it back at the timeout. + TEST_ASSERT_TRUE(waitForReads(sensor, 1)); + TEST_ASSERT_FALSE(sensor.getMetrics(&t)); +} + +void test_CommandSensor_destructor_stops_the_worker() +{ + // Destroying the sensor must join its worker rather than leave a thread running a destroyed object. + const std::string runs = scratchFile("runs-after-destroy"); + std::remove(runs.c_str()); + const std::string cmd = "echo x >> " + runs + "; echo 1"; + portduino_config.command_sensors.push_back({"weight", cmd, 1.0f}); + { + CommandSensor sensor; + TEST_ASSERT_TRUE(sensor.initDevice(nullptr, nullptr)); + TEST_ASSERT_TRUE(waitForReads(sensor, 1)); + } + Time::setTestMillis(1000 + 60 * 1000); + usleep(300 * 1000); + TEST_ASSERT_EQUAL(1, countLines(runs)); + std::remove(runs.c_str()); +} + void setup() { initializeTestEnvironment(); @@ -322,6 +381,8 @@ void setup() RUN_TEST(test_CommandSensor_rejects_scale_overflow); RUN_TEST(test_CommandSensor_all_failing_reports_no_metrics); RUN_TEST(test_CommandSensor_caches_readings_then_refreshes); + RUN_TEST(test_CommandSensor_getMetrics_does_not_wait_for_a_hung_command); + RUN_TEST(test_CommandSensor_destructor_stops_the_worker); const int failures = UNITY_END(); std::remove(scratchFile("runs").c_str());