-
Notifications
You must be signed in to change notification settings - Fork 505
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
Handle supported 'state' changes via FC03 for supported Philips Hue lights #7950
base: master
Are you sure you want to change the base?
Conversation
029bceb
to
67d937d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this. Not too happy about the reformatting (makes it difficult to identify the real changes), but the logic looking very good to me.
bool addTaskHueEffect(TaskItem &task, QString &effect); | ||
bool validateHueGradient(const ApiRequest &req, ApiResponse &rsp, QVariantMap &gradient, quint16 styleBitmap); | ||
bool addTaskHueGradient(TaskItem &task, QVariantMap &gradient); | ||
bool addTaskHueManufacturerSpecific(TaskItem &task, HueManufacturerSpecificPayloads &payloadItems, QVariantMap &items); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this routine going to be invoked outside hue.cpp
? If not, better define it locally in hue.cpp
rather than including it in DeRestPluginPrivate
. In that case, also define HueManufacturerSpecificPayload
locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided on having it here cause I noticed a couple of places in rest_lights.cpp
that call out to addTaskHueEffect()
and figured addTaskHueManufacturerSpecific()
would eventually succeed them.
de_web_plugin_private.h
Outdated
@@ -1432,9 +1449,13 @@ public Q_SLOTS: | |||
// Advanced features of Hue lights. | |||
QStringList getHueEffectNames(quint64 effectBitmap, bool colorloop); | |||
QStringList getHueGradientStyleNames(quint16 styleBitmap); | |||
bool isHueEffectLight(const LightNode *lightNode); | |||
bool isMapableToManufacturerSpecific(const QVariantMap &map); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this, making sure we don't break anything. I would like to go through all non-mappable items and see if we really cannot handle them from setHueLightState()
. I don't care about hue
and sat
, but bri_inc
and ct_inc
would be nice (and I kind of expect the Hue bridge to handle these though FC03). And ontime
to trigger an equivalent of On with Timed Off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bri_inc
and ct_inc
are now dimming_delta
and color_temperature_delta
in the v2 API. The Hue Bridge sends out Step
and ColorTemperatureStep
commands for them - it doesn't seem to use 0xfc03
(yet?). I can't find any documentation for ontime
, but including it as an additional key in the v2 API commands doesn't actually do anything.
@@ -1432,9 +1449,13 @@ public Q_SLOTS: | |||
// Advanced features of Hue lights. | |||
QStringList getHueEffectNames(quint64 effectBitmap, bool colorloop); | |||
QStringList getHueGradientStyleNames(quint16 styleBitmap); | |||
bool isHueEffectLight(const LightNode *lightNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's probably some more places in rest_light.cpp
where this could be leveraged.
\return true - on success | ||
false - on error | ||
*/ | ||
bool DeRestPluginPrivate::addTaskHueManufacturerSpecific(TaskItem &task, HueManufacturerSpecificPayloads &payloadItems, QVariantMap &items) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addTaskHueEffect()
could probably be refactored simply to call addTaskHueManufacturerSpecific()
.
Thanks. Noticed the formatting a little too late - but I've corrected that now to make the changes clear. |
67d937d
to
f11b247
Compare
f11b247
to
f4d4596
Compare
Hi thanks for the PR. @hanskroner @ebaauw is this ready to be merged or wait for next beta? |
I've been running it for a few days on a small test network (3 supported lights) with no surprises. I'm unsure if @ebaauw has had the chance to test this, but if I'm the only one so far it might be more sensible to wait for the next beta. |
Not yet. |
1e7a854
to
dac50b2
Compare
'setLightState()' calls out to this new helper method when dealing with a Hue light that supports effects with a request that contains only items that can be handled by the manufacturer-specific command. The new method will eventually build a '0x00' command from the '0xfc03' cluster to control the light, mimicking what the Hue Bridge.
The new method will be responsible for building a '0x00' command from the '0xfc03' cluster and queuing a task for it to be sent out the radio. The method signature is expected to change to allow passing payload items and their content descriptor easily.
Use a quint16 enum, wrapped in QFlags, to track which items are present in the payload of a ‘’0xfc03’ 0x00’ command.
For Philips Hue lights that support the ‘0xfc03’ cluster, handle changes to their “on” state using the manufacturer-specific cluster.
dac50b2
to
5e66010
Compare
The Philips Hue API now also allows customising effect speed/colour for all the effects. When this is supported in deCONZ, please reuse existing |
For some effects (e.g., Modifying an effect's speed also requires firmware (at least) v1.222.2. The CLIP v2 API exposes it through Using snake case for the key names seems more consistent with both the deCONZ REST and the CLIP v2 APIs to me. |
What happens after you turn the effect off? Does the light remain on the effect colour, or does it revert to the previous colour from before the effect? In the latter case, we don’t need Note that The camel case is my bad; at some time I had hoped we’d be moving to that, but since many new snake case or simple lowercase items have been introduced (even by me). |
After turning the effect off, the light will revert to the effect's color. Starting the
It doesn't seem to be possible, no. Passing in only |
The Philips Hue Bridge uses the manufacturer-specific
0xfc03
cluster to control lights that support the cluster. The0x00
command of this cluster provides a mechanism for changing multiple state properties of a light with a single command. This PR changes the way the REST API processes PUTs that would change selected 'state' keys of these lights to mimic the Hue Bridge's behavior.The
0x00
command begins with a 2-byte encoding of the command's payload. This PR addresses only some of the state changes supported by the command. Using the nomenclature of the CLIP v2 API, they are:The PR avoids introducing additional complexity to
setLightState()
by processing only supported state changes of supported lights separately, insetHueLightState()
. The functionality introduced in the this PR also overlaps with functionality thataddTaskHueEffect()
provides today - this could be removed at a later stage.Bit 6 of the
0x00
command is very likelygradient
, judging by the structure of the command used byaddTaskHueGradient()
- I unfortunately don't have a gradient light to confirm this.