add ServiceData advertising element

[dnlwgnd]

trafficstars

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?

[dnlwgnd]

YES thank you. It now works also on linux!

[ryanrolds]

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]

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]

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]

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]

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]

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]

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?

[dnlwgnd]

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.

[dnlwgnd]

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]

@aykevl any further feedback before we squash/merge?

[deadprogram]

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

@dnlwgnd that has now been corrected.

[deadprogram]

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!