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

Hue bulb not updating brightness when controller with hue dimmer remote #2108

Closed
sjorge opened this issue Oct 9, 2019 · 12 comments
Closed

Comments

@sjorge
Copy link
Contributor

sjorge commented Oct 9, 2019

Bug Report

What happened

After I managed to bind my hue dimmer to my hue bulb, I noticed the bulb does not report the correct brightness after changing it with the dimmer.

zigbee2mqtt:info  2019-10-09T17:59:33: MQTT publish: topic 'zigbee2mqtt/bedroom/remote_desk_lamp', payload '{"counter":1,"linkquality":86,"brightness":255,"battery":100,"action":"off-press","duration":0}'
zigbee2mqtt:info  2019-10-09T17:59:33: MQTT publish: topic 'zigbee2mqtt/bedroom/desk_lamp', payload '{"state":"OFF","linkquality":86,"brightness":254,"color":{"x":0.7006,"y":0.2993},"color_temp":37}'
zigbee2mqtt:info  2019-10-09T17:59:39: MQTT publish: topic 'zigbee2mqtt/bedroom/remote_desk_lamp', payload '{"counter":1,"linkquality":86,"brightness":255,"battery":100,"action":"on-press","duration":0}'
zigbee2mqtt:info  2019-10-09T17:59:39: MQTT publish: topic 'zigbee2mqtt/bedroom/desk_lamp', payload '{"state":"ON","linkquality":86,"brightness":254,"color":{"x":0.7006,"y":0.2993},"color_temp":37}'
zigbee2mqtt:info  2019-10-09T17:59:42: MQTT publish: topic 'zigbee2mqtt/bedroom/remote_desk_lamp', payload '{"counter":1,"linkquality":86,"brightness":223,"battery":100,"action":"down-hold","duration":0.803}'
zigbee2mqtt:info  2019-10-09T17:59:43: MQTT publish: topic 'zigbee2mqtt/bedroom/remote_desk_lamp', payload '{"counter":1,"linkquality":86,"brightness":143.2,"battery":100,"action":"down-hold","duration":1.601}'
zigbee2mqtt:info  2019-10-09T17:59:44: MQTT publish: topic 'zigbee2mqtt/bedroom/remote_desk_lamp', payload '{"counter":1,"linkquality":86,"brightness":62.999999999999986,"battery":100,"action":"down-hold","duration":2.403}'
zigbee2mqtt:info  2019-10-09T17:59:44: MQTT publish: topic 'zigbee2mqtt/bedroom/remote_desk_lamp', payload '{"counter":1,"linkquality":86,"brightness":62.999999999999986,"battery":100,"action":"down-hold-release","duration":2.946}'
zigbee2mqtt:info  2019-10-09T18:00:18: MQTT publish: topic 'zigbee2mqtt/bedroom/remote_desk_lamp', payload '{"counter":1,"linkquality":94,"brightness":62.999999999999986,"battery":100,"action":"off-press","duration":0}'
zigbee2mqtt:info  2019-10-09T18:00:19: MQTT publish: topic 'zigbee2mqtt/bedroom/desk_lamp', payload '{"state":"OFF","linkquality":94,"brightness":254,"color":{"x":0.7006,"y":0.2993},"color_temp":37}'
zigbee2mqtt:info  2019-10-09T18:00:23: MQTT publish: topic 'zigbee2mqtt/bedroom/remote_desk_lamp', payload '{"counter":1,"linkquality":94,"brightness":62.999999999999986,"battery":100,"action":"on-press","duration":0}'
zigbee2mqtt:info  2019-10-09T18:00:23: MQTT publish: topic 'zigbee2mqtt/bedroom/desk_lamp', payload '{"state":"ON","linkquality":94,"brightness":254,"color":{"x":0.7006,"y":0.2993},"color_temp":37}'

What did you expect to happen

To get a update (via attReport) after a while with the new brightness, but it keeps being the old brightness. Toggling state will trigger a correct update... but brightness is still left untouched.

How to reproduce it (minimal and precise)

  • Bind a hue dimmer to a hue bulb
  • use on or off button
  • get update of bulb state
  • use brightness up or down buttons
  • <no update>
  • <wait for a few minutes, no attReports are arriving>
  • use off or on button
  • get update of bulb state, but only the state field.

Debug Info

zigbee2mqtt version: 1.6.0 (commit #ba92f73)
CC253X firmware version: {"type":"zStack12","meta":{"transportrev":2,"product":0,"majorrel":2,"minorrel":6,"maintrel":3,"revision":20190425}}

@sjorge
Copy link
Contributor Author

sjorge commented Oct 9, 2019

Looks like I did get an attReport after a while, brightness not updated though :(

zigbee2mqtt:info  2019-10-09T18:10:14: MQTT publish: topic 'zigbee2mqtt/bedroom/desk_lamp', payload '{"state":"ON","linkquality":78,"brightness":254,"color":{"x":0.7006,"y":0.2993},"color_temp":37}'

@Koenkk
Copy link
Owner

Koenkk commented Oct 10, 2019

Known issue (limitation of bulb)

Hue bulbs only report on/off state (limitation of Hue bulbs)

See OP of #1064

@Koenkk Koenkk closed this as completed Oct 10, 2019
@sjorge
Copy link
Contributor Author

sjorge commented Oct 10, 2019

That sucks, the other issue does mention the values can be querried somehow... but It didn;t mention how. A quick look through the docs doesn't seem to get me any further.

How would one poll the bulb say every 3 minutes + after a state change? That would probably be sufficient for my workflow.

@Koenkk
Copy link
Owner

Koenkk commented Oct 10, 2019

zigbee2mqtt/[DEVICE_ID]/get with payload {"brightness": ""}

@sjorge
Copy link
Contributor Author

sjorge commented Oct 10, 2019

I've stubbed out the a general plan on how I can work around this in node-red, but maybe this will work in zigbee2mqtt too?

  • somehow tag all effected bulbs
  • for group in bulb.groups
  • XX subscribe to group updates -> trigger update of brightness/color/color_tmp values
  • for binding in bulb.bound_to
  • xxx subscribe to bind actions (atleast a hue dimmer still sends actions) -> trigger update of brightness/color/color_tmp

We might not have to query the propery and maybe we can just use the value published byu thge bind? (do groups do that too?)

And maybe query the actually bulb after some sort of timer that gets reset every update, to not cause too much traffic.

Anyway, I know I can do this with node-red and it will probably take me a few weekends. But maybe it is easier in zigbee2mqtt? If so, where would one need to look to add something like this? There seems to be an extesions framework but is that sufficient?

@Koenkk
Copy link
Owner

Koenkk commented Oct 10, 2019

If we want to add this to zigbee2mqtt it should be done in the reporting extension (https://github.com/Koenkk/zigbee2mqtt/blob/dev/lib/extension/deviceReport.js). There we know all the binds.

@sjorge
Copy link
Contributor Author

sjorge commented Oct 10, 2019

I'll give that a try first, I'm sure that would benefit other people... So for now the general plan is to start small and only do brightness for starting.

So in deviceReport.js when endpoint.supportsInputCluster is false for genLevelCtrl to sort of go into a 'emulated' mode for it. Well first attempt would just to log if it is false to see if the hue bulbs correctly report that they do not support genLevelCtrl. If they lie, that idea is down the drain.

If it does not lie, then I guess I need to create a endpoint.emulateReporting function and take it from therem but this will be my first time doing typescript so I imagine progress will be slow.

@sjorge
Copy link
Contributor Author

sjorge commented Oct 10, 2019

Ouch, not off to a great start... I tried adding logger.error("XXX - setupReporting called"); to the top of the async setupReporting(device) { block but it never gets logged, not for my ikea bulbs nor for my hue bulbs.

Does logger not work in an async block?

@sjorge
Copy link
Contributor Author

sjorge commented Oct 10, 2019

After some more digging, it looks like the bulb already has device.meta.reporting set to 1 which matches reportKey... so shouldSetupReporting will return false for the bulb.

I also tried power cycling the bulb, so that it resets and reannounces it self to the network, it then fires the shouldSetupReporting functions a few times with different messages, but device.meta.reporting is always 1

So my plan to add an else block here that would check for genLevelCtrl (and a report_emulation device configuration property) to then setup some sort emulation as mentioned earlier won't work.

I had a quick look around, but it looks like setupReporting will set the device.meta.reporting property and this is somehow permanent?

Is my basic approuch here wrong? Maybe I can do the check in the onZigbeeStartup and add a shouldSetupEmulatedReporting and setupEmulatedReporting functions/event callbacks instead?

@sjorge
Copy link
Contributor Author

sjorge commented Oct 11, 2019

I think I have bitten of more than I can chew, I have been digging around in zigbee2mqtt, but also zigbee-herdman to try and figure out a workable plan... but I seem to be missing a lot of things :(

I'm getting stuck basically at the beginning, I want to modify deviceReport's onZigbeeStarted to something like this:

edit: bad draft code removed

Then the idea is to modify onZigbeeEvent to loop the device's bind devices excluding the coordinator and check if the device has the report_emulate property, if so then call the updates for the required property.

The do the same for all group members when a group gets an update.

@sjorge
Copy link
Contributor Author

sjorge commented Oct 11, 2019

Going to call it a night for today, will have an hour or 4 to work on this tomorrow, so far I have:

diff --git a/lib/extension/deviceReport.js b/lib/extension/deviceReport.js
index 6faa855..03699fd 100644
--- a/lib/extension/deviceReport.js
+++ b/lib/extension/deviceReport.js
@@ -2,6 +2,7 @@ const zigbeeHerdsmanConverters = require('zigbee-herdsman-converters');
 const logger = require('../util/logger');
 const CC2530Router = zigbeeHerdsmanConverters.devices.find((d) => d.model === 'CC2530.ROUTER');
 const utils = require('../util/utils');
+const settings = require('../util/settings');
 const BaseExtension = require('./baseExtension');

 const defaultConfiguration = {
@@ -64,6 +65,22 @@ class DeviceReport extends BaseExtension {
         this.configuring.delete(device.ieeeAddr);
     }

+    async setupReportingEmulation(deviceSettings, device) {
+       for (const emulate_cluster of deviceSettings.report_emulate) {
+            if (clusters.hasOwnProperty(emulate_cluster)) {
+               // XXX: somehow mark update 'device' as needing emulation for this cluster
+               //      can we device.meta for this, and just reset this every startup?
+                logger.info(
+                    `Succesfully setup reporting emulation for '${device.ieeeAddr}' - ${emulate_cluster}`
+                );
+            } else {
+                logger.error(
+                    `Failed to setup reporting emluation for '${device.ieeeAddr}' - unknown cluster ${emulate_cluster}`
+                );
+            }
+       }
+    }
+
     shouldSetupReporting(mappedDevice, device, messageType) {
         if (!device) return false;

@@ -80,6 +97,12 @@ class DeviceReport extends BaseExtension {
         return true;
     }

+    shouldSetupReportingEmulation(deviceSettings, device) {
+        if (deviceSettings.hasOwnProperty("report_emulate") && Array.isArray(deviceSettings.report_emulate))
+            return true;
+        return false;
+    }
+
     async onZigbeeStarted() {
         this.coordinatorEndpoint = this.zigbee.getDevicesByType('Coordinator')[0].endpoints[0];

@@ -88,6 +111,10 @@ class DeviceReport extends BaseExtension {
             if (this.shouldSetupReporting(mappedDevice, device, null)) {
                 this.setupReporting(device);
             }
+            const deviceSettings = settings.getDevice(device.ieeeAddr);
+            if (this.shouldSetupReportingEmulation(deviceSettings, device)) {
+               this.setupReportingEmulation(deviceSettings, device);
+            }
         }
     }

@Koenkk see my XXX comment

+               // XXX: somehow mark update 'device' as needing emulation for this cluster
+               //      can we device.meta for this, and just reset this every startup?

I need a place to store 'runtime' info on which clusters to emulate reporting for, we update this info every startup from configuration.yaml so they don't need to be stored in the database. Reading the settings from the configuration on every event seems to be a huge waste of resources, but even in that case we still need to set some sort of flag on the device that we want to do the reporting emulation for it. we can just check if we have the config and quickly return if we do not. Edit: on digging though util/settings it looks like it mostly cached so it may not be a massive overhead after all. Would the approach of checking it every time we get an event be better?

But I don't see a 'runtime' equivalent of device.meta.

@sjorge
Copy link
Contributor Author

sjorge commented Oct 12, 2019

I got something working, I will open a PR.

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

No branches or pull requests

2 participants