Fix CodeRabbit issues:

- Avoid leaving segments permanently frozen; restore previous freeze state
- Fix negative modulo in local-time marker logic
- Handle invalid segments safely
- Clarify duplicate "N" table row
- Fix country normalizeLocation bug
- Don't include the API key in logging
- Tighten wording; avoid hardcoding OWM plan limits
- Fix rate-limit reset logic
- Harden rate-limit check and keep informative logging
- Add network timeout to avoid long blocking on bad links
- Check non-2xx status codes before parsing
- Right-size JSON buffer per target
- Fix potential buffer overrun in emitSeriesMDHM
- Use float instead of double for DataPoint.value
- Restore seg.freeze after rendering (and avoid freezing on zero-length)
- use PROGMEM to reduce RAM use
- Sanitize HSV inputs
- Clamp HSV values in conversion to RGB
- Clarify SAFETY_DELAY_MS type
- Clamp and wrap HSV inputs to prevent overflow/underflow artifacts
- Normalize wind direction to [0,360) before mapping to hue
- fixed compiler warning about shadowed variables
- delay initial checkhistory after fetch

Author: ksedgwic

usermods/usermod_v2_skystrip/FAQ.md

@@ -39,7 +39,7 @@ The mapping between wind direction and hue can be approximated as:
 | SW        | 90      | Lime   |
 | W         | 120     | Green  |
 | NW        | 180     | Cyan   |
-| N         | 240     | Blue   |
+| N         | 240     | Blue   | (wraps around)
 
 
 ## Temperature View (TV)

usermods/usermod_v2_skystrip/cloud_view.cpp

@@ -7,7 +7,7 @@
 #include <limits>
 
 static constexpr int16_t DEFAULT_SEG_ID = -1; // -1 means disabled
-const char CFG_SEG_ID[] = "SegmentId";
+const char CFG_SEG_ID[] PROGMEM = "SegmentId";
 
 static bool isDay(const SkyModel &m, time_t t) {
   const time_t MAXTT = std::numeric_limits<time_t>::max();
@@ -50,12 +50,12 @@ void CloudView::view(time_t now, SkyModel const &model, int16_t dbgPixelIndex) {
     return;
 
   Segment &seg = strip.getSegment((uint8_t)segId_);
-  seg.freeze = true;
   int start = seg.start;
   int end = seg.stop - 1;
   int len = end - start + 1;
-  if (len == 0)
+  if (len <= 0)
     return;
+  skystrip::util::FreezeGuard freezeGuard(seg);
 
   constexpr double kHorizonSec = 48.0 * 3600.0;
   const double step = (len > 1) ? (kHorizonSec / double(len - 1)) : 0.0;
@@ -73,9 +73,9 @@ void CloudView::view(time_t now, SkyModel const &model, int16_t dbgPixelIndex) {
   time_t sunriseTOD = 0;
   time_t sunsetTOD = 0;
   if (useSunrise)
-    sunriseTOD = (sunrise + offset) % DAY;
+    sunriseTOD = (((sunrise + offset) % DAY) + DAY) % DAY; // normalize to [0, DAY)
   if (useSunset)
-    sunsetTOD = (sunset + offset) % DAY;
+    sunsetTOD = (((sunset + offset) % DAY) + DAY) % DAY;   // normalize to [0, DAY)
 
   auto nearTOD = [&](time_t a, time_t b) {
     time_t diff = (a >= b) ? (a - b) : (b - a);
@@ -87,7 +87,7 @@ void CloudView::view(time_t now, SkyModel const &model, int16_t dbgPixelIndex) {
   auto isMarker = [&](time_t t) {
     if (!useSunrise && !useSunset)
       return false;
-    time_t tod = (t + offset) % DAY;
+    time_t tod = (((t + offset) % DAY) + DAY) % DAY; // normalize to [0, DAY)
     if (useSunrise && nearTOD(tod, sunriseTOD))
       return true;
     if (useSunset && nearTOD(tod, sunsetTOD))

usermods/usermod_v2_skystrip/delta_view.cpp

@@ -7,7 +7,7 @@
 #include "wled.h"
 
 static constexpr int16_t DEFAULT_SEG_ID = -1; // -1 means disabled
-const char CFG_SEG_ID[] = "SegmentId";
+const char CFG_SEG_ID[] PROGMEM = "SegmentId";
 
 struct Stop {
   double f;
@@ -74,12 +74,12 @@ void DeltaView::view(time_t now, SkyModel const &model, int16_t dbgPixelIndex) {
     return;
 
   Segment &seg = strip.getSegment((uint8_t)segId_);
-  seg.freeze = true;
   int start = seg.start;
   int end = seg.stop - 1;
   int len = end - start + 1;
-  if (len == 0)
+  if (len <= 0)
     return;
+  skystrip::util::FreezeGuard freezeGuard(seg);
 
   constexpr double kHorizonSec = 48.0 * 3600.0;
   const double step = (len > 1) ? (kHorizonSec / double(len - 1)) : 0.0;

usermods/usermod_v2_skystrip/open_weather_map_source.cpp

@@ -20,12 +20,12 @@ static constexpr unsigned DEFAULT_INTERVAL_SEC = 3600;	// 1 hour
 // - these are user visible in the webapp settings UI
 // - they are scoped to this module, don't need to be globally unique
 //
-const char CFG_API_BASE[] = "ApiBase";
-const char CFG_API_KEY[] = "ApiKey";
-const char CFG_LATITUDE[] = "Latitude";
-const char CFG_LONGITUDE[] = "Longitude";
-const char CFG_INTERVAL_SEC[] = "IntervalSec";
-const char CFG_LOCATION[] = "Location";
+const char CFG_API_BASE[] PROGMEM = "ApiBase";
+const char CFG_API_KEY[] PROGMEM = "ApiKey";
+const char CFG_LATITUDE[] PROGMEM = "Latitude";
+const char CFG_LONGITUDE[] PROGMEM = "Longitude";
+const char CFG_INTERVAL_SEC[] PROGMEM = "IntervalSec";
+const char CFG_LOCATION[] PROGMEM = "Location";
 
 // keep commas; encode spaces etc.
 static void urlEncode(const char* src, char* dst, size_t dstSize) {
@@ -46,6 +46,27 @@ static void urlEncode(const char* src, char* dst, size_t dstSize) {
   dst[di] = '\0';
 }
 
+// Redact the API key in a URL by replacing the value after "appid=" with '*'.
+static void redactApiKeyInUrl(const char* in, char* out, size_t outLen) {
+  if (!in || !out || outLen == 0) return;
+  const char* p = strstr(in, "appid=");
+  if (!p) {
+    strncpy(out, in, outLen);
+    out[outLen - 1] = '\0';
+    return;
+  }
+  size_t prefixLen = (size_t)(p - in) + 6; // include "appid="
+  if (prefixLen >= outLen) {
+    // Not enough space; best effort copy and terminate
+    strncpy(out, in, outLen);
+    out[outLen - 1] = '\0';
+    return;
+  }
+  memcpy(out, in, prefixLen);
+  out[prefixLen] = '*';
+  out[prefixLen + 1] = '\0';
+}
+
 // Normalize "Oakland, CA, USA" → "Oakland,CA,US" in-place
 static void normalizeLocation(char* q) {
   // trim spaces and commas
@@ -57,9 +78,8 @@ static void normalizeLocation(char* q) {
   *out = '\0';
   len = strlen(q);
   if (len >= 4 && strcasecmp(q + len - 4, ",USA") == 0) {
-    q[len - 2] = 'U';
-    q[len - 1] = 'S';
-    q[len] = '\0';
+    // Truncate the trailing 'A' so ",USA" → ",US" without corrupting chars
+    q[len - 1] = '\0';
   }
 }
 
@@ -200,11 +220,14 @@ std::unique_ptr<SkyModel> OpenWeatherMapSource::fetch(std::time_t now) {
     return nullptr;
 
   lastFetch_ = now;
+  lastHistFetch_ = now; // history fetches should wait
 
   // Fetch JSON
   char url[256];
   composeApiUrl(url, sizeof(url));
-  DEBUG_PRINTF("SkyStrip: %s::fetch URL: %s\n", name().c_str(), url);
+  char redacted[256];
+  redactApiKeyInUrl(url, redacted, sizeof(redacted));
+  DEBUG_PRINTF("SkyStrip: %s::fetch URL: %s\n", name().c_str(), redacted);
 
   auto doc = getJson(url);
   if (!doc) {
@@ -253,12 +276,12 @@ std::unique_ptr<SkyModel> OpenWeatherMapSource::fetch(std::time_t now) {
   model->sunset_ = sunset;
   for (JsonObject hour : hourly) {
     time_t dt    = hour["dt"].as<time_t>();
-    model->temperature_forecast.push_back({ dt, hour["temp"].as<double>() });
-    model->dew_point_forecast.push_back({ dt, hour["dew_point"].as<double>() });
-    model->wind_speed_forecast.push_back({ dt, hour["wind_speed"].as<double>() });
-    model->wind_dir_forecast.push_back({ dt, hour["wind_deg"].as<double>() });
-    model->wind_gust_forecast.push_back({ dt, hour["wind_gust"].as<double>() });
-    model->cloud_cover_forecast.push_back({ dt, hour["clouds"].as<double>() });
+    model->temperature_forecast.push_back({ dt, (float)hour["temp"].as<double>() });
+    model->dew_point_forecast.push_back({ dt, (float)hour["dew_point"].as<double>() });
+    model->wind_speed_forecast.push_back({ dt, (float)hour["wind_speed"].as<double>() });
+    model->wind_dir_forecast.push_back({ dt, (float)hour["wind_deg"].as<double>() });
+    model->wind_gust_forecast.push_back({ dt, (float)hour["wind_gust"].as<double>() });
+    model->cloud_cover_forecast.push_back({ dt, (float)hour["clouds"].as<double>() });
     JsonArray wArr = hour["weather"].as<JsonArray>();
     bool hasRain = false, hasSnow = false;
     if (hour.containsKey("rain")) {
@@ -278,10 +301,13 @@ std::unique_ptr<SkyModel> OpenWeatherMapSource::fetch(std::time_t now) {
         hasSnow = true;
     }
     int ptype = hasRain && hasSnow ? 3 : (hasSnow ? 2 : (hasRain ? 1 : 0));
-    model->precip_type_forecast.push_back({ dt, double(ptype) });
-    model->precip_prob_forecast.push_back({ dt, hour["pop"].as<double>() });
+    model->precip_type_forecast.push_back({ dt, (float)ptype });
+    model->precip_prob_forecast.push_back({ dt, (float)hour["pop"].as<double>() });
   }
 
+  // Stagger history fetch to avoid back-to-back GETs in same loop iteration
+  // and reduce risk of watchdog resets. Enforce at least 15s before history.
+  lastHistFetch_ = skystrip::util::time_now_utc();
   return model;
 }
 
@@ -298,7 +324,9 @@ std::unique_ptr<SkyModel> OpenWeatherMapSource::checkhistory(time_t now, std::ti
   snprintf(url, sizeof(url),
            "%s/data/3.0/onecall/timemachine?lat=%.6f&lon=%.6f&dt=%ld&units=imperial&appid=%s",
            apiBase_.c_str(), latitude_, longitude_, (long)fetchDt, apiKey_.c_str());
-  DEBUG_PRINTF("SkyStrip: %s::checkhistory URL: %s\n", name().c_str(), url);
+  char redacted[256];
+  redactApiKeyInUrl(url, redacted, sizeof(redacted));
+  DEBUG_PRINTF("SkyStrip: %s::checkhistory URL: %s\n", name().c_str(), redacted);
 
   auto doc = getJson(url);
   if (!doc) {
@@ -321,12 +349,12 @@ std::unique_ptr<SkyModel> OpenWeatherMapSource::checkhistory(time_t now, std::ti
   for (JsonObject hour : hourly) {
     time_t dt = hour["dt"].as<time_t>();
     if (dt >= oldestTstamp) continue;
-    model->temperature_forecast.push_back({ dt, hour["temp"].as<double>() });
-    model->dew_point_forecast.push_back({ dt, hour["dew_point"].as<double>() });
-    model->wind_speed_forecast.push_back({ dt, hour["wind_speed"].as<double>() });
-    model->wind_dir_forecast.push_back({ dt, hour["wind_deg"].as<double>() });
-    model->wind_gust_forecast.push_back({ dt, hour["wind_gust"].as<double>() });
-    model->cloud_cover_forecast.push_back({ dt, hour["clouds"].as<double>() });
+    model->temperature_forecast.push_back({ dt, (float)hour["temp"].as<double>() });
+    model->dew_point_forecast.push_back({ dt, (float)hour["dew_point"].as<double>() });
+    model->wind_speed_forecast.push_back({ dt, (float)hour["wind_speed"].as<double>() });
+    model->wind_dir_forecast.push_back({ dt, (float)hour["wind_deg"].as<double>() });
+    model->wind_gust_forecast.push_back({ dt, (float)hour["wind_gust"].as<double>() });
+    model->cloud_cover_forecast.push_back({ dt, (float)hour["clouds"].as<double>() });
     JsonArray wArr = hour["weather"].as<JsonArray>();
     bool hasRain = false, hasSnow = false;
     if (hour.containsKey("rain")) {
@@ -346,8 +374,8 @@ std::unique_ptr<SkyModel> OpenWeatherMapSource::checkhistory(time_t now, std::ti
         hasSnow = true;
     }
     int ptype = hasRain && hasSnow ? 3 : (hasSnow ? 2 : (hasRain ? 1 : 0));
-    model->precip_type_forecast.push_back({ dt, double(ptype) });
-    model->precip_prob_forecast.push_back({ dt, hour["pop"].as<double>() });
+    model->precip_type_forecast.push_back({ dt, (float)ptype });
+    model->precip_prob_forecast.push_back({ dt, (float)hour["pop"].as<double>() });
   }
 
   if (model->temperature_forecast.empty()) return nullptr;
@@ -384,7 +412,9 @@ bool OpenWeatherMapSource::geocodeOWM(std::string const & rawQuery,
   snprintf(url, sizeof(url),
            "%s/geo/1.0/direct?q=%s&limit=5&appid=%s",
            apiBase_.c_str(), enc, apiKey_.c_str());
-  DEBUG_PRINTF("SkyStrip: %s::geocodeOWM URL: %s\n", name().c_str(), url);
+  char redacted[512];
+  redactApiKeyInUrl(url, redacted, sizeof(redacted));
+  DEBUG_PRINTF("SkyStrip: %s::geocodeOWM URL: %s\n", name().c_str(), redacted);
 
   auto doc = getJson(url);
   resetRateLimit();	// we want to do a fetch immediately after ...

usermods/usermod_v2_skystrip/readme.md

@@ -10,10 +10,9 @@ Add `usermod_v2_skystrip` to `custom_usermods` in your PlatformIO environment.
 ## Configuration
 
 Acquire an API key from
-[OpenWeatherMap](https://openweathermap.org/api/one-call-3).  The skystrip
-module makes one API call per hour plus 24 calls when initially started
-up; it should remain comfortably under the free-tier limit of 1000 per
-day.
+[OpenWeatherMap](https://openweathermap.org/api/one-call-3). The SkyStrip
+module makes one API call per hour, plus up to 24 calls on first startup.
+This typically stays within free-tier limits, but check your current plan.
 
 Enter the latitude and longitude for the desired forecast. There are
 several ways to do this:

usermods/usermod_v2_skystrip/rest_json_client.cpp

@@ -8,16 +8,19 @@ RestJsonClient::RestJsonClient()
 }
 
 void RestJsonClient::resetRateLimit() {
-  // pretend we just made the last fetch RATE_LIMIT_MS ago
-  lastFetchMs_ = millis() - static_cast<unsigned long>(-static_cast<long>(RATE_LIMIT_MS));
+  // pretend we fetched RATE_LIMIT_MS ago (allow immediate next call)
+  lastFetchMs_ = millis() - RATE_LIMIT_MS;
 }
 
 DynamicJsonDocument* RestJsonClient::getJson(const char* url) {
   // enforce a basic rate limit to prevent runaway software from making bursts
   // of API calls (looks like DoS and get's our API key turned off ...)
   unsigned long now_ms = millis();
-  if (now_ms - lastFetchMs_ < RATE_LIMIT_MS) {
-    DEBUG_PRINTLN("SkyStrip: RestJsonClient::getJson: RATE LIMITED");
+  // compute elapsed using unsigned arithmetic to avoid signed underflow
+  unsigned long elapsed = now_ms - lastFetchMs_;
+  if (elapsed < RATE_LIMIT_MS) {
+    unsigned long remaining = RATE_LIMIT_MS - elapsed;
+    DEBUG_PRINTF("SkyStrip: RestJsonClient::getJson: RATE LIMITED (%lu ms remaining)\n", remaining);
     return nullptr;
   }
   lastFetchMs_ = now_ms;
@@ -42,9 +45,11 @@ DynamicJsonDocument* RestJsonClient::getJson(const char* url) {
   }
   DEBUG_PRINTF("SkyStrip: RestJsonClient::getJson: free heap before GET: %u\n", ESP.getFreeHeap());
   int code = http_.GET();
-  if (code <= 0) {
+  // Treat network errors (<=0) and non-2xx statuses as failures.
+  // Optionally consider 204 (No Content) as failure since there is no body to parse.
+  if (code <= 0 || code < 200 || code >= 300 || code == 204) {
     http_.end();
-    DEBUG_PRINTF("SkyStrip: RestJsonClient::getJson: http get error code: %d\n", code);
+    DEBUG_PRINTF("SkyStrip: RestJsonClient::getJson: HTTP error/status: %d\n", code);
     return nullptr;
   }
 

usermods/usermod_v2_skystrip/rest_json_client.h

@@ -25,8 +25,12 @@ class RestJsonClient {
 
 protected:
   static constexpr unsigned RATE_LIMIT_MS = 10u * 1000u; // 10 seconds
-  static constexpr size_t MAX_JSON_SIZE = 32 * 1024;      // 32kB fixed buffer
-
+#if defined(ARDUINO_ARCH_ESP8266)
+  static constexpr size_t MAX_JSON_SIZE = 16 * 1024;     // 16kB on 8266
+#else
+  static constexpr size_t MAX_JSON_SIZE = 32 * 1024;     // 32kB on ESP32
+#endif
+  
 private:
   HTTPClient http_;
   unsigned long lastFetchMs_;

usermods/usermod_v2_skystrip/skymodel.cpp

@@ -122,15 +122,25 @@ static inline void emitSeriesMDHM(Print &out, time_t now, const char *label,
 
   size_t i = 0;
   size_t off = 0;
+  const size_t cap = sizeof(line);
   for (const auto& dp : s) {
     if (i % 6 == 0) {
-      off = snprintf(line, sizeof(line), "SkyModel:");
+      int n = snprintf(line, cap, "SkyModel:");
+      off = (n < 0) ? 0u : ((size_t)n >= cap ? cap - 1 : (size_t)n);
     }
     skystrip::util::fmt_local(tb, sizeof(tb), dp.tstamp);
-    off += snprintf(line + off, sizeof(line) - off,
-                    " (%s, %6.2f)", tb, dp.value);
+    if (off < cap) {
+      size_t rem = cap - off;
+      int n = snprintf(line + off, rem, " (%s, %6.2f)", tb, dp.value);
+      if (n > 0) off += ((size_t)n >= rem ? rem - 1 : (size_t)n);
+    }
     if (i % 6 == 5 || i == s.size() - 1) {
-      if (i == s.size() - 1) off += snprintf(line + off, sizeof(line) - off, " ]");
+      if (i == s.size() - 1 && off < cap) {
+        size_t rem = cap - off;
+        int n = snprintf(line + off, rem, " ]");
+        if (n > 0) off += ((size_t)n >= rem ? rem - 1 : (size_t)n);
+      }
+      if (off >= cap) off = cap - 1; // ensure space for newline
       line[off++] = '\n';
       out.write((const uint8_t*)line, off);
     }

usermods/usermod_v2_skystrip/skymodel.h

@@ -10,7 +10,7 @@ class Print;
 
 struct DataPoint {
   time_t tstamp;
-  double value;
+  float value;
 };
 
 class SkyModel {

usermods/usermod_v2_skystrip/temperature_view.cpp

@@ -10,7 +10,7 @@ static constexpr int16_t DEFAULT_SEG_ID = -1; // -1 means disabled
 // - these are user visible in the webapp settings UI
 // - they are scoped to this module, don't need to be globally unique
 //
-const char CFG_SEG_ID[] = "SegmentId";
+const char CFG_SEG_ID[] PROGMEM = "SegmentId";
 
 // Map dew-point depression (°F) -> saturation multiplier.
 // dd<=2°F  -> minSat ; dd>=25°F -> 1.0 ; smooth in between.
@@ -77,12 +77,12 @@ void TemperatureView::view(time_t now, SkyModel const &model,
   if (segId_ < 0 || segId_ >= strip.getMaxSegments())
     return;
   Segment &seg = strip.getSegment((uint8_t)segId_);
-  seg.freeze = true;
   int start = seg.start;
   int end = seg.stop - 1; // inclusive
   int len = end - start + 1;
-  if (len == 0)
+  if (len <= 0)
     return;
+  skystrip::util::FreezeGuard freezeGuard(seg);
 
   constexpr double kHorizonSec = 48.0 * 3600.0;
   const double step = (len > 1) ? (kHorizonSec / double(len - 1)) : 0.0;
@@ -97,9 +97,7 @@ void TemperatureView::view(time_t now, SkyModel const &model,
       return 0.f;
 
     time_t local = t + tzOffset; // convert to local seconds
-    time_t s = local % DAY;      // seconds since local midnight
-    if (s < 0)
-      s += DAY;
+    time_t s = (((local % DAY) + DAY) % DAY); // seconds since local midnight (normalized)
 
     // Seconds-of-day for markers + per-marker width multipliers.
     static const time_t kMarkers[] = {0 * 3600,  3 * 3600,  6 * 3600,