Skip to content

feat(led_strip): Support more pixel orders of 3 colors (IEC-126)#344

Merged
suda-morris merged 2 commits intoespressif:masterfrom
Kainarx:feat/led_strip_support_more_bytes_orders
Oct 25, 2024
Merged

feat(led_strip): Support more pixel orders of 3 colors (IEC-126)#344
suda-morris merged 2 commits intoespressif:masterfrom
Kainarx:feat/led_strip_support_more_bytes_orders

Conversation

@Kainarx
Copy link
Copy Markdown
Collaborator

@Kainarx Kainarx commented Jun 20, 2024 •
edited by suda-morris
Loading

Checklist

  • Component contains License
  • Component contains README.md
  • Component contains idf_component.yml file with url field defined
  • Component was added to upload job
  • Component was added to build job
  • Optional: Component contains unit tests
  • CI passing

Change description

Closes #341

@github-actions github-actions Bot changed the title feat(led_strip): Support more pixel orders of 3 colors feat(led_strip): Support more pixel orders of 3 colors (IEC-126) Jun 20, 2024
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch from 5e93906 to 65e14f5 Compare June 20, 2024 06:07
@igrr igrr requested a review from suda-morris July 11, 2024 16:13
robertlipe
robertlipe reviewed Aug 2, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@robertlipe robertlipe left a comment

Choose a reason for hiding this comment

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

I'm not a reviewer, so addressing my input isn't required.

I do have some ideas on performance (don't recompute per pixel what you can compute at construction time) as well as some maintainability/readability feedback just from a drive-by on this code.

Thank you for making it more awesome

Comment thread led_strip/src/led_strip_rmt_dev.c Outdated
Comment thread led_strip/src/led_strip_rmt_dev.c Outdated
Comment thread led_strip/src/led_strip_rmt_dev_idf4.c Outdated
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch from 65e14f5 to ee682b8 Compare August 6, 2024 03:13
This was referenced Aug 6, 2024
Open
Merged
@suda-morris suda-morris mentioned this pull request Aug 14, 2024
Merged
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch from ee682b8 to a77d1c5 Compare August 15, 2024 10:36
@Kainarx Kainarx closed this Aug 19, 2024
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch from a77d1c5 to 60c1426 Compare August 19, 2024 10:23
@robertlipe
Copy link
Copy Markdown
Contributor

robertlipe commented Aug 19, 2024

After all that, you silently withdraw the PR?

I get that it's been open for a month while being ignored by the maintainers, but IMO this is work worth picking up and if you're just exhausted trying to get it in, I may run with it (adding a second white channel) but based on my recent experience trying to change IDF, I'm not at all sure that I wouldn't just rather fork it and maintain it myself and bypass the reviewer clique.

Why was this dropped?

@suda-morris suda-morris reopened this Aug 20, 2024
github-advanced-security[bot]
github-advanced-security AI found potential problems Aug 20, 2024
View reviewed changes
Comment thread led_strip/src/led_strip_rmt_dev.c Fixed
Comment thread led_strip/src/led_strip_rmt_dev.c Fixed
Comment thread led_strip/src/led_strip_rmt_dev.c Fixed
Comment thread led_strip/src/led_strip_rmt_dev.c Fixed
Comment thread led_strip/src/led_strip_rmt_dev.c Fixed
Comment thread led_strip/src/led_strip_spi_dev.c Fixed
Comment thread led_strip/src/led_strip_spi_dev.c Fixed
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch 2 times, most recently from 0f5696b to 3c33ebf Compare August 20, 2024 04:00
@Kainarx
Copy link
Copy Markdown
Collaborator Author

Kainarx commented Aug 20, 2024

After all that, you silently withdraw the PR?

I get that it's been open for a month while being ignored by the maintainers, but IMO this is work worth picking up and if you're just exhausted trying to get it in, I may run with it (adding a second white channel) but based on my recent experience trying to change IDF, I'm not at all sure that I wouldn't just rather fork it and maintain it myself and bypass the reviewer clique.

Why was this dropped?

Sorry, I didn't really mean to close it, it was a mistake on my part. I've re-pushed it. Thank you for your attention.

igrr
igrr reviewed Aug 20, 2024
View reviewed changes
Comment thread led_strip/examples/led_strip_spi_ws2812/main/led_strip_spi_ws2812_main.c Outdated
igrr
igrr reviewed Aug 20, 2024
View reviewed changes
Comment thread led_strip/examples/led_strip_spi_ws2812/main/led_strip_spi_ws2812_main.c Outdated
robertlipe
robertlipe reviewed Aug 20, 2024
View reviewed changes
Comment thread led_strip/api.md Outdated
Comment thread led_strip/api.md Outdated
Comment thread led_strip/examples/led_strip_spi_ws2812/main/led_strip_spi_ws2812_main.c Outdated
Comment thread led_strip/include/led_strip_types.h Outdated
Comment thread led_strip/src/led_strip_rmt_dev.c Outdated
@robertlipe robertlipe mentioned this pull request Sep 30, 2024
Closed
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch 2 times, most recently from ad5231d to aa2630c Compare October 22, 2024 05:20
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/examples/led_strip_rmt_ws2812/main/led_strip_rmt_ws2812_main.c Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/examples/led_strip_rmt_ws2812/main/led_strip_rmt_ws2812_main.c Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/idf_component.yml Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/CHANGELOG.md Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/include/esp_private/led_strip_common.h Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/src/led_strip_common.c Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/src/led_strip_common.c Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/src/led_strip_common.c Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/src/led_strip_common.c Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/include/led_strip.h Outdated
suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/src/led_strip_common.c Outdated
suda-morris
suda-morris requested changes Oct 22, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@suda-morris suda-morris left a comment

Choose a reason for hiding this comment

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

Thanks for giving this approach a try, LGTM overall. @Kainarx

suda-morris
suda-morris reviewed Oct 22, 2024
View reviewed changes
Comment thread led_strip/include/led_strip_types.h Outdated
@suda-morris suda-morris mentioned this pull request Oct 22, 2024
Merged
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch 3 times, most recently from 54ae110 to 0680770 Compare October 23, 2024 05:21
@suda-morris suda-morris force-pushed the feat/led_strip_support_more_bytes_orders branch 5 times, most recently from 1ab7622 to 0501326 Compare October 24, 2024 08:03
@suda-morris suda-morris requested a review from igrr October 24, 2024 09:58
@suda-morris suda-morris force-pushed the feat/led_strip_support_more_bytes_orders branch 2 times, most recently from 5314606 to 1539f0d Compare October 25, 2024 09:35
github-advanced-security[bot]
github-advanced-security AI found potential problems Oct 25, 2024
View reviewed changes
Copy link
Copy Markdown

@github-advanced-security github-advanced-security AI left a comment

Choose a reason for hiding this comment

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

clang-tidy found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

@suda-morris suda-morris force-pushed the feat/led_strip_support_more_bytes_orders branch from 1539f0d to 31585c8 Compare October 25, 2024 10:07
github-advanced-security[bot]
github-advanced-security AI found potential problems Oct 25, 2024
View reviewed changes
Comment thread led_strip/src/led_strip_spi_dev.c
__led_strip_spi_bit(0, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * 3]);
uint8_t *pixel_buf = spi_strip->pixel_buf;
led_color_component_format_t component_fmt = spi_strip->component_fmt;
memset(pixel_buf + start, 0, spi_strip->bytes_per_pixel * SPI_BYTES_PER_COLOR_BYTE);

Check warning

Code scanning / clang-tidy

Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]

Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
Comment thread led_strip/src/led_strip_spi_dev.c
__led_strip_spi_bit(blue, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * 2]);
__led_strip_spi_bit(white, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * 3]);
uint8_t *pixel_buf = spi_strip->pixel_buf;
memset(pixel_buf + start, 0, spi_strip->bytes_per_pixel * SPI_BYTES_PER_COLOR_BYTE);

Check warning

Code scanning / clang-tidy

Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]

Call to function 'memset' is insecure as it does not provide security checks introduced in the C11 standard. Replace with analogous functions that support length arguments or provides boundary checks such as 'memset_s' in case of C11 [clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling]
@suda-morris suda-morris merged commit 4769e0a into espressif:master Oct 25, 2024
@Kainarx Kainarx deleted the feat/led_strip_support_more_bytes_orders branch October 30, 2024 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@suda-morris suda-morris suda-morris approved these changes

@igrr igrr Awaiting requested review from igrr

+1 more reviewer

@robertlipe robertlipe robertlipe left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@Kainarx Kainarx

Labels

Component: led_strip Resolution: Done Status: Done

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[led_strip] Add support for bytes order other than GRB (IEC-123)

7 participants

@Kainarx @robertlipe @igrr @suda-morris @github-advanced-security @espressif-bot @tom-borcin