eminence / lifx

LIFX LAN network protocol in rust
Apache License 2.0
20 stars 9 forks source link

SetExtendedColorZones colors definition #15

Open brianmay opened 1 year ago

brianmay commented 1 year ago

I am somewhat uncomfortable with this definition:

https://github.com/eminence/lifx/blob/08526bb6bb260be75fa1f5e90904f1643050fff2/lifx-core/src/lib.rs#L1221

It has the following problems:

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 commented 1 year ago

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 commented 1 year ago

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.