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

If not all DB records are received, try one-by-one #531

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

tstabrawa
Copy link
Contributor

@tstabrawa tstabrawa commented Nov 11, 2024

Also, improve DeviceDbGet & DeviceRefresh test coverage.

Proposed change

Thanks for considering this PR!

This PR changes the DeviceRefresh handling so that, after attempting a bulk database download (with DeviceDbGet), if the database is incomplete, Insteon-MQTT will attempt to retrieve the missing DB entries one-by-one.

This feature was previously proposed by @krkeegan in this message on #233:

I thought about this a bit more. I am still of the mindset that the current system using the single request is preferable. However, I thought of a way to add in support for individual requests as a fallback. The process would look like:

  1. Perform the db get as we do currently. (at the start of this the cached db is emptied)
  2. Whenever a) the process finishes OR b) the process times out
  3. Check the cached DB
  4. If the cached DB lacks either a) a last_entry entry OR b) is missing entries for addresses (the addresses are non-contiguous)
  5. Then spawn a separate handler to a) specifically query the missing entries AND b) query entries at the end until the last_entry is found.

Basically, the separate handler is patching up the holes of what was missed.

The two downsides to this are:

  1. Some complexity in the code (but keeping this as a separate handler should contain things)
  2. We will lose the "future proof" feature of not needing to know the address layout in the devices. This is a bummer, but probably not a big deal.

The one-by-one downloading is performed in DeviceScanManagerI2, to reduce the effect of the added complexity on the rest of the DeviceRefresh/DebiceDbGet code.

Notably, this one-by-one fallback approach appears to be the same technique employed by pyinsteon (which is used by Home Assistant's native Insteon integration).

Flake8 passes without errors, pylint shows no new warnings, and all new code is shown as covered by pytest coverage report. Also, DeviceDbGet and DeviceRefresh are now listed as 100% covered.

Additional information

Example force-refresh output for a particularly persnickety device is attached (device ID's have been changed to so as not to dox innocent devices):

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.
  • Code documentation was added where necessary

If user exposed functionality or configuration variables are added/changed:

  • Documentation added/updated -- N/A

Also, improve DeviceDbGet & DeviceRefresh test coverage.
@krkeegan
Copy link
Collaborator

Thank you for doing this.

I realize I have been absent for a while. I need to spend some time getting back up to speed on how everything works. Hopefully I can get to this by next week. We also have a number of other changes that need to be made. Some changes in HomeAssistant require some modifications on our end.

@tstabrawa
Copy link
Contributor Author

No rush from my end. I actually started on these changes over a year ago, and then life got busy.

On the other hand, if you were looking for an excuse to dive back in, then you're welcome. :-)

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