optimize LED and fix some edge cases

src/core/led.cpp

@@ -37,6 +37,10 @@ constexpr uint16_t T0L = 9; // 900 ns
 constexpr uint16_t T1H = 8; // 800 ns
 constexpr uint16_t T1L = 6; // 600 ns
 
+// rmt_data_t.val layout: duration0:15, level0:1, duration1:15, level1:1 (high then low)
+constexpr uint32_t SYMBOL_0 = T0H | (1UL << 15) | (static_cast<uint32_t>(T0L) << 16);
+constexpr uint32_t SYMBOL_1 = T1H | (1UL << 15) | (static_cast<uint32_t>(T1L) << 16);
+
 constexpr uint32_t RMT_FREQ_HZ = 10000000; // 10 MHz
 constexpr uint32_t RESET_US    = 300;
 constexpr uint8_t  NO_PIN      = 0xFF;
@@ -61,8 +65,14 @@ Channel * init(uint8_t pin) {
     }
 
     if (!free_slot) {
-        // all slots in use: release the oldest one
+        // all slots in use: release the least recently written one
-        free_slot = &channels[0];
+        const uint32_t now = micros();
+        free_slot          = &channels[0];
+        for (auto & ch : channels) {
+            if (now - ch.last_tx_us > now - free_slot->last_tx_us) {
+                free_slot = &ch;
+            }
+        }
         end(free_slot->pin);
     }
 
@@ -92,11 +102,7 @@ bool write(uint8_t pin, uint8_t red, uint8_t green, uint8_t blue, Order order) {
 
     rmt_data_t frame[24];
     for (uint8_t i = 0; i < 24; i++) {
-        const bool one     = bytes[i >> 3] & (0x80 >> (i & 7));
+        frame[i].val = (bytes[i >> 3] & (0x80 >> (i & 7))) ? SYMBOL_1 : SYMBOL_0;
-        frame[i].level0    = 1;
-        frame[i].duration0 = one ? T1H : T0H;
-        frame[i].level1    = 0;
-        frame[i].duration1 = one ? T1L : T0L;
     }
 
     const bool ok  = rmtWrite(pin, frame, 24, RMT_WAIT_FOR_EVER);
@@ -124,10 +130,6 @@ void end(uint8_t) {
 
 #endif
 
-bool off(uint8_t pin) {
-    return write(pin, 0, 0, 0);
-}
-
 } // namespace rgb_led
 
 uuid::log::Logger LED::logger_{F_(led), uuid::log::Facility::KERN};
@@ -135,12 +137,21 @@ uuid::log::Logger LED::logger_{F_(led), uuid::log::Facility::KERN};
 // initialise the LED, fetching the settings from the WebSettingsService
 // set the LED to on or off when in normal operating mode
 void LED::init() {
+    // settings may have changed: turn off and release the previous pin
+    if (led_gpio_) {
+        set_led(Color::OFF);
+        if (led_type_) {
+            rgb_led::end(led_gpio_);
+        }
+    }
+
     // copy the application settings
     EMSESP::webSettingsService.read([&](WebSettings & settings) {
         led_gpio_ = settings.led_gpio;
         led_type_ = settings.led_type;
         hide_led_ = settings.hide_led;
     });
+    last_color_ = COLOR_UNSET;
 
     if (!led_gpio_) { // 0 means disabled
         LOG_INFO("LED disabled");
@@ -183,10 +194,6 @@ bool LED::loop(uint8_t healthcheck, bool button_busy) {
         // check the system health.
         // Set the sequence accordingly, only if the healthcheck is not 0 and has changed
         if (healthcheck != previous_healthcheck_) {
-            color_steps_[0] = Color::OFF;
-            color_steps_[1] = Color::OFF;
-            color_steps_[2] = Color::OFF;
-
             // if the healthcheck is 0, i.e. system is healthy, reset the LED
             if (healthcheck == 0) {
                 reset_led();
@@ -194,25 +201,12 @@ bool LED::loop(uint8_t healthcheck, bool button_busy) {
                 //  1 flash (blue) is the EMS bus is not connected
                 //  2 flashes (red, red) if the network (wifi or ethernet) is not connected
                 //  3 flashes (red, red, blue) is both the bus and the network are not connected
-                bool no_network = (healthcheck & System::HEALTHCHECK_NO_NETWORK) == System::HEALTHCHECK_NO_NETWORK;
+                const bool no_network = (healthcheck & System::HEALTHCHECK_NO_NETWORK) == System::HEALTHCHECK_NO_NETWORK;
-                bool no_bus     = (healthcheck & System::HEALTHCHECK_NO_BUS) == System::HEALTHCHECK_NO_BUS;
+                const bool no_bus     = (healthcheck & System::HEALTHCHECK_NO_BUS) == System::HEALTHCHECK_NO_BUS;
 
-                // set step 1
+                color_steps_[0] = no_network ? Color::RED : (no_bus ? Color::BLUE : Color::OFF);
-                if (no_network) {
+                color_steps_[1] = no_network ? Color::RED : Color::OFF;
-                    color_steps_[0] = Color::RED; // red, no network
+                color_steps_[2] = (no_network && no_bus) ? Color::BLUE : Color::OFF;
-                } else if (no_bus) {
-                    color_steps_[0] = Color::BLUE; // blue, no bus
-                }
-
-                // set step 2
-                if (no_network) {
-                    color_steps_[1] = Color::RED; // red, no network
-                }
-
-                // set step 3
-                if (no_network && no_bus) {
-                    color_steps_[2] = Color::BLUE; // blue, no network and no bus
-                }
             }
 
             previous_healthcheck_ = healthcheck; // must be set after reset_led(), which invalidates it
@@ -235,9 +229,7 @@ bool LED::loop(uint8_t healthcheck, bool button_busy) {
 void LED::reset_led() {
     is_user_led_blink_ = false;
     set_led(hide_led_ ? Color::OFF : Color::GREEN); // Green
-    color_steps_[0] = Color::OFF;
+    color_steps_[0] = color_steps_[1] = color_steps_[2] = Color::OFF;
-    color_steps_[1] = Color::OFF;
-    color_steps_[2] = Color::OFF;
 
     // invalidate the last health state so the sequence is rebuilt on the next check,
     // needed because the color steps above have been cleared
@@ -272,9 +264,10 @@ void LED::led_fast_flash() {
 // set LED on/off or RGB color
 // ignores whether the LED is hidden or not (if hide_led_ is set)
 void LED::set_led(Color color) {
-    if (!led_gpio_) {
+    if (!led_gpio_ || color == last_color_) {
         return;
     }
+    last_color_ = color;
 
     // RGB lookup table indexed by Color enum (must match enum order in led.h)
     static constexpr uint8_t B              = RGB_LED_BRIGHTNESS;
@@ -290,28 +283,26 @@ void LED::set_led(Color color) {
         {0, B, B}, // CYAN
         {H, 0, H}  // PINK
     };
+    static_assert(sizeof(rgb_table) / sizeof(rgb_table[0]) == Color::PINK + 1, "rgb_table must match the Color enum");
 
-    const uint8_t color_idx = static_cast<uint8_t>(color);
+    const uint8_t * rgb = rgb_table[color];
-    const uint8_t idx       = (color_idx < sizeof(rgb_table) / sizeof(rgb_table[0])) ? color_idx : static_cast<uint8_t>(Color::OFF);
-    const uint8_t red       = rgb_table[idx][0];
-    const uint8_t green     = rgb_table[idx][1];
-    const uint8_t blue      = rgb_table[idx][2];
 
     if (led_type_) {
-        rgb_led::write(led_gpio_, red, green, blue);
+        rgb_led::write(led_gpio_, rgb[0], rgb[1], rgb[2]);
     } else {
-        digitalWrite(led_gpio_, (red == 0 && green == 0 && blue == 0) || color == Color::OFF ? !LED_ON : LED_ON);
+        digitalWrite(led_gpio_, color == Color::OFF ? !LED_ON : LED_ON);
     }
 }
 
 // set LED custom routine
 // For example: /api/system/led?data=red:blink1
 // For older non-RGB models, the colour would default to just being on.
-bool LED::set_custom_led_routine(std::string color, std::string pattern) {
+bool LED::set_custom_led_routine(const std::string & color, const std::string & pattern) {
     static constexpr struct {
         const char * name;
         Color        value;
     } color_map[] = {
+        {"", Color::OFF},
         {"off", Color::OFF},
         {"on", Color::ON},
         {"white", Color::ON},
@@ -337,35 +328,33 @@ bool LED::set_custom_led_routine(std::string color, std::string pattern) {
         return false;
     }
 
-    // reset the color steps
+    // build the steps locally so an unrecognized pattern leaves the current sequence untouched
-    color_steps_[0] = Color::OFF;
+    Color steps[3] = {Color::OFF, Color::OFF, Color::OFF};
-    color_steps_[1] = Color::OFF;
-    color_steps_[2] = Color::OFF;
 
     // blink patterns
     if (pattern == "blink1") {
-        color_steps_[0] = color_type;
+        steps[0] = color_type;
     } else if (pattern == "blink2") {
-        color_steps_[0] = color_type;
+        steps[0] = steps[1] = color_type;
-        color_steps_[1] = color_type;
     } else if (pattern == "blink3") {
-        color_steps_[0] = color_type;
+        steps[0] = steps[1] = steps[2] = color_type;
-        color_steps_[1] = color_type;
-        color_steps_[2] = color_type;
 
         // special patterns, ignores the user color
     } else if (pattern == "rgb") {
-        color_steps_[0] = Color::RED;
+        steps[0] = Color::RED;
-        color_steps_[1] = Color::GREEN;
+        steps[1] = Color::GREEN;
-        color_steps_[2] = Color::BLUE;
+        steps[2] = Color::BLUE;
     } else if (pattern == "cpc") {
-        color_steps_[0] = Color::CYAN;
+        steps[0] = Color::CYAN;
-        color_steps_[1] = Color::PINK;
+        steps[1] = Color::PINK;
-        color_steps_[2] = Color::CYAN;
+        steps[2] = Color::CYAN;
     } else {
         return false; // pattern not recognized
     }
 
+    color_steps_[0]    = steps[0];
+    color_steps_[1]    = steps[1];
+    color_steps_[2]    = steps[2];
     is_user_led_blink_ = true; // user routine is active
 
     // when this is called we want the sequence_led to restart immediately and skip the long pause
@@ -395,7 +384,7 @@ void LED::sequence_led() {
 
         if (++led_flash_step_ == 8) {
             // finished first iteration, reset the whole sequence, turn off LED
-            led_long_timer_ = uuid::get_uptime();
+            led_long_timer_ = current_time;
             led_flash_step_ = 0;
             set_led(Color::OFF); // turn off the LED
 
@@ -407,24 +396,8 @@ void LED::sequence_led() {
             return;
         }
 
-        if (led_flash_step_ % 2) {
+        // odd steps 3, 5 and 7 show the three configured colors, even steps turn the LED off to make it flash
-            // handle the three step events (on odd numbers 3,5,7 etc). see if we need to set a LED color
+        set_led(led_flash_step_ % 2 ? color_steps_[(led_flash_step_ - 3) / 2] : Color::OFF);
-            switch (led_flash_step_) {
-            case 3: // first flash
-                set_led(color_steps_[0]);
-                break;
-            case 5: // second flash
-                set_led(color_steps_[1]);
-                break;
-            case 7: // third flash
-                set_led(color_steps_[2]);
-                break;
-            default:
-                break;
-            }
-        } else {
-            set_led(Color::OFF); // turn off on even number count, to make it flash
-        }
     }
 }
 

src/core/led.h

@@ -43,7 +43,6 @@ enum class Order : uint8_t { GRB, RGB }; // WS2812B is GRB
 
 // Blocks ~30 us for the transmit, plus up to the reset gap if called again right after the previous update
 bool write(uint8_t pin, uint8_t red, uint8_t green, uint8_t blue, Order order = Order::GRB);
-bool off(uint8_t pin);
 void end(uint8_t pin); // release the pin's RMT channel, e.g. before reusing the pin for something else
 
 } // namespace rgb_led
@@ -66,7 +65,7 @@ class LED {
     bool loop(uint8_t healthcheck, bool button_busy);
 
     void start_led_fast_flash(uint8_t duration); // duration in seconds
-    bool set_custom_led_routine(std::string color, std::string pattern);
+    bool set_custom_led_routine(const std::string & color, const std::string & pattern);
 
   private:
     static uuid::log::Logger logger_;
@@ -83,28 +82,31 @@ class LED {
     static constexpr uint32_t LED_FLASH_INTERVAL_MS               = 100;  // LED toggle period during factory-reset flash
     static constexpr uint8_t  RGB_LED_BRIGHTNESS                  = 20;   // 255 is max brightness
     static constexpr uint8_t  LED_ON                              = HIGH; // LED on
+    static constexpr Color    COLOR_UNSET                         = static_cast<Color>(0xFF); // forces the next set_led() to write
+
+    // led_fast_flash() state
+    uint32_t led_flash_start_time_ = 0;
+    uint32_t led_flash_duration_   = 0;
+    uint32_t last_toggle_time_     = 0;
+
+    // sequence_led() state
+    uint32_t led_long_timer_  = 1; // 1 will kick it off immediately
+    uint32_t led_short_timer_ = 0;
 
     // local copies of the application settings
     uint8_t led_gpio_ = 0;
     uint8_t led_type_ = 0;
     bool    hide_led_ = false;
 
-    bool     led_fast_flash_timer_ = false;
+    bool led_fast_flash_timer_ = false;
-    uint32_t led_flash_start_time_ = 0;
+    bool led_flash_state_      = false;
-    uint32_t led_flash_duration_   = 0;
 
-    // led_flash() state
+    bool    last_button_busy_ = false;
-    bool     led_flash_state_  = false;
+    uint8_t led_flash_step_   = 0; // 0 means we're not in the short flash timer
-    uint32_t last_toggle_time_ = 0;
 
-    // sequence_led() state
+    // set_custom_led_routine() and healthcheck state
-    bool     last_button_busy_ = false;
-    uint32_t led_long_timer_   = 1; // 1 will kick it off immediately
-    uint32_t led_short_timer_  = 0;
-    uint8_t  led_flash_step_   = 0; // 0 means we're not in the short flash timer
-
-    // set_led_routine() state
     Color color_steps_[3] = {Color::OFF, Color::OFF, Color::OFF};
+    Color last_color_     = COLOR_UNSET; // last color written, to skip redundant writes
 
     // if true, the user has requested a custom LED blink, this always has preference over the button or showing the system health
     bool is_user_led_blink_ = false;

src/core/system.cpp

@@ -2924,11 +2924,13 @@ bool System::load_board_profile(std::vector<int8_t> & data, const std::string &
 // https://github.com/emsesp/EMS-ESP32/issues/3063
 // /api//system/led command that takes an argument in the form [color]:[pattern]
 // color is red, green, blue, yellow, white
+// color is optional
 // pattern is
 //  blink1 for 1 time
 //  blink2 for 2 times
 //  blink3 for 3 times
 //  rgb for RGB
+//  cpc for CPC
 // For example: /api/system/led?data=red:blink1
 // For older non-RGB models, the colour would default to just being on.
 bool System::command_led(const char * value, const int8_t) {
@@ -2937,16 +2939,19 @@ bool System::command_led(const char * value, const int8_t) {
     }
 
     std::string arg = value;
-    if (arg.find(':') == std::string::npos) {
+    std::string color;
-        LOG_ERROR("LED command must be in the form [color]:[pattern]");
+    std::string pattern;
-        return false; // not in the form [color]:[pattern]
+    auto        sep = arg.find(':');
+    if (sep == std::string::npos) {
+        pattern = arg; // no color given, only a pattern
+    } else {
+        color   = arg.substr(0, sep);
+        pattern = arg.substr(sep + 1);
     }
-    std::string color   = arg.substr(0, arg.find(':'));
-    std::string pattern = arg.substr(arg.find(':') + 1);
 
     // set and validate the color and pattern
     if (!EMSESP::led_.set_custom_led_routine(color, pattern)) {
-        LOG_ERROR("Invalid color or pattern.");
+        LOG_ERROR("Invalid color or pattern");
         return false;
     }
 
@@ -3207,7 +3212,7 @@ bool System::uploadFirmwareURL(const char * url) {
     }
     basic_client.setTimeout(FIRMWARE_UPLOAD_READ_TIMEOUT_S * 1000); // socket-level read timeout, in ms
     ssl_client.setTimeout(FIRMWARE_UPLOAD_READ_TIMEOUT_S);          // Stream::readBytes timeout used by Update
-    ssl_client.setClient(&basic_client, is_https); // enableSSL = false for plain HTTP
+    ssl_client.setClient(&basic_client, is_https);                  // enableSSL = false for plain HTTP
 
     const uint16_t port           = is_https ? 443 : 80;
     String         url_remain     = saved_url.substring(scheme_len);
@@ -3355,9 +3360,9 @@ bool System::uploadFirmwareURL(const char * url) {
 
     // explicit chunked read loop instead of Update.writeStream():
     uint8_t buf[FIRMWARE_UPLOAD_CHUNK_SIZE];
-    size_t             total_read = 0;
+    size_t  total_read = 0;
-    bool               magic_ok   = false;
+    bool    magic_ok   = false;
-    int                last_pct   = -1;
+    int     last_pct   = -1;
 
     while (total_read < (size_t)firmware_size) {
         // a cancel is signalled by the WebUI dropping the status below UPLOADING (back to NORMAL)

test/test_api/api_test.sh

@@ -1,53 +1,88 @@
-#!/bin/sh
+#!/usr/bin/env bash
-
 #
 # Command line test for the API
 #
 
-emsesp_url="http://ems-esp.local"
+emsesp_url="http://ems-espT.local"
 
 # get the token from the Security page. This is the token for the admin user, unless changed it'll always be the same
 emsesp_token="eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJ1c2VybmFtZSI6ImFkbWluIiwiYWRtaW4iOnRydWV9.2bHpWya2C7Q12WjNUBD6_7N3RCD7CMl-EGhyQVzFdDg"
 
 # This example will get the system info, via a GET request
-curl -X GET ${emsesp_url}/api/system/info
+test0() {
-echo "\n"
+    curl -X GET ${emsesp_url}/api/system/info
+}
 
 # This example will execute a read command on product ID 8 and type ID 1
-curl -X POST \
+test1() {
-    -H "Authorization: Bearer ${emsesp_token}" \
+    curl -X POST \
-    -H "Content-Type: application/json" \
+        -H "Authorization: Bearer ${emsesp_token}" \
-    -d '{"data":"8 1"}' \
+        -H "Content-Type: application/json" \
-    ${emsesp_url}/api/system/read
+        -d '{"data":"8 1"}' \
-
+        ${emsesp_url}/api/system/read
-echo "\n"
+}
 
 # This example will use the message command to get the locale (en)
-curl -X POST \
+test2() {
-    -H "Authorization: Bearer ${emsesp_token}" \
+    curl -X POST \
-    -H "Content-Type: application/json" \
+        -H "Authorization: Bearer ${emsesp_token}" \
-    -d '{"value":"system/settings/locale"}' \
+        -H "Content-Type: application/json" \
-    ${emsesp_url}/api/system/message
+        -d '{"value":"system/settings/locale"}' \
-
+        ${emsesp_url}/api/system/message
-echo "\n"
+}
 
 # This example will export all values to a json file, including custom entities, sensors and schedules
-curl -X POST \
+test3() {
-    -H "Authorization: Bearer ${emsesp_token}" \
+    curl -X POST \
-    -H "Content-Type: application/json" \
+        -H "Authorization: Bearer ${emsesp_token}" \
-    -d '{"action":"export", "param":"allvalues"}' \
+        -H "Content-Type: application/json" \
-    ${emsesp_url}/rest/action
+        -d '{"action":"export", "param":"allvalues"}' \
-
+        ${emsesp_url}/rest/action
-echo "\n"
+}
 
 # Get all versions
-curl -X POST \
+test4() {
+    curl -X POST \
     -H "Authorization: Bearer ${emsesp_token}" \
     -H "Content-Type: application/json" \
-    -d '{"action":"getVersions"}' \
+        -d '{"action":"getVersions"}' \
-    ${emsesp_url}/rest/action
+        ${emsesp_url}/rest/action
+}
+
+# led tests
+test5() {
+    curl -X POST \
+    -H "Authorization: Bearer ${emsesp_token}" \
+    -H "Content-Type: application/json" \
+    -d '{"data":"red:blink1"}' \
+    ${emsesp_url}/api/system/led
+}
+
+test6() {
+    curl -X POST \
+    -H "Authorization: Bearer ${emsesp_token}" \
+    -H "Content-Type: application/json" \
+    -d '{"data":"rgb"}' \
+    ${emsesp_url}/api/system/led
+}
+
+# Create a list of test function names to run
+tests_to_run=(
+    # test0
+    # test1
+    # test2
+    # test3
+    # test4
+    test5
+    test6
+)
+
+for test_func in "${tests_to_run[@]}"; do
+    echo "Running $test_func..."
+    $test_func
+    echo ""
+done
 
-echo "\n"
 
 # This example is how to call a service in Home Assistant via the API
 # Which can be added to an EMS-EPS schedule