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

Fix #213 2 characteristics with the same UUID #215

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

Conversation

truongsinh
Copy link

@truongsinh truongsinh commented Mar 11, 2019

Fix #213 2 characteristics with the same UUID

@CLAassistant
Copy link

CLAassistant commented Mar 11, 2019

CLA assistant check
All committers have signed the CLA.

@truongsinh truongsinh changed the title Fix https://github.com/pauldemarco/flutter_blue/issues/213 2 characteristics with the same UUID Fix #213 2 characteristics with the same UUID Mar 11, 2019
@pauldemarco
Copy link
Owner

pauldemarco commented Mar 13, 2019

Hi @truongsinh,

Look good! Thanks for your submission.

Some nits:

  • I would prefer to "flatten" out the BluetoothCharacteristicIdentifier class back into the BluetoothCharacteristic class. That way we don't break any API by requiring users to get the uuid with c.id.uuid.
  • The proto messages should also flatten out the characteristic id as a string, alongside the others like:
message BluetoothDescriptor {
    string uuid = 1;
    string serviceUuid = 2; // The service that this descriptor belongs to.
    string characteristicUuid = 3; // The characteristic that this descriptor belongs to.
    string characteristicInstanceId  = 4; // The instance id of the characteristic.
    bytes value = 5;
}
  • I'm not sure how I feel about the instance ID being an integer. That's an assumption that any future platforms we support must be sending back integers as their instance id's. Perhaps we should encode Android and iOS into Guid's to keep things future proof, thoughts?
  • I like how you use the hash on iOS side as the instance id, that's pretty slick! Just double checking on this, have we verified this instance id cannot be grabbed from the CoreBluetooth API at all?

Thanks again,
Paul

@truongsinh
Copy link
Author

truongsinh commented Mar 13, 2019

"flatten" out the BluetoothCharacteristicIdentifier

I don't have strong idea on this, but you're right about backward compatibility. On the other hand, if we keep it flatten, some documentation must be provided to explain UUID is not an "identifier" 😂. I can make some changes on this.

The proto messages should also flatten out

Assuming we keep flatten BluetoothCharacteristic, should we still keep proto messages counter part at current state, because it's not causing backward incompatible anyway, and it's clearer UUID is not an "identifier", when communicating to platform.

instance ID being an integer

For this, I think that Instance ID is only to distinguish between 2 characteristics of the same UUID (i.e. you can't distinguish between 2 characteristics only with Instance ID; 2 characteristics with same Instance ID but different UUID are still different) I doubt any device has 2^31 characteristics with the same UUID 😬

have we verified this instance id cannot be grabbed from the CoreBluetooth API

Yes I did some checking, but can't guarantee my checking was through 😜. I did search for getInstanceID equivalent on iOS without success. Let me also find the Stackoverflow link I read describing this behavior on iOS Swift.

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.

2 characteristics with the same UUID
3 participants