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

add definition WS2812_BYTE_ORDER to fix RGB LED issues #10184

Merged
merged 12 commits into from
Dec 6, 2020

Conversation

hineybush
Copy link
Contributor

@hineybush hineybush commented Aug 27, 2020

Description

While flashing and repairing some hbcp PCBs, I realized that the LEDs were not showing the correct color - specifically that the red and green channels were reversed. The LEDs on the board are WS2812B-2020 from WorldSemi. I replaced one LED with a WS2812C-2020, and it was the correct colors, according to the VIA output (and QMK). After examining the datasheets for each LED, I found that even though the software is the same (G, R then B, 1 byte each) the hardware is not - the red and green LEDs are reversed physically!

https://i.imgur.com/kB3mPyI.jpg

https://i.imgur.com/dmB1aY1.png

I created a definition, WS2812_BYTE_ORDER in color.h to define the byte order for the LED. The RGB definition swaps the R and G bytes and corrects the color issue.

https://i.imgur.com/sLcRLYU.jpg

If WS2812B-2020 LEDs are used, #define WS2812_BYTE_ORDER WS2812_BYTE_ORDER_RGB should be placed in the config.h file. I tested this successfully with the hbcp code.

This added definition should not affect any other existing PCBs without it, due to defaulting to the "standard" GRB structure. I tested this with a few PCBs I had on hand, namely h88, h87a and some prototypes.

Thanks to @Xelus22 for the assistance.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

  • Incorrect RGB coloring with WS2812B-2020 LEDs due to byte structure difference

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@fauxpark
Copy link
Member

fauxpark commented Aug 27, 2020

This should probably be made more generic just in case there are other WS2812 parts with this/other byte orders.

eg. WS2812_ORDER_RGB etc.

@hineybush
Copy link
Contributor Author

making this more generic, will update

@hineybush
Copy link
Contributor Author

@fauxpark updated!

@hineybush hineybush changed the title add definition for WS2812B-2020 to fix RGB issues add definition WS2812_BYTE_ORDER to fix RGB LED issues Aug 27, 2020
quantum/color.h Outdated Show resolved Hide resolved
quantum/color.h Outdated Show resolved Hide resolved
docs/ws2812_driver.md Outdated Show resolved Hide resolved
@fauxpark
Copy link
Member

This PR will also need to be retargeted and rebased onto develop.

quantum/color.h Show resolved Hide resolved
quantum/color.h Show resolved Hide resolved
docs/ws2812_driver.md Outdated Show resolved Hide resolved
docs/ws2812_driver.md Outdated Show resolved Hide resolved
@tzarc
Copy link
Member

tzarc commented Aug 27, 2020

This won't work for any of the ChibiOS drivers, as they have to manipulate the data as it goes out.

@tzarc
Copy link
Member

tzarc commented Aug 27, 2020

I've actually got some WS2812C-2020's already, will see if I can get something breadboardable for testing purposes.

EDIT: Never mind, was the B's that have the reversed order.

docs/ws2812_driver.md Show resolved Hide resolved
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

Aligning docs to match tz's suggested changes.

docs/ws2812_driver.md Outdated Show resolved Hide resolved
@hineybush
Copy link
Contributor Author

@tzarc I would definitely appreciate help with the ChibiOS side as I don't have any development platforms currently, nor do I plan to have any in the near future.

@tzarc
Copy link
Member

tzarc commented Aug 28, 2020

Should be relatively straightforward, based on the #define you added:

  • Bitbang (ws2812.c) can just have the lines I linked to reordered
  • PWM (ws2812_pwm.c) can have the 0, 1, and 2 modified
  • SPI (ws2812_spi.c) can have the color.r, color.g, color.b reordered

@hineybush
Copy link
Contributor Author

@tzarc for testing, you can still use the WS2812C's - the code will do the opposite however. when #define WS2812_BYTE_ORDER WS2812_BYTE_ORDER_RGB is used it will reverse the green and red channels.

@drashna drashna requested a review from a team September 5, 2020 22:47
@stale
Copy link

stale bot commented Oct 21, 2020

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

@hineybush
Copy link
Contributor Author

@tzarc my man @Xelus22 just added the PR to my repo, which I just merged into here.

@tzarc
Copy link
Member

tzarc commented Oct 27, 2020

@tzarc my man @Xelus22 just added the PR to my repo, which I just merged into here.

Awesome, cheers for that. I believe there was a request to get RGBW sorted, but I'm okay with leaving that for a separate PR... @qmk/collaborators thoughts?

docs/ws2812_driver.md Outdated Show resolved Hide resolved
Co-authored-by: Ryan <fauxpark@gmail.com>
@fauxpark fauxpark requested a review from a team November 22, 2020 11:17
Copy link
Member

@noroadsleft noroadsleft left a comment

Choose a reason for hiding this comment

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

Slight __attribute__ ((weak)) ✔️

@tzarc tzarc merged commit c59f87a into qmk:master Dec 6, 2020
xgnxs pushed a commit to xgnxs/qmk_firmware that referenced this pull request Jan 9, 2021
* add define for WS2812B-2020 to fix RGB issues

* update driver doc

* add WS2812_BYTE_ORDER definition to correct RGB byte issues

* add definition variable thing

* update per PR request

* update per PR reqs

* update per PR request

* inital changes

* move defines to color.h and add rgbw incase

* Update docs/ws2812_driver.md

Co-authored-by: Ryan <fauxpark@gmail.com>

Co-authored-by: hineybush <hineybushkeyboards@gmail.com>
Co-authored-by: Xelus22 <preyas22@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
drashna pushed a commit to zsa/qmk_firmware that referenced this pull request Jan 13, 2021
* add define for WS2812B-2020 to fix RGB issues

* update driver doc

* add WS2812_BYTE_ORDER definition to correct RGB byte issues

* add definition variable thing

* update per PR request

* update per PR reqs

* update per PR request

* inital changes

* move defines to color.h and add rgbw incase

* Update docs/ws2812_driver.md

Co-authored-by: Ryan <fauxpark@gmail.com>

Co-authored-by: hineybush <hineybushkeyboards@gmail.com>
Co-authored-by: Xelus22 <preyas22@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
mgmanzella pushed a commit to mgmanzella/qmk_firmware that referenced this pull request Feb 16, 2021
* add define for WS2812B-2020 to fix RGB issues

* update driver doc

* add WS2812_BYTE_ORDER definition to correct RGB byte issues

* add definition variable thing

* update per PR request

* update per PR reqs

* update per PR request

* inital changes

* move defines to color.h and add rgbw incase

* Update docs/ws2812_driver.md

Co-authored-by: Ryan <fauxpark@gmail.com>

Co-authored-by: hineybush <hineybushkeyboards@gmail.com>
Co-authored-by: Xelus22 <preyas22@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* add define for WS2812B-2020 to fix RGB issues

* update driver doc

* add WS2812_BYTE_ORDER definition to correct RGB byte issues

* add definition variable thing

* update per PR request

* update per PR reqs

* update per PR request

* inital changes

* move defines to color.h and add rgbw incase

* Update docs/ws2812_driver.md

Co-authored-by: Ryan <fauxpark@gmail.com>

Co-authored-by: hineybush <hineybushkeyboards@gmail.com>
Co-authored-by: Xelus22 <preyas22@gmail.com>
Co-authored-by: Ryan <fauxpark@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants