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

Added support for PendingIntent based scans in Android O+. This allow… #401

Merged
merged 4 commits into from
Jun 7, 2018

Conversation

uKL
Copy link
Collaborator

@uKL uKL commented Mar 20, 2018

…s scanning without an active application, letting the system to wake up the process.

Fixes #369

@uKL uKL requested a review from dariuszseweryn March 20, 2018 10:21
@uKL uKL self-assigned this Mar 26, 2018
Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

Finally had some time for looking into this

@@ -69,6 +74,17 @@ class MockRxBleAdapterWrapper extends RxBleAdapterWrapper {

}

@RequiresApi(Build.VERSION_CODES.O)
public int startLeScan(List<ScanFilter> scanFilters, ScanSettings scanSettings, PendingIntent callbackIntent) {
return 0
Copy link
Owner

Choose a reason for hiding this comment

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

Indent

* @throws com.polidea.rxandroidble2.exceptions.BleScanException thrown if not possible to start the scan
*/
@RequiresApi(Build.VERSION_CODES.O)
void scanBleDeviceInBackground(@NonNull PendingIntent callbackIntent, ScanSettings scanSettings, ScanFilter... scanFilters);
Copy link
Owner

Choose a reason for hiding this comment

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

Thinking wether this should be Completable or not... On one hand this may only throw and is a synchronous call. On the other hand having a Completable would make it more consistent. Thoughts?

@Override
public void scanBleDeviceInBackground(@NonNull PendingIntent callbackIntent, ScanSettings scanSettings, ScanFilter... scanFilters) {
if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) {
Log.w(TAG, "PendingIntent based scanning is available for Android O and higher only.");
Copy link
Owner

Choose a reason for hiding this comment

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

RxBleLog?

final android.bluetooth.le.ScanSettings nativeScanSettings = scanObjectsConverter.toNativeSettings(scanSettings);
final int scanStartResult = rxBleAdapterWrapper.startLeScan(nativeScanFilters, nativeScanSettings, callbackIntent);

if (scanStartResult != 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

scanStartResult != NO_ERROR?

public interface BackgroundScanner {

/**
* Submits a scan request that will survive even if your process gets killed by the system. You can use this API to maintain a
Copy link
Owner

Choose a reason for hiding this comment

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

survive => work — we should not use colloquialisms imo ;)


/**
* Submits a scan request that will survive even if your process gets killed by the system. You can use this API to maintain a
* background scan without a need to keep your application foreground and active. The system will manage the scan for you wand
Copy link
Owner

Choose a reason for hiding this comment

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

wand => and

import static com.polidea.rxandroidble2.scan.ScanCallbackType.CALLBACK_TYPE_ALL_MATCHES

@Config(manifest = Config.NONE)
class BackgroundScannerTest extends RoboSpecification {
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't we also add tests for RxBleClient to call the BackgroundScanner?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The only thing it does it that it returns an instance that was injected through a constructor. No other interactions. I think it's not worth testing.

@@ -59,7 +65,7 @@ public void onScanToggleClick() {
.setCallbackType(ScanSettings.CALLBACK_TYPE_ALL_MATCHES)
.build(),
new ScanFilter.Builder()
.setDeviceAddress("B4:99:4C:34:DC:8B")
.setDeviceAddress("5C:31:3E:BF:F7:34")
// add custom filters if needed
Copy link
Owner

Choose a reason for hiding this comment

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

afaik there is need to put at least one filters—this could be mentioned here and in the docs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm gonna revert this change anyway. This is not relevant to a change we're introducing.

.build(),
new ScanFilter.Builder()
.setDeviceAddress("5C:31:3E:BF:F7:34")
// add custom filters if needed
Copy link
Owner

Choose a reason for hiding this comment

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

maybe add a spinner for filtering with HRM service/weight scales/dunno other?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This may be a good idea if we'll be introducing a new example dedicated to filters.

uKL added 2 commits June 4, 2018 14:23
…s scanning without an active application, letting the system to wake up the process.

Fixes #369
@uKL uKL force-pushed the feature/pending_intent_scan branch from 25b6b6f to 220e2c5 Compare June 4, 2018 12:34
@uKL
Copy link
Collaborator Author

uKL commented Jun 4, 2018

Discussed changes were applied.

CHANGELOG.md Outdated
@@ -1,5 +1,8 @@
Change Log
==========
Version 1.7.0
Copy link
Owner

Choose a reason for hiding this comment

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

Version 1.7.0-SNAPSHOT ;) + we should make appropriate changes in gradle.properties

Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

Still I see Log references in the code (should be RxBleLog)

- Replaced Robospock with Electricspock. Robospock is not maintained and does not support new APIs ;(
Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

Two minor changes and we are ready to go 🚀

@@ -32,7 +32,10 @@ dependencies {
testImplementation rootProject.ext.libs.junit
testImplementation rootProject.ext.libs.groovy
testImplementation rootProject.ext.libs.spock
testImplementation rootProject.ext.libs.robospock
testImplementation(rootProject.ext.libs.robolectric)
testImplementation (rootProject.ext.libs.electricspock){
Copy link
Owner

Choose a reason for hiding this comment

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

Can we have consistent spaces?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

@@ -13,11 +13,13 @@ ext {
rxandroid : 'io.reactivex.rxjava2:rxandroid:2.0.2',
rxrelay : 'com.jakewharton.rxrelay2:rxrelay:2.0.0',
junit : 'junit:junit:4.12',
groovy : 'org.codehaus.groovy:groovy:2.4.12:grooid',
groovy : 'org.codehaus.groovy:groovy:2.4.13',
spock : ['org.spockframework:spock-core:1.1-groovy-2.4',
'cglib:cglib-nodep:3.2.6',
'org.objenesis:objenesis:2.6'],
robospock : 'org.robospock:robospock:1.0.1',
Copy link
Owner

Choose a reason for hiding this comment

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

No longer needed I suppose

Copy link
Owner

@dariuszseweryn dariuszseweryn left a comment

Choose a reason for hiding this comment

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

💯

@uKL uKL merged commit 28a2f97 into master Jun 7, 2018
@uKL uKL deleted the feature/pending_intent_scan branch June 7, 2018 11:17
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.

2 participants