-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fix: Disable automatic reconfigure when configureKey changes #22088
Conversation
+1 for introducing some sort of 'needs reconfigure' flag that can be shown and making it a user action to do so. I think it would still be nice to somehow calculate this automatically and not rely on a fixed value to be update somehow. (If we do go that route, I suggest a simple unixtimestamp. That also makes comparisons easy as we can just store the 'last configure for this device' as a simple timestamp.) There have been some complaints in the past too of reconfigure wiping user configured reporting. Having a flag will also partially cover that. As it would become a user action to do so. (I do believe there is a issue open for that somewhere on how to mark manual configure items as such in de db, but I'm traveling and couldn't immediately find it) |
I don't think I understand this, could you explain it a bit more? |
I'm not a big fan of going back to a variable to determine if a reconfiguration is need. But if we do, I'd suggest using a unixtimestamp as the value. We can then simply store the last time a configure was execute in the db. And it's easy to maintain the value in zhc as we just set it to the current timestamp when we make a change to the device that requires reconfiguring. We could probably also write a script that checks the git history of the last change to a devices configure block/extends to grab the initial value once. |
@sjorge but won't this break if e.g.
|
Good point, I didn't consider downgrading. I guess we could flag then differently? In either case we need to flag it so the user knows they might need to run a reconfigure. |
I think the easiest is to just re-introduce the |
We should probably look at a way to make it opt-out (device option?) as I myself and other frequently run into the issue that reconfigure will wipe reporting changes we made. Somehow only not touching those and doing the others seems harder than optionally not forcing a reconfigure. I originally understood you disabled it and we would somehow indicate to manually trigger it (e.g. after they woke up a device, although this should generally be better now with the new sending policies) or they can opt not to do it, when they did a lot of reporting changes themselves. I've been traveling and near a laptop for a few days now so probably why I misunderstood. |
@sjorge the idea is that an automatic reconfigure will not happen anymore, it will always be a manual action which should prevent the issues you describe |
This PR disables the automatic reconfigure whenever the configureKey changes due to the
configure()
method being changed. The initial trigger for me to disable this came from Koenkk/zigbee-herdsman-converters#7319, which removes thelogger
argument fromconfigure()
and thus triggering a reconfigure for all devices. In the past this already caused a lot of issues (e.g. with the modern extend refactor).Note that the automatic reconfigure is broken anyway for devices that use modern extend (because
configure()
is a loop over all modern extends configure, theconfigureKey
never changes).We have to find a better way to deal with reconfigure, but I'm thinking something like this:
meta: {configureKey: XX}
(can be added partially automatic for modern extend based devices)configureKey
is found, mark the device in the frontend, so that the user can press the reconfigure button (yellow refresh icon)This also fixes the issue that devices are unexpectedly being reconfigured causing them to break (e.g. last month due to an automatic reconfigure, my Hue Wall Switch lost it's bindings).
CC: @Nerivec @kirovilya @sjorge @mrskycriper (I'm merging this PR immediately to prevent reconfigure for all devices on the dev branch but feel free to provide feedback)