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

Fix ordering of tuple returned by WS2812.get #232

Merged
merged 1 commit into from
Jan 25, 2022

Conversation

waveform80
Copy link
Contributor

There are a few problems with WS2812.get:

  • The tuple returned contains the types (int, float, float, float) instead of (int, int, int, int) (which is slightly more efficient and also consistent with the types used by set_rgb, and the internal structures); APA102.get had the same issue so I fixed that up at the same time.
  • The tuple returned simply copies the rgbw fields directly, but in the case of WS2812 (though not APA102) these aren't necessarily in the rgbw order (i.e. set_rgb corrects for color_order, but get doesn't "undo" this correction).
  • Finally, the internal implementation is pointing at APA102.get (which really confused me while working on this as every alteration I made to WS2812.get didn't seem to do anything! :)

This commit should fix things up, though I must apologize for my dreadful C++ knowledge; I'm sure there must be a more elegant way to declare the get_orders array than with a whole pile of static_cast, but alas I don't know it!

(and types of numbers returned by both WS2812.get and APA102.get)
@waveform80
Copy link
Contributor Author

Hmmm, actually there's a possible further issue here in that WS2812.get and APA102.get both return the values after gamma correction which is probably not what's intended (if a user sets an RGB value, then later queries it, presumably they'd expect to get the same value back)?

Let me know if this is also something that needs correcting, and I can have a look at either shifting the gamma lookup into the PIO logic or using two buffers, one post-gamma-correction to feed the PIO.

@Gadgetoid
Copy link
Member

Thanks- you're right, retrieving gamma corrected values is a little... odd. It's not - afaik - easily done in PIO since there's no way to lookup values.

However issue #236 makes a very strong case for double-buffering, and it would make sense to:

  1. have an RGB(w) "front" buffer that stores raw, uncorrected values
  2. have a uint32_t "back" buffer with corrected values
  3. copy values from the front to back buffer and gamma correct them in "update"

This would also allow APA102 and WS2812 to have a commonly accepted definition of an "RGB(w)" colour and for things like colour order - currently handled in a fairly heinous way for WS2812 - to also happen in the front->back copy rather than on the fly, per pixel.

It should be possible to fix these things without completely breaking the API, but it will expose the double-sized buffer wart to Python and require changes to any code that explicitly supplies a buffer.

@waveform80
Copy link
Contributor Author

Thanks- you're right, retrieving gamma corrected values is a little... odd. It's not - afaik - easily done in PIO since there's no way to lookup values.

Yes, discovered that after posting this (was assuming some kind of pointer arithmetic could be used, but nope!)

However issue #236 makes a very strong case for double-buffering, and it would make sense to:

1. have an RGB(w) "front" buffer that stores raw, uncorrected values

2. have a uint32_t "back" buffer with corrected values

3. copy values from the front to back buffer and gamma correct them in "update"

This would also allow APA102 and WS2812 to have a commonly accepted definition of an "RGB(w)" colour and for things like colour order - currently handled in a fairly heinous way for WS2812 - to also happen in the front->back copy rather than on the fly, per pixel.

Yup, that sounds like an excellent idea!

It should be possible to fix these things without completely breaking the API, but it will expose the double-sized buffer wart to Python and require changes to any code that explicitly supplies a buffer.

Indeed -- in the meantime, it's probably worth a look at this PR anyway just because it does correct the ref for WS2812.get, but it's certainly worth looking seriously at double-buffering for the lib. I'll see if I can find some spare time to look at it (but it likely won't be for a while yet)

@Gadgetoid Gadgetoid merged commit ab60213 into pimoroni:main Jan 25, 2022
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.

2 participants