Fix reading of descriptor value as `NSData` #706

FabioCornelli · 2024-07-02T09:49:31Z

Hi,

this PR originated from #704 , but it is solving a separate issue, hence this PR.

The problem is that, on apple platform, descriptor value is force casted to NSData, but seems that descriptor.value type depends on descriptor.UUID.UUIDString.

Apple documentation & online references about this:

From a quick Look at the code, this issue seems to affect only apple platforms.

On Apple descriptor.value is returned as generic object Any?
by the platform as documented here

On Android descriptor.value is returned as byte[] by the platform as documented here

On JS descriptor.value is returned as ArrayBuffer by the platform as documented here

So we need to properly cast to ByteArray only on apple platforms.

Let me know if you think some changes are required

davidtaylor-juul · 2024-07-02T19:10:50Z

core/src/appleMain/kotlin/CBPeripheralCoreBluetoothPeripheral.kt

+            CBUUIDCharacteristicAggregateFormatString,
+            CBUUIDCharacteristicUserDescriptionString,


For CBUUIDCharacteristicAggregateFormatString the apple docs do not specify what the contents of the descriptor value are. Do we know for sure this one has String content?

No actually I'm not sure, I kind of trusted the StackOverflow answer for that value.

davidtaylor-juul · 2024-07-02T19:16:35Z

core/src/appleMain/kotlin/CBPeripheralCoreBluetoothPeripheral.kt

+            CBUUIDCharacteristicExtendedPropertiesString,
+            CBUUIDClientCharacteristicConfigurationString,
+            CBUUIDServerCharacteristicConfigurationString,
+            CBUUIDL2CAPPSMCharacteristicString,


The Apple docs specify that the value for all of these are an NSNumber object, except for CBUUIDL2CAPPSMCharacteristicString which it states is little-endian UInt16. It is unclear whether the API resents that as an NSNumber or not. This part of the spec is really difficult to grok, and the Apple API is awkward, so I'm not certain if that specific one is correct or not. Do you have a real-world example to test this one on, as that would be ideal, otherwise I suppose we go with our best-guess/stack-overflow comments?

Yes Apple API is awkward, indeed.
I've also searched online a bit and the only useful doc I've found is this one, but still is not clear to me which is the correct type we should expect for each different value.

Do you have a real-world example to test this one on, as that would be ideal, otherwise I suppose we go with our best-guess/stack-overflow comments?

i've tested with an IOT device that mount an ESP32 BLE Board on it.
I'm using Kable to rewrite in Kotlin Multiplatform this Espressif SDK which reads descriptors and use it for different kind of messages.

I've also tested with a Xiaomi Mi Smart Band 5 (nothing work related, its just that i have it with me), but since is not something I need for work, I've just verified that the code didn't crash.

This week I'm kind of busy at work, but maybe in the weekend I can run some tests on these two devices and provide you some extra information about this.

Thanks for the detail. I agree with your deductions in the other comment as well. The only reference I found in the docs is for the type alias https://developer.apple.com/documentation/corebluetooth/cbl2cappsm/ which also hints that this the value for CBUUIDL2CAPPSMCharacteristicString is exposed as a raw 16bit integer, and is not a NSNumber or NSData object. It is for L2CAP so to test a real world example would require reading characteristics from a device that supports L2CAP channels. I'll update this thread if I find a concrete example.

FabioCornelli · 2024-07-07T11:09:21Z

I took some time to dig in the Gatt Part G document and match the information there with Apple documentation:

Apple UUID constants	UUID value	Apple Documented value Type	GATT Part G document	Tested
CBUUIDCharacteristicExtendedPropertiesString	2900	NSNumber	3.3.3.1 - Attribute Value shall be two octets in length and shall contain the Characteristic Extended Properties Bit Field	✅
CBUUIDCharacteristicUserDescriptionString	2901	NSString	3.3.3.2 - Attribute Value shall be set to the characteristic user description UTF-8 string.	✅
CBUUIDClientCharacteristicConfigurationString	2902	NSNumber	3.3.3.3 - The Attribute Value shall be two octets in length and shall be set to the characteristic descriptor value	✅
CBUUIDServerCharacteristicConfigurationString	2903	NSNumber	3.3.3.4 - The Attribute Value shall be two octets in length and shall be set to the characteristic descriptor value.	❌
CBUUIDCharacteristicFormatString (Presentation Format )	2904	NSData	3.3.3.5 - The characteristic presentation format value is composed of five parts: format (1 octet), exponent (1 octet), unit (2 octets), name space (1 octet), and description (2 octet).	❌
CBUUIDCharacteristicAggregateFormatString	2905	Not documented 👎🏻	3.3.3.6 - The Characteristic Aggregate Format value is composed of a list of Attribute Handles of Characteristic Presentation Format declarations, where each Attribute Handle points to a Characteristic Presentation Format declaration.	❌
CBUUIDL2CAPPSMCharacteristicString	???	UInt16	The document mention L2CAP, but i didn’t find any useful information.	❌

Given these information it seems that:

CBUUIDCharacteristicExtendedPropertiesString, CBUUIDClientCharacteristicConfigurationString, CBUUIDServerCharacteristicConfigurationString and CBUUIDL2CAPPSMCharacteristicString are all two bytes in length.
CBUUIDL2CAPPSMCharacteristicString is returned by Apple as UInt16 instead of NSNumber, so it should be handled separately
CBUUIDCharacteristicAggregateFormatString while is not documented by Apple, I think we can assume it will be returned as NSData since it is an aggregation of CBUUIDCharacteristicFormatString, but of course we can't be sure until we test it.

As shown in the table above, I was able to test some of the different descriptor types and the value was read correctly.

I've updated the PR to reflect these changes, let me know your feedback!

…eceived uuidString

twyatt · 2024-07-09T07:29:24Z

@FabioCornelli Thanks for being so thorough and deep-diving with the testing! I'll review this PR soon.

davidtaylor-juul · 2024-07-09T18:27:29Z

core/src/appleMain/kotlin/CBPeripheralCoreBluetoothPeripheral.kt

+            }
+            CBUUIDL2CAPPSMCharacteristicString,
+            -> {
+                return (updatedDescriptor.value as UInt16).toNSData()


The docs are saying that the value here is UInt16, but I doubt the API expects us to dereference/typecast directly as a raw pointer. It's not completely out of the question but feels a little odd for Apple to do that? My best guess is the is already an NSData (of two bytes length) so we should just return as NSData here.

I'm digging around for a device that exposes this characteristic so I can test a real example to know for sure.

Not sure if it confirms your suspicions @davidtaylor-juul, but this code retrieves the NSData as-is (without UInt16 casting, as far as I can tell, at least).

twyatt

I'm not an expert on iOS BLE, but I may have stumbled on a simpler approach to processing the descriptor value?

I did not test it yet, but wanted to share my findings to get your thoughts.

twyatt · 2024-07-14T08:02:22Z

core/src/appleMain/kotlin/CBPeripheralCoreBluetoothPeripheral.kt

+        // CBDescriptor "value" property type depends on cbDescriptor.UUID.UUIDString
+        // https://developer.apple.com/documentation/corebluetooth/characteristic-descriptors
+        // see type conversion table https://github.com/JuulLabs/kable/pull/706
+        when (updatedDescriptor.UUID.UUIDString) {
+            CBUUIDCharacteristicFormatString -> {
+                return updatedDescriptor.value as NSData
+            }
+            CBUUIDCharacteristicUserDescriptionString,
+            -> {
+                return (updatedDescriptor.value as String)
+                    .encodeToByteArray()
+                    .toNSData()
+            }
+            CBUUIDCharacteristicExtendedPropertiesString,
+            CBUUIDClientCharacteristicConfigurationString,
+            CBUUIDServerCharacteristicConfigurationString,
+            -> {
+                return (updatedDescriptor.value as NSNumber)
+                    .unsignedShortValue
+                    .toNSData()
+            }
+            CBUUIDL2CAPPSMCharacteristicString,
+            -> {
+                return (updatedDescriptor.value as UInt16).toNSData()
+            }
+            CBUUIDCharacteristicAggregateFormatString,
+            -> {
+                logger.warn {
+                    message = "Best effort descriptor value conversion for undocumented $CBUUIDCharacteristicAggregateFormatString uuid"
+                }
+                return (updatedDescriptor.value as? NSData) ?: byteArrayOf().toNSData()
+            }
+            else -> {
+                logger.warn { message = "cannot read descriptor for unknown uuid string ${updatedDescriptor.UUID.UUIDString}" }
+                return byteArrayOf().toNSData()
+            }
+        }


As I was looking how others read descriptors, I stumbled on this:

// TODO: Convert NSNumber types too if( [descriptor.value isKindOfClass:[NSData class]] ) { return descriptor.value; } if( [descriptor.value isKindOfClass:[NSString class]] ) { return [descriptor.value dataUsingEncoding:NSUTF8StringEncoding]; } if( [descriptor.value isKindOfClass:[NSNumber class]] ) { int value = [descriptor.value intValue]; return [NSData dataWithBytes:&value length:sizeof(value)]; }

Which hints that the data we get from Core Bluetooth carries the type info, meaning we could possibly do something similar to the code snippet above?

Suggested change

// CBDescriptor "value" property type depends on cbDescriptor.UUID.UUIDString

// https://developer.apple.com/documentation/corebluetooth/characteristic-descriptors

// see type conversion table https://github.com/JuulLabs/kable/pull/706

when (updatedDescriptor.UUID.UUIDString) {

CBUUIDCharacteristicFormatString -> {

return updatedDescriptor.value as NSData

}

CBUUIDCharacteristicUserDescriptionString,

-> {

return (updatedDescriptor.value as String)

.encodeToByteArray()

.toNSData()

}

CBUUIDCharacteristicExtendedPropertiesString,

CBUUIDClientCharacteristicConfigurationString,

CBUUIDServerCharacteristicConfigurationString,

-> {

return (updatedDescriptor.value as NSNumber)

.unsignedShortValue

.toNSData()

}

CBUUIDL2CAPPSMCharacteristicString,

-> {

return (updatedDescriptor.value as UInt16).toNSData()

}

CBUUIDCharacteristicAggregateFormatString,

-> {

logger.warn {

message = "Best effort descriptor value conversion for undocumented $CBUUIDCharacteristicAggregateFormatString uuid"

}

return (updatedDescriptor.value as? NSData) ?: byteArrayOf().toNSData()

}

else -> {

logger.warn { message = "cannot read descriptor for unknown uuid string ${updatedDescriptor.UUID.UUIDString}" }

return byteArrayOf().toNSData()

}

}

return when (val value = updatedDescriptor.value) {

is NSData -> value

is NSString -> value.dataUsingEncoding(NSUTF8StringEncoding)

?: byteArrayOf().toNSData().also {

logger.warn {

message = "Failed to decode descriptor"

detail("type", "NSString")

detail(descriptor)

}

}

is NSNumber -> memScoped {

NSData.dataWithBytes(

bytes = alloc<IntVar>().also { it.value = value.intValue }.ptr,

length = sizeOf<IntVar>().convert(),

)

}

else -> byteArrayOf().toNSData().also {

logger.warn {

message = "Unable to read descriptor of unknown type"

detail(descriptor)

}

}

Though, I'm not sure what is meant by the // TODO: Convert NSNumber types too code comment.

Additional instances where the type of the descriptor value was used to process it:

FlutterBluePlusPlugin.m#L2137-L2160

BleManager.m#L121-L142

Yes this approach of checking types directly can work as well.
It was actually my first implementation, but then I opted on checking the descriptor.UUID.UUIDString as it was what apple doc suggested and It probably felt less arbitrary to somebody looking at code for the first time.

Though, I'm not sure what is meant by the // TODO: Convert NSNumber types too code comment.

I guess this is a drawback of this approach.
I think that what the comment is trying to say is that NSNumber is always being converted to an Int (i.e. 4 bytes), but it is actually a class that can contain any type of number, such as Long (8 bytes), Short (2 bytes) etc.
This means that if NSNumber actually contains a Long, then we lost 4 bytes of information by converting to Int.

This approach is also used on Dotnet which tries to workaround this issue by casting to UInt64 (the biggest possible number type, so no bytes are lost).

But even with or without Dotnet workaround, there a still probably some inconsistencies across platforms.

For example take CBUUIDClientCharacteristicConfigurationString.
The Bluetooth specification states:

The Attribute Value shall be two octets in length and shall be set to the characteristic descriptor value

But with this approach we will return either 4 bytes (without Dotnet workaround) or 8 bytes (with Donet workaround), when the specification states that should be 2 bytes.
Furthermore, on other platforms (i.e. Android, JS) 2 bytes are returned (I didn't test it, it's just my guess, but seems very likely)

I'm not trying to dismiss this approach as it is indeed simpler, less verbose and seems to be widely adopted by other libraries / Multiplatform frameworks, but just wanted to highlight some of the potentials limitations.

Thanks for the insight! I'm not attached to one approach over the other. As you've identified, they both have trade offs.

Perhaps a mix of the two approaches might make the most sense? Where we first process based on type, then when an NSNumber: determine what size number to process as based on UUID.

Using only UUID, as you currently have it, does seem like a really good approach (and it's nice to follow recommendations set forth by the documentation) but being that the documentation leaves a lot to be desired (at times it isn't clear exactly what type will be provided), it feels safer to process by type first (for the non-number types). 🤷‍♂️

Perhaps a mix of the two approaches might make the most sense? Where we first process based on type, then when an NSNumber: determine what size number to process as based on UUID.

Yes I guess we could go along this way since we couldn't find anywhere the expected type returned by Apple.

I've updated the PR by reworking the conversion approach:

For types that can be converted directly to NSData without ambiguity (i.e. NSString, NSData, UInt16), I convert it directly without further checks on UUID string

For NSNumber (that can't be converted directly to NSData without ambiguity):

I convert it to the documented numeric type by checking on the UUID string where possible

otherwise I convert it to ULong to avoid information loss (Dotnet approach)

Let me know your feedback!

- first type is checked - if type cannot be uniquely converted, uuid string is used

twyatt · 2024-07-17T00:03:22Z

core/src/appleMain/kotlin/CBPeripheralCoreBluetoothPeripheral.kt

+                    else -> value.unsignedLongValue.toNSData()
+                }
+            }
+            is UInt16 -> value.toNSData()


I'm guessing you have this here for CBUUIDL2CAPPSMCharacteristicString?
Although the documentation lists it as UInt16, I'd be surprised if this didn't come in as an NSNumber?
Aside from CBUUIDL2CAPPSMCharacteristicString, did you see any indication of what type this comes in as?

Yes I've kept Uint16 for CBUUIDL2CAPPSMCharacteristicString

Aside from CBUUIDL2CAPPSMCharacteristicString, did you see any indication of what type this comes in as?

Only other reference was found by @davidtaylor-juul

Thanks for the detail. I agree with your deductions in the other comment as well. The only reference I found in the docs is for the type alias https://developer.apple.com/documentation/corebluetooth/cbl2cappsm/ which also hints that this the value for CBUUIDL2CAPPSMCharacteristicString is exposed as a raw 16bit integer, and is not a NSNumber or NSData object. It is for L2CAP so to test a real world example would require reading characteristics from a device that supports L2CAP channels. I'll update this thread if I find a concrete example.

and seems to indicate that it might return as UInt16, since CBL2CAPPSM is an alias for UInt16.

On the other hand, what you have found here (although it is for CBCharacteristic value property which is defined as NSData and not for CBDescriptor value property which is defined as Any)

Not sure if it confirms your suspicions @davidtaylor-juul, but this code retrieves the NSData as-is (without UInt16 casting, as far as I can tell, at least).

Seems to indicate it might be returned as NSData.

Given all these considerations, I think I can add management for CBUUIDL2CAPPSMCharacteristicString in the NSNumber block so that:

if it is actually returned as NSData, as indicated by the code you found for CBCharacteristic, we are already covered by the current code. It will be returned directly.

if it is actually an UInt16 as Apple doc hints, we are already covered by the current code. It will be converted and returned.

if it is, for some reason, actually returned as NSNumber I will now add specific management and convert it to a two bytes array as documented by GATT Part G document.

I'll update the PR in a moment, let me know what you think about this.

IMHO we could omit this case, but I suppose there is no harm leaving it in.

from Apple doc is not 100% clear what type is returned for CBUUIDL2CAPPSMCharacteristicString. GATT part G specify that should be a two bytes in length.

twyatt

I plan to add a few code comments and make some minor code style related changes prior to merging, but this looks great to me! Thanks so much @FabioCornelli!

I'll get this in after we get another approval on it.

twyatt · 2024-07-17T08:52:17Z

core/src/appleMain/kotlin/CBPeripheralCoreBluetoothPeripheral.kt

+                    CBUUIDL2CAPPSMCharacteristicString,
+                    -> value.unsignedShortValue.toNSData()


This feels like a good approach to me. Thanks for the thorough explanation around this. ❤️

davidtaylor-juul

I think we arrived at a sensible solution here - huge thank you for unravelling that stuff!

twyatt · 2024-07-22T19:14:44Z

I plan to add a few code comments and make some minor code style related changes prior to merging

I'll try to find some time to do this in the coming days, then get this merged in.

FabioCornelli requested review from davidtaylor-juul, twyatt and a team as code owners July 2, 2024 09:49

FabioCornelli mentioned this pull request Jul 2, 2024

Discover descriptors on connect on Apple #704

Merged

davidtaylor-juul reviewed Jul 2, 2024

View reviewed changes

twyatt added apple patch Changes that should bump the PATCH version number labels Jul 3, 2024

FabioCornelli force-pushed the read-descriptor-format branch from b571602 to 9145d7c Compare July 7, 2024 11:09

FabioCornelli added 2 commits July 7, 2024 14:06

add UShort toNSData extension function

7320f2f

fix descriptor.value NSData conversion, the correct type depends on r…

0b9ff95

…eceived uuidString

FabioCornelli force-pushed the read-descriptor-format branch from 9145d7c to 0b9ff95 Compare July 7, 2024 12:06

twyatt requested a review from davidtaylor-juul July 8, 2024 19:39

twyatt mentioned this pull request Jul 8, 2024

Rename module directories to match Maven artifact names #709

Merged

davidtaylor-juul reviewed Jul 9, 2024

View reviewed changes

twyatt reviewed Jul 14, 2024

View reviewed changes

FabioCornelli added 2 commits July 16, 2024 11:28

add ULong.toNSData extension function

5e6f5a9

reworked approach for descriptor when converting descriptor value

c450499

- first type is checked - if type cannot be uniquely converted, uuid string is used

twyatt reviewed Jul 17, 2024

View reviewed changes

add CBUUIDL2CAPPSMCharacteristicString management also in NSNumber block

75b2ea3

from Apple doc is not 100% clear what type is returned for CBUUIDL2CAPPSMCharacteristicString. GATT part G specify that should be a two bytes in length.

twyatt approved these changes Jul 17, 2024

View reviewed changes

twyatt requested a review from davidtaylor-juul July 17, 2024 08:53

davidtaylor-juul approved these changes Jul 22, 2024

View reviewed changes

twyatt added 3 commits July 24, 2024 01:37

Code clean up

aff8e77

Merge branch 'main' into read-descriptor-format

5ac5974

Add code comment & formatting

75fd80b

twyatt changed the title ~~fix descriptor.value NSData conversion the correct type depends on received uuidString~~ Fix reading of descriptor value as NSData Jul 24, 2024

twyatt enabled auto-merge (squash) July 24, 2024 08:46

twyatt merged commit 5abbbab into JuulLabs:main Jul 24, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reading of descriptor value as `NSData` #706

Fix reading of descriptor value as `NSData` #706

FabioCornelli commented Jul 2, 2024 •

edited

Loading

davidtaylor-juul Jul 2, 2024

FabioCornelli Jul 4, 2024

davidtaylor-juul Jul 2, 2024

FabioCornelli Jul 4, 2024 •

edited

Loading

davidtaylor-juul Jul 9, 2024

FabioCornelli commented Jul 7, 2024 •

edited

Loading

twyatt commented Jul 9, 2024 •

edited

Loading

davidtaylor-juul Jul 9, 2024

twyatt Jul 14, 2024

twyatt left a comment

twyatt Jul 14, 2024

twyatt Jul 14, 2024

FabioCornelli Jul 14, 2024 •

edited

Loading

twyatt Jul 14, 2024

FabioCornelli Jul 16, 2024

twyatt Jul 17, 2024 •

edited

Loading

FabioCornelli Jul 17, 2024 •

edited

Loading

davidtaylor-juul Jul 22, 2024

twyatt left a comment

twyatt Jul 17, 2024

davidtaylor-juul left a comment

twyatt commented Jul 22, 2024

		CBUUIDL2CAPPSMCharacteristicString,
		-> value.unsignedShortValue.toNSData()

Fix reading of descriptor value as NSData #706

Fix reading of descriptor value as NSData #706

Conversation

FabioCornelli commented Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioCornelli Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioCornelli commented Jul 7, 2024 • edited Loading

twyatt commented Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioCornelli Jul 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

FabioCornelli Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twyatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidtaylor-juul left a comment

Choose a reason for hiding this comment

twyatt commented Jul 22, 2024

Fix reading of descriptor value as `NSData` #706

Fix reading of descriptor value as `NSData` #706

FabioCornelli commented Jul 2, 2024 •

edited

Loading

FabioCornelli Jul 4, 2024 •

edited

Loading

FabioCornelli commented Jul 7, 2024 •

edited

Loading

twyatt commented Jul 9, 2024 •

edited

Loading

FabioCornelli Jul 14, 2024 •

edited

Loading

twyatt Jul 17, 2024 •

edited

Loading

FabioCornelli Jul 17, 2024 •

edited

Loading