[Feature] Support multiple service/characteristic instances with same UUID

[nebkat]

Untested on iOS, working on Android.

Based on https://github.com/pauldemarco/flutter_blue/pull/215

Closes #795

[chipweinberger]

wow thanks!! this looks pretty good.

1. please test on iOS. you can buy an old iOS device for about $150.

2. use shorter names. serviceIndex,secondaryServiceIndex, characteristicIndex

3. iOS: hashes are not guaranteed to be unique. so your code will not always work. you should just use it's array location, i.e. index. e.g.

for (int characteristicIndex = 0; characteristicIndex < characteristics.length; characteristicIndex++);

4. in dart, the index should default to 0

class BluetoothCharacteristic {
  final DeviceIdentifier remoteId;
  final Guid serviceUuid;
  final int serviceIndex;
  final Guid? secondaryServiceUuid;
  final int? secondaryServiceIndex;
  final Guid characteristicUuid;
  final int characteristicIndex;

  BluetoothCharacteristic({
    required this.remoteId,
    required this.serviceUuid,
    this.serviceIndex = 0,
    this.secondaryServiceUuid,
    this.secondaryServiceIndex = 0,
    required this.characteristicUuid,
    required this.characteristicIndex = 0,
  });

[nebkat]

1. please test on iOS. you can buy an old iOS device for about $150.

Is it possible to build for iOS on a Windows machine? Anyway I've since had a colleague confirm that this is working as expected on iOS!

2. use shorter names. serviceIndex,secondaryServiceIndex, characteristicIndex

Currently this is named after the Android instanceId. I was thinking it could be better to make a Guid/int pair type, perhaps called BluetoothAttribute, and use it everywhere where we have the two? Can also provide getters to still be able to access the uuid property directly.

3. hashes are not guaranteed to be unique. so your code will not always work. you should just use it's array location, i.e. index. e.g.

for (int serviceIndex = 0; serviceIndex < services.length; serviceIndex++);

Can we be certain that the order will never change? Based on https://github.com/onmyway133/Runtime-Headers/blob/master/macOS/10.12/CoreBluetooth.framework/CBCharacteristic.h I suspect the internal hash function is just combining the handle and the uuid - so unless there are hundreds of characteristics with the same UUID it is highly unlikely they would collide.

4. in dart, the index should default to 0

class BluetoothCharacteristic {
  final DeviceIdentifier remoteId;
  final Guid serviceUuid;
  final int serviceIndex;
  final Guid? secondaryServiceUuid;
  final int? secondaryServiceIndex;
  final Guid characteristicUuid;
  final int characteristicIndex;

  BluetoothCharacteristic({
    required this.remoteId,
    required this.serviceUuid,
    this.serviceIndex = 0,
    this.secondaryServiceUuid,
    this.secondaryServiceIndex = 0,
    required this.characteristicUuid,
    required this.characteristicIndex = 0,
  });

Sure - will do unless you like my proposal for BluetoothAttribute.

[chipweinberger]

Is it possible to build for iOS on a Windows machine?

ah good point. I don't think so.

Currently this is named after the Android instanceId. I was thinking it could be better to make a Guid/int pair type, perhaps called BluetoothAttribute, and use it everywhere where we have the two? Can also provide getters to still be able to access the uuid property directly.

I think just keep it simple. What you have now is good.

I just dont like the name. it's longer, more complicated, not available on iOS. But I understand it makes sense if this feature was android-only.

Can we be certain that the order will never change?

Yes. They guarantee order on iOS, because it's important for things like this.

[nebkat]

Have completed the changes and will have colleague verify iOS again.

The [characteristic.service.characteristics indexOfObject:characteristic] everywhere feels somewhat convoluted but I believe this will achieve the desired effect.

Also implemented primary/secondary service support for descriptors.

[chipweinberger]

I have been refactoring secondary services, so we are going to have a merge conflict, sorry about that.

I'm about to push the changes to master branch.

[chipweinberger]

here is the commit.

https://github.com/chipweinberger/flutter_blue_plus/commit/2064c18e40d1a01e2bf1e382c746acc588cc029f

please read this comment for context: https://github.com/chipweinberger/flutter_blue_plus/issues/948#issuecomment-2461056547

[nebkat]

No worries, I was actually considering proposing that exact change so I am glad to see it! Will refactor tomorrow.

[chipweinberger]

also, you'll need to mirror some of the changes I made.

1. always use the same order. I use, in order: remoteId, svcUuid, chrUuid, descUuid, primarySvcUuid

for you, you should do: remoteId, svcUuid, svcIndex, chrUuid, chrIndex, descUuid, primarySvcUuid, primarySvcIndex

2. you need to update code that makes keys

                    String key = remoteId + ":" + serviceUuid + ":" + characteristicUuid + ":" + descriptorUuid + ":" + primaryServiceUuid;

e..g mWriteDesc, mWriteChr, and the same for iOS: writeChrs, writeDesc

[chipweinberger]

also make sure you update this code, to check that the indexes are equal

  // get known service
  BmBluetoothService? get _bmsvc {
    if (FlutterBluePlus._knownServices[remoteId] != null) {
      for (var s in FlutterBluePlus._knownServices[remoteId]!.services) {
        if (s.serviceUuid == serviceUuid) {
          if (s.primaryServiceUuid == primaryServiceUuid) {
            return s;
          }
        }
      }
    }
    return null;
  }

  /// get known characteristic
  BmBluetoothCharacteristic? get _bmchr {
    if (_bmsvc != null) {
      for (var c in _bmsvc!.characteristics) {
        if (c.characteristicUuid == uuid) {
          return c;
        }
      }
    }
    return null;
  }

[nebkat]

Although having had a quick look at it I'm not sure I expected includedServices to be removed. I think that was a good representation of the service hierarchy as it is defined in GATT.

Could we make it bi-directional by retaining includedServices alongside primaryServiceUuid? It would just involve initializing all BluetoothService instances first with primaryServiceUuid assigned, then on a 2nd pass populating the includedServices.

[chipweinberger]

@nebkat , I added it back: https://github.com/chipweinberger/flutter_blue_plus/commit/a42ca5dd5f1a933b4ce124f0150ecd1a0576aad8

I just temporarily removed it.

The underlying representation changed.

Now instead of returning a List<List<BmBluetoothService>>, we now return List<BmBluetoothService> + added a primaryServiceUuid field.

The more important change is that I wanted to switch from secondaryServiceUuid, to primaryServiceUuid. It's simpler to think about and now all code can use the primaryService concept instead of mixing and matching primary/secondary + includedServices as we move around the codebase.

Also, iOS and Android don't use secondaryServiceUuid either. They just call it serviceUuid + a isSecondary flag. So now our API matches theirs.

[chipweinberger]

I'm thinking about your change more.

I think you're right.

on ios youre probably right it's better to use the hash. but we could also make our own simple hash too if possible. that's probably better.

[chipweinberger]

I'm also wondering if we should remove primaryServiceUuid from the public API.

tbh I'm not sure the best way to represent all of this stuff, especially with the addition of handles. But I know secondaryServiceUuid was not the right solution.

[chipweinberger]

looking at this, they call it handle as well

@interface CBCharacteristic : NSObject {
    CBUUID * _UUID;
    NSArray * _descriptors;
    NSNumber * _handle;
    BOOL  _isBroadcasted;
    BOOL  _isNotifying;
    CBPeripheral * _peripheral;
    long long  _properties;
    CBService * _service;
    NSData * _value;
    NSNumber * _valueHandle;
}

and in the BLE spec as well, they use handles

Service UUID: 0x180F (Battery Service)
  - Handle: 0x0001
  - Attribute: Included Service
    - Handle: 0x0002 (reference to Environmental Sensing Service)
    - UUID: 0x181A (Environmental Sensing Service)

Service UUID: 0x181A (Environmental Sensing Service)
  - Handle: 0x0002
  - Characteristics: ...

But I think we should keep it index.

[chipweinberger]

better idea, what if we instead added a index to Guid?

class Guid {
  final List<int> bytes;
  final String index = 0;

string representation:

if (index != 0) {
   // 0000-0000-1000-8000-00805f9b34fb:$index
}

normally it will be set to 0.

and only in cases where there are dupicate services, or duplicate characteristics in a service, or duplicate descriptors within a charachteristic, will it have a value.

I think we should try to make it work with an index, first. That seems best to me.

[nebkat]

better idea, what if we instead added a index to Guid?

I would have to disagree there. A Guid class should represent exactly that, not a compound of Guid and index. If we wanted to go down that route I think a BluetoothAttribute class would make more sense. Also worth noting a descriptor for example doesn't support multiple of the same UUID.

---

However, if we are open to wider changes I would propose the following:

GATT does internally use handle to identify specific entries in the table, but both Android and iOS hide this from us and expect us to rely on references to specific instances of BluetoothGattCharacteristic/CBCharacteristic (and service equivalent). In other words, the index/instanceId/hash are all implementation details at the Java/Objective-C level.

Consequently it would make sense that we hide this implementation detail from the user in Dart, and rely on the same principle of identifying by specific instances of BluetoothCharacteristic.

Thus I would suggest the following structure:

class BluetoothAttribute {
    final DeviceIdentifier remoteId;
    final Guid uuid;
    final int index = 0;
}

class BluetoothService extends BluetoothAttribute {
    final bool isPrimary;
    final BluetoothService? parent;
    final List<BluetoothCharacteristic> characteristics = [];
    final List<BluetoothService> includedServices = [];

    BluetoothService._({
        required super.remoteId,
        required super.uuid,
        required super.index,
        required this.isPrimary,
        this.parent,
    });

    void _addIncludedService(BluetoothService service) {
        includedServices.add(service);
    }

    void _addCharacteristic(BluetoothCharacteristic characteristic) {
        characteristics.add(characteristic);
    }
}

class BluetoothCharacteristic extends BluetoothAttribute {
  final BluetoothService service;
  final List<BluetoothDescriptor> descriptors;

  BluetoothCharacteristic._({
    required super.remoteId,
    required super.uuid,
    required super.index,
    required this.service,
  });

  void _addDescriptor(BluetoothDescriptor descriptor) {
    descriptors.add(descriptor);
  }
}

class BluetoothDescriptor extends BluetoothAttribute {
  final BluetoothCharacteristic characteristic;

  BluetoothDescriptor._({
    required super.remoteId,
    required super.uuid,
    required this.characteristic
  });
}

Upon receiving the discovered services we would perform the two-stage mapping: initializing all the classes with their parent service/characteristic, and then adding them to the respective lists of children.

This would have the following benefits:

• Significant code simplification
• Lightweight - all classes are just holding their own UUID/index and potentially references to their parents/children
• Closely mirrors the GATT representation of it
• Also works similarly to native Android/iOS implementation
• No longer need to reference service, primary service, characteristic etc at all levels of the tree
• Still able to access all that information e.g. descriptor.characteristic.service.parent.uuid
• Can provide getters to simplify this e.g. get service => characteristic.service, get parentService => characteristic.service.parent
• Can provide getters to simplify transition/deprecation e.g. get serviceUuid => service.uuid

If this is something we would be open to I would gladly provide a PR.

[chipweinberger]

I like it. However, I would use the same existing names where possible. i.e. characteristicUuid instead of just uuid.

we already have a uuid convenience accessor.

In fact, I think we can implement this with basically no breaking changes to the API?

class BluetoothCharacteristic extends BluetoothAttribute {

  final BluetoothAttribute _service;
  final BluetoothAttribute? _primaryService;

  Guid get serviceUuid => _service.uuid;
  Guid get characteristicUuid => uuid;
  Guid? get primaryServiceUuid = > _primaryService.uuid;

  final List<BluetoothDescriptor> descriptors;

  BluetoothCharacteristic({
    required super.remoteId,
    required serviceUuid,
    required characteristicUuid,
    this.primaryServiceUuid,
    serviceIndex = 0,
    characteristicIndex = 0,
    primaryServiceIndex = 0
  }) super.uuid = characteristicUuid, 
    _service =  BluetoothAttribute(remoteId, serviceUuid, serviceIndex) ;

  void _addDescriptor(BluetoothDescriptor descriptor) {
    descriptors.add(descriptor);
  }
}

To be honest, I like your approach more. But I'd rather not have to go "2.0" with the API.

Let's push a "1.0" compatible version first.

Or maybe we can go 2.0 if we provide 1.0 compatible constructors as well.

[chipweinberger]

but the more a think about it. I think the clean 1.0 way is to not have a BluetoothAttribute class.

just add indexes to the existing classes. put them all together not interleaved, looks nicer:

  final DeviceIdentifier remoteId;
  final Guid serviceUuid;
  final Guid characteristicUuid;
  final Guid? primaryServiceUuid;

  final int serviceIndex;
  final int characteristicIndex;
  final int? primaryServiceIndex;

However, Internally, I still like the idea of using the uuid:$index strings. For example, in the Bm* classes, and in the locateCharacteristic functions. There's no need to add a bunch more arguments everywhere. It just gets too ugly / complicated.

[chipweinberger]

I would have to disagree there. A Guid class should represent exactly that, not a compound of Guid and index. If we wanted to go down that route I think a BluetoothAttribute class would make more sense. Also worth noting a descriptor for example doesn't support multiple of the same UUID.

a guid is supposed to be globally unique. So I think it still makes more sense than what we have today. Obviously it was poorly named in the original flutter_blue.

I think adding index to Guid is the best "1.0" compatible way to add this feature. Open to alternatives. small breaking are changes okay, but maximum 1 or 2.

[nebkat]

I like it. However, I would use the same existing names where possible. i.e. characteristicUuid instead of just uuid.

BluetoothCharacteristic.characteristicUuid seems a bit redundant which is why I was thinking a deprecated getter would make sense. Similarly BluetoothCharacteristic.service.uuid is easier to read than BluetoothCharacteristic.serviceUuid and only costs 1 extra character.

Sure it's mostly obvious what a serviceUuid is to a characteristic, but with a descriptor it almost implies that the descriptor is somehow tied to the service, which it is not directly - having the object path makes the relationships clearer. This is mostly an implementation detail because we needed that data to identify the descriptor in the plugin.

Or maybe we can go 2.0 if we provide 1.0 compatible constructors as well.

Which of these classes should actually be constructible by the user? If I'm not mistaken none of these characteristics/services will work unless they are discovered in the low level API, so a user should really get a reference to our instance of the BluetoothCharacteristic rather than creating their own.

It would be great if we could save a round of development and go straight to 2.0, I don't think any of these changes would involve much migration - if any.

However, Internally, I still like the idea of using the uuid:$index strings. For example, in the Bm* classes, and in the locateCharacteristic functions. There's no need to add a bunch more arguments everywhere. It just gets too ugly / complicated.

Internally I don't mind, we could even reference everything in the format: $PRIMARY_SERVICE_UUID:$PRIMARY_SERVICE_INDEX/$SERVICE_UUID:$SERVICE_INDEX/$CHARACTERISTIC_UUID:$CHARACTERISTIC_INDEX/$DESCRIPTOR_UUID. Would need a little bit more thought but you get the idea. We do exchange a lot of mapped data that is probably unnecessary if we could directly identify the service/char/descriptor. It is also duplicated all over the Java/Objective-C classes.

If you are open to it there are many ways we could significantly simplify the code that communicates between Dart/Java/Objective-C.

a guid is supposed to be globally unique. So I think it still makes more sense than what we have today. Obviously it was poorly named in the original flutter_blue.

This is a confusing concept within GATT but a UUID is meant to uniquely identify the type of the data, not the characteristic itself. If we attempt to correct this by changing what a Guid is (which is just another name for UUID) we will add to the further confusion - instead I think we should educate users on the distinction.

[chipweinberger]

All fair. Just want it to be 1.0 api compatible, with no more than 1 or 2 small breaking changes.

Any suggestions? I think modifying Guid is the easiest way to support this feature in 1.0 friendly way.

BluetoothCharacteristic.characteristicUuid seems a bit redundant which is why I was thinking a deprecated getter would make sense. Similarly BluetoothCharacteristic.service.uuid is easier to read than BluetoothCharacteristic.serviceUuid and only costs 1 extra character.

not necessarily. If it was just elm.uuid, it's no longer clear. its a minor detail. But I have no plans to deprecate characteristicUuid. Having both is better. In fact, the original code just used uuid, but I know from experience - it got annoying fast.

For example, at quick glance, what does .where((c) => c.uuid == uuid) refer to?

It probably took you about 0.2 seconds to find out. But when it's named characteristicUuid, its 0.0 seconds. Navigating a code base with dozens of instances just like this, becomes annoying fast.

By using characteristicUuid, there is a clear path from dart to native, all using the same variable name. Greppability is an underrated code metric

Having both is the best.

[nebkat]

All fair. Just want it to be 1.0 api compatible, with no more than 1 or 2 small breaking changes.

Any suggestions? I think modifying Guid is the easiest way to support this feature in 1.0 friendly way.

Let me make a PR this weekend, I think it will be possible with 0 or 1 breaking changes.

not necessarily. If it was just elm.uuid, it's no longer clear. its a minor detail. But I have no plans to deprecate characteristicUuid. Having both is better. In fact, the original code just used uuid, but I know from experience - it got annoying fast.

If elm.uuid is unclear it is elm which is the issue - I tend to name my characteristics heartbeatChar. But I take your point, can leave characteristicUuid as a getter.

It probably took you about 0.2 seconds to find out. But when it's named characteristicUuid, its 0.0 seconds. Navigating a code base with dozens of instances just like this, becomes annoying fast.

These instances will practically all be eliminated - there will be a single place in the code where we map from the $SERVICE_UUID:$SERVICE_INDEX/$CHARACTERISTIC_UUID:$CHARACTERISTIC_INDEX style string to an instance of BluetoothCharacteristic and nowhere else internally will care about UUIDs at all.

[nebkat]

On further investigation a lot of things seems strangely overcomplicated with all of the BluetoothXXXX classes acting more like tokens providing views into maps within the main class. Is there a reason why for example a BluetoothCharacteristic doesn't maintain it's own value, list of descriptors, etc rather than pulling from _knownServices?

[chipweinberger]

yes. because users can instantiate these objects themselves. This function has always been part of the API, and users have used it since day 1.

  BluetoothCharacteristic.fromProto(protos.BluetoothCharacteristic p)
      : uuid = new Guid(p.uuid),
        deviceId = new DeviceIdentifier(p.remoteId),
        serviceUuid = new Guid(p.serviceUuid),
        secondaryServiceUuid = (p.secondaryServiceUuid.length > 0)
            ? new Guid(p.secondaryServiceUuid)
            : null,
        descriptors = p.descriptors
            .map((d) => new BluetoothDescriptor.fromProto(d))
            .toList(),
        properties = new CharacteristicProperties.fromProto(p.properties),
        _value = BehaviorSubject.seeded(p.value);

so they need to be able to access the last value from a global store

But also, I think the design of global + tokens makes more sense.

I know theres a lot of breaking changes we could make, but I try not to break more than ~1 thing at a time. This makes it easier for most users to update.

[chipweinberger]

Another thing to keep in mind, verbose logs.

this $SERVICE_UUID:$SERVICE_INDEX/$CHARACTERISTIC_UUID:$CHARACTERISTIC_INDEX style string sounds like it would be more annoying for viewing the verbose logs, which a lot of users - and me - find important. So whatever solution you make, keep the verbose logs legible.

I think adding index to Guid is still the most obvious solution that meets all criteria, without blowing up the code base to add a feature almost no one cares about.

[nebkat]

This is ending up a slightly bigger change than anticipated but I'll get it finished and you can consider the option. So far looking like a pretty huge reduction in code bloat.

There is possibly a pretty neat solution to not breaking the token style construction with something like:

  factory BluetoothCharacteristic({
    DeviceIdentifier remoteId,
    Guid this.serviceUuid,
    Guid this.characteristicUuid,
    Guid? primaryServiceUuid,
  }) {
    return FlutterBluePlus.findCharacteristic(remoteId, serviceUuid, primaryServiceUuid, characteristicUuid);
  }

I think adding index to Guid is still the most obvious solution that meets all criteria, without blowing up the code base.

I don't doubt that this is the simplest solution but I think it's important to ensure that implementation details are not leaking into the public API and complicating things in the future. If we were to do this would a Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", null) be equivalent to Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", 1)?

[chipweinberger]

If we were to do this would a Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", null) be equivalent to Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", 1)?

No. They are not equivalent.

serviceUuid = Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", null)

this refers to the first service with uuid = a87ad7d7-b3a9-4a80-a34e-300e7398c92c

serviceUuid = Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", 1)

this refers to the second service with uuid = a87ad7d7-b3a9-4a80-a34e-300e7398c92c

If users want to compare uuid, they will need to compare guid.uuid instead.

for most users, its a completely transparent change, since this feature is rarely important. which is exactly what we want.

only when a users specifically wants to refer to the second service of a given uuid will they need to use Guid("a87ad7d7-b3a9-4a80-a34e-300e7398c92c", 1)

---

instead of representing a UUID, GUID now represents a specific svc/chr/desc.

[chipweinberger]

This is ending up a slightly bigger change than anticipated

it's unlikely to be merged if its a drastic change. but ill review it.

this feature is not important enough to make big changes IMO.

[chipweinberger]

return FlutterBluePlus.findCharacteristic(remoteId, serviceUuid, primaryServiceUuid, characteristicUuid);

this is not entirely the same meaning. You can instantiate characteristic that don't exist yet / before discoverServices.

so it would better be called

FlutterBluePlus.createCharacteristic(remoteId, serviceUuid, primaryServiceUuid, characteristicUuid);

which in that case you might as well just make it part of the constructor.

---

I'd only want to get rid of the global map if we remove the public constructor.

but again, this feature does not warrant such large changes IMO.

it's better to do things slowly, over time.

[nebkat]

If users want to compare uuid, they will need to compare guid.uuid instead. instead of representing a UUID, GUID now represents a specific svc/chr/desc.

Note that there is no difference in meaning of the term GUID and UUID (globally vs universally) so I am failing to see how this would not confuse users... But to save us going in circles I suggest we get some outside opinions on it if possible.

this is not entirely the same meaning. You can instantiate characteristic that don't exist yet / before discoverServices.

I may try to represent these as AnonymousBluetoothCharacteristic or BluetoothCharacteristicToken. IMO this usage should be discouraged over performing service/characteristic discovery as is done on Android/iOS natively.

it's unlikely to be merged if its a drastic change. but ill review it.

Of course. I understand this will be very difficult to merge, at this stage it is more of an exploration of what could be possible in future.

Currently I have over 600 lines removed on the Dart side alone while maintaining almost identical external API and functionality, plus fully supporting instances of BluetoothService/BluetoothCharacteristic/BluetoothDescriptor that uniquely identify their respective back-end classes.

If you are open to it I think it could be a good development for the library, as I see many parts have not really changed since the original library but code size has ballooned quite a bit.

this feature is not important enough to make big changes IMO.

Unfortunately our application requires complex usage of BLE so for me it is essential to have these features. It consists of a hub device that is connected by wire to a number of other devices, each of which is represented by a primary service + their respective secondary services i.e. duplicate UUIDs everywhere.

This is for sure an exotic setup but I have personally found that poor library support contributes a lot to such use cases being rare, rather than there not being demand.

---

In any case thank you for all your responses so far!

[chipweinberger]

If you are open to it I think it could be a good development for the library, as I see many parts have not really changed since the original library but code size has ballooned quite a bit.

I know you don't intend it as an insult, but what you don't see are over ~50 bugs in the original library fixed, some that were design problems. Among a handful of features added, comprehensive error handling, etc.

[nebkat]

I know you don't intend it as an insult, but what you don't see are over ~50 bugs in the original library fixed, some that were design problems. Among a handful of features added, comprehensive error handling, etc.

No not at all, we are very grateful for the improvements from the original!

I am mainly just referring to areas of duplicated code (in particular the invoke method blocks and the _methodStream mapping). You can have a look at the WIP so far, I have extracted a lot of this code into functions and mixins:

https://github.com/nebkat/flutter_blue_plus/commit/1ecf36dbdecc4fa00ef05a1ec8173177332a874b

I discovered yesterday that once the index parameter is introduced it is no longer necessary to pass around the parent service UUID, since each service can be uniquely identified by its own uuid/index alone.

Also learned that a primary service can include other primary services, so the presence of a parent primary service does not necessarily make it a secondary service.

Lastly, I have yet to test but I think one possible advantage of using hash on iOS may be that it preserves the same value as long as the handle remains the same, whereas index may change if a service/characteristic is injected/removed at an earlier index. This may be important if a services reset occurs to avoid the need to invalidate all existing instances of BluetoothCharacteristic.

[chipweinberger]

Also learned that a primary service can include other primary services, so the presence of a parent primary service does not necessarily make it a secondary service.

I dont think this is correct. If you have a parent, you're not primary.

[nebkat]

I dont think this is correct. If you have a parent, you're not primary.

From Bluetooth Core Spec v5.4 | Vol 3, Part G | Page 1472:

So primary → primary is possible, as well as primary → secondary → secondary.

The include is simply a link to another service definition in the table which exists independently. The only difference between primary and secondary, aside from semantics, is that only primary services show up in the Discover All Primary Services sub-procedure, whereas secondary have to be manually requested, however the Bluetooth stacks on Android/iOS already handle this for us.

The good news is on Android it appears that all the secondary services are included in BluetoothGatt.getServices() anyway - so they can always be uniquely identified by their UUID and their handle without the need for hierarchy information. I presume iOS is the same, will find out hopefully later today.

[chipweinberger]

ah okay,

the first thing we should do is to fix the existing code then

maybe let's call it 'parentServiceUuid'

that said, it would probably be a list, because it could be included by multiple parents i assume

or i guess its intended to treat each parent/child pair as a separate service.

and we should use the isPrimary/isSecondary flags from the host OS, and copy them into the BmService struct like they used to be

---

and add docs to the codebase with explanations of all this stuff

[chipweinberger]

my product launched this week, so i won't have time for awhile

[nebkat]

How about we use the pointer address of the CBCharacteristic/CBService instances, (optionally together with the hash), to uniquely identify each one? This will be easier than finding the index. Then we flatten the services list with an NSSet - so we simply find the service with the matching pointer. When didModifyServices gets called we remove from the set.

[chipweinberger]

pointer seems like a good idea

except i'm not sure how a user would instantiate it themself then before discoverServices

maybe we should just break that use case

i imagine it's used to do things like create chr listeners before you connect, for simpler code

or we can just assume null pointer is the first instance and not support that feature for included services

---

on second thought

index seems better

it's more helpful for debugging too

[nebkat]

I think if we allow the index parameter to be null, and only filter by it if it is not null, we will solve 99% of that use case.

So if you do devices.servicesList.firstWhere((s) => s.uuid == ...).characteristics.firstWhere((c) => c.uuid == ...) you are given a concrete characteristic that is fully indexed, whereas if you initialize it with BluetoothCharacteristic(characteristicUuid: ..., serviceUuid: ...) it's anonymous and will match the first service and characteristic.

If someone needs to do multi-UUID (service or characteristic) setups they should use discovery mode. This is the same logic as BluetoothGatt.getService(UUID).

Indices will break if services are inserted/removed at an earlier position in the table, whereas pointers should remain valid.

[chipweinberger]

Okay, sure, lets do ptr.

but keep it null unless there are multiple of the same service, chr, etc.

if there are multiple, lets give them all an pointer.

and I still think we should modify Guid.

[chipweinberger]

looking reasonable. Some nice improvements, and some I think are not needed. Early feedback:

1. no need for BluetoothValueAttribute. Too much DRY. I want the code to be able to diverge easily. Since chr & desc are really not the same thing. In fact, I've considered changing desc.value to some other name many times. Plus having code that looks for either of the underlying OnXXXExent types is not so clean.

2. your changes for this code may be fewer lines, but they are harder to read & understand then before. Keeping the separate hasNotify, hasIndicate lines is easier to read & understand for the next person.

  bool get isNotifying {
    List<int> lastValue = cccd?.lastValue ?? [];
    return lastValue.isNotEmpty && (lastValue[0] & 0x03) > 0;
  }

[chipweinberger]

Okay, I kept trying to give more feedback, but the PR is too big for me to wrap my head around right now.

There's a a lot of changes here.

A lot of it I do like, and would merge.

Now that you have a very good understanding of the FBP code, can you submit PR requests incrementally? A lot of these changes can be their own PR.

^ I'm sure you already realized this. And I know you've just been exploring whats possible.

[chipweinberger]

AdvertisementData should not have a platform name. BLE advertisments do not have a platform name.

  AdvertisementData({
    required this.advName,
    required this.platformName,

[chipweinberger]

I find this really hard to read.

// 00000000-0000-0000-0000-000000000000:12345678/00000000-0000-0000-0000-000000000000:12345678/00000000-0000-0000-0000-000000000000:12345678

It's easier to read like this:

{
 service_uuid: xxxx,
 characteristic_uuid: xxxx,
 descriptor_uuid: xxxx,
}

You might think "internal" representations don't matter. But it's not internal. We log these everywhere. Heck, I even expose these in my app when users look at the logs. So it's user facing too. Keep it as legible as before.

In other words, please turn on verbose logs, and make it look decent to read. Mind you, it needs to be performant too. I once tried to pretty format everything, and performance tanked.

[nebkat]

This was an accidental push to the PR as I had a reference to the fork@master, but yeah it's a current snapshot of WIP.

Okay, I kept trying to give more feedback, but the PR is too big for me to wrap my head around right now.

Yeah sorry it's not very pretty right now but I've had to get it to a working state as I have deadlines with our project.

Now that you have a very good understanding of the FBP code, can you submit PR requests incrementally? A lot of these changes can be their own PR.

Can do my best to break up changes into their own commits. Maybe if you could comment in the code the areas you are 100% happy with can start extracting and rebasing piece by piece until its a smaller diff?

---

no need for BluetoothValueAttribute. Too much DRY. I want the code to be able to diverge easily.

Personally suggest maximum DRY until the divergence actually occurs. It's pretty easy to un-abstract if the need arises, but without great care the duplicated code can quickly balloon to a difficult to manage state.

Since chr & desc are really not the same thing. In fact, I've considered changing desc.value to some other name many times.

As far as the BLE spec is concerned they are almost identical (see 4.8 CHARACTERISTIC VALUE READ and 4.12 CHARACTERISTIC DESCRIPTORS), and the descriptor value is referred to as such. What were your thoughts behind changing it?

Plus having code that looks for either of the underlying OnXXXExent types is not so clean.

Note that prior to these changes the same functions were re-parsing the events everywhere:

Stream<List<int>> get lastValueStream => FlutterBluePlus._methodStream.stream
      .where((m) => m.method == "OnCharacteristicReceived" || m.method == "OnCharacteristicWritten")
      .map((m) => m.arguments)
      .map((args) => BmCharacteristicData.fromMap(args))

So I figured delegating discrimination to the .where((e) => e.attribute == this) was a pretty elegant solution as it's 0 cost.

---

your changes for this code may be fewer lines, but they are harder to read & understand then before. Keeping the separate hasNotify, hasIndicate lines is easier to read & understand for the next person.

bool get isNotifying {
   List<int> lastValue = cccd?.lastValue ?? [];
   return lastValue.isNotEmpty && (lastValue[0] & 0x03) > 0;
 }```

Agree it's rather vague as is, how about lastValue[0] & (CCCD_BIT_NOTIFY | CCCD_BIT_INDICATE)?

I do believe as a general principle that brevity like this is beneficial and makes classes more readable:

bool get isNotifying => (cccd?.lastValue?._firstOrNull ?? 0) & (CCCD_BIT_NOTIFY | CCCD_BIT_INDICATE) > 0

---

AdvertisementData should not have a platform name. BLE advertisments do not have a platform name.

Ok so this should live in ScanResult rather than AdvertisementData then, must have accidentally put it there as I was reducing reliance on the Bm*** classes.

---

I find this really hard to read. // 00000000-0000-0000-0000-000000000000:12345678/00000000-0000-0000-0000-000000000000:12345678/00000000-0000-0000-0000-000000000000:12345678

Yeah this is not strictly the final form, was just convenient for now, however...

Mind you, it needs to be performant too. I once tried to pretty format everything, and performance tanked.

The fact that we can't pretty print everything means that you will get { service_id: 00000000-0000-0000-0000-000000000000:12345678, characteristic_id: 00000000-0000-0000-0000-000000000000:12345678, descriptor_id: 00000000-0000-0000-0000-000000000000:12345678 } which isn't exactly great when your screen gets spammed with UUID characters.

Would you be open to this? 20% less characters but still comparatively quite readable:

[ S@00000000-0000-0000-0000-000000000000:12345678, C@00000000-0000-0000-0000-000000000000:12345678, D@00000000-0000-0000-0000-000000000000:12345678 ]

Either way I'm happy with some separation of them to cut out the string parsing on native side.

[chipweinberger]

too much to discuss at one time.

let's just do it one PR at a time.

[chipweinberger]

class OnDescriptorWrittenEvent with GetDeviceMixin, GetAttributeValueMixin, GetDescriptorMixin, GetExceptionMixin {

this is not easier to understand than just having the very very little code it replaces in the class.

too much DRY.

Having code duplicate is not bad. It often leads to easier to understand code, easier to modify code, and flatter class hierarchies.

When designing code "look a common pattern! I can replace all these lines into a single new concept!" it not a good guiding principal. Instead "this code is too complicated, and I can make it easier to understand", is all we should care about.

---

Edit: sorry if I sound preachy here ^. Just my point of voice. DRY often leads to more complicated code, for little benefit.

[nebkat]

I think it would be splitting hairs here to suggest that a mixin is somehow significantly less readable than the line it replaced - when that line is copied 2-10x in the file. Could equally argue the mixins allow one to focus on what is unique to each event and really this is the perfect use-case for them.

It is however an issue of maintainability when a simple change in the parameters of a class requires 10 identical changes elsewhere. This introduces a very real risk of missing one of the required changes and makes it more difficult to understand and review changes.

To take a concrete example: https://github.com/chipweinberger/flutter_blue_plus/commit/2064c18e40d1a01e2bf1e382c746acc588cc029f, the line .where((p) => p.primaryServiceUuid is added 7 times in this relatively simple change. Would an external contributor remember to put in all 7? From viewing the diff alone it's impossible to assess that.

---

There is of course no single right answer on the degree to which the code should be abstracted, but I can tell you from my perspective as an external contributor that in its current form the library is quite difficult to work with primarily due to the amount of duplicated code. Every time I see a duplicated block I must ask myself why isn't this abstracted if it's doing the exact same thing, is there something unique about it?

In addition to that, while it is merely a metric - when I see files with hundreds or even thousands of lines of code it is very daunting. The 700 lines this branch removed while maintaining the same functionality is 700 less lines I have to understand as a contributor. Look especially at the before and after of service/char/desc classes, I can now get the gist of what they are responsible for without even having to scroll.

[chipweinberger]

I had to google to remember the rules of mixins. Thats certainly not more readable than a few extra lines. Eventually you end up with the hell of C++, where you need to understand huge textbooks to read 100 lines of code.

mixins: "At first, I found this concept somewhat difficult to understand" https://medium.com/flutter-community/dart-what-are-mixins-3a72344011f3

Simply put, agree to disagree.

The code is not perfect, ofc. Some refactors are overdue. We dont need to introduce the concept of mixins to this codebase.

readability > "maintainability"

[chipweinberger]

RE: contributors & "700" less lines of code.

Lines of code is not a good metric.

I gave up on flutter_reactive_ble because it was too abstracted, and used too many swift language features that I didn't want to have to learn, in order to reduce lines of code.

look at this file: https://github.com/PhilipsHue/flutter_reactive_ble/blob/master/packages/reactive_ble_mobile/ios/Classes/ReactiveBle/Central.swift

^ look at connect (line 145). Does it makes any sense to you?

or this file: https://github.com/PhilipsHue/flutter_reactive_ble/blob/master/packages/reactive_ble_mobile/ios/Classes/ReactiveBle/Tasks/CharacteristicWrite/CharacteristicWriteTaskSpec.swift

this file: https://github.com/PhilipsHue/flutter_reactive_ble/blob/master/packages/reactive_ble_mobile/ios/Classes/ReactiveBle/Tasks/CharacteristicWrite/CharacteristicWriteTaskController.swift

And look where they ended up. They went from the most popular BLE library, to the 2nd most popular. And soon to be the 3rd.

You should look at their code and see if it would have been easier for you to contribute to.

It does the same exact stuff as this library.

Keeping code "flat" is important.

---

that said this code base is not perfect either. the obj-c & java files are too big. They're not complicated. But they're too big now. Just some simple util files needed.