add ServiceData advertising element #243

dnlwgnd · 2024-02-18T21:17:58Z

this PR add support for ServiceData advertising elements (0x16: Service Data - 16-bit UUID)
It is based on the work of #123 of which I merged all commits.
fixes #241

I added

handling of advertising (sd, linux)
handling of scan (sd, linux)
bugfixes
testcase

I tested successfully

advertise with ServiceData from softdevice
scan with ServiceData from softdevice
scan with ServiceData from linux

There is an error I can not figure out when advertising with ServiceData on linux. I keep getting:

panic: failed to start adv: bluetooth: could not start advertisement (register): Failed to parse advertisement.

Maybe s.th. is wrong with the formating of the ServiceData fields. Do you know what this should look like? Could you test on your end?

…ervicedata this was PR tinygo-org#123

aykevl

Have you looked at the documentation here?
https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/doc/org.bluez.LEAdvertisement.rst

aykevl · 2024-02-19T15:03:19Z

gap_linux.go

+	var serviceData = make(map[string][]byte)
+	for uuid, data := range options.ServiceData {
+		serviceData[New16BitUUID(uuid).String()] = data.([]byte)
+	}


My guess would be that you need to use map[string]interface{} instead of map[string][]byte (like manufacturer data but with a different key type).

aykevl · 2024-02-19T15:04:51Z

gap_linux.go

+	serviceData := make(map[uint16][]byte)
+	if sdata, ok := props["ServiceData"].Value().(map[string]dbus.Variant); ok {
+		for k, v := range sdata {
+			// looks like bluez delivers the long-128bit-uuid even for 16-bit uuid ad-element and it needs to be converted


Yes 128-bit UUIDs are commonly used in BlueZ.

dnlwgnd · 2024-02-19T19:05:54Z

YES thank you. It now works also on linux!

ryanrolds · 2024-02-19T19:45:18Z

I'm new to Bluetooth, so please excuse my ignorance. Should the list of service UUIDs include the 128-bit UUIDs for the service data?

dnlwgnd · 2024-02-19T20:21:10Z

well yes. there is 16, 32 and 128 Bit uuid for service data. see the exhaustive list of possible advertising element types
This PR so far only covers the 16-Bit uuid case as this was my pain.

That beeing said, i recon the larger uuids are uncommon since there are only 31 bytes total space in the advertisment.

aykevl · 2024-02-20T20:29:46Z

Looking at it, there's a problem: the service data is specified as a map and maps are unordered in Go. This means the output won't be consistent. I think that's a problem. It should really be something that keeps order, like a slice.
The same applies to ManufacturerData, which actually has a few more problems. I'll look into fixing this.

deadprogram · 2024-02-21T11:40:45Z

Hello @dnlwgnd thank you for working on this.

Now that #244 has been merged, we will need to make a similar change to the ServiceData.

aykevl · 2024-02-21T14:32:59Z

Also, I'm thinking we should perhaps use bluetooth.UUID types for the ServiceData fields? Most people will be using 16-bit UUIDs but the specification also allows 32-bit and 128-bit UUIDs which I think should be supported as well (unless we want to break the API again when someone eventually comes along to ask for this feature).

Sorry for all the back-and-forth, I really appreciate your work on this but would like to get the API right.

dnlwgnd · 2024-02-21T20:51:27Z

Hi all,
thanks for working on this with me. I am learning a lot!
I do see your points on using a slice type and also on getting the API right. Pieces of this I already saw, but was afraid of changing too much around.
I will look into straightening the ServiceData parts up.

dnlwgnd · 2024-02-22T06:29:58Z

two questions:

Shouldn't CompanyID be also a UUID Type?

type ManufacturerDataElement struct {
	// The company ID, which must be one of the assigned company IDs.
	// The full list is in here:
	// https://www.bluetooth.com/specifications/assigned-numbers/
	// The list can also be viewed here:
	// https://bitbucket.org/bluetooth-SIG/public/src/main/assigned_numbers/company_identifiers/company_identifiers.yaml
	// The value 0xffff can also be used for testing.
	CompanyID uint16

	// The value, which can be any value but can't be very large.
	Data []byte
}

Then we could reuse this Type also for Service Data (and call it more generically). Maybe for ManufacturerData we check if it is 16-Bit only.

I saw this error declaration

	errAdvertisementPacketTooBig = errors.New("bluetooth: advertisement packet overflows")

I think addFromOptions should return this error instead of false if the payload does not fit?

merge because of tinygo-org#244

dnlwgnd · 2024-02-24T11:38:10Z

right. I merged changes from #244 and did similar changes to ServiceData.
Also I went ahead and implemented changes suggested in my last comment.
Tests with linux and softdeviced succeeded on my end.

I had problems with vscode working on the mac and windows files, as they were not recognized properly and were not parsed.

aykevl

Shouldn't CompanyID be also a UUID Type?

No. CompanyID is always 16 bits, it's not a UUID (and in fact in a different namespace compared to other BLE UUIDs). It's just a 16-bit number representing a particular company.

I think addFromOptions should return this error instead of false if the payload does not fit?

It's returned at the caller side, so I don't think there's a problem right now. Though the code might be a bit cleaner if the error is generated inside addFromOptions (so that it can return other kinds of errors too).
That said, I think it mostly makes sense in addFromOptions, in other methods like addFlags it doesn't make much sense to me.

aykevl · 2024-02-20T19:51:42Z

gap_linux.go

@@ -53,6 +53,10 @@ func (a *Advertisement) Configure(options AdvertisementOptions) error {
 	for _, uuid := range options.ServiceUUIDs {
 		serviceUUIDs = append(serviceUUIDs, uuid.String())
 	}
+	var serviceData = make(map[string]interface{})
+	for uuid, data := range options.ServiceData {
+		serviceData[New16BitUUID(uuid).String()] = data.([]byte)


This type assert doesn't seem to be necessary.

Suggested change

serviceData[New16BitUUID(uuid).String()] = data.([]byte)

serviceData[New16BitUUID(uuid).String()] = data

aykevl · 2024-02-25T12:54:00Z

gap.go

+func (buf *rawAdvertisementPayload) ManufacturerData() []AdvertismentDataElement {
+	var manufacturerData []AdvertismentDataElement
+	for _, data := range buf.findAllFields(0xFF) { 
+		manufacturerData = append(manufacturerData, AdvertismentDataElement{
+			UUID: New16BitUUID(uint16(data[0])+(uint16(data[1])<<8)),
+			Data: data[2:],


The old code has been written in a way that avoids heap allocations. The rewrite introduces new heap allocations, which I think is a regression.

I agree the code is somewhat hard to read. It might be possible to make it easier to read using function iterators, though I'm not entirely sure about that.

aykevl · 2024-02-25T12:55:28Z

gap.go

+	var serviceData []AdvertismentDataElement
+	for _, data := range buf.findAllFields(0x16) { //16-Bit uuid
+		serviceData = append(serviceData, AdvertismentDataElement{
+			UUID: New16BitUUID(uint16(data[0])+(uint16(data[1])<<8)),
+			Data: data[2:],
+		})
+	}
+	for _, data := range buf.findAllFields(0x20) { //32-Bit uuid
+		serviceData = append(serviceData, AdvertismentDataElement{
+			UUID: New32BitUUID(uint32(data[0])+(uint32(data[1])<<8)+(uint32(data[2])<<16)+(uint32(data[3])<<24)),
+			Data: data[4:],
+		})
+	}
+	for _, data := range buf.findAllFields(0x21) { //128-Bit uuid
+		var uuidArray [16]byte
+		copy(uuidArray[:], data[:16])
+		serviceData = append(serviceData, AdvertismentDataElement{
+			UUID: NewUUID(uuidArray),
+			Data: data[16:],
+		})
+	}
+	return serviceData


With a loop like the one that was previously in ManufacturerData, you could make a single loop that loops through all fields only once (and avoids lots of unnecessary heap allocations!).

aykevl · 2024-02-25T12:56:40Z

gap.go

-	buf.data[buf.len+0] = uint8(fieldLength - 1)
-	buf.data[buf.len+1] = 0xff
-	buf.data[buf.len+2] = uint8(key)
-	buf.data[buf.len+3] = uint8(key >> 8)
-	copy(buf.data[buf.len+4:], value)
+	buf.data[buf.len+0] = byte(fieldLength - 1)
+	buf.data[buf.len+1] = 0xFF
+	buf.data[buf.len+2] = byte(uuid.Get16Bit())
+	buf.data[buf.len+3] = byte(uuid.Get16Bit() >> 8)
+	copy(buf.data[buf.len+4:], data)


With all these extra (unrelated!) changes it becomes a bit hard to review this PR. I appreciate the effort but I'd prefer to keep PRs small and focused on a single thing.

The larger a PR, the harder it is to review and the longer it'll take to merge the features you actually care about.

aykevl · 2024-02-25T12:57:26Z

gap.go

+// addServiceData adds service data ([]byte) entries to the advertisement payload.
+func (buf *rawAdvertisementPayload) addServiceData(uuid UUID, data []byte) (err error) {
+	switch {
+	case uuid.Is16Bit(): {


This is Go, I'm pretty sure you don't need the extra curly braces.

aykevl · 2024-02-25T12:58:34Z

gap.go

 // addFlags adds a flags field to the advertisement buffer. It returns true on
 // success (the flags can be added) and false on failure.
-func (buf *rawAdvertisementPayload) addFlags(flags byte) (ok bool) {
+func (buf *rawAdvertisementPayload) addFlags(flags byte) (err error) {


I think returning a boolean is fine here? It's an internal function, and the only way it could fail is when there's not enough space in the buffer.

(Same goes for other similar methods).

dnlwgnd · 2024-02-27T21:55:25Z

Hi,
thank you for those valueable comments! and sry for making this more messy then needed.
I removed unrelated changes and tried to cater for the heap allocations by using your code pattern, although beeing very new to go I wasnt very aware of this issue.
Unfortunately I cannot see why the check is now failing, it is not on my end.

deadprogram · 2024-02-28T19:25:55Z

@aykevl any further feedback before we squash/merge?

deadprogram · 2024-02-28T19:26:30Z

Unfortunately I cannot see why the check is now failing, it is not on my end.

@dnlwgnd that has now been corrected.

aykevl

Looks good to me! Just two minor comments.
Also, can you run go fmt on this package? It looks like some files aren't formatted correctly (normally IDEs do this for you).

aykevl · 2024-02-29T13:51:47Z

adapter_darwin.go

+	for _, svcData := range advFields.ServiceData {
+		cbgoUUID := svcData.UUID
+		uuid, err := ParseUUID(cbgoUUID.String())
+		if err != nil || uuid.String() != cbgoUUID.String() {


Why the extra check whether the UUID matches? (if it doesn't, it means the UUID either is formatted differently for example due to capitalization, or there's a bug in ParseUUID which isn't the responsibility of the calling code).

Suggested change

if err != nil || uuid.String() != cbgoUUID.String() {

if err != nil {

(Also, there should never be an error here but as it's an external API it doesn't hurt to check for the error response anyway).

actually that piece came with the other PR in the beginning. but sure that check is redundant.

aykevl · 2024-02-29T13:54:40Z

gap.go

+	// service uuid or company uuid
+	// The list can also be viewed here:
+	// https://bitbucket.org/bluetooth-SIG/public/src/main/assigned_numbers/company_identifiers/company_identifiers.yaml
+	// https://bitbucket.org/bluetooth-SIG/public/src/main/assigned_numbers/uuids/service_uuids.yaml


This is always the service UUID (which can be 16/32/128-bit) - not a company ID.

I thought so as well, but indeed in case of bthome which is my usecase, they use a company id (0xFCD2) with service data. Hence my comment on the function .

It really is a service UUID, not a company ID. You can check for yourself here: https://bitbucket.org/bluetooth-SIG/public/src/413977fdb5a21c25947f3abc4d7cf26947cd0e63/assigned_numbers/uuids/member_uuids.yaml#lines-1149
Company IDs are listed here, and don't include 0xFCD2: https://bitbucket.org/bluetooth-SIG/public/src/main/assigned_numbers/company_identifiers/company_identifiers.yaml

ok fine. i didnt think that members weren't companies. please go ahead and change that comment to suit you.

I will change in a subsequent PR.

Here is a PR to fix the comment: #275

deadprogram · 2024-03-18T21:14:45Z

Looks like all of the feedback has been addressed, except for the one comment that I can handle in a subsequent PR.

@radhus and @dnlwgnd thank you both so very much for all of the effort on this, and to @aykevl for your patient coaching to get this feature added.

I will have to squash/merge due to the convoluted commit history. But at least we are getting it completed. Thanks again everyone!

* gap: fix comment * gap: expose ServiceData() in AdvertisementFields * macos: include ServiceData in AdvertisementFields * gap/linux: include ServiceData in AdvertisementFields * gap: add unimplemented ServiceData() to raw advertisement * added ServiceData advertising element also to the sending pieces * more explicitly use the ad element type ids * added a test case for ServiceData * linux: added ServiceData advertising element * sd: fix: handle no servicedata present * linux: bluez uses string uuids for service data * linux: fix: correct datatype for advertise with ServiceData * uuid: add 32-Bit functions * ServiceData now also uses a slice instead of a map as in #244 * Revert unnessesary changes * formatting * remove extra check --------- Co-authored-by: William Johansson <[email protected]>

radhus and others added 12 commits October 9, 2022 19:42

gap: fix comment

8a40ada

gap: expose ServiceData() in AdvertisementFields

07fa144

macos: include ServiceData in AdvertisementFields

2b64fa4

gap/linux: include ServiceData in AdvertisementFields

a8f1440

gap: add unimplemented ServiceData() to raw advertisement

89f28d2

Merge remote-tracking branch 'radhus/servicedata-in-advfields' into s…

7008639

…ervicedata this was PR tinygo-org#123

added ServiceData advertising element also to the sending pieces

3f12574

more explicitly use the ad element type ids

1081f5f

added a test case for ServiceData

132d7de

linux: added ServiceData advertising element

a4c7183

sd: fix: handle no servicedata present

1cfab88

linux: bluez uses string uuids for service data

7879cc3

aykevl reviewed Feb 19, 2024

View reviewed changes

linux: fix: correct datatype for advertise with ServiceData

01f10d9

aykevl mentioned this pull request Feb 20, 2024

all: change ManufacturerData from a map to a slice #244

Merged

Merge remote-tracking branch 'origin/dev' into servicedata

1cc585e

merge because of tinygo-org#244

dnlwgnd force-pushed the servicedata branch 3 times, most recently from 88d7297 to f0fb62b Compare February 24, 2024 11:30

aykevl reviewed Feb 25, 2024

View reviewed changes

uuid: add 32-Bit functions

e368ed6

ServiceData now also uses a slice instead of a map as in tinygo-org#244

b052dcb

dnlwgnd force-pushed the servicedata branch from f0fb62b to b052dcb Compare February 27, 2024 21:30

Revert unnessesary changes

8182ad0

aykevl reviewed Feb 29, 2024

View reviewed changes

dnlwgnd added 2 commits February 29, 2024 20:56

formatting

24bd8fc

remove extra check

14e58f6

deadprogram merged commit e0d5fd4 into tinygo-org:dev Mar 18, 2024
5 checks passed

radhus mentioned this pull request Mar 18, 2024

Expose ServiceData() in AdvertisementFields #123

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add ServiceData advertising element #243

add ServiceData advertising element #243

dnlwgnd commented Feb 18, 2024

aykevl left a comment

aykevl Feb 19, 2024

aykevl Feb 19, 2024

dnlwgnd commented Feb 19, 2024

ryanrolds commented Feb 19, 2024

dnlwgnd commented Feb 19, 2024 •

edited

Loading

aykevl commented Feb 20, 2024

deadprogram commented Feb 21, 2024

aykevl commented Feb 21, 2024 •

edited

Loading

dnlwgnd commented Feb 21, 2024

dnlwgnd commented Feb 22, 2024 •

edited

Loading

dnlwgnd commented Feb 24, 2024

aykevl left a comment

aykevl Feb 20, 2024

aykevl Feb 25, 2024

aykevl Feb 25, 2024

aykevl Feb 25, 2024 •

edited

Loading

aykevl Feb 25, 2024

aykevl Feb 25, 2024

dnlwgnd commented Feb 27, 2024

deadprogram commented Feb 28, 2024

deadprogram commented Feb 28, 2024

aykevl left a comment

aykevl Feb 29, 2024

dnlwgnd Feb 29, 2024

aykevl Feb 29, 2024 •

edited

Loading

dnlwgnd Feb 29, 2024

aykevl Mar 2, 2024

dnlwgnd Mar 6, 2024

deadprogram Mar 18, 2024

aykevl May 25, 2024

deadprogram commented Mar 18, 2024

	serviceData[New16BitUUID(uuid).String()] = data.([]byte)
	serviceData[New16BitUUID(uuid).String()] = data

	if err != nil \|\| uuid.String() != cbgoUUID.String() {
	if err != nil {

add ServiceData advertising element #243

add ServiceData advertising element #243

Conversation

dnlwgnd commented Feb 18, 2024

aykevl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnlwgnd commented Feb 19, 2024

ryanrolds commented Feb 19, 2024

dnlwgnd commented Feb 19, 2024 • edited Loading

aykevl commented Feb 20, 2024

deadprogram commented Feb 21, 2024

aykevl commented Feb 21, 2024 • edited Loading

dnlwgnd commented Feb 21, 2024

dnlwgnd commented Feb 22, 2024 • edited Loading

dnlwgnd commented Feb 24, 2024

aykevl 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

aykevl Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnlwgnd commented Feb 27, 2024

deadprogram commented Feb 28, 2024

deadprogram commented Feb 28, 2024

aykevl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aykevl Feb 29, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deadprogram commented Mar 18, 2024

dnlwgnd commented Feb 19, 2024 •

edited

Loading

aykevl commented Feb 21, 2024 •

edited

Loading

dnlwgnd commented Feb 22, 2024 •

edited

Loading

aykevl Feb 25, 2024 •

edited

Loading

aykevl Feb 29, 2024 •

edited

Loading