Integrate car sensors with automotive, and added permissions #4122
Conversation
| override val name: Int | ||
| get() = R.string.sensor_name_car | ||
|
|
||
| override suspend fun getAvailableSensors(context: Context): List<SensorManager.BasicSensor> { | ||
| this.context = context.applicationContext |
There was a problem hiding this comment.
Those lateinit context set are a bit sketchy. I had to set it from more methods, as it wasn't set in some calls. I added it to every method accepting a context, just in case it happens again in the future.
I'm considering 2 other options: Adding it on construction (Not sure if possible), or simply drilling it through method calls, so we're sure we actually have a context when calling something that needs it.
I'd like to see more opinions about this
There was a problem hiding this comment.
So we do actually use a latestContext method of storing the latest context received in the app for some sensors, very similar to what you are doing here.
There was a problem hiding this comment.
I see. Renaming to latestContext, to keep the same naming as the other sensors
app/src/main/java/io/homeassistant/companion/android/sensors/CarSensorManager.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/io/homeassistant/companion/android/sensors/CarSensorManager.kt
Show resolved
Hide resolved
|
|
||
| private fun allDisabled(): Boolean = sensorsList.none { isEnabled(context, it) } | ||
| if (isAutomotive && !carSensor.automotiveEnabled || !isAutomotive && !carSensor.autoEnabled) { |
There was a problem hiding this comment.
Do we need to add some parenthesis here so the conditions are properly evaluated?
There was a problem hiding this comment.
Adding them to improve readability
|
Looks really good, will be following with interest |
Summary
Integrate car sensors with Android Automotive:
To better separate Auto from Automotive, I decided to add the core differences (Sensor availability and permissions) to the sensors themselves. This way, we have all the sensor logic and requirements in the same location, which is interesting when we start adding more sensors.
For that I made a CarSensor wrapper class of BasicSensor (I considered inheritance, but data classes are final, and I'm not sure changing them would have been worth it). Everything else is quite the same, but using the sensor objects instead.
Screenshots
Example entities on an Automotive emulator:
The permission request on enabling some sensors:
Link to pull request in Documentation repository
We could change the "Currently unavailabe for AAOS users" to "Currently unavailabe for AAOS users from Google Play" (Or similar). But I'd say it's not worth it yet, at least until the full flow is ready (?)
Any other notes
Only available in Automotive Full flavor (Not in the Google Play version yet). Sensors must be manually enabled, which can't be done in the minimal flavor. Enabling the sensors from the server won't ask for permissions from what I tested, so it may only work for some.