Skip to content

Conversation

@allenporter
Copy link
Contributor

@allenporter allenporter commented Dec 2, 2025

Update cache interface to add a device specific cache.

This fixes invalid use today where the device data is not stored based on device id in all cases.

Related to #615

Copilot AI review requested due to automatic review settings December 2, 2025 15:48
@allenporter allenporter marked this pull request as draft December 2, 2025 15:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new per-device caching structure to fix issues where device information was being clobbered across multiple devices. The changes introduce DeviceCacheData to store device-specific information and migrate from global cache fields to per-device storage indexed by device DUID.

  • Introduces DeviceCacheData class to encapsulate per-device cached information
  • Adds device_info field to CacheData for storing per-device cache data indexed by device DUID
  • Deprecates existing top-level cache fields in favor of per-device storage

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Lash-L
Lash-L previously approved these changes Dec 3, 2025
Copy link
Collaborator

@Lash-L Lash-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few small comments - I do not think this PR should be held up for these - maybe in a follow up later.

As well, what will happen right now for users who have a CacheData that is stored but now are have completely different typing? I don't think we have a versioning system or anything in place but maybe i have missed it

But that's more on the HA side, but just something I want to make sure we have covered

Comment on lines +249 to +250
if device_cache_data.home_map_content_base64 is None:
device_cache_data.home_map_content_base64 = {}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

may be worth setting home_map_content_base64 to a default factory so we never have to worry about it being none.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize we set the default value to dict and this can never be none. I'll drop this in a followup PR.

"""Home cache content for each map data indexed by map_flag.
This is deprecated in favor of `home_map_content_base64`.
This is deprecated. Use the per-device `home_map_content_base64` field instead.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to hold onto the deprecated value anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i want to just let it die and we can drop the old field name in the next major release as a breaking change.

@allenporter
Copy link
Contributor Author

few small comments - I do not think this PR should be held up for these - maybe in a follow up later.

As well, what will happen right now for users who have a CacheData that is stored but now are have completely different typing? I don't think we have a versioning system or anything in place but maybe i have missed it

But that's more on the HA side, but just something I want to make sure we have covered

My plan was to bump the store version and blow away the old cache entirely.

@Lash-L
Copy link
Collaborator

Lash-L commented Dec 3, 2025

My plan was to bump the store version and blow away the old cache entirely.

Perfect!

Lash-L
Lash-L previously approved these changes Dec 3, 2025
@allenporter allenporter merged commit ee02a71 into Python-roborock:main Dec 3, 2025
7 checks passed
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