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.

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

Support GRB neopixels as well as RGB as the status indicator #8469

Closed
deshipu opened this issue Oct 9, 2023 · 4 comments
Closed

Support GRB neopixels as well as RGB as the status indicator #8469

deshipu opened this issue Oct 9, 2023 · 4 comments
Milestone

Comments

@deshipu
Copy link

deshipu commented Oct 9, 2023

The Waveshare ESP32-S3-Zero board (#8468) uses a Neopixel with different color order than RGB as the status LED, which means that it blinks green on error, and red on success. This is confusing for the users.

It would be nice to be able to specify a variable in the mpconfigboard.h for that board to swap the colors, so the errors are red and success is green.

I think the easiest way to do that is to add an ifdef in the supervisor/shared/status_leds.c file around line 257-259 (https://github.com/adafruit/circuitpython/blob/main/supervisor/shared/status_leds.c#L257-L259) with an alternate color order.

I could submit a PR for this, but it seems to be a good first issue, so maybe someone wants to take it.

@deshipu
Copy link
Author

deshipu commented Oct 9, 2023

You don't need a ESP32-S3-Zero board to test it, any board with an on-board neopixel should work, you will just verify that the colors are swapped.

@dhalbert dhalbert added this to the 9.0.0 milestone Oct 10, 2023
@tannewt tannewt modified the milestones: 9.0.0, Long term Oct 10, 2023
@Axeia
Copy link

Axeia commented Oct 11, 2023

I have added support for this to my fork:

https://github.com/Axeia/circuitpython-ESP32-S3-Zero/blob/1477bc403bb3e58a64dc1ede92567b44934dfee3/supervisor/shared/status_leds.c#L261-L266

(and a #define MICROPY_HW_NEOPIXEL_ORDER_GRB (1) line to mpconfigboard.h)

Disclaimer: I am definitely out my element in C as those are the first lines I've ever written in it. Initially I tried adding:
#define MICROPY_HW_NEOPIXEL_GRB (&pin_GPIO21)
to mpconfigboard.h as an alternative to:
#define MICROPY_HW_NEOPIXEL (&pin_GPIO21)
but came to the conclusion that it would require a fair few changes to supervisor/shared/status_leds.c as it checks if MICROPY_HW_NEOPIXEL is defined in multiple places (to include the MICROPY_HW_NEOPIXEL_COUNT file, in the deinit method and a few more places) and it seemed overly complicated. I figured just a single ifdef MICROPY_HW_NEOPIXEL_ORDER_GRB was a lot cleaner.

I didn't submit this as a PR yet as its part of my ESP32-S3-Zero board support PR which currently has the wrong USB PID. I'm waiting for Waveshare to get back to me if they'll sort out a USB PID or if I should try to do so myself

@tannewt
Copy link
Member

tannewt commented Oct 12, 2023

That sounds like a good approach! Thanks for the update.

@dhalbert
Copy link
Collaborator

dhalbert commented Feb 1, 2025

Fixed by #8497.

@dhalbert dhalbert closed this as completed Feb 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants