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

fix: battery zero incorrect #144

Merged
merged 5 commits into from
Nov 20, 2021
Merged

Conversation

dahlb
Copy link
Contributor

@dahlb dahlb commented Nov 16, 2021

No description provided.

async_dispatcher_send(self.hass, self.topic_update)
if (
self.get_child_value("vehicleStatus.engine") == False
and previous_vehicle_status.engine == False
Copy link
Member

Choose a reason for hiding this comment

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

Does this line really work? Have you tried this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fuatakgun nope that syntax is my old ruby showing, I've updated it to correct python syntax and verified it causes no regressions anymore locally, it will take a day or two in order to verify fully

and self.get_child_value("vehicleStatus.evStatus.batteryStatus") == 0
and previous_vehicle_status["evStatus"]["batteryStatus"] != 0
):
await self.force_update()
Copy link
Member

Choose a reason for hiding this comment

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

Let's fire this as a separate request rather than waiting results

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I see your point

@dahlb dahlb linked an issue Nov 19, 2021 that may be closed by this pull request
@dahlb dahlb added the bug Something isn't working label Nov 19, 2021
@@ -53,6 +53,7 @@ def __init__(

async def update(self):
try:
previous_vehicle_status = self.get_child_value("vehicleStatus")
Copy link
Member

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 :)
image

Copy link
Member

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)

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

@dahlb dahlb merged commit c0c70b5 into Hyundai-Kia-Connect:master Nov 20, 2021
@dahlb dahlb deleted the fix_battery_zero branch November 20, 2021 00:30
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Battery Incorrect Report - Drops to zero randomly
2 participants