eminence/lifx

SetExtendedColorZones colors definition

Opened this issue · 2 comments

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 :-)

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

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.