fix: Don't emit events for bulbs known offline #931

dljsjr · 2023-08-23T20:32:24Z

Requires tracking some non-persistent state for devices.

github-actions · 2023-08-23T20:34:06Z

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 18aaccb

github-actions · 2023-08-23T20:35:11Z

Channel deleted.

github-actions · 2023-08-23T20:35:37Z

Test Results

    54 files   345 suites 0s ⏱️
1 622 tests 1 622 ✔️ 0 💤 0 ❌
2 842 runs 2 842 ✔️ 0 💤 0 ❌

Results for commit 18aaccb.

♻️ This comment has been updated with latest results.

dljsjr · 2023-08-23T20:36:03Z

drivers/SmartThings/philips-hue/src/init.lua

@@ -681,6 +687,7 @@ light_added = function(driver, device, parent_device_id, resource_id)

  driver.light_id_to_device[device_light_resource_id] = device

+  device:set_field(Fields.IS_ONLINE, true)


Default to ONLINE if the bulb is added

Bulbs still are added if they are offline, correct? I think we should not emit any events until we know a device is online. In this case we will do the refresh for the device and emit initial state making the ST platform think the device is online when its not.

I thought it would be better to have the field follow the synthetic state we establish in added to populate DW, and then let the SSE onopen code handle rectifying this.

EDIT I see where this falls down after thinking about it a little harder, but this would require more code changes to make sure we have some approximation of a synthetic event emit when the device finally comes online so that the presentation isn't messed up in the mobile app.

dljsjr · 2023-08-23T20:36:36Z

drivers/SmartThings/philips-hue/src/init.lua

@@ -751,8 +758,10 @@ local function do_bridge_network_init(driver, device, bridge_url, api_key)
              if status.status == "connected" then
                child_device.log.trace("Marking Online after SSE Reconnect")
                child_device:online()
+                child_device:set_field(Fields.IS_ONLINE, true)


This runs every time the SSE stream opens, so will run at driver start

dljsjr · 2023-08-23T20:37:11Z

drivers/SmartThings/philips-hue/src/init.lua

@@ -55,12 +55,18 @@ local function emit_light_status_events(light_device, light)
    if light.status then
      if light.status == "connected" then
        light_device:online()
+        light_device:set_field(Fields.IS_ONLINE, true)


This is where the SSE event gets processed.

varzac · 2023-08-23T20:40:30Z

drivers/SmartThings/philips-hue/src/init.lua

        return
      end
    end

+    if not light_device:get_field(Fields.IS_ONLINE) then


I would like to understand the consequences of the difference between

if not is_online

and

if is_online is false

I was actually debating on making this an explicit boolean check, so I'm glad you asked that question, cuz it's forcing me to think about it harder.

if not <val> will also capture the nil case, e.g., the value hasn't been set yet, e.g., we haven't had a status update for that bulb in any capacity. I figured it was good to treat that the same as the false case and so treat that bulb as offline. So if not <val> will only happen if value is false or nil; true or any other value will evaluate truthy.

if <val> == false explicitly looks for that value being assigned false, meaning in the case that we don't know what's going on, this boolean check would fail and we'd continue on to emit events.

If anything, what we may want to do is make an explicit check against the value of true, which is what I was going back and forth on. In other words:

if light_device:get_field(Fields.IS_ONLINE) ~= true then return end

cjswedes

My commit fixed an onboarding issue I was having where a bulb was onboarded as online despite being offline. Another option we could do is scan connectivity every time after a bridge comes online, and then also do a scan 1 min or so after the bridge added event. This way we have responsive device health for children when bridge is naturally coming online/offline and also attempt to correct any offline devices that are marked online immediately post join due to the synthetic capability event emission.

cjswedes · 2023-08-24T16:00:39Z

drivers/SmartThings/philips-hue/src/init.lua

+        -- Wait bit before doing querying to allow initial state for added devices to be reported
+        -- Children will all remain offline for this amount of time when a bridge comes back online.
+        -- Children will all be online for this amount of time when a bridge id first joined.
+        cosock.socket.sleep(15) --TODO is this long enough for large hue setup initial onboarding to complete for all devices?


A delay is needed for the onboarding case where we do emit events for offline devices. Perhaps there is an event based solution to do this scan at the opportune time, but I struggled to find an event for "all devices added" during the onboarding of a hue setup.

Requires tracking some non-persistent state for devices.

dljsjr · 2023-10-18T23:03:59Z

@cjswedes after merging #926 and rebasing this on top of it, then cleaning up the conflicts, I was able to remove the hard-coded cosock delay and I believe this is now behaving the way we want it to. Could use another review, though.

NoahCornell · 2023-10-19T19:30:06Z

Does this need its field set or does it not matter because its the bridge and not a light? https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/931/files#diff-01ac799356794c771516edfa70fbd5b602c9fa11a15603b8566c2bc245fd22d5R711

NoahCornell

Looks good to me, the only thing I can think of is that it would be nice if you could easily override the device table's online and offline methods so that they could do this without having to add it everywhere we set the device offline and online but I know that is difficult/not possible with the way device tables are setup.

dljsjr · 2023-10-19T20:23:47Z

Does this need its field set or does it not matter because its the bridge and not a light? #931 (files)

Right now it only needs to be set on lights.

dljsjr · 2023-10-19T20:24:35Z

Looks good to me, the only thing I can think of is that it would be nice if you could easily override the device table's online and offline methods so that they could do this without having to add it everywhere we set the device offline and online but I know that is difficult/not possible with the way device tables are setup.

Yeah I discovered this when I did the tracing to validate the changes in #926. I had to replicate the entire device.lua file in-tree.

Longer term the solution is to make the logic a little more centralized, but I want to finish developing a testing strategy for LAN devices (even if we're not mocking the comms layer) before undertaking any large-scale refactors of these drivers.

Or, alternatively, figuring out the underlying plumbing work needed to have :is_online() available on the device table.

dljsjr added the do not merge label Aug 23, 2023

dljsjr requested review from greens, varzac, cjswedes, ctowns and NoahCornell August 23, 2023 20:32

dljsjr force-pushed the fix/hue-dont-emit-events-for-offline-bulbs branch from 731e777 to 45c9aed Compare August 23, 2023 20:33

dljsjr commented Aug 23, 2023

View reviewed changes

varzac reviewed Aug 23, 2023

View reviewed changes

dljsjr requested a review from varzac August 23, 2023 20:56

cjswedes reviewed Aug 24, 2023

View reviewed changes

dljsjr mentioned this pull request Oct 18, 2023

Poll bulbs for their Zigbee Connectivity status explicitly when refreshing #926

Merged

fix: Don't emit events for bulbs known offline

bedab4e

Requires tracking some non-persistent state for devices.

dljsjr force-pushed the fix/hue-dont-emit-events-for-offline-bulbs branch from b5c3b6e to 11f470c Compare October 18, 2023 23:03

dljsjr removed the do not merge label Oct 18, 2023

cjswedes and others added 2 commits October 19, 2023 13:49

fixup! fix: Don't emit events for bulbs known offline

65540ac

fixup! fix: Don't emit events for bulbs known offline

18aaccb

dljsjr force-pushed the fix/hue-dont-emit-events-for-offline-bulbs branch from 11f470c to 18aaccb Compare October 19, 2023 18:49

NoahCornell approved these changes Oct 19, 2023

View reviewed changes

dljsjr mentioned this pull request Oct 19, 2023

fix: Only allow a single discovery handler #1023

Merged

cjswedes approved these changes Oct 19, 2023

View reviewed changes

dljsjr merged commit b4b4fd0 into main Oct 19, 2023
10 checks passed

dljsjr deleted the fix/hue-dont-emit-events-for-offline-bulbs branch October 19, 2023 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Don't emit events for bulbs known offline #931

fix: Don't emit events for bulbs known offline #931

dljsjr commented Aug 23, 2023

github-actions bot commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023 •

edited

Loading

dljsjr Aug 23, 2023

cjswedes Aug 23, 2023

dljsjr Aug 23, 2023 •

edited

Loading

dljsjr Aug 23, 2023

dljsjr Aug 23, 2023

varzac Aug 23, 2023

dljsjr Aug 23, 2023 •

edited

Loading

cjswedes left a comment

cjswedes Aug 24, 2023

dljsjr commented Oct 18, 2023

NoahCornell commented Oct 19, 2023

NoahCornell left a comment

dljsjr commented Oct 19, 2023

dljsjr commented Oct 19, 2023 •

edited

Loading

		@@ -681,6 +687,7 @@ light_added = function(driver, device, parent_device_id, resource_id)

		driver.light_id_to_device[device_light_resource_id] = device

		device:set_field(Fields.IS_ONLINE, true)

fix: Don't emit events for bulbs known offline #931

fix: Don't emit events for bulbs known offline #931

Conversation

dljsjr commented Aug 23, 2023

github-actions bot commented Aug 23, 2023 • edited Loading

github-actions bot commented Aug 23, 2023 • edited Loading

github-actions bot commented Aug 23, 2023 • edited Loading

Test Results

dljsjr Aug 23, 2023

Choose a reason for hiding this comment

cjswedes Aug 23, 2023

Choose a reason for hiding this comment

dljsjr Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

dljsjr Aug 23, 2023

Choose a reason for hiding this comment

dljsjr Aug 23, 2023

Choose a reason for hiding this comment

varzac Aug 23, 2023

Choose a reason for hiding this comment

dljsjr Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

cjswedes left a comment

Choose a reason for hiding this comment

cjswedes Aug 24, 2023

Choose a reason for hiding this comment

dljsjr commented Oct 18, 2023

NoahCornell commented Oct 19, 2023

NoahCornell left a comment

Choose a reason for hiding this comment

dljsjr commented Oct 19, 2023

dljsjr commented Oct 19, 2023 • edited Loading

github-actions bot commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023 •

edited

Loading

github-actions bot commented Aug 23, 2023 •

edited

Loading

dljsjr Aug 23, 2023 •

edited

Loading

dljsjr Aug 23, 2023 •

edited

Loading

dljsjr commented Oct 19, 2023 •

edited

Loading