Conversation
f87f504 to
e2af075
Compare
Agree
I would propose to go for |
|
@Koenkk can you take a closer look now that it's fully tested? In particular:
Note: we'll have to rebuild the all-settings page in docs for this one. |
|
|
||
| await this.configure(data.device, "zigbee_event"); | ||
| }); | ||
| // TODO: this is triggering for any `data.status`, but should only for `successful`? (relies on `device.definition?` early-return?) |
There was a problem hiding this comment.
I think that even when a device interview fails, it's worth configuring it as it might be due to a non crucial error and then the configure makes the device at least partially working.
| this.eventBus.onGroupMembersChanged(this, this.onGroupMembersChanged); | ||
| this.eventBus.onDeviceAnnounce(this, this.onZigbeeEvent); | ||
| this.eventBus.onDeviceJoined(this, this.onZigbeeEvent); | ||
| // TODO: this is triggering for any `data.status`? |
There was a problem hiding this comment.
I think it makes sense to do this always, similar to https://github.com/Koenkk/zigbee2mqtt/pull/30566/changes#r2722613265 the error might be not critical and maybe all info needed to identify the device has been received, thus it can discovered.
| }); | ||
| this.eventBus.onDeviceInterview(this, async (data) => { | ||
| // TODO: this is triggering for any `data.status`, any use outside `successful`? | ||
| // ZHC definition would skip from `device.definition?` but OnEvent from ZHC index wouldn't => triple triggering? |
There was a problem hiding this comment.
Interview failed doesn't (always) mean there is no definition, so I think this is fine.
There was a problem hiding this comment.
The problem I was thinking for this and all above is the fact status is: "started" | "successful" | "failed". So it always triggers twice?
We can take a closer look in a separate PR though, just though it was worth a TODO comment when I passed by these 😁
There was a problem hiding this comment.
It depends a bit wether we also want to trigger the onEvent on started, I guess not so only on failed or successful makes sense.
There was a problem hiding this comment.
Yes, I think it's worth a look in a follow-up PR, same for the other 2 in HA and configure extensions, should de-dupe calls a bit.
Replied!
Look good
Let's keep them for now, these OTAs are fairly small so shouldn't be a problem. |
lib/extension/otaUpdate.ts
Outdated
| device.zh.save(); | ||
| } | ||
|
|
||
| await device.reInterview(this.eventBus); |
There was a problem hiding this comment.
I'm thinking we might want to decouple this from the update process? Currently it would count as an OTA failure (response/logs) if the interview fails. Although, we probably should avoid triggering this immediately in the background, because the early-return would also trigger the read swBuildId/dateCode (lots of requests at once).
@Koenkk what do you think?
dc155b4
I think that's about as best as we can do?
There was a problem hiding this comment.
It should definitely not count towards an OTA failure indeed, I'm not sure if 5 sec is too long, maybe battery powered devices will fall asleep?
There was a problem hiding this comment.
We might need to adjust this yes. I figure the call for read sw/date might it keep it awake long enough. Always tricky this sort of things we ZED anyway, probably, if it has to fail due to quick-sleep, it will fail the sw/date read... 😅
|
Thank you so much! |
Co-authored-by: Koen Kanters <koenkanters94@gmail.com>
Design changes
updatelogic into single function for consistencycheck/update/schedule)updateresponse payload to avoid weirdness with swBuildId/dateCode often unavailablelatest_source(URL/filesystem),latest_release_notesto OTA state payloadupdate(clearavailableifupdatereturned "no image")schedulewith custom URL/filesystemupdateorschedule)Due to MQTT request payloads still allowing simple string for OTA, it duplicates the code a bit for now (marked deprecated for 3.0).
@Koenkk
nullsource/release_notes in#getEntityPublishPayload? I'm hesitating betweennullor undefined so it's taken out on stringify.TODO:
restartrequired from OTA data settings in schemaotasubdir of data dir