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

Add CarInfo sensors #3399

Merged
merged 31 commits into from
Jul 17, 2023
Merged

Add CarInfo sensors #3399

merged 31 commits into from
Jul 17, 2023

Conversation

drosoCode
Copy link
Contributor

@drosoCode drosoCode commented Mar 5, 2023

Summary

Add basic support for CarInfo sensors (as mentionned in #3260)

Screenshots

Screenshot_20230305-234536_Home Assistant (2)

Link to pull request in Documentation repository

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

Any other notes

Can be tested using the DHU by running it with -c config/default_sensors.ini and typing commands like fuel 40 in the cli.

For some reason this doesn't work on my oneplus (7T pro, android 12) but seems to works fine on samsung (tested on S23 (android 13) and S9+ (android 10)).

As using the CarContext seems to be the only way to obtain the CarInfo class, the android auto app needs to be started to make the sensor work.

app/build.gradle.kts Outdated Show resolved Hide resolved
@jpelgrom jpelgrom linked an issue Mar 6, 2023 that may be closed by this pull request
app/build.gradle.kts Outdated Show resolved Hide resolved
@dshokouhi
Copy link
Member

As using the CarContext seems to be the only way to obtain the CarInfo class, the android auto app needs to be started to make the sensor work.

Is there really no way to get this in the background? I wonder if there is maybe something we can do to make the situation better then for users to avoid cases where they may forget to launch the app. I don't think a notification command can open a car app right? I wonder if we can use the existing intent for the connection sensor? 🤔

We also need to get a docs PR attached to this PR (once ready of course)

@drosoCode
Copy link
Contributor Author

I didn't found any way of getting the CarContext in the background. But I did find a way to show a notification on the car screen that is able to launch the android auto app when clicked (see the picture below).

Just for testing purposes, I put the notification code with the existing connection sensor, but I am wondering if I should put everything in the existing AndroidAutoSensorManager (since it's also android auto related)

notif

@jpelgrom
Copy link
Member

jpelgrom commented Apr 9, 2023

IMO you shouldn't force a notification when connecting to Android Auto. If the user wants this, they can do it themselves using the Android Auto connected sensor.

@dshokouhi
Copy link
Member

IMO you shouldn't force a notification when connecting to Android Auto. If the user wants this, they can do it themselves using the Android Auto connected sensor.

I was under the impression a standard HA notification will not show up, at least in my Android Auto setup they do not show up at all. I think they need to be a conversation or extend the car platform. Unless Android Automotive does something different than Android Auto when it comes to showing a notification.

@jpelgrom
Copy link
Member

I was under the impression a standard HA notification will not show up, at least in my Android Auto setup they do not show up at all. I think they need to be a conversation or extend the car platform. Unless Android Automotive does something different than Android Auto when it comes to showing a notification.

Haven't tested it but if something special is required shouldn't that be added as a 'normal' notification option instead of forcing it in here?

@dshokouhi
Copy link
Member

I was under the impression a standard HA notification will not show up, at least in my Android Auto setup they do not show up at all. I think they need to be a conversation or extend the car platform. Unless Android Automotive does something different than Android Auto when it comes to showing a notification.

Haven't tested it but if something special is required shouldn't that be added as a 'normal' notification option instead of forcing it in here?

That may be a lil trickier than normal as the extended notification code is coming from the car app library which is only available to users on the full version of the app.

@dshokouhi
Copy link
Member

What do we think about also updating the state of sensors that require CarContext to mention that the app needs to be started? It would be similar behavior to the Bluetooth Transmitter and Beacon Monitor sensors when Bluetooth is off. This would allow the sensors to have a better state instead of being stuck until the next time it is updated.

@jpelgrom
Copy link
Member

Haven't tested it but if something special is required shouldn't that be added as a 'normal' notification option instead of forcing it in here?

That may be a lil trickier than normal as the extended notification code is coming from the car app library which is only available to users on the full version of the app.

Good point. Still, I don't like this and would prefer giving the user control. I can't imagine the notification always being useful. "CarInfo" is also quite technical.

Something with a extension function returning different information depending on the flavor, similar to how code is shared between the main app and Wear OS, could work to make a notification feature to get the required intent, but it is out of scope of the PR.

@drosoCode
Copy link
Contributor Author

Ok, shoud I leave the android auto connection sensor here or also move it in main ?

@dshokouhi
Copy link
Member

Ok, shoud I leave the android auto connection sensor here or also move it in main ?

It can stay here (full) as there is no such thing as Open Source Android Auto so it shouldn't be functional in non-play store builds.

@drosoCode
Copy link
Contributor Author

For automotive support we need to ask for different permissions, is there a way to know if we are running the automotive version of the app ?

@dshokouhi
Copy link
Member

For automotive support we need to ask for different permissions, is there a way to know if we are running the automotive version of the app ?

yes you can determine that using a check like below (minus the build config check which checks to see if its the play store version 😃 )

https://github.com/home-assistant/android/blob/master/app/src/main/java/io/homeassistant/companion/android/launch/LaunchActivity.kt#L86

@drosoCode
Copy link
Contributor Author

For automotive support we need to ask for different permissions, is there a way to know if we are running the automotive version of the app ?

yes you can determine that using a check like below (minus the build config check which checks to see if its the play store version smiley )

https://github.com/home-assistant/android/blob/master/app/src/main/java/io/homeassistant/companion/android/launch/LaunchActivity.kt#L86

I'm not sure on how to make it work in the requiredPermissions function, as we don't have access to the context (to get the packageManager)

@dshokouhi
Copy link
Member

For automotive support we need to ask for different permissions, is there a way to know if we are running the automotive version of the app ?

yes you can determine that using a check like below (minus the build config check which checks to see if its the play store version smiley )
https://github.com/home-assistant/android/blob/master/app/src/main/java/io/homeassistant/companion/android/launch/LaunchActivity.kt#L86

I'm not sure on how to make it work in the requiredPermissions function, as we don't have access to the context (to get the packageManager)

yea that might require a refactor of that method to add context

@dshokouhi
Copy link
Member

Since the user needs to be on Android 8+ we should probably also use hasSensor() to hide the entire section in Manage Sensors screen from those devices. Just realized the android auto sensor does not do that but we can just do it here and fix that one in a separate PR :)

@dshokouhi
Copy link
Member

@drosoCode appreciate your patience during this process 😃 Don't believe I have any more comments left 😅

@drosoCode
Copy link
Contributor Author

@drosoCode appreciate your patience during this process smiley Don't believe I have any more comments left sweat_smile

I also appreciate the time taken to review this contribution 😃

Copy link
Member

@dshokouhi dshokouhi left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@JBassett JBassett merged commit 33dfc69 into home-assistant:master Jul 17, 2023
3 checks passed
@drosoCode drosoCode deleted the feature/carinfo branch July 25, 2023 13:04
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.

Add support for CarInfo
6 participants