Add support for writing to descriptors #453

ened · 2021-11-17T07:59:40Z

Extends the plugins to support writing to specific descriptors.
I have copied the approach from QualifiedCharacteristic to become a QualifiedDescriptor.

There are quite a few generated files (tests, protobuf) which are a bit tedious to handle. Appreciate if someone could point me to a developer docs page.

iOS implementation is still TBD.

remonh87 · 2021-11-19T18:35:02Z

thanks! so far it looks good

werediver

Looks pretty good. Good job, @ened 👍

There is at least one "TODO" mark out there, so I consider this a work-in-progress for now.

werediver · 2021-11-22T15:41:31Z

...e_mobile/android/src/main/kotlin/com/signify/hue/flutterreactiveble/ble/ReactiveBleClient.kt

+                        )
+                    }
+                }
+            }.first(CharOperationFailed(deviceId, "Writechar timed-out"))


@remonh87 CharOperation* types are used multiple types in this method (and possibly in other places), while it's not exactly about characteristics, but rather about descriptors (which, admittedly, are associated with characteristics). Do you think it's fine or do you think a new set of types should be introduced?

I don't have a strong opinion on this, mainly because I'm not that familiar with the Android part. Just drawing your attention.

werediver · 2021-11-22T15:44:51Z

packages/reactive_ble_mobile/ios/Classes/ReactiveBle/Central.swift

+            throw Failure.descriptorNotFound(qualifiedDescriptor)
+        }
+
+        guard let response = characteristic.service?.peripheral?.writeValue(value, for: descriptor)


@ened Is writing to a descriptor inherently a "without response" (without acknowledgement) operation? If so, then using -WithoutResponse is reasonable, but please clarify.

Writing to descriptors works via https://developer.apple.com/documentation/corebluetooth/cbperipheral/1519107-writevalue . There is no response per se, but the docs at https://developer.apple.com/documentation/corebluetooth/cbperipheraldelegate/1519062-peripheral state that a delegate method will be called once the write is completed. If it fails, the error is given as well. Do you think the plugin should implement that delegate and return a value only once that is done?

WithoutResponse still applies based on that logic, but I am wondering what the plugin should do.

Do you think the plugin should implement that delegate and return a value only once that is done?

If waiting for the events delivered to the delegate adds anything, it might be a potential improvement, but as of now, I'd say, this method should be implemented similar to

flutter_reactive_ble/packages/reactive_ble_mobile/ios/Classes/ReactiveBle/Central.swift

Line 264 in d3d2781

func writeWithoutResponse(

As long as they match, it should be enough.

werediver · 2021-11-22T15:48:40Z

packages/reactive_ble_mobile/protos/bledata.proto

@@ -70,6 +70,16 @@ message WriteCharacteristicInfo {
    GenericFailure failure = 3;
 }

+message WriteDescriptorRequest {
+    DescriptorAddress descriptor = 1;
+    bytes value = 2;


@ened Do descriptors always have 16-bit IDs and cannot have longer or shorter IDs?

werediver · 2021-11-22T15:49:55Z

packages/reactive_ble_mobile/pubspec.lock

-    source: hosted
+      path: "../reactive_ble_platform_interface"
+      relative: true
+    source: path


Not sure this should be committed.

CC @remonh87

ened · 2021-11-23T10:10:46Z

@werediver @remonh87 944e2a9 is not strictly part of this MR, but was needed for compatibility when including this package on iOS. Should it be separate?

werediver

Some comments regarding 944e2a9.

packages/reactive_ble_mobile/ios/Classes/ReactiveBlePlugin.m

packages/reactive_ble_mobile/ios/reactive_ble_mobile.podspec

example/ios/Runner.xcodeproj/project.pbxproj

werediver · 2021-11-23T10:37:05Z

Some comments regarding 944e2a9.

I suppose it's fine to include those changes in this pull request, but please go through all the comments already left by me.

Aside from those comments, the commit history of this pull request is dirty: contains merge commits (better rebase your feature branches), doesn't seem well structured and includes commit messages mentioning work-in-progress (WIP). If you are not going to clean the history up at some point (through interactive rebase), the pull request should be squashed before merging (can be done at the moment of merging through GitHub interface). It's okay, just don't be surprised.

ened · 2021-11-23T11:00:26Z

Some comments regarding 944e2a9.

I suppose it's fine to include those changes in this pull request, but please go through all the comments already left by me.

OK & yes will do.

Aside from those comments, the commit history of this pull request is dirty: contains merge commits (better rebase your feature branches), doesn't seem well structured and includes commit messages mentioning work-in-progress (WIP). If you are not going to clean the history up at some point (through interactive rebase), the pull request should be squashed before merging (can be done at the moment of merging through GitHub interface). It's okay, just don't be surprised.

Oh yes, I was fully expecting to squash all the commits before removing the Draft label.

Thank you for the reviews.

remonh87 · 2021-11-23T18:37:44Z

I can test the project setup on the m1 machine (I have one) if needed.

ened · 2021-11-29T07:57:13Z

packages/reactive_ble_mobile/ios/Classes/Plugin/PluginController.swift

+            completion(.success(result))
+        } else {
+            do {
+                try central.discoverDescriptors(


@werediver @remonh87 would like your opinion if this is the correct approach. I think there is no need to discover all characteristics from the get go. It may also imply a API change to declare what to discover.

The code currently discovers on-demand but may change the library behavior from what the use expects?

Characteristic discovery works differently on iOS and Android. On iOS a client can specify a list of services and characteristics to discover when connecting to a device (the client is only allowed to interact with the discovered characteristics later on), while on Android no discovery is triggered beforehand (I think it's happening on the fly then, but not entirely sure; CC @remonh87).

In terms of design, a simple solution is to extend the existing discovery mechanism to also discover descriptors, as specified when connecting. That would be consistent and quite efficient.

Discovering descriptors one by one is definitely not as efficient (the more descriptors a client uses, the larger the overhead is). This is also not consistent with how the library works with characteristics (on iOS).

@werediver got you regarding the differences - and will think how to extend the initial discovery. It does mean the existing iOS discovery code will get another layer (currently counts how many services are left to discover, this will be extended with the number of characteristics to explore).

werediver · 2021-11-29T08:55:17Z

Reminding of my earlier question:

@ened Do descriptors always have 16-bit IDs and cannot have longer or shorter IDs?

CBDescriptor.id has type CBUUID and that thing can contain IDs of different length, typically 128, 32, or 16 bits, while strictly 16 bits is reserved in Protobuf structure.

I'd suggest to find a suitable example in existing Protobuf structures and reuse it.

Rework or clarify Protobuf descriptor ID format

ened · 2021-11-29T11:54:07Z

Reminding of my earlier question:

@ened Do descriptors always have 16-bit IDs and cannot have longer or shorter IDs?

CBDescriptor.id has type CBUUID and that thing can contain IDs of different length, typically 128, 32, or 16 bits, while strictly 16 bits is reserved in Protobuf structure.

I'd suggest to find a suitable example in existing Protobuf structures and reuse it.

Rework or clarify Protobuf descriptor ID format

At the moment, the code defines DescriptorAddress like this:

message DescriptorAddress {
    string deviceId = 1;
    Uuid serviceUuid = 2;
    Uuid characteristicUuid = 3;
    Uuid descriptorUuid = 4;
}

and Uuid is defined as:

message Uuid {
    bytes data = 1;
}

So that looks good?

werediver · 2021-11-29T13:38:07Z

At the moment, the code defines DescriptorAddress like this:
message DescriptorAddress {
    string deviceId = 1;
    Uuid serviceUuid = 2;
    Uuid characteristicUuid = 3;
    Uuid descriptorUuid = 4;
}
and Uuid is defined as:
message Uuid {
    bytes data = 1;
}
So that looks good?

Oh, sure, it looks good. I misread a piece of protobuf specification, sorry.

corecode · 2021-12-24T05:56:46Z

packages/reactive_ble_mobile/ios/Classes/Plugin/PluginController.swift

+        }
+
+        guard let peripheral = central.activePeripherals[descriptor.peripheralID],
+              let service = peripheral.services?.first(where: { service in


how will .first work with devices that provide several instances of the same service (UUID)? Same for several instances of the same characteristic UUID.

Devices can provide multiple services with the same UUID? I have to admit that's a part of the spec I do not know about!

Technically this May be a more fundamental problem in this package then, looking as QualifiedCharacteristic for example, a full lookup is done using the UUIDs but not via index?

In case anyone wants to continue with this in the future. Have a look at #776 where the problem of multiple characteristics of services with the same id has been addressed. Something similar would be needed here.

albydarned · 2022-04-18T22:04:59Z

I need to be able to read a characteristic's user descriptor... Does this PR just include writes?

ened added 4 commits November 17, 2021 07:15

Initial descriptors implementation on Android

5d5ba51

Some tests

c3b044a

Few android unit tests

72113cd

iOS WIP

0c74e4f

remonh87 requested review from werediver and remonh87 November 19, 2021 18:34

ened added 2 commits November 21, 2021 08:37

WIP descriptors

038ccf8

Merge branch 'master' into descriptors

fff4534

werediver reviewed Nov 22, 2021

View reviewed changes

ened added 2 commits November 23, 2021 08:17

WIP

427534a

Improve iOS module compat

944e2a9

werediver reviewed Nov 23, 2021

View reviewed changes

packages/reactive_ble_mobile/ios/Classes/ReactiveBlePlugin.m Show resolved Hide resolved

packages/reactive_ble_mobile/ios/reactive_ble_mobile.podspec Show resolved Hide resolved

example/ios/Runner.xcodeproj/project.pbxproj Show resolved Hide resolved

ened added 3 commits November 29, 2021 07:45

discover descriptors on demand

621a09b

pbx updates

0560971

swift_version 5.0

c04a926

ened commented Nov 29, 2021

View reviewed changes

Merge branch 'master' into descriptors

41440f6

ened mentioned this pull request Dec 7, 2021

Support for Descriptor Read/Writes #423

Open

corecode reviewed Dec 24, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for writing to descriptors #453

Add support for writing to descriptors #453

ened commented Nov 17, 2021

remonh87 commented Nov 19, 2021

werediver left a comment

werediver Nov 22, 2021

werediver Nov 22, 2021

ened Nov 29, 2021

werediver Nov 29, 2021 •

edited

Loading

werediver Nov 22, 2021

werediver Nov 22, 2021

ened commented Nov 23, 2021

werediver left a comment

werediver commented Nov 23, 2021

ened commented Nov 23, 2021

remonh87 commented Nov 23, 2021

ened Nov 29, 2021

werediver Nov 29, 2021 •

edited

Loading

ened Nov 29, 2021 •

edited

Loading

werediver commented Nov 29, 2021 •

edited

Loading

ened commented Nov 29, 2021 •

edited by werediver

Loading

werediver commented Nov 29, 2021 •

edited

Loading

corecode Dec 24, 2021

ened Dec 24, 2021

spkersten Sep 15, 2023

albydarned commented Apr 18, 2022

Add support for writing to descriptors #453

Are you sure you want to change the base?

Add support for writing to descriptors #453

Conversation

ened commented Nov 17, 2021

remonh87 commented Nov 19, 2021

werediver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

werediver Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ened commented Nov 23, 2021

werediver left a comment

Choose a reason for hiding this comment

werediver commented Nov 23, 2021

ened commented Nov 23, 2021

remonh87 commented Nov 23, 2021

Choose a reason for hiding this comment

werediver Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

ened Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

werediver commented Nov 29, 2021 • edited Loading

ened commented Nov 29, 2021 • edited by werediver Loading

werediver commented Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albydarned commented Apr 18, 2022

werediver Nov 29, 2021 •

edited

Loading

werediver Nov 29, 2021 •

edited

Loading

ened Nov 29, 2021 •

edited

Loading

werediver commented Nov 29, 2021 •

edited

Loading

ened commented Nov 29, 2021 •

edited by werediver

Loading

werediver commented Nov 29, 2021 •

edited

Loading