Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from

Conversation

Kainarx
Copy link
Contributor

@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
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

led_strip/src/led_strip_rmt_dev.c Outdated Show resolved Hide resolved
led_strip/src/led_strip_rmt_dev.c Outdated Show resolved Hide resolved
led_strip/src/led_strip_rmt_dev_idf4.c Show resolved Hide resolved
@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
Contributor

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 bot found potential problems Aug 20, 2024
View reviewed changes
led_strip/src/led_strip_rmt_dev.c Fixed Show fixed Hide fixed
led_strip/src/led_strip_rmt_dev.c Fixed Show fixed Hide fixed
led_strip/src/led_strip_rmt_dev.c Fixed Show fixed Hide fixed
led_strip/src/led_strip_rmt_dev.c Fixed Show fixed Hide fixed
led_strip/src/led_strip_rmt_dev.c Fixed Show fixed Hide fixed
led_strip/src/led_strip_spi_dev.c
__led_strip_spi_bit(blue, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE * 2]);
uint8_t *pixel_buf = spi_strip->pixel_buf;
uint8_t *offset = spi_strip->led_pixel_offset;
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] Warning

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]
led_strip/src/led_strip_spi_dev.c
__led_strip_spi_bit(red, &spi_strip->pixel_buf[start + SPI_BYTES_PER_COLOR_BYTE]);
__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]);
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] Warning

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]
@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
Contributor 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.

robertlipe
robertlipe reviewed Aug 20, 2024
View reviewed changes
led_strip/api.md Outdated Show resolved Hide resolved
led_strip/api.md Outdated Show resolved Hide resolved
led_strip/examples/led_strip_spi_ws2812/main/led_strip_spi_ws2812_main.c Outdated Show resolved Hide resolved
led_strip/include/led_strip_types.h Outdated Show resolved Hide resolved
led_strip/src/led_strip_rmt_dev.c Outdated Show resolved Hide resolved
@robertlipe robertlipe mentioned this pull request Sep 30, 2024
Open
@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
led_strip/CHANGELOG.md Outdated Show resolved Hide resolved
suda-morris
suda-morris requested changes Oct 22, 2024
View reviewed changes
Copy link
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 mentioned this pull request Oct 22, 2024
Open
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch 2 times, most recently from 743e8aa to 54ae110 Compare October 23, 2024 05:16
@Kainarx Kainarx force-pushed the feat/led_strip_support_more_bytes_orders branch from 54ae110 to 0680770 Compare October 23, 2024 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igrr igrr igrr left review comments

@robertlipe robertlipe robertlipe left review comments

@suda-morris suda-morris suda-morris requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Component: led_strip
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)
5 participants
@Kainarx @robertlipe @igrr @suda-morris @tom-borcin