-
Notifications
You must be signed in to change notification settings - Fork 20
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
Add Tile support #25
base: main
Are you sure you want to change the base?
Add Tile support #25
Conversation
object and callback. removed validation clutter from set/getTileState64 added some comments in the example
- changed api of setUserPosition to args parsed by props object added full stack tests - added full stack tests to getDeviceChain - added full stack tests to setTileState64 - added full stack tests to getTileState64
chore: remove some comment clutter
@mabels I believe you closed it yourself. If not, then it was by mistake... |
Yes might be my fault. Is there a change to get this merged? |
Sorry @mabels. I haven't been able to give any time for my hobby projects. Life got in the way. I'll try to find time during next week but can't promise. Notification of this stares me every day at the top of my inbox as a starred message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote a few comments on some things I noticed, but a lot of this is pretty low-to-the-hardware UDP on how to get a device chain and send messages for the Tile API.
@ristomatti I think this is probably safe to merge. If you wanna do a quick skim, I'd feel more comfortable.
const client = new LifxClient(); | ||
|
||
const LOOPTIME = 1000; | ||
const TILE_LABEL = process.argv.length > 2 ? process.argv[process.argv.length - 1] : '*'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this to come from the command line execution or could it be done differently? Also, there's a library yargs
which makes it a lot easier to pull in command line parameters. If we're doing this in Node.js anyway, why not pull it in from a config when creating the LIFX client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on this one. At least the script should fail if run without arguments and instruct on the correct usage. Example:
Usage: node tile.js <label>, e.g. node tile.js "My Tiles"
setTimeout(() => setBits(light, 0, chain), LOOPTIME); | ||
return; | ||
} | ||
console.log('getBits', idx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this debugging info that needs to be removed? There are a few instances like these. Do we want to use console.info
or console.debug
instead? I always think of console.log
statements as temporary for local development only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it an test example programm, so why not 'log' that something is happens here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are fine, they indicate at least what part of the code is currently executing. They could of course be more informative as I'm suspecting the API might be quite difficult to graps for many.
I can do a quick test today with my Tile set. I'm on sick leave for the @Sawtaytoes If we go ahead and pull this in, it's OK for me to publish a release but I'd |
Pulling the changes now. 🤞 |
I got it to do what it is supposed to do I believe on my second try. First attempt (my Tile sets label is "Tile"):
I checked the code, and it's trying to match against the light
End result was pastel shaded randomly changing gradients. Sounds reasonable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge this if you can:
- a) make this work with the light
label
and not theid
(calllight.getState
for each found light to match against the label given by the user. OR - b) make the script interactive, so that it will give a list of Tile's found using
light.getHardwareVersion
and allow selecting one by entering a number - implement the other changes requested by me and @Sawtaytoes
This merge would be for further refinement. As there's no update on README.md
, I'd vote for a only a change log comment "Experimental Tile support added" and leave the API undocumented in the README.
const client = new LifxClient(); | ||
|
||
const LOOPTIME = 1000; | ||
const TILE_LABEL = process.argv.length > 2 ? process.argv[process.argv.length - 1] : '*'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on this one. At least the script should fail if run without arguments and instruct on the correct usage. Example:
Usage: node tile.js <label>, e.g. node tile.js "My Tiles"
setTimeout(() => setBits(light, 0, chain), LOOPTIME); | ||
return; | ||
} | ||
console.log('getBits', idx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are fine, they indicate at least what part of the code is currently executing. They could of course be more informative as I'm suspecting the API might be quite difficult to graps for many.
} | ||
|
||
client.on('light-new', (light) => { | ||
console.log(TILE_LABEL, light.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this console.log
, it only prints out mysterious garbage of lights that aren't matching the given label (or in this case actually the id).
# Conflicts: # babelhook.js # src/lifx/light.js # test/unit/light-test.js
@mabels I managed to resurrect my problematic Tile set a while ago. I noticed you've kept these changes on your clone and even merged in some other updates. Have you been using the added functionality / has it worked throughout the 4 years of testing 😁? |
I just looked into my HA and there is still this code -> and my tiles cheer me every day with some random patterns. meno |
Yes! Please publish a release @ristomatti! :) |
@mabels I got the branch rebased to the point where tests pass but I'm hesitant to push the changes back as it's your master branch and I haven't even tested if everything works properly. That will need to wait for another day. It's 2:23 and I just ran the unit tests, forgetting there's that damn test that turns on all the lights (while my partner is asleep). 😬 |
I've pushed the rebased branch here: feature/tile-support. |
Finally when I'd be on vacation and would have had time to do this, my Tile set PSU dies. I had just painstakingly* detached and reattached 4 of the tiles while debugging why they stopped working. This was only to find that the issue was actually a bad connection on the PSU end of the cable and the whole process was pointless. Just when I had tested the whole set works again, I then accidentally wiggled the connector while adjusting the power cable (with the set powered up). The connector started sparking. After I detached it, I realize the PSU is still sparking inside the connector. The magic smoke had escaped. *) When detaching the tiles, 10 out of 16 of those 3M Dual Lock fasteners left the sticky side of the tape on the wall. I didn't have spare tape on hand so I had to super glue the detached locking half of each tape back before reattaching the tiles. It looks like I'm not gonna find a replacement any time soon either. LIFX has ran out of stock of replacements and the PSU is completely custom built and not just any 24V power brick. It not only has a proprietary UC-E6 8-pin connector but it also sends a 50-60Hz 3.3V square wave "ID signal" through one of the wires. There's even this 4 year old thread on Reddit on building a custom power supply, including tips from LIFX employees but it appears no one has been able to succeed getting more than one Tile working this way. I might give that a shot at some point though as I happen to have the required gear to do it, but first I'll need to source a cable with a female UC-E6 connector in order to even check if the Tile's got damaged in the process also. At the end of the day, I even looked up if I could find a used or new complete Tile set but they seem to have been hunted to extinction.
I'll end this comment with another quote. System_Virus @ /r/lifx 7mo ago
|
Hello,
i don't know why my previous PR was closed?
There was some tests missing which I added now, and I fixed the example program.
Could you please add these now?