Do the Additions outweigh the mess?#11
Conversation
…em.drawing.color, add new tricks This bugs me. Also, make LifxColor a class that can handle RGB and HSB values seamlessly, versus having to do weird stuff to convert between.
First of several new additions - this is for zoned devices.
Start adding necessary methods to control multi-zone devices.
Generic linting. Fix names, privacy declarations, other misc stuff.
Add legacy zone setters/getters, response messages.
Now we can just serialize any color directly to a bytewise HSBK value.
…es() Convert else/if to switch, add option to serialize LifxColor directly, instead of having to do it before calling our broadcast method.
Use proper variable types everywhere as specified by Lifx docs. Code cleanup
Bump .net standard version to 2.0, update project details. Add "Payload" class to handle smarter/cleaner parsing of responses from Lifx with auto error-handling and logging. Add tile message types. Add tile response types. Split devices into their own classes for easier management. Create TileGroup, Switch, and Strip device types, instead of calling everything "bulb". Add device product check on discovery to determine proper device type to return. Update readme Add default initializer for LifxColor.
This should be a near-100% complete set of features, as per the official documentation
Function should return a StateColorZones, not LifxResponse.
It would probably be "better" to create an individual device class for each one, but that requires an additional call to the device to figure out what it is, which seems to be a bit out of scope for the library currently. So, instead, we'll just use the generic "device".
Update logs in netcore sample app to provide more info so we can remotely debug issues. Update object passed to device calls (device vs. bulb)
|
@dotMorten - So, it's been 15 days since I submitted this...have you had a chance to review it at all? |
No I have not (obviously ;-) ). |
Make StateMultizoneRequest return StateMultiZone response, versus StateZone response.
Ensure payload can properly serialize a tile and datetime.
Testing encoding/decoding of responses when you don't actually own any Lifx Devices is pretty hard, so I created a simple "LifxEmulator" app that can sit within a LAN and respond to messages from LifxNet like a real device. Some emulation features are still WIP, but the majority of "the things" work.
Make the .netcore app return basic info about switch/tile/multizone devices if discovered.
Thanks to a few intrepid users who collected me "proper" packet responses from Lifx Tile devices, we now have Chain responses working properly.
Use proper terminology when describing payload methods. Make _data readonly.
Because the command is buffered, we can choose whether to apply it immediately or not.
Add Statepower response for testing purposes Add more logging, colored console
I understand the time constraints. At this point, a week later, the disparity between our repositories only continues to grow. As it's going to be incredibly difficult to examine the code directly, the best I can do is summarize:
So, while it looks like a lot of stuff, the end result isn't really that many changes... |
Add methods to retrieve Lifx Color formatted values directly. Make documentation more useful Replace HSB conversion method.
This one only needs the device and color, and an optional transition time in MS...
| public string GetString(long length = -1) { | ||
| if (length == -1) length = _len - 1 - _ms.Position; | ||
| try { | ||
| return _br.ReadChars((int) length).ToString(); |
There was a problem hiding this comment.
I think that this should be return new string(_br.ReadChars((int) length)); . As it stands, this line returns the typename eg System.Char[] when it's called via LifxClient.GetDeviceLabelAsync()
| client = await LifxNet.LifxClient.CreateAsync(); | ||
| client.DeviceDiscovered += Client_DeviceDiscovered; | ||
| client.DeviceLost += Client_DeviceLost; | ||
| client = new LifxClient(); |
There was a problem hiding this comment.
This line is wrong, the only LifxClient constructor is marked as Private
| { | ||
| private LifxClient() { | ||
| IPEndPoint end = new IPEndPoint(IPAddress.Any, Port); | ||
| _socket = new UdpClient(end) {Client = {Blocking = false}, DontFragment = true}; |
There was a problem hiding this comment.
DontFragment = true throws a NotSupported exception with .NET 5 on MacOs Big Sur. Setting the property to false seems to cure the issue.
3 participants
I'll be the first to admit, I made a mess with this one.
Generally, when I do pull requests, I try to be a lot more fastidious about separating out features from reformatting and code cleanup, but in this case, there was just so much, it's going to be really hard to do without another git rebase, and then trying to cherry-pick out commits.
I'm not sure if you'd like to review all the things I've done and then pull it in as you see fit, or just merge it all and do a version bump, or what...but I did at the very least try to revert any breaking changes I made...except for re-factoring "Color" to "LifxColor", which I think is a useful enough addition to warrant one breaking change.
The new LifxColor class serves as both a wrapper for standard System.Drawing.Color functions, and also as a simple way to convert between HSBK/RGB/HSL color values.
Additionally, I've created a new "Payload" class that allows for rapid and clean serialization/deserialization of message values...and implemented it everywhere. Works wonderfully.
Aside from new classes and cleanup...I've added support for all of the messages described in the Lifx Documentation, which was greatly aided by the new Payload class.
As such, I've also added support for all of the various Lifx Devices. All the multizone stuff, tile support, and switches.
However, one thing I'm kind of iffy on, or where I didn't follow the model you had started with - is "other" devices. I see you have a base "device" class, and then "LightBulb" for bulbs...but I did not create classes for the other devices, as there didn't seem to be a "clean" way to enumerate the device type on discovery. I tried it - it broke things.
So, that's one thing I'd like to do, but not sure the right approach. Ideally, in my mind, Discovery returns a list of specifically classed devices so the user knows what features are available. Otherwise, option B would be to make all devices generic devices, and then make the user responsible for determining what capabilities a device has.
Similarly...I was thinking that it might be good to have all messages return an Acknowledgement if applicable...versus just discarding them?
So, consider this more of a "Hey, let's talk about this stuff" versus a cut-and-dry PR. I'd like to do this right and keep the library in your hands.