search

Hyundai-Kia-Connect / kia_uvo

A Home Assistant HACS integration that supports Kia Connect(Uvo) and Hyundai Bluelink. The integration supports the EU, Canada and the USA.
MIT License
413 stars 85 forks source link

Gaps in data - USA/Hyundai #207

Closed rappazzo closed 1 year ago

rappazzo commented 2 years ago

Region and Brand of car Hyundai USA

Describe the bug I've been tracking my EV battery in a widget on the UI, and I noticed periods of time in the history where the value is 0 for both short and long periods of time. image

This seems to come from the server side, so maybe this is a feature request to ignore some responses

Debug logs if an error occurred This is the results from the server with zero'd data for the battery zero-batt.txt

To Reproduce Steps to reproduce the behavior:

  1. Track the EV battery status
  2. Observe dips and blips in the history where the battery goes from to 0 and eventually back. Sometimes it is a dip, where the zero value lasts a few fetches, and sometimes it is just a blip where it is momentarily zero

Expected behavior At least for an EV, we would never expect the battery value to be zero. Even if zero was a legitimate case, a near-zero low value should suffice. So, I would like to ignore at least the EV data when the battery value reports 0. Not sure about other attributes. It does clearly have values for other attributes (the rough location with truncated decimals as per #196 from the vehicle status response).

fuatakgun commented 2 years ago

@dahlb implemented a very clean solution here: https://github.com/fuatakgun/kia_uvo/issues/55 Please double check if problem statement is different.

rappazzo commented 2 years ago

This case does seem to match #144.
I do see:

zero battery api error, force_update started to correct data

In the logs, but it must still not be triggering in all cases (see the graph above).

cdnninja commented 2 years ago

What version number are you running?

rappazzo commented 2 years ago

v1.2.3

cdnninja commented 2 years ago

Based on the logic I see the potential issue. It looks like it checks for 0 for current poll and not last poll. I am wondering if since it is multiple sets of data reporting wrong the system isn't ignoring it.

Are you seeing that zero battery API message multiple times throughout where it is 0 or just the first poll?

cdnninja commented 2 years ago

I think we will need debug log spanning the refreshes to see what is happening here. I am assuming the API just keeps returning a 0 value but would like to confirm. If so maybe we change the logic to use the last good value and keep trying so other things will refresh. It would show this static and not correct until the API decides to work.

rappazzo commented 2 years ago

I will see if I can add some specific logging this weekend to isolate the issue

dahlb commented 2 years ago

you could backport the improved fix I did here a few weeks ago for us kia, and today for us hyundai https://github.com/dahlb/ha_kia_hyundai/blob/master/custom_components/ha_kia_hyundai/api_cloud_us_hyundai.py#L231 You could also give it a try using that version, it hasn't had any hyundai testers yet but if there are any issues it should just be typo issue I can fix quickly with a ticket. It doesn't support EU yet but otherwise I think it includes all the issues in this repo fixed and some with easier design patterns and some better best practices.

the logic for the improved fix goes, the old fix asks the api to improve the data but the api can choose to ignore that ... the new fix let's the api force logic handle attempts at updating stale sync issues but ensures obviously flawed data doesn't reach the HA UI.

fuatakgun commented 2 years ago

Here you are, asking for help and telling users to use something else, classic @dahlb , talking about his best practices. In essence, he was just interested to keep his car warm to change diapers, now, he wants to compete while moving all know-how into his claimed repository.

My bad, you are not welcome here @dahlb .

rappazzo commented 2 years ago

I'm not sure where that is coming from, but I am open to any suggestions. @fuatakgun I suggest that if you don't want any contributions from someone, let's just ignore them, and try to keep it civil. No need to insult anyone, as that will turn away other would-be contributors.

Now, back to our regularly scheduled program...

I put in some logging, and I will be monitoring those logs over the course of this weekend. Once I get some reports on those logs I will work out a solution.

fuatakgun commented 2 years ago

Logs will be useful, let's wait and see. If existing if condition is true, this was not supposed to leak to UI at all, but somehow leaking through. But it is surely the response of servers, i can have the same issue on my kia connect app.

About contributions and discussions, any contribution is more than welcome, moreover, most of CA and US regions are done by contributors, with little or zero guidance from me. Anyway, it was my mistake to ask help from him.

@rappazzo , which specific thing you think is insult i would like to fix it as there was no intention to insult.

fuatakgun commented 2 years ago

@rappazzo any updates here?

rappazzo commented 2 years ago

The logging that I put in hasn't provided me anything too useful. I've put in code to detect the lack of ev data case and to use the previous result (Hyundai USA only). I've been running it for a day or two now. I want to make sure it holds up. I'm not sure if it would be worth moving that check to a base class or not.

rappazzo commented 2 years ago

So far the ev status hasn't dipped at all yet, so I feel good about this. As mentioned, I have only implemented this for Hyundai USA. Does anyone think that this is necessary for other brands or regions?

fuatakgun commented 2 years ago

Yes it is happening to me for eu, time to time, would be useful for all users

rappazzo commented 2 years ago

So I tried to move the implementation that was working for me to the Vehicle class, but it seems that the previous vehicle data is somewhat transient. In the Hyundai USA impl, I made a class variable to track the previous status. 

if vehicle_status["vehicleStatus"]["evStatus"]["batteryStatus"] == 0 and HyundaiBlueLinkAPIUSA.old_vehicle_status is not None:
    vehicle_status["vehicleStatus"]["evStatus"] = HyundaiBlueLinkAPIUSA.old_vehicle_status["vehicleStatus"]["evStatus"]

I guess I can put a class variable in the base class for this.

cdnninja commented 1 year ago

Could you test out this on the 2.X branch and let me know how it goes? I am most likely weeks away from making this the master release. It will be easier to fix in the new architecture if an issue.

rappazzo commented 1 year ago

I haven't touched my codebase in a long time. I imagine that it is quite out of date with the 2.0 baseline. I would be OK closing this issue as withdrawn, and if the issue persists in the 2.0 release, I can open a new issue for that, and possibly rewrite the code according to that.