From 35ec6dfdb50a3c791df3fb6d403d25a95445ec9a Mon Sep 17 00:00:00 2001 From: Ryan Malloy Date: Tue, 17 Feb 2026 20:18:14 -0700 Subject: [PATCH] Harden safety-critical paths from Hamilton review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix lock hierarchy: stopAll() cancels pulse before touching axes - Add configASSERT bounds checks on axis index in move/pulseGuide - Enforce ST4Pulse singleton with configASSERT - Check esp_timer_start_once return, rollback hardware on failure - Validate SYNC coordinates (reject garbage → silent 0.0) - Discard truncated serial commands on buffer overflow - Guard WiFi update()/broadcastState() against null ws_ pointer - Report connection errors to WebSocket clients on move/pulse - Remove redundant Serial.begin() from pulse_guide example --- examples/pulse_guide/pulse_guide.ino | 4 +--- include/ST4Serial.h | 1 + src/ST4Controller.cpp | 5 +++- src/ST4Pulse.cpp | 13 +++++++++- src/ST4Serial.cpp | 36 +++++++++++++++++++++------- src/ST4WiFi.cpp | 10 ++++++++ 6 files changed, 56 insertions(+), 13 deletions(-) diff --git a/examples/pulse_guide/pulse_guide.ino b/examples/pulse_guide/pulse_guide.ino index 0eb8529..64cb614 100644 --- a/examples/pulse_guide/pulse_guide.ino +++ b/examples/pulse_guide/pulse_guide.ino @@ -14,15 +14,13 @@ ST4Controller controller; ST4Serial st4Serial; void setup() { - Serial.begin(115200); - Serial.println("ST4 Pulse Guide Example"); - controller.begin( ST4_PIN_RA_PLUS, ST4_PIN_RA_MINUS, ST4_PIN_DEC_PLUS, ST4_PIN_DEC_MINUS, ST4_PIN_LED ); + // st4Serial.begin() opens Serial at 57600 baud and prints INITIALIZED# st4Serial.begin(controller, Serial); controller.connect(); diff --git a/include/ST4Serial.h b/include/ST4Serial.h index bccbb20..c494417 100644 --- a/include/ST4Serial.h +++ b/include/ST4Serial.h @@ -13,6 +13,7 @@ class ST4Serial { HardwareSerial* serial_; String buffer_; bool extendedMode_; + bool bufferOverflow_; void processCommand(const String& cmd); void processExtendedCommand(const String& cmd); diff --git a/src/ST4Controller.cpp b/src/ST4Controller.cpp index cae024f..24814c5 100644 --- a/src/ST4Controller.cpp +++ b/src/ST4Controller.cpp @@ -90,6 +90,7 @@ bool ST4Controller::isConnected() const { void ST4Controller::move(ST4AxisId axis, ST4Direction dir) { if (!connected_) return; + configASSERT(static_cast(axis) < 2); xSemaphoreTake(mutex_, portMAX_DELAY); int idx = static_cast(axis); @@ -108,6 +109,8 @@ void ST4Controller::stopAxis(ST4AxisId axis) { } void ST4Controller::stopAll() { + // Cancel pulse first -- consistent lock hierarchy (Pulse before Axis) + pulse_.cancel(); xSemaphoreTake(mutex_, portMAX_DELAY); int raIdx = static_cast(ST4AxisId::RA); int decIdx = static_cast(ST4AxisId::DECLINATION); @@ -116,11 +119,11 @@ void ST4Controller::stopAll() { axes_[decIdx].stop(); trackers_[decIdx].stop(); xSemaphoreGive(mutex_); - if (pulse_.isActive()) pulse_.cancel(); } bool ST4Controller::pulseGuide(ST4AxisId axis, ST4Direction dir, uint32_t ms) { if (!connected_) return false; + configASSERT(static_cast(axis) < 2); xSemaphoreTake(mutex_, portMAX_DELAY); int idx = static_cast(axis); diff --git a/src/ST4Pulse.cpp b/src/ST4Pulse.cpp index 5aece3f..e74a600 100644 --- a/src/ST4Pulse.cpp +++ b/src/ST4Pulse.cpp @@ -66,6 +66,7 @@ void ST4Pulse::pulseTaskFunc(void* arg) { } void ST4Pulse::begin() { + configASSERT(instance_ == nullptr); instance_ = this; mutex_ = xSemaphoreCreateMutex(); @@ -122,7 +123,17 @@ bool ST4Pulse::pulse(ST4Axis& axis, ST4Tracker& tracker, tracker.start(slewRate); // Start one-shot timer (microseconds) - esp_timer_start_once(timer_, static_cast(ms) * 1000); + esp_err_t timerErr = esp_timer_start_once(timer_, static_cast(ms) * 1000); + if (timerErr != ESP_OK) { + // Timer failed -- rollback hardware activation + axis.stop(); + tracker.stop(); + active_ = false; + activeAxis_ = nullptr; + activeTracker_ = nullptr; + xSemaphoreGive(mutex_); + return false; + } xSemaphoreGive(mutex_); return true; diff --git a/src/ST4Serial.cpp b/src/ST4Serial.cpp index 0c1d2e7..ebfd85e 100644 --- a/src/ST4Serial.cpp +++ b/src/ST4Serial.cpp @@ -5,7 +5,8 @@ #include "ST4Serial.h" ST4Serial::ST4Serial() - : controller_(nullptr), serial_(nullptr), extendedMode_(true) {} + : controller_(nullptr), serial_(nullptr), extendedMode_(true), + bufferOverflow_(false) {} void ST4Serial::begin(ST4Controller& controller, HardwareSerial& serial, bool extendedMode) { @@ -105,10 +106,22 @@ void ST4Serial::processExtendedCommand(const String& cmd) { String rest = cmd.substring(5); int spaceIdx = rest.indexOf(' '); if (spaceIdx > 0) { - double ra = rest.substring(0, spaceIdx).toDouble(); - double dec = rest.substring(spaceIdx + 1).toDouble(); - controller_->sync(ra, dec); - serial_->println("OK#"); + String raStr = rest.substring(0, spaceIdx); + String decStr = rest.substring(spaceIdx + 1); + decStr.trim(); + // Validate: must start with digit, sign, or dot (reject garbage → 0.0) + bool raValid = raStr.length() > 0 && + (isDigit(raStr[0]) || raStr[0] == '-' || raStr[0] == '+' || raStr[0] == '.'); + bool decValid = decStr.length() > 0 && + (isDigit(decStr[0]) || decStr[0] == '-' || decStr[0] == '+' || decStr[0] == '.'); + if (raValid && decValid) { + double ra = raStr.toDouble(); + double dec = decStr.toDouble(); + controller_->sync(ra, dec); + serial_->println("OK#"); + } else { + serial_->println("ERR:INVALID_COORDS#"); + } } else { serial_->println("ERR:INVALID_COORDS#"); } @@ -137,14 +150,21 @@ void ST4Serial::update() { while (serial_->available()) { char c = serial_->read(); if (c == '#') { - buffer_.trim(); - if (buffer_.length() > 0) { - processCommand(buffer_); + if (bufferOverflow_) { + // Discard the truncated command entirely + bufferOverflow_ = false; + } else { + buffer_.trim(); + if (buffer_.length() > 0) { + processCommand(buffer_); + } } buffer_ = ""; } else if (c != '\r' && c != '\n') { if (buffer_.length() < 64) { buffer_ += c; + } else { + bufferOverflow_ = true; } } } diff --git a/src/ST4WiFi.cpp b/src/ST4WiFi.cpp index b91e0d0..9f655ac 100644 --- a/src/ST4WiFi.cpp +++ b/src/ST4WiFi.cpp @@ -76,6 +76,10 @@ void ST4WiFi::processCommand(AsyncWebSocketClient* client, if (!cmd) return; if (strcmp(cmd, "move") == 0) { + if (!controller_->isConnected()) { + client->text("{\"error\":\"not connected\"}"); + return; + } const char* axisStr = doc["axis"]; const char* dirStr = doc["dir"]; if (!axisStr || !dirStr) return; @@ -94,6 +98,10 @@ void ST4WiFi::processCommand(AsyncWebSocketClient* client, broadcastState(); } else if (strcmp(cmd, "pulse") == 0) { + if (!controller_->isConnected()) { + client->text("{\"error\":\"not connected\"}"); + return; + } const char* axisStr = doc["axis"]; const char* dirStr = doc["dir"]; uint32_t ms = doc["ms"] | 0; @@ -128,6 +136,7 @@ void ST4WiFi::processCommand(AsyncWebSocketClient* client, } void ST4WiFi::broadcastState() { + if (!ws_) return; if (ws_->count() > 0) { ws_->textAll(stateJson()); } @@ -159,6 +168,7 @@ String ST4WiFi::stateJson() const { } void ST4WiFi::update() { + if (!ws_) return; ws_->cleanupClients(); ST4Direction currentRa = controller_->axisDirection(ST4AxisId::RA);