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

fix: Disable automatic reconfigure when configureKey changes #22088

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

Koenkk
Copy link
Owner

@Koenkk Koenkk commented Apr 4, 2024

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 the logger argument from configure() 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, the configureKey never changes).

We have to find a better way to deal with reconfigure, but I'm thinking something like this:

  • Add back meta: {configureKey: XX} (can be added partially automatic for modern extend based devices)
  • When a new 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)

@Koenkk Koenkk merged commit 9d251db into dev Apr 4, 2024
15 checks passed
@Koenkk Koenkk deleted the fix/disable-automatic-reconfigure branch April 4, 2024 19:47
@sjorge
Copy link
Sponsor Contributor

sjorge commented Apr 4, 2024

+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)

@Koenkk
Copy link
Owner Author

Koenkk commented Apr 6, 2024

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.

I don't think I understand this, could you explain it a bit more?

@sjorge
Copy link
Sponsor Contributor

sjorge commented Apr 6, 2024

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.

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.

@Koenkk
Copy link
Owner Author

Koenkk commented Apr 7, 2024

@sjorge but won't this break if e.g.

  1. Configure key for a device is bumped to e.g. time 100 -> 200
  2. You configure on an older version z2m version on time 250
  3. You update to the new version, z2m will think no reconfigure is needed because 250 > 200

@sjorge
Copy link
Sponsor Contributor

sjorge commented Apr 7, 2024

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.

@Koenkk
Copy link
Owner Author

Koenkk commented Apr 7, 2024

I think the easiest is to just re-introduce the configureKey, if configureKey != configuredConfigureKey, users need to reconfigure.

@sjorge
Copy link
Sponsor Contributor

sjorge commented Apr 7, 2024

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.

@Koenkk
Copy link
Owner Author

Koenkk commented Apr 7, 2024

@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

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

Successfully merging this pull request may close these issues.

2 participants