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 BLE ibeacon transmitter as sensor to allow room presence. #1343

Merged
merged 17 commits into from
Feb 26, 2021

Conversation

Alfiegerner
Copy link
Contributor

@Alfiegerner Alfiegerner commented Feb 2, 2021

Summary

Added BLE emitting sensor. This sends out iBeacon messages over bluetooth every second, so that room presence tracking can be accomplished using roomassistant or esp32-mqtt-room. See issue #960

Screenshots

Screenshot_20210203-004725_Home Assistant
Screenshot_20210203-004918_Home Assistant

Link to pull request in Documentation repository

Documentation: home-assistant/companion.home-assistant#449
Documentation: home-assistant/companion.home-assistant#451 - for updated docs for new Command Notification, command_ble_transmitter

Any other notes

For the corresponding iphone changes, a good reference point would be app produced over at roomassistant: mKeRix/room-assistant#309

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.

Some of these requests may just be a question as I am not overly familiar with how this works. There are some things we need to address as well.

@dshokouhi
Copy link
Member

testing out the latest debug APK on my pixel 4 and I get the following error when I turn on the sensor:

2021-02-03 21:49:57.269 8695-8830/io.homeassistant.companion.android.debug E/BeaconTransmitter: Cannot start advertising due to exception
    java.lang.NullPointerException: Attempt to invoke virtual method 'void android.bluetooth.le.BluetoothLeAdvertiser.startAdvertising(android.bluetooth.le.AdvertiseSettings, android.bluetooth.le.AdvertiseData, android.bluetooth.le.AdvertiseCallback)' on a null object reference
        at org.altbeacon.beacon.BeaconTransmitter.startAdvertising(BeaconTransmitter.java:213)
        at org.altbeacon.beacon.BeaconTransmitter.startAdvertising(BeaconTransmitter.java:159)
        at io.homeassistant.companion.android.bluetooth.ble.TransmitterManager.startTransmitting(TransmitterManager.kt:51)
        at io.homeassistant.companion.android.sensors.BluetoothSensorManager.updateBLEtransmitter(BluetoothSensorManager.kt:151)
        at io.homeassistant.companion.android.sensors.BluetoothSensorManager.requestSensorUpdate(BluetoothSensorManager.kt:75)
        at io.homeassistant.companion.android.sensors.SensorReceiver.updateSensors(SensorReceiver.kt:150)
        at io.homeassistant.companion.android.sensors.SensorWorker$doWork$2.invokeSuspend(SensorWorker.kt:80)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

this is what the sensor screen stays at as well.

image

@Alfiegerner
Copy link
Contributor Author

testing out the latest debug APK on my pixel 4 and I get the following error when I turn on the sensor:

2021-02-03 21:49:57.269 8695-8830/io.homeassistant.companion.android.debug E/BeaconTransmitter: Cannot start advertising due to exception
    java.lang.NullPointerException: Attempt to invoke virtual method 'void android.bluetooth.le.BluetoothLeAdvertiser.startAdvertising(android.bluetooth.le.AdvertiseSettings, android.bluetooth.le.AdvertiseData, android.bluetooth.le.AdvertiseCallback)' on a null object reference
        at org.altbeacon.beacon.BeaconTransmitter.startAdvertising(BeaconTransmitter.java:213)
        at org.altbeacon.beacon.BeaconTransmitter.startAdvertising(BeaconTransmitter.java:159)
        at io.homeassistant.companion.android.bluetooth.ble.TransmitterManager.startTransmitting(TransmitterManager.kt:51)
        at io.homeassistant.companion.android.sensors.BluetoothSensorManager.updateBLEtransmitter(BluetoothSensorManager.kt:151)
        at io.homeassistant.companion.android.sensors.BluetoothSensorManager.requestSensorUpdate(BluetoothSensorManager.kt:75)
        at io.homeassistant.companion.android.sensors.SensorReceiver.updateSensors(SensorReceiver.kt:150)
        at io.homeassistant.companion.android.sensors.SensorWorker$doWork$2.invokeSuspend(SensorWorker.kt:80)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:571)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:750)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:678)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:665)

this is what the sensor screen stays at as well.

image

Looking now, just tried reinstalling latest apk from scratch and enabling / disabling and worked fine on my end.

@dshokouhi
Copy link
Member

Looks like I just needed to restart my phone lol, I got an ID now so I'll check out room assistant to test it out for the first time :)

@Alfiegerner
Copy link
Contributor Author

I was able to repro that issue with BT turned off, have just committed a fix to make sure we don't transmit if BT is off and add that info as a state message.

@dshokouhi
Copy link
Member

So I was able to play around with feature last night. One thing that occurred to me was that it seemed to give a decent hit on the battery life for keeping the service up for 1 hour. In the one hour test I did it drained as much battery as the production version that was there all day. So makes me wonder if we should do a few things:

  • Tie the service to a sensor setting so users do not "Enable all sensors" and then wonder why they start to have excessive battery drain. Once tied to a setting we don't have to worry about this and the command can continue to work as normal. It kinda defeats the purpose of enable all here and I can see a lot of users hitting this issue in that case.
  • Add a warning in the docs about high battery drain and also in sensor description
  • Make the transmitter a persistent foreground service so users know its active and to prevent it from ever getting killed by the system.

These 3 features came to mind since we use the same precautions for the new High Accuracy Mode setting that we have.

Any thoughts on this?

@Alfiegerner
Copy link
Contributor Author

Happy to look at these. Also could implement a transmission strength setting (Low, Medium,
High) which could save battery too.

I'm not 100% sure on the UX of having two multiple foregrounded services, would that mean having 2 persistent notifications?

Copy link
Collaborator

@JBassett JBassett left a comment

Choose a reason for hiding this comment

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

@dshokouhi I agree we should make the description obvious that this is going to drain the battery. I like the idea of a foreground service but I think this is ok for a first pass. I'm not sure about the enable all sensors issue, part of me wonders how many people are actually using it at this point. If you enable them all there are a lot of sensors!

…luetoothSensorManager.kt

Co-authored-by: Justin Bassett <bassett.justint@gmail.com>
@dshokouhi
Copy link
Member

@dshokouhi I agree we should make the description obvious that this is going to drain the battery. I like the idea of a foreground service but I think this is ok for a first pass. I'm not sure about the enable all sensors issue, part of me wonders how many people are actually using it at this point. If you enable them all there are a lot of sensors!

LOL I know I'm not the only crazy one 😛

My thoughts here were that currently when you enable all sensors (like I currently do) the battery drain is very minimal. I only see 1-3% usage in a full day for all sensors enabled. If a user enables them all on accident they will have no way of knowing without disabling each sensor one by one or realizing its a new feature where the culprit is. By keeping it in a setting users can keep all sensors enabled (if they wish) and turn on the setting via notification when they want. Some may have some OCD due to the enable all toggle lol. I am ok either way but I have a feeling we will see a few bug reports on it.

@Alfiegerner
Copy link
Contributor Author

@dshokouhi @JBassett , How about:

  • i add battery warning to description / docs
  • I add the setting as per High accuracy mode, to not be part of 'enable all'.
  • I don't implement the additional foreground service, but leave as is in sensor service.

And we can revisit the service if it's getting killed. Sound okay?

@JBassett
Copy link
Collaborator

JBassett commented Feb 5, 2021

@Alfiegerner sounds good.

  - Power transmitter values between high and ultra low.  Defaults to ultra low, should help with Battery concerns
  - A setting which allows the sensor to be turned on by default when the Enable all toggle is turned on.  This is off by default, users have to actively seek out this sensor to turn it on.
# Conflicts:
#	app/src/main/java/io/homeassistant/companion/android/sensors/BluetoothSensorManager.kt
@Alfiegerner
Copy link
Contributor Author

Alfiegerner commented Feb 6, 2021

Hi @dshokouhi @JBassett,

I've added the following:

  • Added a setting that allows users to specify power output of BLE transmitter, from Ultra Low, Low, Medium and High
  • Added a setting that by default prevents the sensor being turned of when the Enable all sensors is toggled. User can turn this off if they want.
  • Added warnings on battery drain to docs and description

I haven't done the refactor to inject TransmitterManager, I had a quick stab at this but couldn't get it working - but I can revisit that if you'd like.

Thanks!

p.s. there's a Changes Requested flag open but I can't see what it relates too, can you let me know if I've missed anything? Thanks.

@Alfiegerner
Copy link
Contributor Author

Hi @JBassett @dshokouhi - just checking anything else you need from me on this?

Thanks!

@JBassett JBassett merged commit e2783a8 into home-assistant:master Feb 26, 2021
Alfiegerner added a commit to Alfiegerner/android that referenced this pull request Feb 28, 2021
…ssistant#1343)

* First cut add BLE beacon as new sensor.

* Lint issues, also using hardcoded strings for state.

* Fixes and small tidies following review.

* Add command_ble_transmitter to MessagingService.kt, to control BLE Transmitter from HA notifications.

* Make description more specific for settings.

* Return if sensor not enabled

* Spaced.

* Command turn on for transmitter should stop it first, as state could be incorrect on Bluetooth being disabled.

* Don't attempt to transmit if BT is off, and include that information in the state.

* Update app/src/main/java/io/homeassistant/companion/android/sensors/BluetoothSensorManager.kt

Co-authored-by: Justin Bassett <bassett.justint@gmail.com>

* Add 2 new settings:
  - Power transmitter values between high and ultra low.  Defaults to ultra low, should help with Battery concerns
  - A setting which allows the sensor to be turned on by default when the Enable all toggle is turned on.  This is off by default, users have to actively seek out this sensor to turn it on.

* Add battery warning to description.

* Fix build - stray merge comma deletion.

* Fix small description text error.

Co-authored-by: Justin Bassett <bassett.justint@gmail.com>
JBassett added a commit that referenced this pull request Mar 1, 2021
…ter, improve robustness (#1384)

* First cut add BLE beacon as new sensor.

* Lint issues, also using hardcoded strings for state.

* Fixes and small tidies following review.

* Add command_ble_transmitter to MessagingService.kt, to control BLE Transmitter from HA notifications.

* Make description more specific for settings.

* Return if sensor not enabled

* Spaced.

* Command turn on for transmitter should stop it first, as state could be incorrect on Bluetooth being disabled.

* Don't attempt to transmit if BT is off, and include that information in the state.

* Update app/src/main/java/io/homeassistant/companion/android/sensors/BluetoothSensorManager.kt

Co-authored-by: Justin Bassett <bassett.justint@gmail.com>

* Add 2 new settings:
  - Power transmitter values between high and ultra low.  Defaults to ultra low, should help with Battery concerns
  - A setting which allows the sensor to be turned on by default when the Enable all toggle is turned on.  This is off by default, users have to actively seek out this sensor to turn it on.

* Add battery warning to description.

* Fix build - stray merge comma deletion.

* Fix small description text error.

* Fix left notification when high accuracy mode was disabled (#1366)

Some points which may fix this issue
* Make notification objects static
* Use ServiceCompat for stopForeground
* Additionally call cancel to remove notification

* Bump some app dependencies (#1374)

* Add BLE ibeacon transmitter as sensor to allow room presence. (#1343)

* First cut add BLE beacon as new sensor.

* Lint issues, also using hardcoded strings for state.

* Fixes and small tidies following review.

* Add command_ble_transmitter to MessagingService.kt, to control BLE Transmitter from HA notifications.

* Make description more specific for settings.

* Return if sensor not enabled

* Spaced.

* Command turn on for transmitter should stop it first, as state could be incorrect on Bluetooth being disabled.

* Don't attempt to transmit if BT is off, and include that information in the state.

* Update app/src/main/java/io/homeassistant/companion/android/sensors/BluetoothSensorManager.kt

Co-authored-by: Justin Bassett <bassett.justint@gmail.com>

* Add 2 new settings:
  - Power transmitter values between high and ultra low.  Defaults to ultra low, should help with Battery concerns
  - A setting which allows the sensor to be turned on by default when the Enable all toggle is turned on.  This is off by default, users have to actively seek out this sensor to turn it on.

* Add battery warning to description.

* Fix build - stray merge comma deletion.

* Fix small description text error.

Co-authored-by: Justin Bassett <bassett.justint@gmail.com>

* And fix service fields being added too many times (#1377)

* Improve robustness when BT is turned on and off. Also, tidy code, keep sense of when device parameters have changed in one place.

Co-authored-by: Justin Bassett <bassett.justint@gmail.com>
Co-authored-by: chriss158 <edgi@arcor.de>
Co-authored-by: Daniel Shokouhi <dshokouhi@gmail.com>
Co-authored-by: Bram Kragten <mail@bramkragten.nl>
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.

4 participants