fix: request Q10 maps through dpMultiMap - #901
Conversation
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
Please see requested changes to where the code lives
| # only updates the fields that it is responsible for. | ||
| for trait in self._updatable_traits: | ||
| trait.update_from_dps(message.dps) | ||
| await self.map.update_from_dps(message.dps) |
There was a problem hiding this comment.
does it make sense for map to be at the end of the list of _updatable_traits?
There was a problem hiding this comment.
Its DPS update is synchronous. Q10PropertiesApi can then request the map content after the list update.
|
|
||
| async def refresh(self) -> None: | ||
| """Request the current saved map independently of general status.""" | ||
| if self._command is None: |
There was a problem hiding this comment.
This seems like a case that would never happen give it is always passed into the constructor. Update the typing to be CommandTrait?
| multi_map: dpMultiMap | None = field(default=None, metadata={"dps": B01_Q10_DP.MULTI_MAP}) | ||
|
|
||
|
|
||
| class MapContentTrait(MapListDps, TraitUpdateListener): |
There was a problem hiding this comment.
My previous comment to move things here may have been based on a misunderstanding of what is happening. Perhaps you can explain the overview of whats happening here. I see multiple requests happening... One is to refresh the list of maps and one is to get the map content?
In v1 we use separate traits for these separate concepts:
- maps: gets the list of maps
- map_content: gets the image byte content of for the current map
- home: combines all the data from rooms/maps/etc to hold state e.g.in case different map flags are loaded etc
It seems like we're moving to do something more like that here were we need muliple things, but i'm not sure so I don't want to give you bad advice.
There was a problem hiding this comment.
The current code combines two separate operations:
dpMultiMap with op: list requests the map list. The list arrives later in the DPS stream.
The code reads the first string map ID and sends dpMultiMap with op: get.
The get command does not contain the map data. The map data arrives later as a protocol-301 MAP_RESPONSE.
I combined these operations in MapContentTrait because the first response is asynchronous. However, this mixes the map-list state with the map-content state.
I propose this split:
MapsTrait owns the map list, the typed converter, and the list request.
MapContentTrait owns the get request, map packets, and rendered content.
Q10PropertiesApi connects the asynchronous map-list response to the map-content request. This has the same role as the V1 home-level coordination.
Does this match the design that you expect?
There was a problem hiding this comment.
Having split responsibilities like that seems good to me.
However on implementation details: I don't think we necessarily need to coordinate a map list with async updating map content. My take is that map lists may or may not change over time independent of map content. There may be a dependency, but only initially? I do understand that loading a new map can invalidate the map image which can be handled when accessing the map content.
Thank you for the thoughtful consideration on approach.
There was a problem hiding this comment.
Agreed. The map list is only a dependency when no map ID is available or when the selected map changes. Map content can otherwise refresh independently with the stored map ID.
So let's split the responsibilities:
- MapsTrait owns the map list
- MapContentTrait gets content for the stored map ID
- Q10PropertiesApi does not request content after each list update
- I will also require CommandTrait and add MapsTrait to _updatable_traits
2 participants
Summary
This PR separates the Q10 saved-map list from the Q10 map content.
MapsTraitrequests and stores the saved-map list.MapContentTraitrequests content for a stored map ID.Q10PropertiesApiroutes both map-list data and other DPS data through its normal update list.Why
The map list and map content can change at different times. A general status refresh does not reliably publish the map. The Q10 map request has this flow:
dpMultiMapwithop: list.dpMultiMapwithop: getand a string map ID.MAP_RESPONSE.The map list is an initial dependency only when no map ID is available. After that, a caller can refresh map content without a new list request.
Changes
MapsTraitfor the map list, its typed converter, and the list request.MapContentTrait.CommandTraitin both traits that send commands.MapsTraitto_updatable_traits.op: getacknowledgements inMapsTraitso they cannot replace a valid list.Validation
pytest -q: 895 tests passed and 86 snapshots passed.maincommit.Relationship to #900
This PR is independent and targets
main. The rendering work is split into #902, #903, and #904. PR #900 remains the full real-device and Home Assistant reference.