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

SetExtendedColorZones colors definition #15

Open
brianmay opened this issue Jan 29, 2023 · 2 comments
Open

SetExtendedColorZones colors definition #15

brianmay opened this issue Jan 29, 2023 · 2 comments

Comments

@brianmay
Copy link

I am somewhat uncomfortable with this definition:

colors: Box<[HSBK; 82]>,

It has the following problems:

  • The 82 value is hard-coded, perhaps at bare minimum should be a constant.
  • Creating a Box<[_; 82]> type is non-trivial.
  • Contrary to what the docs say, I don't believe this needs to be exactly 82 entries long in outgoing packets. Sending a packet with less then 82 entries and the correct count in colors_count is OK also. This results in a outgoing packet that is considerably larger then required.

My feeling is this could be replaced with a Vec<HSBK> type.

This in turn could make the colors_count redundant. As the Vec type has its own length. Unless you really do want to ability to parse all 82 colours on incoming packets where colors_count is less then 82.

I could work on a pull request if you want, but thought I should perhaps discuss the issue first :-)

@eminence
Copy link
Owner

At the time, my reading of the LIFX docs suggested that all 82 entries are required. If that's not true, we can adjust things.

Have you tested sending less than 82 fields? Do you what other LIFX libraries do? A took a very brief look at a python library and it also seems to require all 82 color objects.

This might be a good question for the LIFX developer forums

@brianmay
Copy link
Author

brianmay commented Feb 1, 2023

Yes, I tested this with my Elixir library, and it did seem to work: https://github.com/brianmay/lifx/blob/07a8490c418a809b65953927d8d377e8dec9fe20/lib/lifx/protocol.ex

Regardless might still be worth asking in the developer forums. But it kind of seems strange to require a starting number, a length, and then require exactly 82 colors anyway.

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

No branches or pull requests

2 participants