-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Refactored management of lights, using classes and integers instead of floats. #5702
Refactored management of lights, using classes and integers instead of floats. #5702
Conversation
@Jason2866 I don't have access to an Alexa device until the end of the week. I did thorough testing via an alternative iOS app (Hue Lights) and through the Web API. Would you be kind enough to try with an actual Alexa device and confirm it still works? |
Switching on / off and dimmer still working. Not working is setting color via Alexa. Command "Alexa Light blue" ends up in this (still lightning white)
Alex light red:
|
Changing colors via Alexa App Color select doesnt work too. |
Thanks. I don't know why Alexa sends XY colors and not HSB. It seems I need to handle XY colors after all. Code is ready, it should be quick to fix but will increase slightly code size. |
I think I understand the issue. The device was in CT mode "colormode":"ct" and Alexa wants to send a color. Since both Hue/Sat and XY are available, it chooses XY. When in colormode, "colormode":"hs" so Alexa sends only Hue/Sat. I will change to not send XY values when in CT mode, hoping that Alexa will send only Hue/Sat. |
@Jason2866 Could you please try with this new commit? |
Short test. Doesnt work. Now Alexa give error messages. "Something went wrong..."
|
Hmmm. You should have the same issue with the development branch (the Alexa code just merged). Could I ask one more test: please do "Color 1" (force RGB mode to red) before doing the Alexa device discovery. I just want to make sure it's linked to the CT/HS mode. |
OTA didnt work (said successfull but still old version)... Light is switching on / off. Bad thing development branch doesnt work for devices with relais. I overlooked to test with powering off Alexa and doing a "fresh" discovery. Status in Alexa App "Device doesnt respond" Log Alexa "light red" (before it was set to green via color 2)
|
Thanks. I'm worried about the relais. I thought I tested it successfully. Do you mean device like Sonoff Basic? Let's put on hold until next week, I'll have an Alexa device back. |
Sonoff Basic doesnt work anymore with Alexa and latest dev branch. Yes, i think it is best if you can test directly with a Alexa device. |
This is a tentative for 'xy' color control. It works with the iOS app "Hue Lights" but completely untested with Alexa. Will be tested with Alexa next week. |
Strike! Alexa voice light control does work :-) |
Sonoff Basic still not working Log
|
Good news for color control. Thanks for testing promptly. What are the symptoms with Sonoff Basic? It looks like Alexa is missing some information. I'm afraid that adding "Bri" will bring back the dimmer control. Do you have a device with only a Dimmer control? ie only one PWM1. |
Sonoff Basic: In Alexa App Error message text "Device doesnt respond" |
I now have an Alexa back. I tested with a Sonoff Basic set-up and it works fine: the lamp does show in Alexa app and can be voice controlled (no dimmer). The only way I can reproduce your issue is to trigger a device discovery with a color device (like Arilux) and then switch Tasmota to Sonoff Basic. Re-reading our first discussion on #5667 it seems the Dimmer is mandatory in your Alexa version. I will push back this shortly. The downside is that we will always see a dimmer even if it does not make sense. |
WIll look at single PWM issue tomorrow. |
I think I got the issue with only one PWM1. I reported the code from sonoff/xdrv04_light.ino, lines 672-674:
to new code line 687-688:
Actually I misunderstoof why this code was there in the first place, it was linked to dimmer calculation. It does not make sense anymore with the new implementation. Fix coming! |
Good news for dimmer device. |
Fix ready for testing. Now Sonoff Basic should work but with a useless slider/dimmer. |
I can confirm:
|
I found a bug, command "CT" always returns 65437. Will look at it tomorrow. |
Tested with Sonoff Basic -> works (again) Nice work!! |
Fixed CT command, better management of XY colors (would not reflect changes made outside of Hue commands). @Jason2866 Do you have a cold/warm bulb? Currently Cold white would set 100% on Cold White leds, Warm white would set 100% of Warm white leds. But Medium white will set 50% on each led. I'm wondering whether we should send 100% on both leds instead? Would that be too bright? Would that overload power supply of the bulb? Any thoughts? |
Everything still working after last changes :-) |
A minor thing (I'll need to double-check it though): If I configure LOG_LEVEL_DEBUG to the *_LOG_LEVEL defines, then the bulb just keeps rebooting. With the defaults (_NONE and _INFO) it boots up fine. Then I started to poke around a bit with the 'cmnd/sonoff/color' command at the web console, and I've found some strange behaviour. Maybe I'm doing something wrong, but it seems that the 'color' command just sets the hue+sat, the brightness is controlled by only the 'dimmer'. Here is what I tried. Let's start with the smallest bit of red:
I expected a minimal red, got it at 100% OK, lets change that R channel:
No change, red is doing its best...
So, dimmer can lower the intensity, but it seems to override the brightness of the raw rgb data. OK, let's try it with blue (dimmer still at 50!):
It seems that whenever I set an rgb value, dimmer gets back to 100. Cold white behaves like the other channels:
Warm white the same:
So the |
On the other hand, the
As of the question about the mid-point, whether the cold and warm values should be 100% + 100% or 50% + 50%, I would vote for the 50+50 (as it is now), for two reasons:
@kueblc has found that the original firmware never drives even the colour and the white leds at the same time, maybe for a similar reason.
These being said, I must confess that when I really need the brightest light the bulb can produce, I do open its console and send |
I have accidentally confirmed, that if both cold and warm LEDs are at full power, the Feit 9W bulb will draw 18 watts. This is easy to do by using the slider to bring it to maximum brightness, with the initial firmware values for color. I think it is a safe bet, that the power and heat management design of a commodity bulb would not handle running at 200% of its advertised rating for long. Before I realized what was going on power wise, I made the device unstable by having the RGB LEDs and the Cold and Warm LEDs on at the same time, it started flashing. The RGB LEDs by themselves draw around an additional 2-3 watts. |
Thanks, this is valuable feedback. I will dive into it tomorrow. I need to do some small changes, the current PR does not allow to have both cold and warm white at 100%. I understand this should not be the default and keep it 50%-50% by default; but still offer the possibility to specify each PWM values. About Color command, I falsely understood it would always reset dimmer to 100%. Afterthoughts, that should be only for "Color 1-12" and not when you specify RGB. Will correct it. |
Fixed the issue with Color command, thanks @gsimon75. |
I'll do a test build right away! Meanwhile I've found the source of the reboots when LOG_LEVEL_DEBUG: support_rtc.ino. It's completely unrelated to your code, the problem most probably is that it's attached as a callback and the logging seems not to be reentrant, so if it's invoked during another log being processed, then we've got trouble. Anyway, it's a completely different issue, way out of scope of this PR. |
(Sorry for the delay, a soldering got loose and pried up the pcb pad, needed some patching with coil wire.) The fix seems fine :D, as far as I can tell, Just for completeness' sake I'll test it with an SM16716 bulb, too, but I don't expect too much differences. |
SM16716 works fine, So I think this PR is OK :) (and thanks for the great work at the refactoring, it improved a lot on the clarity of the code) ! |
@gsimon75 Thx for testing and your feedback @arendst Could you please merge this PR? It is tested to work with Alexa as before |
Thanks @Jason2866 It's probably better to merge this PR and I will submit another one for this small change. |
Great to see your work while i'm on holiday in Portugal ;-) |
Thanks for building, testing and merging this PR guys 👍🏽🎉 |
Description:
This is a refactoring of the way Lights are managed.
Please use the following singletons:
Final good news, code is 1.1KB smaller.
**Related issue (if applicable):
Checklist: