Format fixes in light module (#1874)

* Light: proper printf format for HSV, use lround

* core: warn about map() and 2.3.0, type-strict constrain()

* use generic format

* value already has brightness

code/espurna/config/general.h

@@ -1112,15 +1112,18 @@
 #define LIGHT_SAVE_DELAY        5           // Persist color after 5 seconds to avoid wearing out
 #endif
 
+#ifndef LIGHT_MIN_PWM
+#define LIGHT_MIN_PWM           0
+#endif
 
 #ifndef LIGHT_MAX_PWM
 
 #if LIGHT_PROVIDER == LIGHT_PROVIDER_MY92XX
 #define LIGHT_MAX_PWM           255
-#endif
-
-#if LIGHT_PROVIDER == LIGHT_PROVIDER_DIMMER
+#elif LIGHT_PROVIDER == LIGHT_PROVIDER_DIMMER
 #define LIGHT_MAX_PWM           10000        // 10000 * 200ns => 2 kHz
+#else
+#define LIGHT_MAX_PWM           0
 #endif
 
 #endif // LIGHT_MAX_PWM
@@ -1129,14 +1132,25 @@
 #define LIGHT_LIMIT_PWM         LIGHT_MAX_PWM   // Limit PWM to this value (prevent 100% power)
 #endif
 
+#ifndef LIGHT_MIN_VALUE
+#define LIGHT_MIN_VALUE         0           // Minimum light value
+#endif
+
 #ifndef LIGHT_MAX_VALUE
 #define LIGHT_MAX_VALUE         255         // Maximum light value
 #endif
 
+#ifndef LIGHT_MIN_BRIGHTNESS
+#define LIGHT_MIN_BRIGHTNESS    0           // Minimum brightness value
+#endif
+
 #ifndef LIGHT_MAX_BRIGHTNESS
-#define LIGHT_MAX_BRIGHTNESS    255         // Maximun brightness value
+#define LIGHT_MAX_BRIGHTNESS    255         // Maximum brightness value
 #endif
 
+#define LIGHT_MIN_KELVIN        1000
+#define LIGHT_MAX_KELVIN        40000
+
 #define LIGHT_MIN_MIREDS        153      // Default to the Philips Hue value that HA also use.
 #define LIGHT_MAX_MIREDS        500      // https://developers.meethue.com/documentation/core-concepts
 

code/espurna/config/prototypes.h

@@ -362,6 +362,17 @@ using thermostat_callback_f = std::function<void(bool)>;
 // -----------------------------------------------------------------------------
 #include "rtcmem.h"
 
+// -----------------------------------------------------------------------------
+// Warn about broken Arduino functions
+// -----------------------------------------------------------------------------
+
+// Division by zero bug
+// https://github.com/esp8266/Arduino/pull/2397
+// https://github.com/esp8266/Arduino/pull/2408
+#if defined(ARDUINO_ESP8266_RELEASE_2_3_0)
+long  __attribute__((deprecated("Please avoid using map() with Core 2.3.0"))) map(long x, long in_min, long in_max, long out_min, long out_max);
+#endif
+
 // -----------------------------------------------------------------------------
 // std::make_unique backport for C++11
 // -----------------------------------------------------------------------------

code/espurna/domoticz.ino

@@ -48,6 +48,8 @@ void _domoticzStatus(unsigned char id, bool status) {
 
 #if LIGHT_PROVIDER != LIGHT_PROVIDER_NONE
 
+#include "light.h"
+
 void _domoticzLight(unsigned int idx, const JsonObject& root) {
 
     if (!lightHasColor()) return;
@@ -74,8 +76,8 @@ void _domoticzLight(unsigned int idx, const JsonObject& root) {
             lightChannel(4, color["cw"]);
         }
 
-        // domoticz uses 100 as maximum value while we're using LIGHT_MAX_BRIGHTNESS
-        unsigned int brightness = (root["Level"].as<uint8_t>() / 100.0) * LIGHT_MAX_BRIGHTNESS;
+        // domoticz uses 100 as maximum value while we're using Light::BRIGHTNESS_MAX (unsigned char)
+        unsigned char brightness = (root["Level"].as<uint8_t>() / 100.0) * Light::BRIGHTNESS_MAX;
         lightBrightness(brightness);
 
         DEBUG_MSG_P(PSTR("[DOMOTICZ] Received rgb:%u,%u,%u ww:%u,cw:%u brightness:%u for IDX %u\n"),

code/espurna/light.h

@@ -0,0 +1,26 @@
+// -----------------------------------------------------------------------------
+// Light
+// -----------------------------------------------------------------------------
+
+#pragma once
+
+namespace Light {
+    constexpr const unsigned char VALUE_MIN = LIGHT_MIN_VALUE;
+    constexpr const unsigned char VALUE_MAX = LIGHT_MAX_VALUE;
+
+    constexpr const unsigned int BRIGHTNESS_MIN = LIGHT_MIN_BRIGHTNESS;
+    constexpr const unsigned int BRIGHTNESS_MAX = LIGHT_MAX_BRIGHTNESS;
+
+    // Default to the Philips Hue value that HA also use.
+    // https://developers.meethue.com/documentation/core-concepts
+    constexpr const unsigned int MIREDS_MIN = LIGHT_MIN_MIREDS;
+    constexpr const unsigned int MIREDS_MAX = LIGHT_MAX_MIREDS;
+
+    constexpr const unsigned int KELVIN_MIN = LIGHT_MIN_KELVIN;
+    constexpr const unsigned int KELVIN_MAX = LIGHT_MAX_KELVIN;
+
+    constexpr const unsigned int PWM_MIN = LIGHT_MIN_PWM;
+    constexpr const unsigned int PWM_MAX = LIGHT_MAX_PWM;
+    constexpr const unsigned int PWM_LIMIT = LIGHT_LIMIT_PWM;
+}
+

code/espurna/light.ino

@@ -8,6 +8,8 @@ Copyright (C) 2016-2019 by Xose Pérez <xose dot perez at gmail dot com>
 
 #if LIGHT_PROVIDER != LIGHT_PROVIDER_NONE
 
+#include "light.h"
+
 #include <Ticker.h>
 #include <ArduinoJson.h>
 #include <vector>
@@ -48,8 +50,8 @@ bool _light_use_white = false;
 bool _light_use_cct = false;
 bool _light_use_gamma = false;
 unsigned long _light_steps_left = 1;
-unsigned char _light_brightness = LIGHT_MAX_BRIGHTNESS;
-unsigned int _light_mireds = round((LIGHT_COLDWHITE_MIRED+LIGHT_WARMWHITE_MIRED)/2);
+unsigned char _light_brightness = Light::BRIGHTNESS_MAX;
+unsigned int _light_mireds = lround((LIGHT_COLDWHITE_MIRED+LIGHT_WARMWHITE_MIRED)/2);
 
 using light_brightness_func_t = void();
 light_brightness_func_t* _light_brightness_func = nullptr;
@@ -89,26 +91,26 @@ const unsigned char _light_gamma_table[] PROGMEM = {
     191, 193, 195, 197, 199, 201, 203, 205, 207, 209, 211, 213, 215, 217, 219, 221,
     223, 225, 227, 229, 231, 233, 235, 238, 240, 242, 244, 246, 248, 251, 253, 255
 };
-static_assert(LIGHT_MAX_VALUE <= sizeof(_light_gamma_table), "Out-of-bounds array access");
+static_assert(Light::VALUE_MAX <= sizeof(_light_gamma_table), "Out-of-bounds array access");
 
 // -----------------------------------------------------------------------------
 // UTILS
 // -----------------------------------------------------------------------------
 
 void _setRGBInputValue(unsigned char red, unsigned char green, unsigned char blue) {
-    _light_channel[0].inputValue = constrain(red, 0, LIGHT_MAX_VALUE);
-    _light_channel[1].inputValue = constrain(green, 0, LIGHT_MAX_VALUE);;
-    _light_channel[2].inputValue = constrain(blue, 0, LIGHT_MAX_VALUE);;
+    _light_channel[0].inputValue = constrain(red, Light::VALUE_MIN, Light::VALUE_MAX);
+    _light_channel[1].inputValue = constrain(green, Light::VALUE_MIN, Light::VALUE_MAX);
+    _light_channel[2].inputValue = constrain(blue, Light::VALUE_MIN, Light::VALUE_MAX);
 }
 
 void _setCCTInputValue(unsigned char warm, unsigned char cold) {
-    _light_channel[0].inputValue = constrain(warm, 0, LIGHT_MAX_VALUE);
-    _light_channel[1].inputValue = constrain(cold, 0, LIGHT_MAX_VALUE);
+    _light_channel[0].inputValue = constrain(warm, Light::VALUE_MIN, Light::VALUE_MAX);
+    _light_channel[1].inputValue = constrain(cold, Light::VALUE_MIN, Light::VALUE_MAX);
 }
 
 void _lightApplyBrightness(unsigned char channels = lightChannels()) {
 
-    double brightness = static_cast<double>(_light_brightness) / static_cast<double>(LIGHT_MAX_BRIGHTNESS);
+    double brightness = static_cast<double>(_light_brightness) / static_cast<double>(Light::BRIGHTNESS_MAX);
 
     channels = std::min(channels, lightChannels());
 
@@ -121,7 +123,7 @@ void _lightApplyBrightness(unsigned char channels = lightChannels()) {
 
 void _lightApplyBrightnessColor() {
 
-    double brightness = (double) _light_brightness / LIGHT_MAX_BRIGHTNESS;
+    double brightness = static_cast<double>(_light_brightness) / static_cast<double>(Light::BRIGHTNESS_MAX);
 
     // Substract the common part from RGB channels and add it to white channel. So [250,150,50] -> [200,100,0,50]
     unsigned char white = std::min(_light_channel[0].inputValue, std::min(_light_channel[1].inputValue, _light_channel[2].inputValue));
@@ -137,11 +139,11 @@ void _lightApplyBrightnessColor() {
 
         // set cold white
         _light_channel[3].inputValue = 0;
-        _light_channel[3].value = round(((double) 1.0 - miredFactor) * white);
+        _light_channel[3].value = lround(((double) 1.0 - miredFactor) * white);
 
         // set warm white
         _light_channel[4].inputValue = 0;
-        _light_channel[4].value = round(miredFactor * white);
+        _light_channel[4].value = lround(miredFactor * white);
     } else {
         _light_channel[3].inputValue = 0;
         _light_channel[3].value = white;
@@ -158,12 +160,12 @@ void _lightApplyBrightnessColor() {
 
     double factor = (max_out > 0) ? (double) (max_in / max_out) : 0;
     for (unsigned char i=0; i < channelSize; i++) {
-        _light_channel[i].value = round((double) _light_channel[i].value * factor * brightness);
+        _light_channel[i].value = lround((double) _light_channel[i].value * factor * brightness);
     }
 
     // Scale white channel to match brightness
     for (unsigned char i=3; i < channelSize; i++) {
-        _light_channel[i].value = constrain(_light_channel[i].value * LIGHT_WHITE_FACTOR, 0, LIGHT_MAX_BRIGHTNESS);
+        _light_channel[i].value = constrain(static_cast<unsigned int>(_light_channel[i].value * LIGHT_WHITE_FACTOR), Light::BRIGHTNESS_MIN, Light::BRIGHTNESS_MAX);
     }
 
     // For the rest of channels, don't apply brightness, it is already in the inputValue
@@ -197,7 +199,7 @@ String lightDesc(unsigned char id) {
 void _fromLong(unsigned long value, bool brightness) {
     if (brightness) {
         _setRGBInputValue((value >> 24) & 0xFF, (value >> 16) & 0xFF, (value >> 8) & 0xFF);
-        _light_brightness = (value & 0xFF) * LIGHT_MAX_BRIGHTNESS / 255;
+        _light_brightness = (value & 0xFF) * Light::BRIGHTNESS_MAX / 255;
     } else {
         _setRGBInputValue((value >> 16) & 0xFF, (value >> 8) & 0xFF, (value) & 0xFF);
     }
@@ -279,10 +281,10 @@ void _fromHSV(const char * hsv) {
     double f = (h - floor(h));
     double s = (double) value[1] / 100.0;
 
-    _light_brightness = round((double) value[2] * 2.55); // (255/100)
-    unsigned char p = round(255 * (1.0 - s));
-    unsigned char q = round(255 * (1.0 - s * f));
-    unsigned char t = round(255 * (1.0 - s * (1.0 - f)));
+    _light_brightness = lround((double) value[2] * 2.55); // (255/100)
+    unsigned char p = lround(255 * (1.0 - s));
+    unsigned char q = lround(255 * (1.0 - s * f));
+    unsigned char t = lround(255 * (1.0 - s * (1.0 - f)));
 
     switch (int(h)) {
         case 0:
@@ -316,36 +318,36 @@ void _fromKelvin(unsigned long kelvin) {
     if (!_light_has_color) {
 
       if(!_light_use_cct) return;
-      
-      _light_mireds = constrain(round(1000000UL / kelvin), LIGHT_MIN_MIREDS, LIGHT_MAX_MIREDS);
-      
+
+      _light_mireds = constrain(static_cast<unsigned int>(lround(1000000UL / kelvin)), Light::MIREDS_MIN, Light::MIREDS_MAX);
+
       // This change the range from 153-500 to 0-347 so we get a value between 0 and 1 in the end.
       double factor = ((double) _light_mireds - (double) LIGHT_COLDWHITE_MIRED)/((double) LIGHT_WARMWHITE_MIRED - (double) LIGHT_COLDWHITE_MIRED);
-      unsigned char warm = round(factor * LIGHT_MAX_VALUE);
-      unsigned char cold = round(((double) 1.0 - factor) * LIGHT_MAX_VALUE);
+      unsigned char warm = lround(factor * Light::VALUE_MAX);
+      unsigned char cold = lround(((double) 1.0 - factor) * Light::VALUE_MAX);
 
       _setCCTInputValue(warm, cold);
-      
+
       return;
     }
 
-    _light_mireds = constrain(round(1000000UL / kelvin), LIGHT_MIN_MIREDS, LIGHT_MAX_MIREDS);
+    _light_mireds = constrain(static_cast<unsigned int>(lround(1000000UL / kelvin)), Light::MIREDS_MIN, Light::MIREDS_MAX);
 
     if (_light_use_cct) {
-      _setRGBInputValue(LIGHT_MAX_VALUE, LIGHT_MAX_VALUE, LIGHT_MAX_VALUE);
+      _setRGBInputValue(Light::VALUE_MAX, Light::VALUE_MAX, Light::VALUE_MAX);
       return;
     }
 
     // Calculate colors
     kelvin /= 100;
     unsigned int red = (kelvin <= 66)
-        ? LIGHT_MAX_VALUE
+        ? Light::VALUE_MAX
         : 329.698727446 * fs_pow((double) (kelvin - 60), -0.1332047592);
     unsigned int green = (kelvin <= 66)
         ? 99.4708025861 * fs_log(kelvin) - 161.1195681661
         : 288.1221695283 * fs_pow((double) kelvin, -0.0755148492);
     unsigned int blue = (kelvin >= 66)
-        ? LIGHT_MAX_VALUE
+        ? Light::VALUE_MAX
         : ((kelvin <= 19)
             ? 0
             : 138.5177312231 * fs_log(kelvin - 10) - 305.0447927307);
@@ -356,7 +358,7 @@ void _fromKelvin(unsigned long kelvin) {
 
 // Color temperature is measured in mireds (kelvin = 1e6/mired)
 void _fromMireds(unsigned long mireds) {
-    unsigned long kelvin = constrain(1000000UL / mireds, 1000, 40000);
+    unsigned long kelvin = constrain(static_cast<unsigned int>(1000000UL / mireds), Light::KELVIN_MIN, Light::KELVIN_MAX);
     _fromKelvin(kelvin);
 }
 
@@ -382,11 +384,10 @@ void _toRGB(char * rgb, size_t len) {
 
 void _toHSV(char * hsv, size_t len, bool target) {
     double h, s, v;
-    double brightness = (double) _light_brightness / LIGHT_MAX_BRIGHTNESS;
 
-    double r = (double) ((target ? _light_channel[0].target : _light_channel[0].inputValue) * brightness) / 255.0;
-    double g = (double) ((target ? _light_channel[1].target : _light_channel[1].inputValue) * brightness) / 255.0;
-    double b = (double) ((target ? _light_channel[2].target : _light_channel[2].inputValue) * brightness) / 255.0;
+    double r = static_cast<double>(target ? _light_channel[0].target : _light_channel[0].value);
+    double g = static_cast<double>(target ? _light_channel[1].target : _light_channel[1].value);
+    double b = static_cast<double>(target ? _light_channel[2].target : _light_channel[2].value);
 
     double min = std::min(r, std::min(g, b));
     double max = std::max(r, std::max(g, b));
@@ -414,7 +415,11 @@ void _toHSV(char * hsv, size_t len, bool target) {
     }
 
     // String
-    snprintf_P(hsv, len, PSTR("%d,%d,%d"), round(h), round(s), round(v));
+    snprintf(hsv, len, "%d,%d,%d",
+        static_cast<int>(lround(h)),
+        static_cast<int>(lround(s)),
+        static_cast<int>(lround(v))
+    );
 }
 
 void _toHSV(char * hsv, size_t len) {
@@ -422,13 +427,13 @@ void _toHSV(char * hsv, size_t len) {
 }
 
 void _toLong(char * color, size_t len, bool target) {
-    
+
     if (!_light_has_color) return;
 
-    snprintf_P(color, len, PSTR("%d,%d,%d"),
-        (int) (target ? _light_channel[0].target : _light_channel[0].inputValue),
-        (int) (target ? _light_channel[1].target : _light_channel[1].inputValue),
-        (int) (target ? _light_channel[2].target : _light_channel[2].inputValue)
+    snprintf_P(color, len, PSTR("%u,%u,%u"),
+        (target ? _light_channel[0].target : _light_channel[0].inputValue),
+        (target ? _light_channel[1].target : _light_channel[1].inputValue),
+        (target ? _light_channel[2].target : _light_channel[2].inputValue)
     );
 
 }
@@ -439,7 +444,7 @@ void _toLong(char * color, size_t len) {
 
 void _toCSV(char * buffer, size_t len, bool applyBrightness, bool target) {
     char num[10];
-    float b = applyBrightness ? (float) _light_brightness / LIGHT_MAX_BRIGHTNESS : 1;
+    float b = applyBrightness ? (float) _light_brightness / Light::BRIGHTNESS_MAX : 1;
     for (unsigned char i=0; i<_light_channel.size(); i++) {
         itoa((target ? _light_channel[i].target : _light_channel[i].inputValue) * b, num, 10);
         if (i>0) strncat(buffer, ",", len--);
@@ -452,14 +457,24 @@ void _toCSV(char * buffer, size_t len, bool applyBrightness) {
     _toCSV(buffer, len, applyBrightness, false);
 }
 
+// See cores/esp8266/WMath.cpp::map
+// Redefining as local method here to avoid breaking in unexpected ways in inputs like (0, 0, 0, 0, 1)
+template <typename T, typename T2> T _lightMap(T x, T in_min, T in_max, T2 out_min, T2 out_max) {
+    T divisor = (in_max - in_min);
+    if (divisor == 0){
+        return -1; //AVR returns -1, SAM returns 0
+    }
+    return (x - in_min) * (out_max - out_min) / divisor + out_min;
+}
+
 // -----------------------------------------------------------------------------
 // PROVIDER
 // -----------------------------------------------------------------------------
 
-unsigned int _toPWM(unsigned long value, bool gamma, bool reverse) {
-    value = constrain(value, 0, LIGHT_MAX_VALUE);
+unsigned int _toPWM(unsigned char value, bool gamma, bool reverse) {
+    value = constrain(value, Light::VALUE_MIN, Light::VALUE_MAX);
     if (gamma) value = pgm_read_byte(_light_gamma_table + value);
-    if (LIGHT_MAX_VALUE != LIGHT_LIMIT_PWM) value = map(value, 0, LIGHT_MAX_VALUE, 0, LIGHT_LIMIT_PWM);
+    if (Light::VALUE_MAX != Light::PWM_LIMIT) value = _lightMap(value, Light::VALUE_MIN, Light::VALUE_MAX, Light::PWM_MIN, Light::PWM_LIMIT);
     if (reverse) value = LIGHT_LIMIT_PWM - value;
     return value;
 }
@@ -572,7 +587,7 @@ void _lightRestoreSettings() {
     for (unsigned int i=0; i < _light_channel.size(); i++) {
         _light_channel[i].inputValue = getSetting("ch", i, i==0 ? 255 : 0).toInt();
     }
-    _light_brightness = getSetting("brightness", LIGHT_MAX_BRIGHTNESS).toInt();
+    _light_brightness = getSetting("brightness", Light::BRIGHTNESS_MAX).toInt();
     _light_mireds = getSetting("mireds", _light_mireds).toInt();
     lightUpdate(false, false);
 }
@@ -595,7 +610,7 @@ void _lightMQTTCallback(unsigned int type, const char * topic, const char * payl
             mqttSubscribe(MQTT_TOPIC_COLOR_HSV);
             mqttSubscribe(MQTT_TOPIC_TRANSITION);
         }
-        
+
         if (_light_has_color || _light_use_cct) {
             mqttSubscribe(MQTT_TOPIC_MIRED);
             mqttSubscribe(MQTT_TOPIC_KELVIN);
@@ -651,7 +666,7 @@ void _lightMQTTCallback(unsigned int type, const char * topic, const char * payl
 
         // Brightness
         if (t.equals(MQTT_TOPIC_BRIGHTNESS)) {
-            _light_brightness = constrain(atoi(payload), 0, LIGHT_MAX_BRIGHTNESS);
+            lightBrightness(atoi(payload));
             lightUpdate(true, mqttForward());
             return;
         }
@@ -873,9 +888,9 @@ unsigned int lightChannel(unsigned char id) {
     return 0;
 }
 
-void lightChannel(unsigned char id, int value) {
+void lightChannel(unsigned char id, unsigned char value) {
     if (id <= _light_channel.size()) {
-        _light_channel[id].inputValue = constrain(value, 0, LIGHT_MAX_VALUE);
+        _light_channel[id].inputValue = constrain(value, Light::VALUE_MIN, Light::VALUE_MAX);
     }
 }
 
@@ -887,15 +902,15 @@ unsigned int lightBrightness() {
     return _light_brightness;
 }
 
-void lightBrightness(int b) {
-    _light_brightness = constrain(b, 0, LIGHT_MAX_BRIGHTNESS);
+void lightBrightness(unsigned int brightness) {
+    _light_brightness = constrain(brightness, Light::BRIGHTNESS_MIN, Light::BRIGHTNESS_MAX);
 }
 
 void lightBrightnessStep(int steps) {
     lightBrightness(_light_brightness + steps * LIGHT_STEP);
 }
 
-unsigned long lightTransitionTime() {
+unsigned int lightTransitionTime() {
     if (_light_use_transitions) {
         return _light_transition_time;
     } else {
@@ -977,7 +992,7 @@ void _lightWebSocketOnAction(uint32_t client_id, const char * action, JsonObject
             }
         }
     }
-    
+
     if (_light_use_cct) {
       if (strcmp(action, "mireds") == 0) {
           _fromMireds(data["mireds"]);