sidoh / ledenet_api

An API for the LEDENET Magic UFO LED WiFi Controller
MIT License
46 stars 12 forks source link

Can't get API to change the colour #4

Open jamesmgreen25 opened 7 years ago

jamesmgreen25 commented 7 years ago

Hi sidoh,

Thanks for your hard work - I was halfway through reverse engineering it myself before I found yours!

I'm trying to get this API to work with my LED device. I can get it to turn on and off, and to give full status readouts, but for the life of me I can't get it to update the colour of the LEDs. I've tried from the command line and from inside a script.

Is there anything you can suggest that I might try? I'd be very grateful for any insight you might have.

Many thanks,

James

sidoh commented 7 years ago

Which device are you using? I'm using this thing. Could be that the TCP APIs differ slightly.

jamesmgreen25 commented 7 years ago

Ah - I’m using one of thesehttps://www.amazon.co.uk/XCSOURCE-Android-Channels-Controller-LD686/dp/B01AA6221S/ref=sr_1_7?ie=UTF8&qid=1483489541&sr=8-7&keywords=xcsource+LED by XCSOURCE It uses the same Magic Home app for control though, and I thought most of these LED controllers all ran on the same chip anyway.

I’ll go back to my packet analysis and hopefully find some difference in the commands.

Thanks again!

On 4 Jan 2017, at 00:19, Chris Mullins notifications@github.com<mailto:notifications@github.com> wrote:

Which device are you using? I'm using this thinghttps://www.amazon.com/LEDENET-Controller-Android-Smartphone-Control/dp/B00MDKOSN0. Could be that the TCP APIs differ slightly.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/sidoh/ledenet_api/issues/4#issuecomment-270263247, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AXwC-bzr5jCTk2teL7dGJKhCAlBcPHMbks5rOuWSgaJpZM4LaJ3g.

sidoh commented 7 years ago

Ah, interesting. Let me know what you find! Would love to incorporate more devices if it's not too hard.

Packet structure for this project is here: https://github.com/sidoh/ledenet_api/tree/master/lib/ledenet/packets

jamesmgreen25 commented 7 years ago

I've had another look at my findings from before, and compared them more closely to the packets your code sends. I now have a list of packets that work when sent manually (at least for colour changing). I've noticed three things:

  1. The packets I caught sending from my phone had 9 bytes of data. There was an extra pair of 0s. I assume this is because my controller runs RGB, WW and CW too (though I only use RGB at the moment).

  2. The "remote or local" thing really doesn't seem to make any difference (as you mention in a comment in the code). The packets I'd traced from my phone simply sent 0x31, the RGB values, then 4 pairs of 00s and the checksum, and these work fine.

  3. The final byte for these 9 byte packets is calculated by getting the checksum from the first 8 (I used this one) and subtracting it from 100 (e.g. Green - 31 00 ff 00 00 00 00 00 has d0, 100-d0 = 30, so 31 00 ff 00 00 00 00 00 30 is the working packet).

At this stage I have a list of packets that, when sent, operate the controller as desired. I don't know how difficult it might be to include all this in the project, but I shall have a go tomorrow and see whether I can get it working (it's about 3am now and I'm tired). Presumably, if I'm right about the extra byte being for the CW channel, it would be possible in theory to update the project to be able to drive that too.

sidoh commented 7 years ago

Ah, yeah. This definitely makes sense. If this ends up being the difference, probably wouldn't be too hard to incorporate it without muddling the interface.

Maybe works out to be the same thing, but I calculate the checksum using sum(packet bytes) % 0x100: https://github.com/sidoh/ledenet_api/blob/master/lib/ledenet/packets/fields/checksum.rb

jamesmgreen25 commented 7 years ago

The checksum does in fact work the same way - I'd not clocked that before.

I've forked the repo and I'm having a go at modifying it. I've identified the following regarding the status response packets, adapting your comments in status_response.rb for my findings:

      On FID   SP RR GG BB WW ?? CW ?? CS
81:25:23:61:21:00:00:ff:6a:26:01:ff:0f:e9
81:25:23:61:21:00:00:ff:6a:26:01:52:0f:3c
81:25:23:25:21:0f:ff:00:00:26:01:52:5a:f0
81:25:23:25:21:04:ff:a6:00:26:01:52:5a:8b
81:25:23:25:21:04:ff:aa:aa:26:01:00:0f:9c

Somewhat strange that the cool white is stored in the 12th not the 11th - perhaps there's another feature of the device used by that byte.

I've managed to adapt it now so that (using the command line app), the --status flag reveals cool-white data and so that the --c flag sets it. I would assume that this change will break functionality on RGBW devices (due to the extra byte) but I don't have one to test. What do you suppose the best course of action is from here? Extra logic on API initialization to include/exclude the CW bytes?

sidoh commented 7 years ago

Yeah, I think that makes sense. Guess there are two high-level approaches I'd consider:

  1. Add another parameter to API initialization here
  2. Pull out common parts of API, stuff what's different into separate implementations of LEDENET::Api and have it delegate to the common methods where there's overlap.

(1) is probably easier, but involves runtime branching, which I'd prefer to avoid. (2) seems nicer in theory, but might end up being a more of a mess than it's worth.

Would also need to update the packet definitions.

It sounds like you're saying that the CW byte is the final byte of the packet -- is that right? I think that complicates the way the checksum byte is being calculated. :(

sidoh commented 7 years ago

I guess (2) is probably also nicer if there are other devices that have slightly different packet structures.

jamesmgreen25 commented 7 years ago

No, sorry - the CW byte is 6th, so the packet 31:00:00:00:00:ff:00:0f:3f sets RGB and WW to 0, and sets CW to 255/ff). Incidentally, I got that packet by Wiresharking the modified command line app. Fortunately, the checksum calculation is the same - I didn't need to touch that, only the checksum is now the 9th byte not the 8th. Here are the changes I've made so far.

I think you're right that 1. is probably easier to implement, but 2. may prove better in the long run. I think I'm understanding that the plan would be to have a common class and then inheriting classes that are then used for each kind of device? Would it be simpler to just create another class which inherits the existing LEDENET::Api and overrides where necessary? Forgive me if I'm being a bit dense here, or there's something I'm missing.

sidoh commented 7 years ago

Got it, makes sense.

To make sure I'm following -- looks like the structure of the status packet is unchanged (with the 24 byte unused payload changing to 8 unused, 8 CW, 8 unused), but the change color packet adds an extra byte. Is that right?

I was thinking composition might be cleaner here, but it could get too messy to have delegate methods for everything that's common.

Definitely not being dense. Thanks for digging into this!

jamesmgreen25 commented 7 years ago

Yes, the status packet is the same - just 8 bytes of the 24 "unused payload" is used for the CW value. I've adjusted the status response only to separate out the used 8 bytes from this section of the packet. The colour change packet does indeed carry an extra byte.

The 'functions' code seems to work for my device, but I don't tend to use them so have focused on just the colours for now.

It seems that there really isn't much difference between the two - it looks like a question of just adjusting the packet depending on the device I suppose. It suddenly occurs to me that I haven't looked at device discovery, and whether that might signal the type of device.

OK, great - I'm happy to defer to your opinion on how to proceed now. From my perspective, I've now got it working as I want on my machine, but I'd be more than glad to help if you're still keen to adapt your project to include this, and I can be of use in doing so.

sidoh commented 7 years ago

Yeah, would love to adapt this code to support the device you have. It's actually significantly cheaper than the ones I developed against. Would imagine plenty of people would prefer to use a controller that's half the price. I just ordered one.

The device discovery response packet doesn't have a ton of info. The "model number" mine responds with looks to be (or at least contain) a part number for a WiFi module. I think the discovery aspect is implemented by this module (some info in the code comments). Not sure if your device uses the same module. Maybe it's using ESP8266 or something.

sidoh commented 7 years ago

Super happy to collaborate on updates to this project. If you wanna throw up a PR, I'll definitely take a look. I'll try to get around to it myself in coming weeks if you'd prefer that.

jamesmgreen25 commented 7 years ago

Oh right - yeah, makes sense. Probably why I bought it at the time (had the thing nearly six months now).

ledenet-ufo --list gives the model number as HF-LPB100-ZJ200 and the MAC address begins ac:cf:23.

Fantastic. I'll have a bit of a go and see if I can manage an elegant way to include both options - no harm in trying - and then I'll submit the PR for review and thoughts.

markusressel commented 7 years ago

Hey there, I have written a python api based off of this library that (from what i've read here) seems to implement this exact protocol. I don't want to advertise my project too much since all of the hard work was already done by @sidoh and I gladly used it, but for anyone interested: https://github.com/markusressel/sunix-ledstrip-controller-client

Huge thx again to @sidoh

sidoh commented 7 years ago

Nice, @markusressel! This one might be of interest as well. Not sure how closely it relates to the hardware you're dealing with.

peterevertz commented 6 years ago

I have mad a fork for the 5 channel Controller: https://github.com/peterevertz/ledenet_api

Thanks to sidoh