feat(led): Update how ISR is installed; move impl to cpp file (#354)

* Make bool for isr service installed static within class, not just constructor
* Add `static void uninstall_isr()` method so that users can uninstall the ISR if they want to
* Update example to use logger for better output
* Update example to show use of `uninstall_isr()` method
* Move LED implementation to cpp file

Author: finger563

components/led/CMakeLists.txt

@@ -1,3 +1,4 @@
 idf_component_register(
   INCLUDE_DIRS "include"
+  SRC_DIRS "src"
   REQUIRES base_component driver task)

components/led/example/main/led_example.cpp

@@ -11,11 +11,11 @@
 using namespace std::chrono_literals;
 
 extern "C" void app_main(void) {
-  fmt::print("Starting led example!\n");
+  espp::Logger logger({.tag = "LED Example", .level = espp::Logger::Verbosity::DEBUG});
   {
     //! [linear led example]
-    fmt::print("Starting linear led example!\n");
     float num_seconds_to_run = 10.0f;
+    logger.info("Starting linear led example for {:.1f}s!", num_seconds_to_run);
     int led_fade_time_ms = 1000;
     std::vector<espp::Led::ChannelConfig> led_channels{{
         .gpio = 2,
@@ -54,9 +54,10 @@ extern "C" void app_main(void) {
 
   {
     //! [breathing led example]
-    fmt::print("Starting gaussian led example!\n");
     float breathing_period = 3.5f; // seconds
     float num_periods_to_run = 2.0f;
+    float num_seconds_to_run = num_periods_to_run * breathing_period;
+    logger.info("Starting gaussian led example for {:.1f}s!", num_seconds_to_run);
     std::vector<espp::Led::ChannelConfig> led_channels{{
         .gpio = 2,
         .channel = LEDC_CHANNEL_5,
@@ -86,13 +87,17 @@ extern "C" void app_main(void) {
     auto led_task =
         espp::Task::make_unique({.callback = led_callback, .task_config = {.name = "breathe"}});
     led_task->start();
-    float wait_time = num_periods_to_run * breathing_period;
-    fmt::print("Sleeping for {:.1f}s...\n", wait_time);
-    std::this_thread::sleep_for(wait_time * 1.0s);
+    logger.debug("Sleeping for {:.1f}s...", num_seconds_to_run);
+    std::this_thread::sleep_for(num_seconds_to_run * 1.0s);
     //! [breathing led example]
   }
 
-  fmt::print("LED example complete!\n");
+  // now uninstall the fade service to free up the ISR, since we have no LEDs
+  // using it anymore
+  logger.info("Uninstalling LED ISR...");
+  espp::Led::uninstall_isr();
+
+  logger.info("LED example complete!");
 
   while (true) {
     std::this_thread::sleep_for(1s);

components/led/include/led.hpp

@@ -2,6 +2,7 @@
 
 #include <algorithm>
 #include <cmath>
+#include <mutex>
 #include <optional>
 #include <vector>
 
@@ -65,79 +66,20 @@ class Led : public BaseComponent {
    * @brief Initialize the LEDC subsystem according to the configuration.
    * @param config The configuration structure for the LEDC subsystem.
    */
-  explicit Led(const Config &config) noexcept
-      : BaseComponent("Led", config.log_level)
-      , duty_resolution_(config.duty_resolution)
-      , max_raw_duty_((uint32_t)(std::pow(2, (int)duty_resolution_) - 1))
-      , channels_(config.channels) {
-
-    logger_.info("Initializing timer");
-    ledc_timer_config_t ledc_timer;
-    memset(&ledc_timer, 0, sizeof(ledc_timer));
-    ledc_timer.duty_resolution = duty_resolution_;
-    ledc_timer.freq_hz = config.frequency_hz;
-    ledc_timer.speed_mode = config.speed_mode;
-    ledc_timer.timer_num = config.timer;
-    ledc_timer.clk_cfg = config.clock_config;
-    ledc_timer_config(&ledc_timer);
-
-    logger_.info("Initializing channels");
-    for (const auto &conf : channels_) {
-      uint32_t actual_duty = std::clamp(conf.duty, 0.0f, 100.0f) * max_raw_duty_ / 100.0f;
-      ledc_channel_config_t channel_conf;
-      memset(&channel_conf, 0, sizeof(channel_conf));
-      channel_conf.channel = conf.channel;
-      channel_conf.duty = actual_duty;
-      channel_conf.gpio_num = conf.gpio;
-      channel_conf.speed_mode = conf.speed_mode;
-      channel_conf.hpoint = 0;
-      channel_conf.timer_sel = conf.timer;
-      channel_conf.flags.output_invert = conf.output_invert;
-      ledc_channel_config(&channel_conf);
-    }
-
-    logger_.info("Initializing the fade service");
-    static bool fade_service_installed = false;
-    if (!fade_service_installed) {
-      auto install_fn = []() -> esp_err_t { return ledc_fade_func_install(0); };
-      auto err = espp::task::run_on_core(install_fn, config.isr_core_id);
-      if (err != ESP_OK) {
-        logger_.error("install ledc fade service failed {}", esp_err_to_name(err));
-      }
-      fade_service_installed = err == ESP_OK;
-    }
-
-    ledc_cbs_t callbacks = {.fade_cb = &Led::cb_ledc_fade_end_event};
-
-    // we associate each channel with its own semaphore so that they can be
-    // faded / controlled independently.
-    logger_.info("Creating semaphores");
-    fade_semaphores_.resize(channels_.size());
-    for (auto &sem : fade_semaphores_) {
-      sem = xSemaphoreCreateBinary();
-      // go ahead and give to the semaphores so the functions will work
-      xSemaphoreGive(sem);
-    }
-    for (int i = 0; i < channels_.size(); i++) {
-      ledc_cb_register(channels_[i].speed_mode, channels_[i].channel, &callbacks,
-                       (void *)fade_semaphores_[i]);
-    }
-  }
+  explicit Led(const Config &config) noexcept;
 
   /**
    * @brief Stop the LEDC subsystem and free memory.
    */
-  ~Led() {
-    // clean up the semaphores
-    for (auto &sem : fade_semaphores_) {
-      // take the semaphore (so that we don't delete it until no one is
-      // blocked on it)
-      xSemaphoreTake(sem, portMAX_DELAY);
-      // and delete it
-      vSemaphoreDelete(sem);
-    }
-    ledc_fade_func_uninstall();
-  }
+  ~Led();
+
+  /**
+   * @brief Uninstall the fade service. This should be called if you want to
+   *       stop the fade service and free up the ISR.
+   * @note This function should only be called if you are sure that no other
+   *       Led objects are using the fade service.
+   */
+  static void uninstall_isr();
 
   /**
    * @brief Can the LED settings can be changed for the channel? If this
@@ -146,30 +88,15 @@ class Led : public BaseComponent {
    * @param channel The channel to check
    * @return True if the channel settings can be changed, false otherwise
    */
-  bool can_change(ledc_channel_t channel) {
-    int index = get_channel_index(channel);
-    if (index == -1) {
-      return false;
-    }
-    auto &sem = fade_semaphores_[index];
-    return uxSemaphoreGetCount(sem) == 1;
-  }
+  bool can_change(ledc_channel_t channel);
 
   /**
    * @brief Get the current duty cycle this channel has.
    * @param channel The channel in question
    * @return The duty percentage [0.0f, 100.0f] if the channel is managed,
    *         std::nullopt otherwise
    */
-  std::optional<float> get_duty(ledc_channel_t channel) const {
-    int index = get_channel_index(channel);
-    if (index == -1) {
-      return {};
-    }
-    const auto &conf = channels_[index];
-    auto raw_duty = ledc_get_duty(conf.speed_mode, conf.channel);
-    return (float)raw_duty / (float)max_raw_duty_ * 100.0f;
-  }
+  std::optional<float> get_duty(ledc_channel_t channel) const;
 
   /**
    * @brief Set the duty cycle for this channel.
@@ -178,21 +105,7 @@ class Led : public BaseComponent {
    * @param channel The channel to set the duty cycle for.
    * @param duty_percent The new duty percentage, [0.0, 100.0].
    */
-  void set_duty(ledc_channel_t channel, float duty_percent) {
-    int index = get_channel_index(channel);
-    if (index == -1) {
-      return;
-    }
-    auto conf = channels_[index];
-    auto &sem = fade_semaphores_[index];
-    // ensure that it's not fading if it is
-    xSemaphoreTake(sem, portMAX_DELAY);
-    uint32_t actual_duty = std::clamp(duty_percent, 0.0f, 100.0f) * max_raw_duty_ / 100.0f;
-    ledc_set_duty(conf.speed_mode, conf.channel, actual_duty);
-    ledc_update_duty(conf.speed_mode, conf.channel);
-    // make sure others can set this channel now as well
-    xSemaphoreGive(sem);
-  }
+  void set_duty(ledc_channel_t channel, float duty_percent);
 
   /**
    * @brief Set the duty cycle for this channel, fading from the current duty
@@ -203,23 +116,12 @@ class Led : public BaseComponent {
    * @param duty_percent The new duty percentage to fade to, [0.0, 100.0].
    * @param fade_time_ms The number of milliseconds for which to fade.
    */
-  void set_fade_with_time(ledc_channel_t channel, float duty_percent, uint32_t fade_time_ms) {
-    int index = get_channel_index(channel);
-    if (index == -1) {
-      return;
-    }
-    auto conf = channels_[index];
-    auto &sem = fade_semaphores_[index];
-    // ensure that it's not fading if it is
-    xSemaphoreTake(sem, portMAX_DELAY);
-    uint32_t actual_duty = std::clamp(duty_percent, 0.0f, 100.0f) * max_raw_duty_ / 100.0f;
-    ledc_set_fade_with_time(conf.speed_mode, conf.channel, actual_duty, fade_time_ms);
-    ledc_fade_start(conf.speed_mode, conf.channel, LEDC_FADE_NO_WAIT);
-    // NOTE: we don't give the semaphore back here because that is the job of
-    // the ISR
-  }
+  void set_fade_with_time(ledc_channel_t channel, float duty_percent, uint32_t fade_time_ms);
 
 protected:
+  static std::mutex fade_service_mutex;
+  static bool fade_service_installed;
+
   /**
    * @brief Get the index of channel in channels_, -1 if not found.
    * @note We implement this instead of using std::find because we cannot use
@@ -228,30 +130,14 @@ class Led : public BaseComponent {
    * @param channel Channel to find.
    * @return -1 if not found, index of channel if found.
    */
-  int get_channel_index(ledc_channel_t channel) const {
-    for (int i = 0; i < channels_.size(); i++) {
-      if (channels_[i].channel == channel) {
-        return i;
-      }
-    }
-    return -1;
-  }
+  int get_channel_index(ledc_channel_t channel) const;
 
   /**
    * This callback function will be called when fade operation has ended
    * Use callback only if you are aware it is being called inside an ISR
    * Otherwise, you can use a semaphore to unblock tasks
    */
-  static bool IRAM_ATTR cb_ledc_fade_end_event(const ledc_cb_param_t *param, void *user_arg) {
-    portBASE_TYPE taskAwoken = pdFALSE;
-
-    if (param->event == LEDC_FADE_END_EVT) {
-      SemaphoreHandle_t sem = (SemaphoreHandle_t)user_arg;
-      xSemaphoreGiveFromISR(sem, &taskAwoken);
-    }
-
-    return (taskAwoken == pdTRUE);
-  }
+  static bool cb_ledc_fade_end_event(const ledc_cb_param_t *param, void *user_arg);
 
   ledc_timer_bit_t duty_resolution_;
   uint32_t max_raw_duty_;