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

feat: add F for USA and leave other regions with C #115

Merged
merged 3 commits into from
Nov 13, 2021

Conversation

dahlb
Copy link
Contributor

@dahlb dahlb commented Nov 9, 2021

made this minimialist change to support F for USA, initially I followed the distance pattern to allow users to pick the unit and handle conversion as you can try here https://github.com/dahlb/kia_uvo/tree/useless_temperature_conversions but realized that HA has a setting it automatically respects for F/C and that even if I convert it to C for the sensor based on a setting in this integration HA converts to back to F based on platform settings

@cdnninja
Copy link
Collaborator

The API returns the unit it uses in Canada. Does the USA side? For things like Miles/KM you can configure the kia side to use a different unit. Now most people leave it in KM or Miles but currently we handle that. Thinking we should do the same for C/F.

@dahlb
Copy link
Contributor Author

dahlb commented Nov 10, 2021

@cdnninja checkout https://github.com/dahlb/kia_uvo/tree/useless_temperature_conversions I did copy the distance pattern, but HA auto converts temperatures so it didn't matter how I converted them the HA platform ensured they were converted to the units it had preference for. I checked the USA Kia android app thoroughly and it has no ability to choose celsius. I didn't see anything in the existing code base for CA to handle F the sensor used to be hard coded to C

@cdnninja
Copy link
Collaborator

I'll take a look. You are correct it isn't handled yet for temp. I'll see if I can implement something for the other regions that detects what the api sends.

@dahlb
Copy link
Contributor Author

dahlb commented Nov 10, 2021

the api sends "unit": 1 for F, but as USA Kia doesn't allow me to choose C I couldn't get an example of what value the unit is for C or even if the units are consistent between implementations. I've been attempting to keep all my changes constrained to not change behavior for the other regions as I have no idea what their data formats are with how loosely defined the vehicle data is and I didn't see any sample requests/responses in the repo. @cdnninja that above mentioned branch does conversions based on api provided units assuming 1 means F and 2 is C, but that seems a really weird way for an API to define those units in EU, another reason I didn't port it to this branch.

@Ceer123
Copy link

Ceer123 commented Nov 11, 2021

This shows properly for me (USA based). API response shows "airTemp":{"value":"72","unit":1}
image

@dahlb dahlb mentioned this pull request Nov 11, 2021
@cdnninja
Copy link
Collaborator

I haven't got to this yet but I posted a Canadian data response here: https://github.com/fuatakgun/kia_uvo/wiki/Example-Data

I will switch it to raw json at some point. So for temp it returns a 0 for unit. I would assume a 1 if I change it in app would mean F. It appears the same for EU. It a pair much like distance.

@cdnninja
Copy link
Collaborator

Well turns out if I change unit from C to F in my app it doesn't actually change reporting in the API.

It changes from:

airTempUnit: C
    airTemp:
      value: 01H
      unit: 0

to

airTempUnit: F
    airTemp:
      value: 01H
      unit: 0

So essentially it is just telling the app to convert to F before it talks to the car. Now when you switch the car to F it does switch the "unit" to 1. Still reports 01h though so that would mean a new list of values in Fahrenheit.

Can you validate does if this is the same type of behavior you see in your car? I suspect in the USA you will get incorrect data since you receive absolute values not hex pointers.

I am going to merge this since I think it solves the high level issue and works for CA. We may also be able to ignore "unit" because of this list. It would mean when you convert your car HA still reports in C until you switch HA. Same would most likely apply for USA. I have tested switching HA to imperial and it does indeed convert over for this.

A to do after this would be to remove the "distance unit" setting in the settings of this. I think this should be following the HA config but open to thoughts. The unit lookup for distance matters much more since it doesn't have a translation table.

@cdnninja cdnninja merged commit e419d9d into Hyundai-Kia-Connect:master Nov 13, 2021
@dahlb
Copy link
Contributor Author

dahlb commented Nov 14, 2021

I agree it would be nice if preferred distance was pulled from the HA setting, especially since the default isn't my preference so I have to change it every time I delete/re-add the integration ;) but HA doesn't auto translate distance like it does for temperature so we'd still need the conversion logic just replace the kia_uvo configuration with the existing HA configuration. I'll try to remember to make a pr for that after the temperature bug this week.

@dahlb dahlb deleted the feat_temp_usa branch November 17, 2021 19:33
@github-actions
Copy link
Contributor

🎉 This PR is included in version 1.0.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants