Skip to content
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

Brightness / Dimness settings fixes #75

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

scoobyshi
Copy link

It seems that via wink-js we are not receiving the expected "desired_state", except for when it's been applied (see winfinit/wink-js#12). Thus, any checks for this will error out incorrectly. While I can see this node typically populated if I query the API directly (eg via Postman), I don't see it returning data via wink-js. I would say the problem is there except that in the Wink documentation for the API it specifically mentions this should be expected "http://docs.winkapiv2.apiary.io/#reference/device/desired-state-and-last-reading/retrieve-all-devices-of-user": "When the device acknowledges that the state has been applied, the server will clear the field from desired_state."

Granted, that is the v2 documentation, and the v1 API specifically states that desired_state will describe writeable states, I suspect Wink has perhaps deprecated that behaviour.

As a result, the checks in accessories/light_bulbs.js result in no brightness characters being set (see #61); However, if we check last_reading, it does get set correctly. I tried this with numerous bulbs and switches (Lutron switch, Ecosmart A19 Bulb, Par20 Pot lights).

Also, due to the same concern with desired_state, when we're checking for whether to retry or not (in lib/wink_accessory.js), based on the behaviour I've been observing and the updated Wink documentation, it seems we should be checking for the absence of desired_state properties, since they get cleared once applied. Otherwise, desired_state.powered (for example) will never equal last_reading.powered... which was the behaviour we observed.

Once I applied the changes in this optional patch, the behaviour seemed much more consistent with how it likely behaved previously.

@ckdake
Copy link

ckdake commented Jan 20, 2017

+1 on this working correctly.

@scoobyshi
Copy link
Author

Any chance of this being merged or reviewed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants