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

Adds support for Android companion pairing, device bonding, iOS device name (via GAP) #760

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

ened
Copy link
Contributor

@ened ened commented Jul 4, 2023

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:

@ened ened force-pushed the android-companion-and-bonding branch from edc4b1b to d292562 Compare July 4, 2023 14:45
@ened ened changed the title Adds support for Android companion pairing & device bonding Adds support for Android companion pairing, device bonding, iOS device name (via GAP) Jul 5, 2023
Copy link
Contributor

@remonh87 remonh87 left a 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.

packages/flutter_reactive_ble/lib/src/reactive_ble.dart Outdated Show resolved Hide resolved
Copy link
Contributor

@remonh87 remonh87 left a 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


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")
Copy link
Contributor

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


@RequiresApi(Build.VERSION_CODES.O)
fun onActivityResult(data: Intent?): ScanResult? {
Log.d(TAG, "onActivityResult: $data")
Copy link
Contributor

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

packages/reactive_ble_mobile/protos/bledata.proto Outdated Show resolved Hide resolved
}
completion(.success(message))
} catch {
completion(.success(nil))
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants