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

[WLED] Fix Nightlight sleep timer to use default time #11389

Merged
merged 1 commit into from Oct 18, 2021

Conversation

svjense
Copy link
Contributor

@svjense svjense commented Oct 15, 2021

[WLED] Fix Nightlight sleep timer to use default time

Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

@svjense svjense requested a review from Skinah as a code owner October 15, 2021 06:26
@lolodomo
Copy link
Contributor

lolodomo commented Oct 17, 2021

Code looks good.
Let's wait for binding author approval first

@Skinah
Copy link
Contributor

Skinah commented Oct 18, 2021

What firmware version has it been tested to work on? I suspect it has changed how it operates since I tested with an older firmware as if you look at the API and the replies back from a camera you will see that NL=1 means it is enabled and not that the 1 is a time in minutes.

happy for it to be merged but I suspect it will fix a newer firmware whilst breaking anyone on the older firmware so if you can reply with version I can look at abstracting the Api to its own class and then extend the class to be different per version. I think this is going to be needed for this fast moving project.

The opensprinkler binding has an example of how the api class can be extended for each firmware version so all versions keep working.

@svjense
Copy link
Contributor Author

svjense commented Oct 18, 2021

Sorry, forgot to mention I tested on the latest stable 0.12.0. Looking into Aircookies code i don't think &ND will break older versions but the oldest version I can find on github is 0.10.0 here: https://github.com/Aircoookie/WLED/blob/71886c162b7df2ce027b32c0334b83152e39884c/wled00/set.cpp#L571
Maybe you can use &ND&NL=1 combined for backward compatibility. That should take the default value for time and ignore the 1 minute in firmware >=0.10.0 (probably older versions as well).

@Skinah
Copy link
Contributor

Skinah commented Oct 18, 2021

Firmware 0.11.1 test results:

&NL=1 over writes the default value so any change to using &ND still uses 1min and not the default amount. Agree this is bug.
&ND&NL=1 Ignores the time as you suggested. Perhaps this was done as a work around, did you see this mentioned somewhere?
&ND works as intended.

Both &ND and &ND&NL=1 work here so either one I'll approve and leave it up to you to choose. I am pretty sure I was triggering the sleep when I went to bed and the Christmas tree would fade over a 2 hour time so perhaps it is in 0.8 or 0.9 when I was doing that.... As mentioned the idea is to abstract it as there are features like the preset cycle that used to be in the firmware that are now removed so the sooner we change to something that can be extended and it will mean people that pull out lights from last Christmas find they still work each year. If we find an issue we can deal with it then, thanks for the bug fix.

@Skinah Skinah added the bug An unexpected problem or unintended behavior of an add-on label Oct 18, 2021
@lolodomo
Copy link
Contributor

Please also consider correct signing of your PR.

Copy link
Contributor

@Skinah Skinah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

For a single line change and a new contributor, I believe the DCO is not a requirement ??? EDIT: Sorry only for grammer/typo mistakes.

https://www.openhab.org/docs/developer/contributing.html#sign-your-work

Please do what this post explains.

https://community.openhab.org/t/dco-check-signing-off-with-github-web-editor-explanation/83330

svjense added a commit to svjense/openhab-addons that referenced this pull request Oct 18, 2021
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>
svjense added a commit to svjense/openhab-addons that referenced this pull request Oct 18, 2021
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>
svjense added a commit to svjense/openhab-addons that referenced this pull request Oct 18, 2021
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>
Signed-off-by: Sven Jensen <sven@s7j.de>
@svjense
Copy link
Contributor Author

svjense commented Oct 18, 2021

Sorry for the commit mess - just executed the commands the DCO recommended! Let me know if there is an easy way to fix the rebase.
I kept ND only since even in 0.5.0 that is supposed to work as designed. I've heard of people who fall asleep within 1 minute when they sleep next to a beautiful WLED X-Mas Tree haha.
In regards to making the code version dependent I would tend to rather keeping the code clean and lean. People should be aware to update smart devices regularly especially if they are connected to Wifi and not keep old firmware running forever.

@lolodomo
Copy link
Contributor

The easiest way I would suggest is to remove your branch, recreate it, reapply your change, commit and force a push ;)

Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>
@svjense
Copy link
Contributor Author

svjense commented Oct 18, 2021

Thanks for the git crash course. Done!

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lolodomo
Copy link
Contributor

Code change is very small and the build of the binding is successful on my local machine, so I merge the PR while the remote build system is currently broken.

@lolodomo lolodomo merged commit b8b2255 into openhab:main Oct 18, 2021
@lolodomo lolodomo added this to the 3.2 milestone Oct 18, 2021
@svjense svjense deleted the patch-1 branch October 19, 2021 11:46
frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>

Co-authored-by: Sven Jensen <github@s7j.de>
dschoepel pushed a commit to dschoepel/openhab-addons that referenced this pull request Nov 9, 2021
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>

Co-authored-by: Sven Jensen <github@s7j.de>
Signed-off-by: Dave J Schoepel <dave@theschoepels.com>
NickWaterton pushed a commit to NickWaterton/openhab-addons that referenced this pull request Dec 30, 2021
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>

Co-authored-by: Sven Jensen <github@s7j.de>
Signed-off-by: Nick Waterton <n.waterton@outlook.com>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Jan 28, 2022
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>

Co-authored-by: Sven Jensen <github@s7j.de>
volkmarnissen pushed a commit to volkmarnissen/openhab-addons that referenced this pull request Mar 3, 2022
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>

Co-authored-by: Sven Jensen <github@s7j.de>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Based on WLED API (https://kno.wled.ge/interfaces/http-api/) current handler uses NL=1 so sleep timer is hard coded to 1 minute. Should be default (ND) to use configured time in WLED.

Fixes openhab#11389

Signed-off-by: Sven Jensen <github@s7j.de>

Co-authored-by: Sven Jensen <github@s7j.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants