-
Notifications
You must be signed in to change notification settings - Fork 335
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
Adds support for Android companion pairing, device bonding, iOS device name (via GAP) #760
base: master
Are you sure you want to change the base?
Conversation
edc4b1b
to
d292562
Compare
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.
Thanks for the PR I think it will be a great addition to the library. I will look into the implementation further this weekend but had now half hour time to overlook the structure. I am a bit concerned about adding the bond mode to connect method. Maybe it is better we add a specific method for companion pairing.
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.
Now the full review: again thank you very much for implementing this I think it is close to be integrated. Please check out my remarks. Also some generic things:
- Add unit tests for the compianonflow on dart
- Add unit tests for the protobuf conversion methods related to device name and companion flow
- Document the public methods Especially with
createBond
it is important that we emphasise that this will only work on android.
Reach out if you need help
...ve_ble_mobile/android/src/main/kotlin/com/signify/hue/flutterreactiveble/PluginController.kt
Outdated
Show resolved
Hide resolved
|
||
override fun onActivityResult(requestCode: Int, resultCode: Int, data: Intent?): Boolean { | ||
if (requestCode == CompanionHandler.SELECT_DEVICE_REQUEST_CODE && resultCode == Activity.RESULT_OK) { | ||
Log.d(TAG, "received result from device selection activity, parsing via companion handler") |
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.
let's remove the debug logs
- remove debug logs
..._ble_mobile/android/src/main/kotlin/com/signify/hue/flutterreactiveble/ble/BondingManager.kt
Outdated
Show resolved
Hide resolved
|
||
@RequiresApi(Build.VERSION_CODES.O) | ||
fun onActivityResult(data: Intent?): ScanResult? { | ||
Log.d(TAG, "onActivityResult: $data") |
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.
let's also remove here the debug logging
...e_ble_mobile/android/src/main/kotlin/com/signify/hue/flutterreactiveble/model/BondingMode.kt
Outdated
Show resolved
Hide resolved
packages/reactive_ble_platform_interface/lib/src/model/association_info.dart
Outdated
Show resolved
Hide resolved
packages/reactive_ble_platform_interface/lib/src/model/bonding_mode.dart
Outdated
Show resolved
Hide resolved
packages/reactive_ble_platform_interface/lib/src/reactive_ble_platform_interface.dart
Show resolved
Hide resolved
} | ||
completion(.success(message)) | ||
} catch { | ||
completion(.success(nil)) |
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.
This branch doesn't look like success to me.
This addresses #272, #628, #641 in the same PR.
For a project, I needed multiple additions, so the code is quite mixed.
Let's look at this PR holistically first. (I needed both sets of functionality for a project).
Whole set of changes:
CBPeripheral.name
method, partially addresses Provide separate access to device name in Generic Access Profile and Advertisement Data. #641scan
- from my App, which is great)