Added ChargingBroadcastReceiver which can instantly update plugged in… #525

yoxjames · 2020-03-22T22:36:44Z

One thing I ran into when doing my own home automation was the desire to figure create an abstract concept for when everyone in the house is asleep. The simplest way to accomplish this was to see if both me and my partner had our phones plugged in. This is something we always do before bed. I wanted the fan to instantly go on in this scenario as well as the noise machine.

I was able to set this up using the battery sensor but I noticed there was a lot of lag time... Around 15 minutes which made me immediately think that this was being driven by WorkManager which has a 15 minute minimum periodic work run time. I pulled down the code and found that to be correct as the code was running SensorWorker every 15 minutes as a Periodic Work Request.

I wanted it to be instant though, and one way I immediately thought to do that was via a BroadcastReceiver. I got this working locally and can now enjoy my sleep automations firing instantly. I thought I should share the little work I have done as I really like this project and would like to contribute more in the future! I figured this would be an interesting way to get my feet wet even if there needs to be some changes.

So what I did was register a Broadcast Receiver for ACTION_POWER_CONNECTED and ACTION_POWER_DISCONNECTED. You might be wondering why I didn't just register a Broadcast Receiver for ACTION_BATTERY_CHANGED. I actually did that in a previous version but found that it was executing a lot. This might be totally fine but I wanted to avoid doing something that had serious performance ramifications. I can totally change it back. However my current approach runs a lot less but has one important caveat. The is_charging state attribute will not immediately update while the charger_type attribute will. This is, presumably, because while Android knows that it was plugged in instantly some time must elapse for Android to be certain that the connection is actually supplying sufficient power to charge the battery. Switching to ACTION_BATTERY_CHANGED remedies this (is_charging is updated shortly after the charger_type) but again I had some concerns about this.

Furthermore I made one additional structural change you can take or leave. I switched from starting the SensorManager in various activities and fragments and instead moved it to the main App class and also registered another BroadcastReceiver for BOOT events so that the SensorWorker would be started when the device boots. I noticed when playing around with this that two SensorWorkers were actually being enqueued. I changed around a couple of the function calls (such as using enqueueUniquePeriodicWork which appears to have remedied this.

This is my first ever open source contribution. I am super open to criticism and being told to redo things. While I have made changes that I think are improvements I recognize there may be things I dont know or failed to consider. I hope that in some form I can contribute this change and hope this starts a conversation about how we might be able to avoid lag time when updating sensors! I look forward to your feedback!

Quick note on testing. I didn't see a great opportunity to add tests to my contribution. I am a champion of the importance of proper unit testing and would be happy to add them if you think that would help. I could probably wedge a presenter into the BroadcastReceivers I have created and do tests that way. However I didn't find that to be super valuable for what I was doing in this PR.

… status

…sage

homeassistant · 2020-03-22T22:36:46Z

Hi @yoxjames,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

yoxjames · 2020-03-22T22:47:28Z

app/src/main/java/io/homeassistant/companion/android/sensors/SensorWorker.kt

                    .build()

-            WorkManager.getInstance(context).cancelAllWorkByTag("sensors")
-            WorkManager.getInstance(context).enqueue(sensorWorker)
+            WorkManager.getInstance(context).enqueueUniquePeriodicWork(TAG, ExistingPeriodicWorkPolicy.REPLACE, sensorWorker)


This change appeared to stop duplicate SensorWorkers from operating. I was noticing that when doing some debugging myself. The only odd thing here is that I used the TAG from the companion object. I could use "sensors" as was done previously if that is desired.

yoxjames · 2020-03-22T22:48:47Z

app/src/main/java/io/homeassistant/companion/android/sensors/ChargingBroadcastReceiver.kt

+
+    private suspend fun BatterySensorManager.registerSensors(context: Context) = apply {
+        getSensorRegistrations(context).forEach {
+            try {


The other place where we do this (SensorWorker) also used this pattern. I had debated finding some way to avoid duplication of logic here but ended up not really being sure where to put that.

balloob · 2020-03-23T23:12:50Z

app/src/main/java/io/homeassistant/companion/android/HomeAssistantApplication.kt

@@ -19,6 +20,8 @@ class HomeAssistantApplication : Application(), GraphComponentAccessor {

        AndroidThreeTen.init(this)
        graph = Graph(this, 0)
+        // Start the sensor worker if they start the app. The only other place we start this a Boot BroadcastReceiver
+        SensorWorker.start(this)


We only report sensor values if location reporting is on. I think with this change it will always report ? Also it looks like it was called inside sendLocationUpdate because it would restart the 15 minute interval. The idea was that it would send at least once every 15 minutes, but more often if other things cause data to be sent (like connected status changed via this PR)

I like the idea of sending sensors even with location disabled personally. That start method will send all available sensors then restart the 15 minute timer, it should be safe to call here.

balloob · 2020-03-23T23:13:46Z

app/src/main/java/io/homeassistant/companion/android/sensors/SensorWorker.kt

+                addAction(Intent.ACTION_POWER_CONNECTED)
+                addAction(Intent.ACTION_POWER_DISCONNECTED)


Does this also trigger for wireless charging or are those other intents?

According to the Android docs it should. I can confirm this with an emulator though.

Unfortunately it doesn't look like the emulator actually supports any plug type other than AC which makes testing this tough. However according to the Android docs this approach is sound as any type of power connection should theoretically fire this Intent.

balloob · 2020-03-23T23:15:44Z

Hey, thanks for the contribution and welcome to the Home Assistant community. I'm not the main Android dev, but did leave some comments.

JBassett

All in all I think it's a good PR!

JBassett · 2020-03-23T23:29:31Z

app/src/main/java/io/homeassistant/companion/android/HomeAssistantApplication.kt

@@ -19,6 +20,8 @@ class HomeAssistantApplication : Application(), GraphComponentAccessor {

        AndroidThreeTen.init(this)
        graph = Graph(this, 0)
+        // Start the sensor worker if they start the app. The only other place we start this a Boot BroadcastReceiver
+        SensorWorker.start(this)


I like the idea of sending sensors even with location disabled personally. That start method will send all available sensors then restart the 15 minute timer, it should be safe to call here.

JBassett · 2020-03-23T23:32:23Z

app/src/main/AndroidManifest.xml

+        <!-- Start things like SensorWorker on device boot -->
+        <receiver android:name=".HomeAssistantBootReceiver">
+            <intent-filter>
+                <action android:name="android.intent.action.BOOT_COMPLETED"/>


If we are going to add a generic boot receiver then we should remove the boot specific logic from LocationBroadcastReceiver and send a broadcast to it instead starting location tracking.

I can look into this change!

JBassett · 2020-03-23T23:41:20Z

app/src/main/java/io/homeassistant/companion/android/sensors/ChargingBroadcastReceiver.kt

+        updateJob?.cancel()
+        updateJob = ioScope.launch {
+            val batterySensorManager = BatterySensorManager().registerSensors(context)
+            integrationUseCase.updateSensors(
+                batterySensorManager.getSensors(context).toTypedArray()
+            )
+        }
+    }


Rather than duplicating the logic why not call SensorWorker.start(context)? That way we get all sensors updating.

Interesting thought. I'll give that a try. It should work as long as it replaces the existing SensorWorker. The SensorWorker should still be running at this point. This just adds an additional sensor update in addition to the 15 minute interval worker we already had.

I could even add a delay to the restart of SensorWorker which might get around is_charging somewhat. It's using a delay as a solution which generally is not the best idea imo. It probably makes more sense to update what the OS immediately knows which is that the charger_type vs trying to hack around getting is_charging to work right.

In any case I'll mess around with this soon.

Let's try not to add more features to this PR. Instead, keep it for a future PR. It's easier to review small PRs.

Also, if you're interested, you can join us on #devs_mobile_apps on the Home Assistant Discord server. It's where we hangout :) https://www.home-assistant.io/join-chat/

Did you try calling SensorWorker.start(context) instead? That should be included in this PR because we don't want to duplicate this logic.

Yeah sorry see my comment I posted at the bottom of the PR. That wasn't working because it still entrusts Android to actually run the SensorWorker code. Therefore it wouldn't reliably immediately execute and could be delayed by up to 15 minutes which is the problem I had hoped to solve with this PR.

Totally missed that. Can we do SensorWorker(context, WorkerParameters()).doWork() instead? If we can't do that then we should extract that code somewhere that we can call it independently so we don't duplicate it.

I fully agree with the sentiment. I originally didn't love that I was duplicating logic but didn't want to start ripping things apart for a first PR. Essentially I played it safe. However I will definitely look into a way of doing it. The best way, to me, feels like taking what SensorWorker and pulling that into a class that can be called elsewhere and composed into other sources of sensor updates besides a work manager. I'll go ahead and try something out.

Ok I added a new interface/impl SensorUpdater which contains that logic in one place. It is not inserted into the dagger graph. In order to do that I would need to make some significant changes that I felt were starting to get out of scope and I didn't want to cause additional delays for this functionality. I can definitely tackle this in a follow up PR where we can focus more exclusively on that. Let me know what you think.

dshokouhi · 2020-03-24T00:12:46Z

This should close #391 however with the caveat that is_charging will not immediately update.

yoxjames · 2020-03-24T04:20:00Z

Updated the PR to address the comments everyone had.

yoxjames · 2020-03-24T04:21:42Z

app/src/main/AndroidManifest.xml

@@ -57,9 +64,6 @@
            android:enabled="true"
            android:exported="true">
            <intent-filter>
-                <action android:name="android.intent.action.BOOT_COMPLETED" />
-                <action android:name="android.intent.action.QUICKBOOT_POWERON" />


I ended up just removing QUICKBOOT_POWERON (both versions). They did not appear to actually do anything that I could see before.

We added these extra actions because some device manufactures use different actions.
https://stackoverflow.com/a/42622385

I should clarify. The LocationBroadcastReceiver appears to me to completely ignore these which is why I thought they could be deleted. It looks like it only processed BOOT_COMPLETED Honestly I am not sure if they are a good idea to include.

https://github.com/home-assistant/home-assistant-android/blob/f6618527d4f5f1233e7e0515f9816eb3ac282e5b/app/src/main/java/io/homeassistant/companion/android/background/LocationBroadcastReceiver.kt#L57

JBassett · 2020-03-24T11:55:03Z

app/src/main/AndroidManifest.xml

@@ -57,9 +64,6 @@
            android:enabled="true"
            android:exported="true">
            <intent-filter>
-                <action android:name="android.intent.action.BOOT_COMPLETED" />
-                <action android:name="android.intent.action.QUICKBOOT_POWERON" />


We added these extra actions because some device manufactures use different actions.
https://stackoverflow.com/a/42622385

app/src/main/java/io/homeassistant/companion/android/HomeAssistantBootReceiver.kt

yoxjames · 2020-03-25T02:23:37Z

Pushed another commit to address comments. @JBassett , I have added the restart location tracking stuff into the Boot Receiver. I did have to make one change to PermissionManager.restartLocationTracking, I changed it so that it only takes Context and not activity. sendBroadcast doesn't require a fully fledged activity (it's actually implemented on Context rather than Activity) so this should be fine, however I wanted to ensure I highlighted it.

Also I had to revert just using SensorWorker.start in the ChargingBroadcastReceiver the reason is that SensorWorker.start(..) still entrusts Android to run the work at it's leisure which I assume to mean some time hopefully in 15 minutes. I noticed that unless I had the app in the foreground it would not instantly update the status. The only way to truly ensure that is to take the WorkManager stuff out and directly update the sensors as I had before.

I finally got on the Discord and would be happy to chat about any of this tomorrow!

yoxjames · 2020-03-29T00:08:27Z

app/src/main/java/io/homeassistant/companion/android/sensors/AllSensorsUpdaterImpl.kt

+            NetworkSensorManager()
+        )
+
+        if (integrationUseCase.isBackgroundTrackingEnabled()) {


Noticed this change from SensorWorker and made sure to implement it here!

yoxjames · 2020-03-29T00:09:46Z

app/src/main/java/io/homeassistant/companion/android/HomeAssistantApplication.kt

+        // This will cause the sensor to be updated every time the OS broadcasts that a cable was plugged/unplugged.
+        // This should be nearly instantaneous allowing automations to fire immediately when a phone is plugged
+        // in or unplugged.
+        registerReceiver(


Ended up moving this into HomeAssistantApplication rather than SensorWorker feels like it belongs here more.

JBassett · 2020-03-29T12:24:33Z

app/src/main/java/io/homeassistant/companion/android/PresenterModule.kt

+
+        @Binds
+        fun bindSensorUpdater(sensorUpdater: AllSensorsUpdaterImpl): SensorUpdater


I noticed you never actually inject this anywhere, if that's the case there is no reason to add it here. If you want to use dagger make sure you always have it injected rather than create it every use.

Ugh yeah this was a relic from when I was putting it into the Dagger graph. The PR got a little large so I backed up from that. Will remove.

Hypfer · 2020-04-01T12:20:59Z

This is the most verbose PR i've ever seen 😄

Xitro01 · 2020-06-14T12:10:23Z

Is this something that could be implemented for iOS as well?

yoxjames added 2 commits March 22, 2020 16:00

Added ChargingBroadcastReceiver which can instantly update plugged in…

9c76a74

… status

Couple formatting changes noticed after pushing. Will squash this mes…

eb09b22

…sage

homeassistant added the cla-needed label Mar 22, 2020

yoxjames commented Mar 22, 2020

View reviewed changes

homeassistant added cla-signed and removed cla-needed labels Mar 22, 2020

balloob reviewed Mar 23, 2020

View reviewed changes

JBassett requested changes Mar 23, 2020

View reviewed changes

Addressed PR Comments

070a13e

yoxjames commented Mar 24, 2020

View reviewed changes

JBassett requested changes Mar 24, 2020

View reviewed changes

More PR comments!

7a39105

yoxjames added 3 commits March 28, 2020 17:57

Pulled out logic into SensorUpdater

bac05b4

Couple thigns I noticed after pushing... because of course I did.

af096c8

Resolved merge conflicts

ad78bd8

yoxjames commented Mar 29, 2020

View reviewed changes

JBassett requested changes Mar 29, 2020

View reviewed changes

Removed unnecessary Binds

da32195

JBassett approved these changes Mar 29, 2020

View reviewed changes

JBassett merged commit 825dd78 into home-assistant:master Mar 29, 2020

This was referenced Mar 29, 2020

Sent battery status update is sent whenever the charging state changes. #391

Closed

Battery state sensor not reporting charging but instead shows not_charging when plugged in #535

Closed

dshokouhi mentioned this pull request Apr 10, 2020

[Beta Issue] New 'Battery_State' sensor goes from 'discharging' to 'not_charging' #550

Closed

dshokouhi mentioned this pull request Aug 12, 2020

Battery and next alarm sensor improvements #740

Merged

		addAction(Intent.ACTION_POWER_CONNECTED)
		addAction(Intent.ACTION_POWER_DISCONNECTED)


		@Binds
		fun bindSensorUpdater(sensorUpdater: AllSensorsUpdaterImpl): SensorUpdater

Added ChargingBroadcastReceiver which can instantly update plugged in… #525

Added ChargingBroadcastReceiver which can instantly update plugged in… #525

Conversation

yoxjames commented Mar 22, 2020 • edited

homeassistant commented Mar 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

balloob commented Mar 23, 2020

JBassett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoxjames Mar 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshokouhi commented Mar 24, 2020

yoxjames commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yoxjames commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypfer commented Apr 1, 2020

Xitro01 commented Jun 14, 2020

yoxjames commented Mar 22, 2020 •

edited

yoxjames Mar 24, 2020 •

edited