Cache and update the last Vehicle

[dvx76]

I hit a wall in skodaconnect/homeassistant-myskoda#632 trying to selectively exclude updates of the Vehicle.health based on the last timestamp:

When intentionally excluding specific capabilities (~endpoints) the
problem is that the corresponding Vehicle fields will be None in the
returned Vehicle.

So either the client/caller must have the knowledge of the
capability->endpoint->field mapping and stitch together the previous and
new Vehicle, or the MySkoda class must store the last Vehicle(s) and
update what can be updated. The second option seems much cleaner and
keeps internal logic encapsulated in the myskoda lib.

I've used the opportunity to refactor how the timestamp field is set. Rather than allowing it to be None and explicitly having to set it in the RestApi methods, we can use field.default_factory to automatically set the timestamp when an instance is created. Also create a common base model to deduplicate the timestamp field.

[WebSpider]

Thanks for working on this, i had encountered this issue as well and it stopped me from implementing the caching for all responses. Great work 🎉

[WebSpider]

Great stuff 🍻

This will allow us to be a lot more flexible in caching and thus use less API credits, yay!

Als a follow-up, we may need to revisit the code that does updates, so we force-update only whats needed, and cache the rest.

Also, i think this generally breaks homeassistant.update_entity, we may need to fix that in a follow-up as well.

[WebSpider]

It may be because im reviewing this on mobile or because of sleep deprivation, but i dont see where this dict gets filled with values?

[dvx76]

In the else of this if statement 😄

self.vehicles[vin] = Vehicle(info, maintenance)

[dvx76]

Als a follow-up, we may need to revisit the code that does updates, so we force-update only whats needed, and cache the rest.

Yeah there's definitely more we can do here. I'll create an issue to look further into this.

Also, i think this generally breaks homeassistant.update_entity, we may need to fix that in a follow-up as well.

How so? The return values from get_vehicle() and get_partial_vehicle() remain the same. I'm only changing the signature of the internal _request_capability_data() method.

[WebSpider]

Also, i think this generally breaks homeassistant.update_entity, we may need to fix that in a follow-up as well.

How so? The return values from get_vehicle() and get_partial_vehicle() remain the same. I'm only changing the signature of the internal _request_capability_data() method.

It is related to the previous remark: update_entity could now no longer update the entity if the cache is valid. I'm not sure that actually is the intention of the action.

[dvx76]

It is related to the previous remark: update_entity could now no longer update the entity if the cache is valid. I'm not sure that actually is the intention of the action.

Ah ok you mean that a forced refresh using the homeassistant.update_entity action would no longer be guaranteed to actually fetch new data for all entities? That would be the result of changes in homeassistant-myskoda (in skodaconnect/homeassistant-myskoda#632), not just the changes in this PR, right?

I'll poke around in the homeassistant-core code to see if we have a way (in the coordinator) to differentiate between a manually triggered update vs a scheduled one.

[WebSpider]

It is related to the previous remark: update_entity could now no longer update the entity if the cache is valid. I'm not sure that actually is the intention of the action.

Ah ok you mean that a forced refresh using the homeassistant.update_entity action would no longer be guaranteed to actually fetch new data for all entities? That would be the result of changes in homeassistant-myskoda (in skodaconnect/homeassistant-myskoda#632), not just the changes in this PR, right?

I'll poke around in the homeassistant-core code to see if we have a way (in the coordinator) to differentiate between a manually triggered update vs a scheduled one.

Yeah, kind of.

Right now the behaviour of calling update_entity is a manual refresh of all entities of a VIN/Coordinator when this action is called. When we introduce this caching and have a cache valid lifetime per Endpoint, this behaviour probably changes.

Since we are communicating calling update_entity with argument any entity as a way to force a refresh of car data, we should be aware that this change will possibly be breaking for the integration.

I think i prefer update_entity to update the Endpoint related to the entity only, but how this would work codewise im not sure yet.

As long as we only record the timestamps in the library and not do any active caching inside the library, we should be fine and this discussion can move to the integration. It would be preferred to not do any caching in the library to me, since we would then have to introduce more state. To me, any state belongs in the integration.

[dvx76]

As long as we only record the timestamps in the library and not do any active caching inside the library, we should be fine and this discussion can move to the integration. It would be preferred to not do any caching in the library to me, since we would then have to introduce more state. To me, any state belongs in the integration.

So with the solution as I have it now in this PR, technically the library does caching, in the sense that it stores the last Vehicle. If get_vehicle() gets called and either some capabilities are excluded, or some endpoints fail, the response will still be a 'full' Vehicle, with the excluded/failed attributes coming from the last successful result.

However, the library does not independently decide to skip fetching a specific capability/endpoint and return the 'cached' result (and I agree that it should never do that).

If we want to completely avoid any form of state/caching in the library I think we need to do away with the concept of a Vehicle within the library and instead present individual methods for each endpoint, returning the underlying object (e.g. return Charging, Positions, ...).

I'm not set on any specific approach but what I didn't want to do is surgically rebuild the Vehicle object in the Coordinator based on it's current value + the partial response of a get_vehicle() call (in this particular case, one with Vehicle.health = None).