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

Expand palette to 24 colors #1418

Merged
merged 6 commits into from
May 6, 2024
Merged

Conversation

EvyBongers
Copy link
Contributor

Following up on #1416, this PR updates the led palette to support 24 colors

Signed-off-by: Evy Bongers <[email protected]>
@EvyBongers
Copy link
Contributor Author

So far it's just the bare minimum to support 24 colors. We should probably use a variable to define the palette size and and update the macro to accept a list of arguments rather than hard coding a fixed number of arguments.

@obra
Copy link
Member

obra commented Apr 20, 2024 via email

@EvyBongers
Copy link
Contributor Author

The way it is structured now, probably. I'm thinking that there should be a way to refactor it that supports any number of colors (like how qukeys are handled). That should supply a default to black (off) in case the sketch supplies less colors that the palette supports.

@obra
Copy link
Member

obra commented Apr 30, 2024

Sorry for my radio silence. I think the only thing left on this PR is the macro change not breaking existing sketches - https://github.com/keyboardio/Kaleidoscope/actions/runs/8772302322/job/24071073901?pr=1418#step:5:996

@EvyBongers EvyBongers force-pushed the expandColorPalette branch from f65db1f to ff8e3b2 Compare May 2, 2024 19:02
@obra
Copy link
Member

obra commented May 2, 2024

@EvyBongers
Copy link
Contributor Author

I noticed. The weird thing is that the M100 sketch I have locally (that uses the same macro) does build just fine. I just haven't included that because it includes most (if not all) the other changes that we discussed in the redesign issue. I have yet to figure out what the relevant difference is.

@obra
Copy link
Member

obra commented May 2, 2024

It may be an issue with the older GCC used for AVR Arduino.

@EvyBongers
Copy link
Contributor Author

It may be an issue with the older GCC used for AVR Arduino.

That will be the issue.

The extra comma in printf("hello",) is a syntax error in C and C++. C++20 solved this problem by adding a new special identifier, VA_OPT. The sequence VA_OPT(x), which is only legal in the substitution list of a variable-argument macro, expands to x if VA_ARGS is non-empty and to nothing if it is empty.

(https://www.scs.stanford.edu/~dm/blog/va-opt.html#variable-argument-macros)

Signed-off-by: Evy Bongers <[email protected]>
@EvyBongers EvyBongers marked this pull request as draft May 3, 2024 18:26
@EvyBongers
Copy link
Contributor Author

I found a way to get the build passing. I have yet to verify if the code functions as expected, which is why I converted it back to draft

@EvyBongers EvyBongers marked this pull request as ready for review May 4, 2024 11:40
Signed-off-by: Evy Bongers <[email protected]>
@obra obra merged commit 71e2735 into keyboardio:master May 6, 2024
8 checks passed
@EvyBongers EvyBongers deleted the expandColorPalette branch May 7, 2024 16:15
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