Merge branch 'develop' into bugfix/serial-subcommands

src/CommandHandler.cpp

@@ -25,7 +25,7 @@ const char* const TAG = "CommandHandler";
 const int64_t KEEP_ALIVE_INTERVAL  = 60'000;
 const uint16_t KEEP_ALIVE_DURATION = 300;
 
-uint32_t calculateEepyTime(int64_t timeToKeepAlive)
+static uint32_t calculateEepyTime(int64_t timeToKeepAlive)
 {
   int64_t now = OpenShock::millis();
   return static_cast<uint32_t>(std::clamp(timeToKeepAlive - now, 0LL, KEEP_ALIVE_INTERVAL));
@@ -316,7 +316,7 @@ bool CommandHandler::HandleCommand(ShockerModelType model, uint16_t shockerId, S
   ScopedReadLock lock__ka(&s_keepAliveMutex);
 
   if (ok && s_keepAliveQueue != nullptr) {
-    KnownShocker cmd {.model = model, .shockerId = shockerId, .lastActivityTimestamp = OpenShock::millis() + durationMs};
+    KnownShocker cmd {.killTask = false, .model = model, .shockerId = shockerId, .lastActivityTimestamp = OpenShock::millis() + durationMs};
     if (xQueueSend(s_keepAliveQueue, &cmd, pdMS_TO_TICKS(10)) != pdTRUE) {
       OS_LOGE(TAG, "Failed to send keep-alive command to queue");
     }

src/Convert.cpp

@@ -13,36 +13,35 @@ using namespace std::string_view_literals;
 
 // Base converter
 template<typename T>
-void fromNonZeroT(T val, std::string& str)
+void fromNonZeroT(T val, std::string& out)
 {
-  static_assert(std::is_integral_v<T>);
+  // Ensure the template type T is an integral type
+  static_assert(std::is_integral_v<T>, "T must be an integral type");
   constexpr std::size_t MaxDigits = OpenShock::Util::Digits10CountMax<T>;
 
-  char buf[MaxDigits + 1];  // +1 for null terminator
+  char buf[MaxDigits];
 
+  // Start from the end of the buffer to construct the number in reverse (from least to most significant digit)
   char* ptr = buf + MaxDigits;
 
-  // Null terminator
-  *ptr-- = '\0';
-
-  // Handle negative numbers
   bool negative = val < 0;
   if (negative) {
-    val = -val;
+    val = -val;  // Make the value positive for digit extraction
   }
 
-  // Convert to string
+  // Extract digits and store them in reverse order in the buffer
   while (val > 0) {
-    *ptr-- = '0' + (val % 10);
+    *--ptr = '0' + (val % 10);
     val /= 10;
   }
 
+  // If the number was negative, add the negative sign
   if (negative) {
-    *ptr-- = '-';
+    *--ptr = '-';
   }
 
-  // Append to string with length
-  str.append(ptr + 1, buf + MaxDigits + 1);
+  // Append the resulting string to the output
+  out.append(ptr, buf + MaxDigits);
 }
 
 // Base converter
@@ -59,54 +58,88 @@ void fromT(T val, std::string& str)
   fromNonZeroT(val, str);
 }
 
+/**
+ * @brief Converts a string view to an integral value of type T.
+ *
+ * This function attempts to parse a string representation of a number into an integer of the specified integral type T.
+ * It performs validation to ensure the string is a valid representation of a number within the range of the target type and checks for overflow or invalid input.
+ *
+ * @tparam T The integral type to which the string should be converted.
+ * @param[in] str The input string view representing the number.
+ * @param[out] out The reference where the converted value will be stored, if successful.
+ * @return true if the conversion was successful and `out` contains the parsed value.
+ * @return false if the conversion failed due to invalid input, overflow, or out-of-range value.
+ *
+ * @note The function handles both signed and unsigned types, for signed types, it processes negative numbers correctly.
+ * @note It also validates the input for edge cases like empty strings, leading zeros, or excessive length.
+ *
+ * @warning Ensure the input string is sanitized if sourced from untrusted input, as this function does not strip whitespace or handle non-numeric characters beyond validation.
+ */
 template<typename T>
 constexpr bool spanToT(std::string_view str, T& out)
 {
-  static_assert(std::is_integral_v<T>);
+  static_assert(std::is_integral_v<T>, "Template type T must be an integral type.");
 
+  // Define the unsigned type equivalent for T.
   using UT = typename std::make_unsigned<T>::type;
 
   constexpr bool IsSigned = std::is_signed_v<T>;
-  constexpr T Threshold   = std::numeric_limits<T>::max() / 10;
 
+  // Define an overflow threshold for type T.
+  // If an integer of type T exceeds this value, multiplying it by 10 will cause an overflow.
+  constexpr T Threshold = std::numeric_limits<T>::max() / 10;
+
+  // Fail on empty input strings.
   if (str.empty()) {
     return false;
   }
 
-  // Handle negative for signed types
+  // Handle negative numbers for signed types.
   bool isNegative = false;
   if constexpr (IsSigned) {
     if (str.front() == '-') {
-      str.remove_prefix(1);
+      str.remove_prefix(1);  // Remove the negative sign.
       isNegative = true;
-      if (str.empty()) return false;
+
+      // Fail if the string only contained a negative sign with no numeric characters
+      if (str.empty()) {
+        return false;
+      }
     }
   }
 
+  // Fail on strings that are too long to be valid integers.
   if (str.length() > OpenShock::Util::Digits10CountMax<T>) {
     return false;
   }
 
   // Special case for zero, also handles leading zeros and negative zero
   if (str.front() == '0') {
+    // Fail if string has leading or negative zero's
     if (str.length() > 1 || isNegative) return false;
+
+    // Simple "0" string
     out = 0;
     return true;
   }
 
+  // Value accumulator.
   UT val = 0;
 
   for (char c : str) {
+    // Return false if the character is not a digit.
     if (c < '0' || c > '9') {
       return false;
     }
 
+    // Check for multiplication overflow before multiplying by 10.
     if (val > Threshold) {
       return false;
     }
 
     val *= 10;
 
+    // Convert the character to a digit and check for addition overflow.
     uint8_t digit = c - '0';
     if (digit > std::numeric_limits<UT>::max() - val) {
       return false;
@@ -115,30 +148,24 @@ constexpr bool spanToT(std::string_view str, T& out)
     val += digit;
   }
 
+  // Special case handling for signed integers.
   if constexpr (IsSigned) {
     if (isNegative) {
-      constexpr UT LowerLimit = static_cast<UT>(std::numeric_limits<T>::max()) + 1;
-
-      if (val > LowerLimit) {
+      // Signed cast undeflow check, checks against the inverse lower limit for the signed type (e.g., 128 for int8_t).
+      if (val > static_cast<UT>(std::numeric_limits<T>::max()) + 1) {
         return false;
       }
 
-      if (val == LowerLimit) {
-        out = std::numeric_limits<T>::min();
-        return true;
-      }
-
-      out = -static_cast<T>(val);
-      return true;
-    }
-
-    if (val > std::numeric_limits<T>::max()) {
+      // Assign negative value bits
+      val = ~val + 1;
+    } else if (val > std::numeric_limits<T>::max()) {
+      // Fail on signed cast overflow
       return false;
     }
   }
 
+  // Cast result to output and return
   out = static_cast<T>(val);
-
   return true;
 }
 

src/GatewayClient.cpp

@@ -137,8 +137,10 @@ void GatewayClient::_sendBootStatus()
     return;
   }
 
+  using namespace std::string_view_literals;
+
   OpenShock::SemVer version;
-  if (!OpenShock::TryParseSemVer(OPENSHOCK_FW_VERSION, version)) {
+  if (!OpenShock::TryParseSemVer(OPENSHOCK_FW_VERSION ""sv, version)) {
     OS_LOGE(TAG, "Failed to parse firmware version");
     return;
   }

src/OtaUpdateManager.cpp

@@ -320,8 +320,6 @@ static void _otaUpdateTask(void* arg)
         OS_LOGE(TAG, "Failed to get requested version");
         continue;
       }
-
-      OS_LOGD(TAG, "Update requested for version %s", version.toString().c_str());  // TODO: This is abusing the SemVer::toString() method causing alot of string copies, fix this
     } else {
       OS_LOGD(TAG, "Checking for updates");
 
@@ -330,15 +328,17 @@ static void _otaUpdateTask(void* arg)
         OS_LOGE(TAG, "Failed to fetch firmware version");
         continue;
       }
-
-      OS_LOGD(TAG, "Remote version: %s", version.toString().c_str());  // TODO: This is abusing the SemVer::toString() method causing alot of string copies, fix this
     }
 
-    if (version.toString() == OPENSHOCK_FW_VERSION) {  // TODO: This is abusing the SemVer::toString() method causing alot of string copies, fix this
+    std::string versionStr = version.toString();  // TODO: This is abusing the SemVer::toString() method causing alot of string copies, fix this
+
+    if (versionStr == OPENSHOCK_FW_VERSION ""sv) {
       OS_LOGI(TAG, "Requested version is already installed");
       continue;
     }
 
+    OS_LOGD(TAG, "Updating to version: %.*s", versionStr.length(), versionStr.data());
+
     // Generate random int32_t for this update.
     int32_t updateId = static_cast<int32_t>(esp_random());
     if (!Config::SetOtaUpdateId(updateId)) {
@@ -369,24 +369,24 @@ static void _otaUpdateTask(void* arg)
 
     // Print release.
     OS_LOGD(TAG, "Firmware release:");
-    OS_LOGD(TAG, "  Version:                %s", version.toString().c_str());  // TODO: This is abusing the SemVer::toString() method causing alot of string copies, fix this
-    OS_LOGD(TAG, "  App binary URL:         %s", release.appBinaryUrl.c_str());
+    OS_LOGD(TAG, "  Version:                %.*s", versionStr.length(), versionStr.data());
+    OS_LOGD(TAG, "  App binary URL:         %.*s", release.appBinaryUrl.length(), release.appBinaryUrl.data());
     OS_LOGD(TAG, "  App binary hash:        %s", HexUtils::ToHex<32>(release.appBinaryHash).data());
-    OS_LOGD(TAG, "  Filesystem binary URL:  %s", release.filesystemBinaryUrl.c_str());
+    OS_LOGD(TAG, "  Filesystem binary URL:  %.*s", release.filesystemBinaryUrl.length(), release.filesystemBinaryUrl.data());
     OS_LOGD(TAG, "  Filesystem binary hash: %s", HexUtils::ToHex<32>(release.filesystemBinaryHash).data());
 
     // Get available app update partition.
     const esp_partition_t* appPartition = esp_ota_get_next_update_partition(nullptr);
     if (appPartition == nullptr) {
-      OS_LOGE(TAG, "Failed to get app update partition");  // TODO: Send error message to server
+      OS_LOGE(TAG, "Failed to get app update partition");
       _sendFailureMessage("Failed to get app update partition"sv);
       continue;
     }
 
     // Get filesystem partition.
     const esp_partition_t* filesystemPartition = esp_partition_find_first(ESP_PARTITION_TYPE_DATA, ESP_PARTITION_SUBTYPE_DATA_SPIFFS, "static0");
     if (filesystemPartition == nullptr) {
-      OS_LOGE(TAG, "Failed to find filesystem partition");  // TODO: Send error message to server
+      OS_LOGE(TAG, "Failed to find filesystem partition");
       _sendFailureMessage("Failed to find filesystem partition"sv);
       continue;
     }
@@ -583,7 +583,7 @@ bool OtaUpdateManager::TryGetFirmwareBoards(const OpenShock::SemVer& version, st
 
 static bool _tryParseIntoHash(std::string_view hash, uint8_t (&hashBytes)[32])
 {
-  if (!HexUtils::TryParseHex(hash.data(), hash.size(), hashBytes, 32) != 16) {
+  if (HexUtils::TryParseHex(hash.data(), hash.size(), hashBytes, 32) != 32) {
     OS_LOGE(TAG, "Failed to parse hash: %.*s", hash.size(), hash.data());
     return false;
   }

src/SemVer.cpp

@@ -245,19 +245,19 @@ std::string SemVer::toString() const
   str.reserve(length);
 
   Convert::FromUint16(major, str);
-  str += '.';
+  str.push_back('.');
   Convert::FromUint16(minor, str);
-  str += '.';
+  str.push_back('.');
   Convert::FromUint16(patch, str);
 
   if (!prerelease.empty()) {
-    str += '-';
-    str.append(prerelease.c_str(), prerelease.length());
+    str.push_back('-');
+    str.append(prerelease);
   }
 
   if (!build.empty()) {
-    str += '+';
-    str.append(build.c_str(), build.length());
+    str.push_back('+');
+    str.append(build);
   }
 
   return str;
@@ -355,7 +355,7 @@ bool OpenShock::TryParseSemVer(std::string_view semverStr, SemVer& semver)
   if (!restStr.empty()) {
     if (plusIdx != std::string_view::npos) {
       semver.build = restStr.substr((plusIdx - patchStr.length()) + 1);
-      patchStr.remove_suffix(semver.build.length() + 1);
+      restStr.remove_suffix(semver.build.length() + 1);
 
       if (!semver.build.empty() && !_semverIsBuild(semver.build)) {
         OS_LOGE(TAG, "Invalid build: %s", semver.build.c_str());
@@ -364,7 +364,7 @@ bool OpenShock::TryParseSemVer(std::string_view semverStr, SemVer& semver)
     }
 
     if (dashIdx != std::string_view::npos) {
-      semver.prerelease = patchStr.substr(1);
+      semver.prerelease = restStr.substr(1);
 
       if (!semver.prerelease.empty() && !_semverIsPrerelease(semver.prerelease)) {
         OS_LOGE(TAG, "Invalid prerelease: %s", semver.prerelease.c_str());

src/http/HTTPRequestManager.cpp

@@ -368,7 +368,7 @@ HTTP::Response<std::size_t> _doGetStream(
   int64_t begin = OpenShock::millis();
   if (!client.begin(OpenShock::StringToArduinoString(url))) {
     OS_LOGE(TAG, "Failed to begin HTTP request");
-    return {HTTP::RequestResult::RequestFailed, 0};
+    return {HTTP::RequestResult::RequestFailed, 0, 0};
   }
 
   for (auto& header : headers) {
@@ -434,7 +434,7 @@ HTTP::Response<std::size_t> _doGetStream(
   WiFiClient* stream = client.getStreamPtr();
   if (stream == nullptr) {
     OS_LOGE(TAG, "Failed to get stream");
-    return {HTTP::RequestResult::RequestFailed, 0};
+    return {HTTP::RequestResult::RequestFailed, 0, 0};
   }
 
   StreamReaderResult result;

src/radio/RFTransmitter.cpp

@@ -159,6 +159,7 @@ void RFTransmitter::TransmitTask()
         auto it = std::find_if(sequences.begin(), sequences.end(), [&cmd](const sequence_t& seq) { return seq.encoder.shockerId() == cmd.shockerId; });
         if (it != sequences.end()) {
           it->encoder.fillSequence(cmd.type, cmd.intensity);
+          it->until = cmd.until;
           continue;
         }
       }

src/serialization/JsonSerial.cpp

@@ -53,7 +53,7 @@ bool JsonSerial::ParseShockerCommand(const cJSON* root, JsonSerial::ShockerComma
     OS_LOGE(TAG, "value at 'type' is not a string");
     return false;
   }
-  ShockerCommandType commandType;
+  ShockerCommandType commandType = ShockerCommandType::Stop;
   if (!ShockerCommandTypeFromString(command->valuestring, commandType)) {
     OS_LOGE(TAG, "value at 'type' is not a valid shocker command (shock, vibrate, sound, light, stop)");
     return false;