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

Set car sensors state appropriately if sensor registers with unit of measurement #3764

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

dshokouhi
Copy link
Member

@dshokouhi dshokouhi commented Aug 7, 2023

Summary

Fixes #3762

When a sensor has a unit of measurement HA will expect certain states if non-numerical otherwise the sensor will not be added.

By default when we do not have carInfo the state will be unavailable. After the app is open user will either see the real state or unknown if the status is not success. All sensors will show the status attribute which will show the status of the sensor.

As the sensors get triggered by a listener we can safely call forceUpdate = true we either get a real state or we are getting a real status of the value to send to the user.

Removed the old string because we actually should not translate the states that we send, missed this in the original review 😬

Screenshots

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#969

Any other notes

@jpelgrom
Copy link
Member

jpelgrom commented Aug 7, 2023

So if the sensor has a unit of measurement we will default to unknown otherwise if the app was not open we will show the same state

Wouldn't unavailable be a better state when the car info isn't available? If it's unavailable there also will not be useful attributes. Different state when car info is not available depending on whether or not a unit of measurement is set is also (too) inconsistent (in my opinion).

we actually should not translate the states that we send

Not sure I agree for a string like this, especially considering core/frontend focus recently on making sure everything can be translated.

@dshokouhi
Copy link
Member Author

Not sure I agree for a string like this, especially considering core/frontend focus recently on making sure everything can be translated.

We can't translate state strings in the app they need to be translated in core for the integration. I think there might be an old issue where we discussed this before.

@jpelgrom
Copy link
Member

jpelgrom commented Aug 7, 2023

Yes so I would avoid strings that need translation like these :)

@dshokouhi
Copy link
Member Author

Yes so I would avoid strings that need translation like these :)

Lol we don't need to avoid them. We are fine with using English. Someone who is skilled enough needs to add support so the states can be translated. iOS has the same issue.

We are actually supposed to keep everything in English because the companion app docs are in English. It makes it easy to find sensors and other things we mention.

@jpelgrom
Copy link
Member

jpelgrom commented Aug 7, 2023

We are actually supposed to keep everything in English because the companion app docs are in English.

Hadn't heard that one before but considering the limitations it makes sense.

Note about unavailable / inconsistency is still relevant I think.

@dshokouhi
Copy link
Member Author

We are actually supposed to keep everything in English because the companion app docs are in English.

Hadn't heard that one before but considering the limitations it makes sense.

can't seem to find it now but I remember Paulus telling us what we needed to do if we wanted custom states translated which involved updating mobile_app integration then as we add more strings we update the integration.

Note about unavailable / inconsistency is still relevant I think.

agreed, its fixed and should be more consistent now.

  1. App not open - unavailable
  2. App open but no status not success - unknown
  3. App open and status success - state

@jpelgrom
Copy link
Member

jpelgrom commented Aug 7, 2023

Thanks for your patience while I also figure out what is a good approach here 🙏

Now that it is easy to follow, should it maybe be mentioned in the docs that unavailable -> Auto not active?

@dshokouhi
Copy link
Member Author

Now that it is easy to follow, should it maybe be mentioned in the docs that unavailable -> Auto not active?

Agreed, updated docs to mention both unavailable and unknown states so users can tell if a sensor is implemented or not :)

Copy link
Member

@jpelgrom jpelgrom left a comment

Choose a reason for hiding this comment

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

Good to get this in before a release so a breaking change is prevented :)

@JBassett JBassett merged commit 2d439f1 into home-assistant:master Aug 7, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HA error due car sensor not numeric
3 participants