added the TileApi to control real colorful lifx tiles#15
added the TileApi to control real colorful lifx tiles#15mabels wants to merge 4 commits intonode-lifx:masterfrom
Conversation
|
@mabels Based on a quick skim, your PR looks very promising! When finalising the work, please remember to:
Also note that you can use arrow functions even though it's not used much if at all on the existing codebase (the ES2015 rewrite was done quite literally overnight). At least I would prefer arrow functions especially when passing an inline function as a parameter. I can't speak for @Sawtaytoes though who set up this fork though. |
mabels
force-pushed
the
master
branch
2 times, most recently
from
acad593 to
88691bb
Compare
|
Massive undertaking. I'll try to review and test this as soon as possible. Sorry if it might take a couple of days! I do have a Tile set to test though. |
|
@mabels I'm sorry I haven't still had time to review and test this. Just letting you know I haven't forgot this! I just noticed a PR submitted to node-lifx-lan a couple of days ago and it'd be cool to have this merged before that. 😂 The problem is though that I'm quite busy today and away from home the rest of the weekend. I'll try to find time today but it doesn't look too good. |
|
My plans for today/tomorrow got cancelled. I'll at least start reviewing/testing now. |
|
It's actually not a good idea to review the code at it's current state as it contains the already merged changes. @mabels could you update your branch and force push the changes? I'll still test now if I get it to work with my Tiles. |
|
Sorry a somewhat off-topic note:
Quoting myself - now when I started to review the changes @mabels has done, I realize there's still two open PR's from me that's aren't included in this repo. When commenting about the arrow functions, I had an odd feeling like I would have done "something" to that already: MariusRumpf/node-lifx#62. This is the rewrite I was talking about. @Sawtaytoes It looks like I forgot PR 62 completely. Would it make sense for me to try to rebase it and do a PR? Or do you prefer working without the use of classes. I've personally been leaning towards more functinal programming approach (mainly at work) but I think using classes would make sense given how the library is written. There'd be a lot less clutter. |
ristomatti
left a comment
ristomatti
left a comment
There was a problem hiding this comment.
@mabels Added a couple of comments but I'll check the other changes after you've rebased the branch.
Whatever your preference is fine. I think bringing in those old PRs from your other repo would be fantastic! I also like arrow functions. As long as our Node.js compatibility is fine, then we're good to go; otherwise, we should just bring up the min Node.js version and everything will be easier going forward. |
|
Node and Arrowfunctions: for i in boron carbon dubnium ST 1 master we should never go to 4.x so on my expirence we are fine. |
we should integrate "Arrow functions completly" after this PR is on master. |
ristomatti
left a comment
ristomatti
left a comment
There was a problem hiding this comment.
Added few comments. My JS limit for today has maxed out now though. I'll see if I could get this running tomorrow or Sunday.
| console.log('New light found.'); | ||
| console.log('ID: ' + light.id); | ||
|
|
||
| light.getDeviceChain(function(err, chain) { |
There was a problem hiding this comment.
This would now attempt to call setTileState64 on any light. Add a const TILE_LABEL = 'MyTile'; and trigger only on a light matching the label.
There was a problem hiding this comment.
This does not work as light label has not resolved at this point. It was partly my bad as I didn't remember this while reviewing this the first time. It would improve the example a lot if a label could be used but for this purpose it would be enough to just rename TILE_LABEL TO TILE_LIGHT_ID for example.
|
@mabels @ristomatti Just your friendly neighborhood ping :). |
I'm still waiting for @mabels. I guess eventually I could try sorting the issues myself but haven't had time to think about this too much unfortunately. On a related note, I ran across this stuff that's based on the "competing" node-lifx-lan lib that looked pretty cool. Haven't tested it though: |
object and callback. removed validation clutter from set/getTileState64 added some comments in the example
|
Thanks @mabels. I will check and test this later today (= within the next 12 hours). If I won't, you're welcome to merge this @Sawtaytoes. |
|
@ristomatti Merging in 1 hour. |
|
@Sawtaytoes Thanks for the heads-up (although I saw it only just now). Sadly I was blocked by a force majeure. Would it be possible for me to email or PM you on this? You can find my email from my commit messages. Your location indicates you're 8 hours behind but I'm a night owl so there should be plenty of hours reach me. |
|
Impossible things can happen - testing now. |
|
@mabels Did you test this after your changes? I can't get this to work. There's multiple issues. When running After replacing renaming 3 calls to |
|
@Sawtaytoes Sorry but this is not ready yet. I don't think we should rush this in as there might be other issues also. For instance |
| * @param {Number} tileIndex unsigned 8-bit integer | ||
| * @param {Number} userX 32-bit float | ||
| * @param {Number} userY 32-bit float | ||
| * @param {Number} reserved 16-bit integer |
There was a problem hiding this comment.
What is this reserved value and how would a user of this library know what to put there? This applies to all functions.
|
with the unused options that is my fault.
I will fix it and build a test that it not happend again
meno
…On Sat, 14 Dec 2019, 16:33 Ristomatti Airo, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
------------------------------
In src/lifx/light.js
<#15 (comment)>
:
> + const sqnNumber = this.client.send(packetObj);
+ this.client.addMessageHandler('stateDeviceChain', function(err, msg) {
+ if (err) {
+ return callback(err, null);
+ }
+ return callback(null, msg);
+ }, sqnNumber);
+};
+
+/**
+ * Sets Tile Position
+ * @example light.setUserPosition(0, 12, 13, 0, () => {})
+ * @param {Number} tileIndex unsigned 8-bit integer
+ * @param {Number} userX 32-bit float
+ * @param {Number} userY 32-bit float
+ * @param {Number} reserved 16-bit integer
What is this reserved value and how would a user of this library know
what to put there? This applies to all functions.
------------------------------
In src/lifx/light.js
<#15 (comment)>
:
> + * each tile.
+ * @param {Number} tileIndex unsigned 8-bit integer
+ * @param {Object} options - options passed to
+ * @param {Number} options.length unsigned 8-bit integer (default 64)
+ * @param {Number} options.x unsigned 8-bit integer (default 0)
+ * @param {Number} options.y unsigned 8-bit integer (default 0)
+ * @param {Number} options.width unsigned 8-bit integer (default 8)
+ * @param {Number} options.reserved unsigned 8-bit integer
+ * @param {Function} callback a function to accept the data
+ */
+Light.prototype.getTileState64 = function(tileIndex, options, callback) {
+ validate.isUint8(tileIndex, 'getTileState64', 'tileIndex');
+ if (typeof options === 'function') {
+ callback = options;
+ options = {};
+ }
What's the purpose of the above block?
------------------------------
In src/lifx/light.js
<#15 (comment)>
:
> + * @param {Number} colors[64] 64 HSBK values
+ * @param {Object} options - options passed to
+ * @param {Number} options.length unsigned 8-bit integer (default 0)
+ * @param {Number} options.x unsigned 8-bit integer (default 8)
+ * @param {Number} options.y unsigned 8-bit integer (default 8)
+ * @param {Number} options.width unsigned 8-bit integer (default 8)
+ * @param {Number} options.duration unsigned 32-bit integer (default 0)
+ * @param {Number} options.reserved unsigned 8-bit integer
+ * @param {Function} [callback] called when light did receive message
+ */
+Light.prototype.setTileState64 = function(tileIndex, colors, options, callback) {
+ validate.isUint8(tileIndex, 'setTileState64', 'tileIndex');
+ if (typeof options === 'function') {
+ callback = options;
+ options = {};
+ }
What's the purpose of the above block? Also note that options is assigned
to {} but options is not used after this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15?email_source=notifications&email_token=AAAEWJWF5L7P4JR5RRZJP3TQYT4DJA5CNFSM4IWUA6Y2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCPGUDFY#pullrequestreview-332218775>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEWJRMQTAPM6KLROLHV2DQYT4DJANCNFSM4IWUA6YQ>
.
|
I could not test this change off course i had no lifx on hand. |
|
|
I need to build a test for the high order function. I'm sorry for this chain of failures. |
3 participants
I need to cleanup this PR some sanity checks are missing and
i was to lazy to build the tests.
I promise that I complete this missing stuff in the next weeks.
This PR is just an reminder to me that i had to completed this!