-
Notifications
You must be signed in to change notification settings - Fork 85
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
fix: battery zero incorrect #144
Changes from 4 commits
8cdcc63
a6421b9
cbff15d
cefa680
5e0ef54
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,18 +53,32 @@ def __init__( | |
|
||
async def update(self): | ||
try: | ||
current_vehicle_location = self.get_child_value("vehicleLocation") | ||
previous_vehicle_status = self.get_child_value("vehicleStatus") | ||
self.vehicle_data = await self.hass.async_add_executor_job( | ||
self.kia_uvo_api.get_cached_vehicle_status, self.token | ||
) | ||
self.set_last_updated() | ||
self.set_engine_type() | ||
if self.enable_geolocation_entity == True: | ||
if self.enable_geolocation_entity: | ||
current_vehicle_location = self.get_child_value("vehicleLocation") | ||
await self.hass.async_add_executor_job( | ||
self.set_geocoded_location, current_vehicle_location | ||
) | ||
|
||
async_dispatcher_send(self.hass, self.topic_update) | ||
if ( | ||
self.get_child_value("vehicleStatus.engine") == False | ||
and previous_vehicle_status is not None | ||
and previous_vehicle_status["engine"] == False | ||
and self.get_child_value("vehicleStatus.evStatus.batteryStatus") == 0 | ||
and previous_vehicle_status["evStatus"]["batteryStatus"] != 0 | ||
): | ||
_LOGGER.debug( | ||
f"zero battery api error, force_update started to correct data" | ||
) | ||
await self.force_update() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's fire this as a separate request rather than waiting results There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fuatakgun I can switch this ti a second request, but the reason I did it with waiting is so that scripts in HA that call the update function are able to tell when it actually finishes. If this is changed to async the scripts will call update thinking the values are updated but they won't be until the force update call finishes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, I see your point |
||
else: | ||
async_dispatcher_send(self.hass, self.topic_update) | ||
|
||
except Exception as ex: | ||
_LOGGER.error( | ||
f"{DOMAIN} - Exception in update : %s - traceback: %s", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we follow same naming below (current_vehicle_location). I am a bit obsessed with naming :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise, let's update below (current into previous)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fuatakgun I moved the definition of the currentLocation below the impl update call, I think it isn't so much a naming consistency issue as a bug. I couldn't think of a reason to update the location using the previous location during an update instead of the current.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to call external endpoint if location coordinates have not changed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment is about naming; if we call current location as current, i was saying we should call current vehicle data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ic, it didn't make sense. until I saw the parameter name is old_vehicle_location, previous_vehicle_location seems more inline with old_vehicle_location then current, and make more sense without having to understand it's use.