Skip to content

Conversation

@rishi9699
Copy link
Contributor

@rishi9699 rishi9699 commented Oct 27, 2024

ESP-BSP Pull Request checklist

Note: For new BSPs create a PR with this link.

  • Version of modified component bumped
  • CI passing

Change description

Recently a new version (v3.0.0) of led_strip was pushed. This changed the led_strip_config_t struct in the

led_strip/include/led_strip_types.h

file.

Please check the latest change of led_strip repository here

Closes #420

@CLAassistant
Copy link

CLAassistant commented Oct 27, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title incorporating relevant changes from led_strip v3.0.0 incorporating relevant changes from led_strip v3.0.0 (BSP-574) Oct 27, 2024
@igrr
Copy link
Member

igrr commented Oct 27, 2024

cc @espzav @tore-espressif

I think we also need to add a version constraint on led_strip somewhere, or possibly have ifdefs for led_strip v2 and v3.

@suda-morris
Copy link
Collaborator

cc @espzav @tore-espressif

I think we also need to add a version constraint on led_strip somewhere, or possibly have ifdefs for led_strip v2 and v3.

We'd better to set a proper dependency in here

@igrr
Copy link
Member

igrr commented Oct 27, 2024

True, led_indicator component should also change the way the dependency is set.

However, since esp_bsp_generic also uses led_strip API directly, it should specify the dependency and not rely on the version set in led_indicator.

Besides, I think it's better to allow both versions of led_strip, if possible. If we force led_strip>=3, we have to make a new major release of the BSP component. If we force led_strip<3, users won't be able to use esp_bsp_generic with led_strip>=3.

The best option would have probably been to not make a breaking change and a major release in led_strip in the first place :)

@suda-morris
Copy link
Collaborator

Besides, I think it's better to allow both versions of led_strip, if possible.

Yes, we can. Just remove the configuration of led_pixel_format and color_component_format when initialize the led_strip, let the driver fall-back to choose the default one. The esp_bsp_generic can compile with led_strip version 2 and 3.

@suda-morris
Copy link
Collaborator

I take the liberty to fix the build errors that are introduced by led_strip v3. Now the bsp package can work with both led_strip v2 and led_strip v3. @igrr @espzav PTAL.

Copy link
Collaborator

@Lzw655 Lzw655 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @suda-morris, LGTM. Please also update the version of the esp32-c3-lcdkit BSP. I tested the LED functionality using the compatible development board, and everything works fine.

@suda-morris
Copy link
Collaborator

Hi @suda-morris, LGTM. Please also update the version of the esp32-c3-lcdkit BSP. I tested the LED functionality using the compatible development board, and everything works fine.

c3-lcdkit already has a contraint to the led_strip:

  led_strip:
    version: "^2"
    public: true

So this PR actually didn't do any bugfix for that BSP, it's just a refactor.

@suda-morris suda-morris merged commit 7d78276 into espressif:master Oct 28, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

esp_bsp_generic fails to build after the the the led_strip change in upstream (BSP-571)

5 participants