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

Update LastUpdated sensor value on service events #416

Closed

Conversation

fursov
Copy link
Contributor

@fursov fursov commented Dec 7, 2024

As discussed in #359, the car_captured_timestamp can be updated with timestamp value available from service events.

As discussed in skodaconnect#359,
the car_captured_timestamp can be updated with timestamp value
available from service events.
Copy link
Contributor

@WebSpider WebSpider left a comment

Choose a reason for hiding this comment

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

Thanks for following up👍

Since there is no garuantee that a received event has a more recent timestamp than what we have in memory, can you add a check to see if the timestamp is more current than what we have in memory?

Copy link
Member

@dvx76 dvx76 left a comment

Choose a reason for hiding this comment

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

I guess this comes down to the question what do we actually want to represent with car_captured_timestamp ?

Is it "the last time we got a full update of the car data" or is it "any data". Individual entities already report when they last changed value so I'm not sure if we really want/need a partial update (e.g. just a few charging entities after a charge event) to also update car_captured_timestamp. But I'm not against it either.

@@ -228,12 +231,18 @@ async def _on_charging_event(self, event: EventCharging):
self.set_updated_vehicle(vehicle)

async def _on_access_event(self, event: EventAccess):
if self.data.vehicle.status and (timestamp := event.event.timestamp):
self.data.vehicle.status.car_captured_timestamp = timestamp
await self.update_vehicle()
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't update_vehicle already update this? For an access event a least the timestamp on the event isn't really representative of the car_capture_timestamp. That is, the event itself didn't really provide any updated data.

@fursov
Copy link
Contributor Author

fursov commented Dec 8, 2024

I guess this comes down to the question what do we actually want to represent with car_captured_timestamp ?

Is it "the last time we got a full update of the car data" or is it "any data". Individual entities already report when they last changed value so I'm not sure if we really want/need a partial update (e.g. just a few charging entities after a charge event) to also update car_captured_timestamp. But I'm not against it either.

Yes, this comes down to getting same behavior as in the application or we will have own meaning for the sensor. As far as I see, if I go to the individual pages in the app, then I see different "updated" timestamp values there, meaning they display e.g. charging updated timestamp on the charging page, while the first overview page might have different time.
I also do not have strong opinion.
The best is to understand, what we use this info for. If it is informative only (e.g. no automations based on that) - I believe it does not really matter, we can have just most fresh timestamp there.

@WebSpider
Copy link
Contributor

WebSpider commented Dec 9, 2024

I guess this comes down to the question what do we actually want to represent with car_captured_timestamp ?

Is it "the last time we got a full update of the car data" or is it "any data". Individual entities already report when they last changed value so I'm not sure if we really want/need a partial update (e.g. just a few charging entities after a charge event) to also update car_captured_timestamp. But I'm not against it either.

Looking at the responses we're seeing, there seems to be a difference in timestamps representing data obtained from the car and timestamps related to events.

Maybe it's a better idea to not use the same sensor for both events, but create a new sensor that represents the timestamp of latest event occurred, as opposed to last poll occurred, which i guess resembles car_captured_timestamp the most. If someone then really wants to know 'when did anything happen', they can automate on any change of either sensor, or create their own template sensor that returns the most recent change.

@dvx76
Copy link
Member

dvx76 commented Dec 14, 2024

Maybe it's a better idea to not use the same sensor for both events, but create a new sensor that represents the timestamp of latest event occurred, as opposed to last poll occurred, which i guess resembles car_captured_timestamp the most.

I like this idea. We already have this for all events related to operations, through the operation sensor. I would prefer to not touch that one so that it can be used specifically in relation to operations and and their current/last state. But perhaps we can have a generate event sensor which just reports the last event received?

I would then leave car_captured as it is. It's a 1-1 mapping from a specific field returned by the API (vehicle.status.car_captured_timestamp).

For individual entities HA already reports the last change and last updated timestamps.

Edit: if that sounds good maybe just close this PR and open an issue to implement a generic event sensor?

@fursov
Copy link
Contributor Author

fursov commented Dec 15, 2024

Thanks for the feedback. That is fine to me.

@fursov fursov closed this Dec 15, 2024
@fursov fursov mentioned this pull request Dec 15, 2024
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.

3 participants