Poll bulbs for their Zigbee Connectivity status explicitly when refreshing #926
Conversation
|
Channel deleted. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 6e83abd |
| @@ -247,6 +248,7 @@ end | |||
| ---@param light_device HueChildDevice | |||
| local function do_refresh_light(driver, light_device) | |||
There was a problem hiding this comment.
This function has become exceedingly smelly, but I didn't think that now was the right time to be dealing with that. One of the major issues is that the whole driver is oriented around light devices on our platform representing a light resource in the Hue representation. We should probably revise that to orient around the Hue device representation instead of the light resource representation, but that's a deep surgery on the entire driver.
| elseif light_resp ~= nil then | ||
| if #light_resp.errors > 0 then | ||
| for _, err in ipairs(light_resp.errors) do | ||
| rest_resp, rest_err = hue_api:get_device_by_id(hue_device_id) |
There was a problem hiding this comment.
So is this call to the device record required because we need this to get the zigbee resource, which is needed to get that connectivity state? After seeing these changes I'm a little more concerned about this approach of adding all this to refresh. Would we be better off explicitly checking all the children connectivity status via a separate method only on the transition from hub offline -> online.
There was a problem hiding this comment.
I'm of two minds about it.
On the one hand, yes, that's a lot of REST calls in a single event handler. We could pare it down to 2 instead of 3 if we stored the zigbee ID on the ST device record, but we're already storing so many strings in the driver that there's a real memory usage concern.
On the other hand, I do think it makes sense to check the connectivity status when you do a pull-to-refresh for a variety of reasons, not least of which being that Hue bridge will sometimes return last-known-state for an offline bulb.
There was a problem hiding this comment.
I think a different option could be not reusing the refresh code in the "refresh all" handler on the bridge, and having the "refresh all" code use the bulk endpoints, but that didn't seem like a change that was straight forward enough to have ready on the timelines we were targeting.
There was a problem hiding this comment.
@varzac See the updated description and the fix-up commit where I address some of this.
dljsjr
changed the base branch from
bugfix/hue-children-online-on-init
to
main
dljsjr
force-pushed
the
fix/hue-poll-connectivity-on-refresh
branch
2 times, most recently
from
5e74b0e to
0f99437
Compare
dljsjr
force-pushed
the
fix/hue-poll-connectivity-on-refresh
branch
from
e867e9f to
fcf3ae8
Compare
|
I validated this locally via a monkeypatched The test I ran:
The reason I had to patch the Device table and use traces to confirm this is because we want to land #931 on top of this PR, and so the mobile app will show online a lot of the time because of the attribute event emits. Based on tracing the offline/online calls, we have the right behavior. We just need to land the next PR on top of this one to get the correct behavior out of the mobile app. |
dljsjr
force-pushed
the
fix/hue-poll-connectivity-on-refresh
branch
from
23ae1a9 to
4121c8f
Compare
Whenever a driver is restart, a bridge will temporarily be marked offline while the event source connection is created. This seems to be affecting the online/offline state of children incorrectly post 0.49.x FW.
dljsjr
force-pushed
the
fix/hue-poll-connectivity-on-refresh
branch
from
4121c8f to
6e83abd
Compare
Adds explicit checking for zigbee connectivity status as part of refresh, instead of always waiting for it to come in on the SSE stream.
Note that the zigbee connectivity status itself within the Hue network can be delayed.
Because we're now doing more network calls during refresh, I also adjusted the "refresh all" handler to use the bulk endpoints.
To keep the changeset on this small, I did it by overloading the functionality of the existing refresh for single lights. It's noted in comments that this isn't the most ideal, however, I'm in the process of planning out a rearch of this driver including with a test coverage plan, so I didn't think deep surgery was warranted given that we're going to be doing a complete redo soon.
Pardon the Mess™