espressif / idf-extra-components

Additional components for ESP-IDF, maintained by Espressif
147 stars 89 forks source link

led_strip: Support for IDF4.4 #253

Closed espzav closed 10 months ago

espzav commented 11 months ago

Checklist

Change description

Updated LED Strip component for working with IDF4.4 (only with RMT)

github-actions[bot] commented 11 months ago

Unit Test Results

  11 files  ±0    11 suites  ±0   15m 19s :stopwatch: +20s   27 tests ±0    27 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  252 runs  ±0  252 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0 

Results for commit 0d63dd1d. ± Comparison against base commit 142661df.

:recycle: This comment has been updated with latest results.

espzav commented 11 months ago

@tore-espressif PTAL (it is updated for Generic BSP)

lijunru-hub commented 11 months ago

@espzav Can support control led by HSV? This makes it easier to control the brightness.

suda-morris commented 10 months ago

@espzav Can support control led by HSV? This makes it easier to control the brightness.

@lijunru-hub It's just a helper function, which is not tied to the led_strip driver very much. You may consider creating a new component e.g. "color_converter" to include e.g. void hsv2rgb(uint32_t h, uint32_t s, uint32_t v, uint32_t *r, uint32_t *g, uint32_t *b)

espzav commented 10 months ago

@espzav Can support control led by HSV? This makes it easier to control the brightness.

@lijunru-hub It's just a helper function, which is not tied to the led_strip driver very much. You may consider creating a new component e.g. "color_converter" to include e.g. void hsv2rgb(uint32_t h, uint32_t s, uint32_t v, uint32_t *r, uint32_t *g, uint32_t *b)

@suda-morris @lijunru-hub I think, that it is good idea to alow set color by HSV. We can convert color by this function inside this component: https://github.com/espressif/esp-idf/blob/8fc8f3f47997aadba21facabc66004c1d22de181/examples/peripherals/rmt/led_strip/main/led_strip_example_main.c#L30 what do you think?

espzav commented 10 months ago

@tore-espressif and @suda-morris Thank you for the review! Everything fixed.

lijunru-hub commented 10 months ago

@espzav @suda-morris I believe adding a esp_err_t led_strip_set_hsv(led_strip_handle_t strip, uint32_t index, uint32_t hue, uint32_t saturation, uint32_t brightness); would make this component more comprehensive.

espzav commented 10 months ago

@suda-morris @lijunru-hub I added HSV set pixel: https://github.com/espressif/idf-extra-components/pull/253/commits/8c299f5d97be02f27ebb996ca0791dc2bd68473b (I can remove it, if you want).

And I tested it with my custom BSP API with led_indicator component like this. It is working!


static void bsp_rgb_led_rgb2hsv(uint16_t red, uint16_t green, uint16_t blue, uint16_t *h, uint8_t *s, uint8_t *v)
{
    double hue, saturation, value;
    double m_max = MAX(red, MAX(green, blue));
    double m_min = MIN(red, MIN(green, blue));
    double m_delta = m_max - m_min;

    value = m_max / 255.0;

    if (m_delta == 0) {
        hue = 0;
        saturation = 0;
    } else {
        saturation = m_delta / m_max;

        if (red == m_max) {
            hue = (green - blue) / m_delta;
        } else if (green == m_max) {
            hue = 2 + (blue - red) / m_delta;
        } else {
            hue = 4 + (red - green) / m_delta;
        }

        hue = hue * 60;

        if (hue < 0) {
            hue = hue + 360;
        }
    }

    *h = (int)(hue + 0.5);
    *s = (int)(saturation * 100 + 0.5);
    *v = (int)(value * 100 + 0.5);
}

static esp_err_t bsp_rgb_led_set_brightness(void *hardware_data, uint32_t brightness)
{
    bsp_led_t led = (bsp_led_t)hardware_data;
    uint32_t hue = 0;
    uint32_t sat = 0;
    uint32_t val = 0;

    /* Set default color, if not set before */
    if (bsp_leds_color[led].red == 0 && bsp_leds_color[led].green == 0 && bsp_leds_color[led].blue == 0) {
        bsp_rgb_led_set_color(led, &BSP_LED_RGB_DEFAULT_COLOR);
    }

    bsp_rgb_led_rgb2hsv(bsp_leds_color[led].red, bsp_leds_color[led].green, bsp_leds_color[led].blue, &hue, &sat, &val);

    /* Set the LED pixel using HSV */
    led_strip_set_pixel_hsv(led_strip, led, hue, sat, brightness);
    /* Refresh the strip to send data */
    led_strip_refresh(led_strip);

    return ESP_OK;
}
espzav commented 10 months ago

@tore-espressif @suda-morris are you okay with keep the HSV function in this component? Can I merge it?

espzav commented 10 months ago

@suda-morris Thank you for review. I fixed everything. I think, we can merge it after tests pass.