From 1fdf60900b05cafa2b151ef6f93216b8424bb834 Mon Sep 17 00:00:00 2001 From: TimoWielink Date: Mon, 6 Jul 2026 18:05:16 +0200 Subject: [PATCH 1/3] spi Fix Signed-off-by: TimoWielink --- src/radio/DuckLoRa.cpp | 63 +++++++++++++++++++++++++++--------------- src/radio/DuckLoRa.h | 4 ++- 2 files changed, 44 insertions(+), 23 deletions(-) diff --git a/src/radio/DuckLoRa.cpp b/src/radio/DuckLoRa.cpp index bb0696d2..175dd1f8 100644 --- a/src/radio/DuckLoRa.cpp +++ b/src/radio/DuckLoRa.cpp @@ -21,7 +21,7 @@ CDPCFG_LORA_CLASS lora = new Module(CDPCFG_PIN_LORA_CS, CDPCFG_PIN_LORA_DIO0, CDPCFG_PIN_LORA_RST, CDPCFG_PIN_LORA_DIO1); #endif -volatile uint16_t DuckLoRa::interruptFlags = 0; +volatile bool DuckLoRa::interruptPending = false; volatile bool DuckLoRa::receivedFlag = false; const LoRaConfigParams DuckLoRa::defaultRadioParams = { @@ -314,79 +314,98 @@ int DuckLoRa::sleep() } void DuckLoRa::serviceInterruptFlags() { - if (DuckLoRa::interruptFlags != 0) { + if (!DuckLoRa::interruptPending) { + return; + } + + // Clear the pending latch BEFORE reading the flags off the radio. If DIO1 + // fires again while we are servicing, the ISR re-sets interruptPending and + // we pick the new event up on the next loop instead of silently dropping it + // (the read-then-clear race that could leave the node parked in standby). + DuckLoRa::interruptPending = false; + + // Safe to touch SPI here: this runs in loop context, not the ISR. + uint16_t flags = lora.getIrqFlags(); + if (flags == 0) { + return; + } #ifdef CDPCFG_RADIO_SX1262 // SX1262 flags - if (DuckLoRa::interruptFlags & RADIOLIB_SX126X_CMD_CLEAR_IRQ_STATUS) { + if (flags & RADIOLIB_SX126X_CMD_CLEAR_IRQ_STATUS) { logdbg_ln("SX1262 Interrupt flag was set: clear IRQ status"); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX126X_CMD_CLEAR_DEVICE_ERRORS) { + if (flags & RADIOLIB_SX126X_CMD_CLEAR_DEVICE_ERRORS) { logdbg_ln("SX1262 Interrupt flag was set: clear device errors"); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX126X_IRQ_CRC_ERR ) { + if (flags & RADIOLIB_SX126X_IRQ_CRC_ERR ) { logdbg_ln("SX1262 Interrupt flag was set: payload CRC error"); + // goToReceiveMode() re-arms the radio (startReceive clears the IRQ + // status). Do NOT call lora.standby() afterwards: that immediately + // knocks the radio back out of receive and the node goes deaf. goToReceiveMode(false); - lora.standby(); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX126X_IRQ_HEADER_ERR ) { + if (flags & RADIOLIB_SX126X_IRQ_HEADER_ERR ) { logdbg_ln("SX1262 Interrupt flag was set: header CRC error"); goToReceiveMode(false); - lora.standby(); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX126X_IRQ_RX_DONE ) { + if (flags & RADIOLIB_SX126X_IRQ_RX_DONE ) { logdbg_ln("SX1262 Interrupt flag was set: packet reception complete"); setReceiveFlag(true); lora.standby(); // we are done receiving, go to standby. We can't sleep because read buffer is not empty } - if (DuckLoRa::interruptFlags & RADIOLIB_SX126X_IRQ_TX_DONE ) { + if (flags & RADIOLIB_SX126X_IRQ_TX_DONE ) { logdbg_ln("SX1262 Interrupt flag was set: payload transmission complete"); lora.finishTransmit(); goToReceiveMode(false); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX126X_IRQ_TIMEOUT ) { + if (flags & RADIOLIB_SX126X_IRQ_TIMEOUT ) { logdbg_ln("SX1262 Interrupt flag was set: timeout"); goToReceiveMode(false); } #else // SX127X flags - if (DuckLoRa::interruptFlags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_RX_TIMEOUT) { + if (flags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_RX_TIMEOUT) { goToReceiveMode(true); // go back to receive mode and reset the receive flag logdbg_ln("SX127x Interrupt flag was set: timeout"); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_RX_DONE) { + if (flags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_RX_DONE) { logdbg_ln("SX127x Interrupt flag was set: packet reception complete"); setReceiveFlag(true); // set the receive flag and we stay in receive mode lora.standby(); // we are done receiving, go to standby. We can't sleep because read buffer is not empty } - if (DuckLoRa::interruptFlags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_PAYLOAD_CRC_ERROR) { + if (flags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_PAYLOAD_CRC_ERROR) { goToReceiveMode(true); // go back to receive mode and reset the receive flag logdbg_ln("SX127x Interrupt flag was set: payload CRC error"); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_VALID_HEADER) { + if (flags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_VALID_HEADER) { logdbg_ln("SX127x Interrupt flag was set: valid header received"); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_TX_DONE) { + if (flags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_TX_DONE) { logdbg_ln("SX127x Interrupt flag was set: payload transmission complete"); goToReceiveMode(false); // go back to receive mode and reset the receive flag } - if (DuckLoRa::interruptFlags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_CAD_DONE) { + if (flags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_CAD_DONE) { logdbg_ln("SX127x Interrupt flag was set: CAD complete"); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_FHSS_CHANGE_CHANNEL) { + if (flags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_FHSS_CHANGE_CHANNEL) { logdbg_ln("SX127x Interrupt flag was set: FHSS change channel"); } - if (DuckLoRa::interruptFlags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_CAD_DETECTED) { + if (flags & RADIOLIB_SX127X_CLEAR_IRQ_FLAG_CAD_DETECTED) { logdbg_ln("SX127x Interrupt flag was set: valid LoRa signal detected during CAD operation"); } #endif - DuckLoRa::interruptFlags = 0; - } } // IMPORTANT: this function MUST be 'void' type and MUST NOT have any arguments! void DuckLoRa::onInterrupt(void) { - interruptFlags = lora.getIrqFlags(); + // ISR context: do NOT touch the SPI bus here. lora.getIrqFlags() issues a + // full SPI transaction (including a BUSY-line wait and the arduino-esp32 SPI + // HAL lock) which can deadlock or corrupt the bus if DIO1 fires while the + // main loop is mid-transfer (startTransmit / readData / startReceive). The + // documented RadioLib pattern is for the ISR to only latch a flag; the flags + // are then read from loop context in serviceInterruptFlags(). + interruptPending = true; } int DuckLoRa::startTransmitData(uint8_t* data, int length) { diff --git a/src/radio/DuckLoRa.h b/src/radio/DuckLoRa.h index 53135c4e..514893f7 100644 --- a/src/radio/DuckLoRa.h +++ b/src/radio/DuckLoRa.h @@ -129,7 +129,9 @@ class DuckLoRa { float getSNR(); private: - static volatile uint16_t interruptFlags; + // Set to true by onInterrupt() (ISR context) only. The actual IRQ flags + // are read from the radio in loop context by serviceInterruptFlags(). + static volatile bool interruptPending; static volatile bool receivedFlag; volatile bool isSetup = false; unsigned long lastReceiveTime = 0L; From 81171455618c184a65ceeed0977f68fcfef8e81a Mon Sep 17 00:00:00 2001 From: TimoWielink Date: Mon, 6 Jul 2026 21:35:55 +0200 Subject: [PATCH 2/3] validate route packets and rx buffer lengths before parsing Signed-off-by: TimoWielink --- src/Ducks/DuckLink.h | 12 +++++-- src/Ducks/MamaDuck.h | 8 +++++ src/Ducks/PapaDuck.h | 12 +++++-- src/radio/DuckLoRa.cpp | 6 ++-- src/routing/RouteJSON.h | 72 ++++++++++++++++++++++++++++++++--------- 5 files changed, 87 insertions(+), 23 deletions(-) diff --git a/src/Ducks/DuckLink.h b/src/Ducks/DuckLink.h index 5249d2f5..b63c16b2 100644 --- a/src/Ducks/DuckLink.h +++ b/src/Ducks/DuckLink.h @@ -168,6 +168,10 @@ class DuckLink : public Duck { switch(rxPacket.topic) { case reservedTopic::rreq: { RouteJSON rreqDoc = RouteJSON(rxPacket.data); + if (!rreqDoc.isValid()) { + logerr_ln("handleReceivedPacket: dropping malformed RREQ"); + break; + } if(!relay) { loginfo_ln("handleReceivedPacket: Sending RREP"); std::optional last = rreqDoc.getlastInPath(); @@ -185,12 +189,16 @@ class DuckLink : public Duck { case reservedTopic::rrep: { //we still need to recieve rreps in case of ttl expiry RouteJSON rrepDoc = RouteJSON(rxPacket.data); + if (!rrepDoc.isValid()) { + logerr_ln("handleReceivedPacket: dropping malformed RREP"); + break; + } std::string sourceDuid(rxPacket.sduid.begin(), rxPacket.sduid.end()); loginfo_ln("Received Route Response from DUID: %s", sourceDuid.c_str()); //destination = sender of the rrep -> the last hop to current duck std::optional last = rrepDoc.getlastInPath(); - Duid lastInPath = last.value(); - + Duid lastInPath = last.has_value() ? last.value() : rxPacket.sduid; + this->router.insertIntoRoutingTable(rrepDoc.getOrigin(), lastInPath, this->getSignalScore()); } break; diff --git a/src/Ducks/MamaDuck.h b/src/Ducks/MamaDuck.h index 8248aeac..8b833019 100644 --- a/src/Ducks/MamaDuck.h +++ b/src/Ducks/MamaDuck.h @@ -190,6 +190,10 @@ private : switch(rxPacket.topic) { case reservedTopic::rreq: { RouteJSON rreqDoc = RouteJSON(rxPacket.data); + if (!rreqDoc.isValid()) { + logerr_ln("handleReceivedPacket: dropping malformed RREQ"); + break; + } //route requests are just forwarded so we can use the sduid as the origin std::optional last = rreqDoc.getlastInPath(); Duid lastInPath = last.has_value() ? last.value() : rxPacket.sduid; @@ -212,6 +216,10 @@ private : case reservedTopic::rrep: { //we still need to recieve rreps in case of ttl expiry RouteJSON rrepDoc = RouteJSON(rxPacket.data); + if (!rrepDoc.isValid()) { + logerr_ln("handleReceivedPacket: dropping malformed RREP"); + break; + } std::optional last = rrepDoc.getlastInPath(); Duid lastInPath = last.has_value() ? last.value() : rxPacket.sduid; std::string sourceDuid(rxPacket.sduid.begin(), rxPacket.sduid.end()); diff --git a/src/Ducks/PapaDuck.h b/src/Ducks/PapaDuck.h index 63df5850..3b4297de 100644 --- a/src/Ducks/PapaDuck.h +++ b/src/Ducks/PapaDuck.h @@ -108,6 +108,10 @@ void ifNotBroadcast(CdpPacket rxPacket, bool relay = false) { switch(rxPacket.topic) { case reservedTopic::rreq: { RouteJSON rreqDoc = RouteJSON(rxPacket.data); + if (!rreqDoc.isValid()) { + logerr_ln("handleReceivedPacket: dropping malformed RREQ"); + break; + } //route requests are just forwarded so we can use the sduid as the origin std::optional last = rreqDoc.getlastInPath(); Duid lastInPath = last.has_value() ? last.value() : rxPacket.sduid; @@ -130,10 +134,14 @@ void ifNotBroadcast(CdpPacket rxPacket, bool relay = false) { case reservedTopic::rrep: { //we still need to recieve rreps in case of ttl expiry RouteJSON rrepDoc = RouteJSON(rxPacket.data); + if (!rrepDoc.isValid()) { + logerr_ln("handleReceivedPacket: dropping malformed RREP"); + break; + } std::optional last = rrepDoc.getlastInPath(); Duid lastInPath = last.has_value() ? last.value() : rxPacket.sduid; - std::string sourceDuid(rxPacket.sduid.begin(), rxPacket.sduid.end()); - loginfo_ln("Received Route Response from DUID: %s", sourceDuid.c_str()); + std::string sourceDuid(rxPacket.sduid.begin(), rxPacket.sduid.end()); + loginfo_ln("Received Route Response from DUID: %s", sourceDuid.c_str()); std::optional nextHop = this->router.getBestNextHop(rrepDoc.getDestination()); if((rrepDoc.getDestination() != this->duid) && (nextHop.has_value()) && (nextHop.value() != rxPacket.sduid)){ //and not papaduck_duid? diff --git a/src/radio/DuckLoRa.cpp b/src/radio/DuckLoRa.cpp index 175dd1f8..b7d4e146 100644 --- a/src/radio/DuckLoRa.cpp +++ b/src/radio/DuckLoRa.cpp @@ -159,10 +159,10 @@ std::optional> DuckLoRa::readReceivedData() { //return a st packet_length = lora.getPacketLength(); - if (packet_length < MIN_PACKET_LENGTH) { + if (packet_length < MIN_PACKET_LENGTH || packet_length > PACKET_LENGTH) { logerr_ln("ERROR handlePacket rx data size invalid: %d", packet_length); - - rxState = goToReceiveMode(true); // go back to receive mode and reset the receive flag + goToReceiveMode(true); // go back to receive mode and reset the receive flag + return std::nullopt; } loginfo_ln("readReceivedData() - packet length returns: %d", packet_length); diff --git a/src/routing/RouteJSON.h b/src/routing/RouteJSON.h index 4b9abfa6..61d68a06 100644 --- a/src/routing/RouteJSON.h +++ b/src/routing/RouteJSON.h @@ -23,9 +23,12 @@ class RouteJSON { * @param sourceDevice the source device DUID */ RouteJSON(Duid targetDevice, Duid sourceDevice) { - json["origin"] = duckutils::hexToString(duckutils::duidAsString(sourceDevice)); - json["destination"] = duckutils::hexToString(duckutils::duidAsString(targetDevice)); + origin = duckutils::hexToString(duckutils::duidAsString(sourceDevice)); + destination = duckutils::hexToString(duckutils::duidAsString(targetDevice)); + json["origin"] = origin; + json["destination"] = destination; json["path"].as(); + valid = true; std::string log; serializeJson(json, log); @@ -35,6 +38,7 @@ class RouteJSON { //Create JSON from rxPacket /** * @brief Construct a new Route JSON object from received packet data + * check isValid() before using the parsed values * * @param packetData the received packet data as a byte vector */ @@ -43,16 +47,42 @@ class RouteJSON { DeserializationError error = deserializeJson(json, packetStr); if (error) { logerr_ln("RouteJSON deserialization failed: %s", error.c_str()); + return; + } + + const char* originPtr = json["origin"].as(); + const char* destinationPtr = json["destination"].as(); + if (originPtr == nullptr || destinationPtr == nullptr) { + logerr_ln("RouteJSON missing origin/destination"); + return; + } + origin = originPtr; + destination = destinationPtr; + if (origin.size() != DUID_LENGTH || destination.size() != DUID_LENGTH) { + logerr_ln("RouteJSON origin/destination length invalid (%d/%d)", + (int)origin.size(), (int)destination.size()); + return; } + for (JsonVariant value : json["path"].as()) { - objPath.push_back(value); // Copy each element to myPath + std::string entry = value.as(); + if (entry.size() != DUID_LENGTH) { + logerr_ln("RouteJSON path entry length invalid (%d)", (int)entry.size()); + return; + } + objPath.push_back(entry); } - origin = json["origin"].as(); - destination = json["destination"].as(); + + valid = true; const std::string serialized = asString(); logdbg_ln("Built RouteJSON from packet data: %s", serialized.c_str()); } + /** + * @brief returns false if the packet data could not be parsed + */ + bool isValid() const { return valid; } + std::string asString(){ std::string out; serializeJson(json, out); @@ -74,12 +104,18 @@ class RouteJSON { } Duid getOrigin(){ Duid originDuid; - std::copy(origin.begin(), origin.end(), originDuid.begin()); + originDuid.fill(0); + if (origin.size() == DUID_LENGTH) { + std::copy(origin.begin(), origin.end(), originDuid.begin()); + } return originDuid; } Duid getDestination(){ Duid destinationDuid; - std::copy(destination.begin(), destination.end(), destinationDuid.begin()); + destinationDuid.fill(0); + if (destination.size() == DUID_LENGTH) { + std::copy(destination.begin(), destination.end(), destinationDuid.begin()); + } return destinationDuid; } @@ -105,13 +141,12 @@ class RouteJSON { std::optional getlastInPath(){ Duid lastDuid; if(objPath.size() > 0){ - auto last = objPath[objPath.size()-1]; - std::copy(last.begin(), last.end(),lastDuid.begin()); - - std::string log; - serializeJson(json, log); - logdbg_ln("RREQ: %s", log.c_str()); - + const std::string& last = objPath[objPath.size()-1]; + if (last.size() != DUID_LENGTH) { + logdbg_ln("RREQ path entry has invalid length, ignoring"); + return std::nullopt; + } + std::copy(last.begin(), last.end(), lastDuid.begin()); return lastDuid; } else{ @@ -123,13 +158,17 @@ class RouteJSON { /** * @brief pop the last duck node from the route response path * - * @param deviceId of the duck node to be removed * @return the newly modified Arduino JSON document */ std::string popFromPath(){ + if (objPath.empty()) { + //pop_back on an empty vector is UB + logdbg_ln("popFromPath: path already empty, nothing to pop"); + return asString(); + } objPath.pop_back(); updateJsonPath(); - + std::string log; serializeJson(json, log); logdbg_ln("Packet: %s", log.c_str()); @@ -142,6 +181,7 @@ class RouteJSON { std::vector objPath; std::string origin; std::string destination; + bool valid = false; void updateJsonPath(){ JsonArray path = json["path"].to(); From 875aa91237b158f32e334e91a6eaf6e19b523357 Mon Sep 17 00:00:00 2001 From: TimoWielink Date: Tue, 7 Jul 2026 14:53:05 +0200 Subject: [PATCH 3/3] Write Floats to memory fix Signed-off-by: TimoWielink --- src/Ducks/DuckLink.h | 9 +++++++-- src/Ducks/MamaDuck.h | 25 +++++++++++++++---------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/Ducks/DuckLink.h b/src/Ducks/DuckLink.h index b63c16b2..6da79d20 100644 --- a/src/Ducks/DuckLink.h +++ b/src/Ducks/DuckLink.h @@ -98,8 +98,13 @@ class DuckLink : public Duck { // min = Teensy power-off threshold, max = power-on/recovery // threshold, both in volts. Store as float under the same // keys seeded in Duck::setupWithDefaults. - this->eeprom_preferences.putFloat("teensy_off", battery_min); - this->eeprom_preferences.putFloat("teensy_on", battery_max); + const size_t offBytes = this->eeprom_preferences.putFloat("teensy_off", battery_min); + const size_t onBytes = this->eeprom_preferences.putFloat("teensy_on", battery_max); + if (offBytes != sizeof(float) || onBytes != sizeof(float)) { + logerr_ln("Failed to persist battery sleep thresholds to Preferences"); + err = DUCK_ERR_EEPROM_WRITE; + break; + } } err = this->broadcastPacket(rxPacket); diff --git a/src/Ducks/MamaDuck.h b/src/Ducks/MamaDuck.h index 8b833019..186824c2 100644 --- a/src/Ducks/MamaDuck.h +++ b/src/Ducks/MamaDuck.h @@ -53,8 +53,8 @@ private : this->router.getFilter().bloom_add(rxPacket.muid.data(), MUID_LENGTH); } - void ifBroadcast(CdpPacket rxPacket) { - int err; + void ifBroadcast(CdpPacket rxPacket) { + int err = DUCK_ERR_NONE; switch(rxPacket.topic) { case reservedTopic::rreq: { if(rxPacket.hopCount <= 0){ @@ -100,14 +100,19 @@ private : break; } - if (err == DUCK_ERR_NONE){ - // min = Teensy power-off threshold, max = power-on/recovery - // threshold, both in volts. Store as float under the same keys - // seeded in Duck::setupWithDefaults so the firmware reads one - // source of truth. - this->eeprom_preferences.putFloat("teensy_off", battery_min); - this->eeprom_preferences.putFloat("teensy_on", battery_max); - } + if (err == DUCK_ERR_NONE){ + // min = Teensy power-off threshold, max = power-on/recovery + // threshold, both in volts. Store as float under the same keys + // seeded in Duck::setupWithDefaults so the firmware reads one + // source of truth. + const size_t offBytes = this->eeprom_preferences.putFloat("teensy_off", battery_min); + const size_t onBytes = this->eeprom_preferences.putFloat("teensy_on", battery_max); + if (offBytes != sizeof(float) || onBytes != sizeof(float)) { + logerr_ln("Failed to persist battery sleep thresholds to Preferences"); + err = DUCK_ERR_EEPROM_WRITE; + break; + } + } err = this->broadcastPacket(rxPacket); if (err != DUCK_ERR_NONE) {