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

ws2812 mix: add all, then divide #2032

Merged
merged 1 commit into from
Oct 22, 2017
Merged

Conversation

nwf
Copy link
Member

@nwf nwf commented Jul 8, 2017

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.

This flips the ws2812 framebuffer mix function to add all the contributions together before dividing; this allows one to, as I do, ensure that a channel stays on even in light of other operations: basefb:fill(1,1,1) ; outfb:mix(255,basefb,256/dimfactor,infb), which results in dimming that doesn't zero out color channels.

No docs changes necessary, I think.

@marcelstoer marcelstoer requested a review from pjsg August 16, 2017 19:49
@marcelstoer
Copy link
Member

@Alkorin does this look reasonable to you?

@Alkorin
Copy link
Contributor

Alkorin commented Aug 31, 2017

What's the size of an 'int' ?
Would be better to use an uint32_t to avoid any undefined behaviour

@marcelstoer
Copy link
Member

@nwf do you plan to address Thomas' feedback?

@nwf
Copy link
Member Author

nwf commented Sep 28, 2017

Mm, sorry, I am busy writing my PhD thesis, so time is basically, uh, zero. That said, I don't object, but I'm not sure a uint32_t is really necessary; a uint16_t would handle mixing hundreds of framebuffers without overflow, no?

@Alkorin
Copy link
Contributor

Alkorin commented Sep 28, 2017

As source[src].values is an uint8_t and factor an int, you already have an overflow, as only a int32_t may store the result. If factor were only an uint8_t, an uint16_t can only store the result of 1 framebuffer (255 * 255).

That's why I think int32_t is a better solution.

@nwf
Copy link
Member Author

nwf commented Sep 28, 2017

@Alkorin Erm, yes, of course. OK. I'll make the change when I get a minute.

This achieves rounding between multiple summed frame buffers
@nwf nwf force-pushed the for-upstream-ws2812 branch from 38835b0 to 3a926e8 Compare September 30, 2017 06:44
@nwf
Copy link
Member Author

nwf commented Sep 30, 2017

Let's that on for size (thesis document done, too!)

@nwf
Copy link
Member Author

nwf commented Oct 22, 2017

@Alkorin @marcelstoer ping

@marcelstoer marcelstoer added this to the 2.1 follow-up II milestone Oct 22, 2017
@marcelstoer marcelstoer merged commit 5c8619e into nodemcu:dev Oct 22, 2017
@nwf nwf deleted the for-upstream-ws2812 branch November 17, 2017 01:26
crasu pushed a commit to crasu/nodemcu-firmware that referenced this pull request Jan 11, 2018
This achieves rounding between multiple summed frame buffers
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.

3 participants