-
Notifications
You must be signed in to change notification settings - Fork 501
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
Refactor logic for setting light state. #2553
Conversation
- Don't replace `TaskSetLevel` tasks. New `state.on` / `state.bri` logic (see #2475) sends _Move to Level with On/Off_ for level 2 and transition time 0, followed by _On_, followed by _Move to Level_ for given brightness and transitiontime. - Fix misleading debug message when replacing task.
Fix `addTaskSetColorLoop()`, see #2475. - Set `lightNode` attributes for colorloop only here; - Set colormode to `xy` (instead of `hs`).
Light sets colormode to `hs` when starting the colorloop.
Refactor `setLightState()`, see #2475.
Refactor `setLightState()`, see #2475.
Implement _Off with Effect_ command.
- Send _Off with Effect_ to turn off Hue lights; - Clear active colorloop when light is turned off. Hue lights do so automatically, but others (e.g. GLEDOPTO) don't.
Error message when trying to modify colour attribute while colorloop is active.
Thanks for the PR, I like it, looks cleaner and easier to understand than the former version. I'll test it in my setup with various IKEA lights. Since it's quite a large diff I'll give it some days for testing and not merge it yet in 2.05.76, but if everything works well in 2.05.77 in about a week. |
@manup, That’s cool, just hope I can keep the PR current with all the other changes happening. Looking forward to your feedback. If this works out, I would want to do something similar for group There's a number of related issues, I'd really like your input on.
|
Add message that `colormode` is not modifyable instead of not available.
Fix update ZCL value debug message
Samsum SmartThings multipupose sensors, see #1848: - Don't configure event reporting for orientation attributes (as these are already sent in the _IAS Zone State Notification_ command); - Set periodic reporting of these to every 5 minutes, in line with temperature and open/close reporting for the same sensor.
Samsumg SmartThings multipurpose sensor, see #1848: - Add `config.duration`.
Samsumg SmartThings multipurpose sensor, see #1848: - Add `config.duration`. - Refactor logic to update `state.vibration`.
Fix setting `state.on` for Aqara B1, see #1654.
Add optional OTAU attributes.
Improve handling of scene states (PUT `/groups/#/scenes/#/lights/#/state`), see #2507: - Handle `ct` in scene light state; - Set colormode for scene light state.
Improve handling of scene states (PUT `/groups/#/scenes/#/lights/#/state`), see #2507: - Bug fix: malformed packet on setting scene light state; - iCasa filament light stores `ct` in _Enhanced Hue_;
- Add `lastannounced` and `lastseen` attributes, see 2590; - Store `state.lastupdated` in msec resolution, see #1687.
Add `lastannounced` and `lastseen` attributes, see 2590.
- Implement `lastseen` attribute (in msec resolution), see 2590; - Bug fix: `state.ontime` not honoured for sirens (warning devices); - Include light `state` in light added event, see #2625.
- Show `state.lastupdated` in msec resolution, see #1687; - Implement `lastsceen`.
@manup, how are the tests proceeding? I haven't yet addressed any of the points 1-4 above. Would really appreciate your input.
|
Set `rx()` for ZGPSwitch `/sensors`, so `lastseen` is updated.
- Handle restore of `state.lastupdated` with both second and milliseccond resolution.
Wow that are heaps of improvements, thanks a lot! I think the PR is ready for merging if you're ok with that?
Works like a charm here, I've tested the on/off, brightness, color and color temperature capabilities.
Indeed that, I agree it will simplify the code by putting the repeating details of the ResourceItems behind a higher level wrapper. It might not be possible for all places but the wast majority can be simplified. Since this is a huge refactor it might be good to coordinate this with other pull requests, perhaps a short code freeze until it's done. So that other PRs can build on top of it. Or as alternative it could be done incrementally?
I'm open to this but it need to be checked with the various API-Clients out there to not break them by switching over to 200 Responses. In case of the Phoscon App it would already work but I'm not sure about others.
Agree, I also think when building the capabilities JSON file(s) (?) they can be used by both the v1 and v2 API. This might actually a good first step to figure out how the new lights API should be modeled.
Separating the attributes makes sense, currently @YKO-de is building the window covering support in the Phoscon App which internally already works on the current API. Since the API layer is very thin it should be easy to adjust it. I'm not sure about the other API clients like Home Assisten and similar. So the change should be coordinated with the maintainers to not cause any surprises :) |
Yes, please. I fear it's getting way too big. Ad 1. I tried to implement setter/getter methods on Ad 2. This would actually simplify Homebridge Hue and other clients that also support the Hue bridge. I wouldn't know all clients, however. Ad 3. Or at least what capabilities we need to record. Ad 4. We could start by adding the new attributes next to the existing ones, who would be deprecated. Next, implement the new attributes on |
@tubalainen, I still see normal transition times when PUTting the light state through the API. With default transition time, as well as when specifying |
Thanks for asking @ebaauw . On/Off works like expected - but with "transition" declared in my automation it does not compute fully with .76 - it just turns off with the default transitiontime (1-2 seconds ish). I use this automation in Home Assistant (dim from 100% down to 0 (off) in 30 seconds):
The following outcome in the debug log from Home Assistant with version .76 (when the button is being pushed) - the lights turns off (with a default transition of 1-2 sec ish?):
And here is the exact same log from Home Assistant using .75 - the lights dim/transitions to off state 30 seconds later:
|
See #2475.
state.on
/state.bri
logic more like the Hue bridge, see rest api incorrectly reports light is turned off after setting bri=0 #1111 (comment). Try setting something fun, like{"on": true, "ontime": 100, "bri": 254, "transitiontime": 100}
;state.speed
for Fan devices, like the Hampton Bay (see Fan Module (Hampton Bay) #932).I've tested this on a Hue bulb and on a GLEDOPTO controller. The latter doesn't turn on on Move to Level (with On/Off), but it does set the level (even while the light is off). The subsequent On then turns the light on at level 2, causing almost the same behaviour as Hue lights. I would appreciate people testing this on other lights, notably IKEA.
For discussion on the functionality, please see #2475.