-
Notifications
You must be signed in to change notification settings - Fork 643
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
Use a foreground service for reliable background transmitting #4347
base: master
Are you sure you want to change the base?
Use a foreground service for reliable background transmitting #4347
Conversation
Maybe it's good to split the Bluetooth scan permission into its own PR which we can quickly approve+merge as that fixes an obvious oversight/bug, whereas the foreground service is an effort to improve things but also introduces new behavior? |
actually the scan permission is required in this PR because we are now using the scanner in this sensor where we were not before |
I missed that's new, sorry. In that case never mind and let's wait for positive results. |
Agreed this requires end users to confirm its working for them as I cannot reproduce |
50ee744
to
bfe0c9c
Compare
May have taken some time but we got end user confirmation :) probably want to keep this in beta for a lil bit to let it soak this is now ready for review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really able to test this myself but user comment sounds promising, and I don't see it causing any harm at the moment.
Edit: actually, have you tested this when scanning and transmitting is enabled? Do these changes interfere with regular scanning?
beaconManager?.enableForegroundServiceScanning(builder.build(), 445) | ||
beaconManager?.setEnableScheduledScanJobs(false) | ||
beaconManager?.beaconParsers?.clear() | ||
beaconManager?.backgroundBetweenScanPeriod = Long.MAX_VALUE | ||
beaconManager?.backgroundScanPeriod = 0 | ||
beaconManager?.foregroundBetweenScanPeriod = Long.MAX_VALUE | ||
beaconManager?.foregroundScanPeriod = 0 | ||
beaconManager?.startMonitoring(region) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to add a comment here explaining this enables a foreground service to improve reliability, so that one doesn't need to blame the code to find out why we are monitoring in a transmitting service.
Minor cleanup so you don't need beaconManager?.
everywhere:
beaconManager?.apply {
enableForegroundServiceScanning(builder.build(), 445)
setEnableScheduledScanJobs(false)
// etc
}
I thought I did that test before but now it seems that only one foreground service can be up at a time 🤔 not sure what to do here. I dont think we can share them either.
|
putting this back in draft while considering new changes based on the latest test comment |
Based on the most recent comment I am thinking of maybe updating our docs and sensor string description to mention the beacon monitor being enabled. |
Summary
Hopefully fixes: #3159 by implementing a similar foreground service like in #3369 based on the docs to create a "fake" scanner that looks for nothing to help keep the transmitter alive.
Needs end user testing. 🤞
Screenshots
Link to pull request in Documentation repository
Documentation: home-assistant/companion.home-assistant#pending
Any other notes
Code may not look pretty for now but wanted to get end user feedback