New feature: adding scores for the last trip and ratio ev_distance / distance for statistics#160
New feature: adding scores for the last trip and ratio ev_distance / distance for statistics#160julesxxl wants to merge 12 commits into
Conversation
for more information, see https://pre-commit.ci
|
OK, The fist workflow failed because I had already put the new version of pytoyoda... which is required before this PR can be merged. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new feature to display driving scores from the last trip and an EV distance percentage in statistics. The changes are well-structured and cover documentation, translations, and the core logic. I've identified a few issues, mainly related to sensor configuration, missing capability checks, and minor bugs in calculations that could lead to incorrect data or errors. My review includes suggestions to fix these issues to ensure the new feature is robust and correctly integrated.
| SCORES_ENTITY_DESCRIPTION = ToyotaScoresEntityDescription( | ||
| key="score", | ||
| translation_key="score", | ||
| icon="mdi:car-info", | ||
| entity_category=EntityCategory.DIAGNOSTIC, | ||
| device_class=SensorDeviceClass.ENUM, | ||
| state_class=None, | ||
| value_fn=lambda vehicle: vehicle.last_trip.score, | ||
| attributes_fn=lambda vehicle: format_scores_attributes( | ||
| vehicle.last_trip, | ||
| vehicle._vehicle_info, # noqa : SLF001 | ||
| ), | ||
| ) |
There was a problem hiding this comment.
The SCORES_ENTITY_DESCRIPTION is misconfigured for a sensor that reports a numeric score.
device_classisSensorDeviceClass.ENUM, which is for sensors with a fixed set of string states. For a numeric score (0-100), this is incorrect. It should beNone.state_classshould beSensorStateClass.MEASUREMENTto indicate the value is a measurement.
These changes are needed for the sensor to be represented correctly in Home Assistant. Additionally, in create_sensor_configurations at lines 267-268, you should set the units for the score sensor to PERCENTAGE, as it is currently None.
| SCORES_ENTITY_DESCRIPTION = ToyotaScoresEntityDescription( | |
| key="score", | |
| translation_key="score", | |
| icon="mdi:car-info", | |
| entity_category=EntityCategory.DIAGNOSTIC, | |
| device_class=SensorDeviceClass.ENUM, | |
| state_class=None, | |
| value_fn=lambda vehicle: vehicle.last_trip.score, | |
| attributes_fn=lambda vehicle: format_scores_attributes( | |
| vehicle.last_trip, | |
| vehicle._vehicle_info, # noqa : SLF001 | |
| ), | |
| ) | |
| SCORES_ENTITY_DESCRIPTION = ToyotaScoresEntityDescription( | |
| key="score", | |
| translation_key="score", | |
| icon="mdi:car-info", | |
| entity_category=EntityCategory.DIAGNOSTIC, | |
| device_class=None, | |
| state_class=SensorStateClass.MEASUREMENT, | |
| value_fn=lambda vehicle: vehicle.last_trip.score, | |
| attributes_fn=lambda vehicle: format_scores_attributes( | |
| vehicle.last_trip, | |
| vehicle._vehicle_info, # noqa : SLF001 | |
| ), | |
| ) |
| "EV_percentage": int((statistics.ev_distance / statistics.distance) * 100) | ||
| if statistics.ev_distance and statistics.distance | ||
| else None, |
There was a problem hiding this comment.
The calculation for EV_percentage has a subtle bug. If statistics.ev_distance is 0, the condition if statistics.ev_distance and statistics.distance will be false, and the attribute will be None. However, the percentage should be 0. The check should be changed to handle this case correctly and be more explicit to avoid division by zero.
| "EV_percentage": int((statistics.ev_distance / statistics.distance) * 100) | |
| if statistics.ev_distance and statistics.distance | |
| else None, | |
| "EV_percentage": int((statistics.ev_distance / statistics.distance) * 100) | |
| if statistics.distance and statistics.ev_distance is not None | |
| else None, |
| return { | ||
| "score_acceleration": last_trip.score_acceleration | ||
| if last_trip.score_acceleration | ||
| else None, | ||
| "score_braking": last_trip.score_braking | ||
| if last_trip.score_braking | ||
| else None, | ||
| "score_advice": last_trip.score_advice if last_trip.score_advice else None, | ||
| "score_constant_speed": last_trip.score_constant_speed | ||
| if last_trip.score_constant_speed | ||
| else None, | ||
| } |
There was a problem hiding this comment.
The logic for formatting score attributes is buggy. A truthiness check on the score value (e.g., if last_trip.score_acceleration) will incorrectly treat a valid score of 0 as a falsy value, causing it to be replaced with None. This can be simplified and corrected by directly returning the score attributes, as None is an acceptable value if the score is not available.
return {
"score_acceleration": last_trip.score_acceleration,
"score_braking": last_trip.score_braking,
"score_advice": last_trip.score_advice,
"score_constant_speed": last_trip.score_constant_speed,
}| @@ -118,9 +122,23 @@ All values will show `None` if no data is available for the period. | |||
| | `Average_fuel_consumed` | The average fuel consumption in the respective period (can be l/100km or mpg). | | |||
| | `EV_distance` | The driving distiance in EV mode in the respective period . | | |||
There was a problem hiding this comment.
|
|
||
|
|
||
| class ScoresData(TypedDict): | ||
| """Representing Vehicle data.""" |
| native_unit: PERCENTAGE, | ||
| suggested_unit: PERCENTAGE, | ||
| ) -> None: | ||
| """Initialise the ToyotaStatisticsSensor class.""" |
There was a problem hiding this comment.
Please bump the pytoyoda version accordingly to the manifest.json value.
| "issue_tracker": "https://github.com/pytoyoda/ha_toyota/issues", | ||
| "requirements": ["pytoyoda>=4.0.1,<5.0", "arrow"], | ||
| "version": "v2.0.16" | ||
| "requirements": ["pytoyoda>=4.0.2,<5.0", "arrow"], |
There was a problem hiding this comment.
please bump the pytoyoda version to 4.1.0 instead of 4.0.2
There was a problem hiding this comment.
Mhmm, wierd. I stille see 4.0.2 instead of 4.1.0 here ^^
|
Hey @julesxxl, do you need any assistance in finalizing the PR? 😊 |
Must be merged AFTER the corresponding PR in pytoyoda repo. That will add the scores for the last trip as a sensor (global score as global value and sub-scores as properties). Global score, acceleration score, braking score, advice score and constant speed score.
Plus: ev_distance / distance percentage for each statistics sensor (interesting if your insurance is based on this value, so that you can get a lower price the next year).
(All checks passed here.)