Replace charging_time_remaining with charging_end_time in bmw_connected_drive #60942
Conversation
|
Hey there @gerard33, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Thanks! Perfectly fine with me. |
| unit_metric=TIME_HOURS, | ||
| unit_imperial=TIME_HOURS, | ||
| "charging_end_time": BMWSensorEntityDescription( | ||
| key="charging_end_time", |
There was a problem hiding this comment.
Just want to make sure, this returns a UTC datetime timestamp?
There was a problem hiding this comment.
It is a tz-aware datetime object (https://github.com/bimmerconnected/bimmer_connected/blob/master/bimmer_connected/vehicle_status.py#L452) and displayed the correct time when testing.
EDIT: Nevermind, it is showing a value moved by one hour (CET timezone). I'll update to return UTC.
EDIT²: Nevermind², the tz-aware datetime values are parsed correctly and shown in the entity registry in UTC. If shown via lovelace they are converted to the correct timezone.
b584822 to
678db85
Compare
|
I just found out that I forgot to add something in the library for this to work. I'll have to release a new version before this can be merged. |
|
Updated the library & adjusted the PR text for the new library changelog & diff. |
|
Please rebase on latest dev branch. Then we can merge. 👍 |
0c7a56e to
c510e2a
Compare
|
Rebased and updated the new sensor to |
|
I think I just figured out why the time was not reported correctly by the API, however it is not yet merged or released in the library (bimmerconnected/bimmer_connected#382). Does it make more sense to:
The second option of course only applies if this PR would make it to 2021.12, don't know what your plans are here. |
|
No, this PR won't make the December release tomorrow. |
|
(y) Thats perfectly fine. |
|
Good. Ping me when we're ready. |
c510e2a to
34d27bf
Compare
|
I think this is ready for HA next. Just wondered if there is an ADR/general statement that one should use a timestamp instead of a duration (that of course changes all the time, cluttering the state machine). I remember reading something similar for a breaking change to |
|
Yes, we don't allow relative times in the state machine if the relative time changes just because time passes. If the time measures an interval and only is updated when the device or service feature changes, it may be ok to store that in the state machine. Eg a device was started, ran for 1 min, then stopped, so run time increased by 1 minute. We normally don't want to use seconds as resolution for run times though, as that is normally not interesting and causes not needed state changes. Minutes is ok. We don't have an ADR for it though. |
|
Should we merge? |
|
I've just added half of a sentence to the breaking change section why we are doing it. If that looks fine for you, this is ready to be merged. Thanks! |
Breaking change
charging_time_remaining(in hours) has been replaced withcharging_end_time(timestamp) to not clutter the HA state machine. Please calculate the remaining time using HA sensors/automations if needed.Proposed change
Replace
charging_time_remaing(in hours) withcharging_end_time(timestamp).As the BMW API seems to be very flaky and we have to parse the end time from a string like
100% at ~04:01AM, a second sensorcharging_end_time_mybmwshows the orignal value extracted from the API.I originally planned to set the original value as sensor attribute, but as this is not desired added it as a second sensor.
The original value is needed as there are many discussions if the data extracted is correct at all (with some users saying that the BMW API itself returns wrong values which is hard to verify if not easily visible in HA):
Upgrade toNo dependency upgrade needed, as 0.8.6 is already part of 2021.12.4.bimmer_connected==0.8.6is needed for this functionalityType of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: