sd: handle system attibutes on reconnect #86

ysoldak · 2021-11-10T23:21:30Z

This PR implements the logic of storing system attributes (think CCCD + checksum, but we treat the data as blob of unknown format) on disconnect and restoring them on BLE_GATTS_EVT_SYS_ATTR_MISSING event on subsequent re-connect.

This implements behaviour mandated by Bluetooth specification.
See comment below.

~~I use this functionality to automatically restore subscription state (CCCD=1) of a characteristic on subsequent re-connects.~~

Well, to tell the truth, I use this to fake "subscribed" state of a characteristic since I'm dealing with a controller that does implement BLE stack in a bit "special" way and never actually subscribes, nonetheless expecting notifications to start arriving.

The proper, expected way to use this functionality would be to call GetAttributes before controlled disconnect to save state and call SetAttributes before or directly after next connect to restore state.

~~Anyway, API is there, in SD headers and after this PR it shall be available to use it in go.~~

aykevl

Hmm, I'm unsure about this. I don't really like to add an API specific to one underlying system. In this case, this looks very specific to the SoftDevice. Is there something similar for Linux (BlueZ) or MacOS to make one unified API?

aykevl · 2021-11-11T00:28:37Z

Well, to tell the truth, I use this to fake "subscribed" state of a characteristic since I'm dealing with a controller that does implement BLE stack in a bit "special" way and never actually subscribes, nonetheless expecting notifications to start arriving.

I'm also not entirely sure about the motivation here. So essentially you're using this API to hack around a firmware bug in another device?

ysoldak · 2021-11-11T00:30:03Z

Hmm, I'm unsure about this. I don't really like to add an API specific to one underlying system. In this case, this looks very specific to the SoftDevice. Is there something similar for Linux (BlueZ) or MacOS to make one unified API?

Characteristic attributes are standard in BLE spec, shall be possible to get and set them on other systems too.

ysoldak · 2021-11-11T00:35:39Z

So essentially you're using this API to hack around a firmware bug in another device?

Well, yes. I've found my way around that other closed system's limitation I have no power over.
The API is were, not exposed. This PR makes it avaliable. Hopefully others will find more noble uses of it.

If you want gory details, the other system is FrSky PARA wireless trainer system.
FrSky RC radios run on OpenTX but the bluetooth stack is closed source and hidden.

aykevl · 2021-11-11T00:48:54Z

Characteristic attributes are standard in BLE spec, shall be possible to get and set them on other systems too.

To make sure this API is indeed portable, can you also implement it on some other system? For example, Linux BlueZ or on macOS. (If you have a Linux/macOS system with Bluetooth, that is. Note that many Raspberry Pis also have BLE).

ysoldak · 2021-11-11T00:51:07Z

Characteristic attributes are standard in BLE spec, shall be possible to get and set them on other systems too.

To make sure this API is indeed portable, can you also implement it on some other system? For example, Linux BlueZ or on macOS. (If you have a Linux/macOS system with Bluetooth, that is. Note that many Raspberry Pis also have BLE).

OK, I'll try and implement this for darwin at least. My MacBook does have bluetooth :)

aykevl · 2023-04-27T01:54:26Z

What is the current state of this PR? Were you able to implement this API on another system, like MacOS?

ysoldak · 2023-04-27T08:33:46Z

Yeah, I believe, I've dropped the ball, sorry. Let me try and look at this again.
This is a feature I use in my project and have to keep a branch in my fork just for that, and that's annoying.

ysoldak · 2023-05-01T12:16:23Z

So, I did my research and this is what I've found out.

Additionally, CCCDs have two special properties that separate them from other attributes:

Their values are unique per connection
In multi-connection scenarios, in which a central is connected to multiple peripherals and also acting as a GATT server, each peripheral will receive its own copy of the CCCD's value when reading it with ATT.

Their values are preserved across connections with bonded devices
Attribute Caching discusses attribute caching in more detail, but that concerns only attribute handles. Values are typically not stored per-device and the GATT server can reset them between connections. This is not the case with CCCDs among bonded devices: the last value written by a client to a CCCD on the server is guaranteed to be restored upon reconnection, regardless of the time elapsed between connections.

From "Getting Started with Bluetooth Low Energy by Kevin Townsend, Carles Cufí, Akiba, Robert Davidson"

Also, from NordicSemi forums:

By "system attributes" we primarily mean the Client Characteristic Configuration Descriptors (CCCD), i.e. attributes that are handled by the SoftDevice and have special requirements. It is mandated by Bluetooth specification that CCCD values are altered only by the peer, so you cannot change them from the application. Since CCCD values for a bonded peer should be stored between connections, an API exist for the application to store the values after disconnection, and to restore them on connection.

While you are in a connection, the SoftDevice will handle the CCCD as appropriate. When the peer writes to CCCD, that will be handled by SoftDevice.

The only thing related to the CCCD that you have to do from the application, is to store the data you get from sd_ble_gatts_sys_attr_get() on disconnection, and provide it back to the SoftDevice with sd_ble_gatts_sys_attr_set() on the BLE_GATTS_EVT_SYS_ATTR_MISSING event. You should treat the data as a blob of data of unknown format, i.e. you should store it as is and provide it back as is, without changing it. You should not try to write to the CCCD directly from the application.

https://devzone.nordicsemi.com/f/nordic-q-a/53548/what-is-a-system-attribute/217385

See also GATTS System Attributes Handling: Bonded Peer

This PR implements the logic of storing system attributes (think CCCD + checksum, but we treat the data as blob of unknown format) on disconnect and restoring them on SYS_ATTR_MISSING event.

System attributes is a unique thing for SoftDevice, no such thing exist for other targets. I don't really know how other implementations satisfy CCCD state restore on reconnect, probably it is hidden/abstracted from application layer.

No new API introduced, everything is internal.

In my project, to hack the subscription state, I'm calling respective SoftDevice function directly via CGo. No extra API needed from bluetooth package.

ysoldak · 2023-05-01T12:23:30Z

adapter_nrf51.go

+			// Store system attributes data
+			dataLen := uint16(0)
+			C.sd_ble_gatts_sys_attr_get(gapEvent.conn_handle, nil, &dataLen, 0) // get data length
+			if int(dataLen) <= cap(DefaultAdapter.systemAttributes) {           // we can not allocate here, so ensure at least data fits the buffer


@aykevl I may need your help to solve this. We don't know in advance how large system attributes blob will be and we can't allocate here.
So far, I've seen sizes around 16 bytes, but I assume size depends on the number of characteristics published.
In this PR, I'm pre-allocating 64 bytes array and just skipping the storing logic if the blob is larger. I'm sure this can be handled better, but I don't know how.

ysoldak · 2023-05-01T12:41:45Z

The tricky question though how to know if the peer is bonded or not.
This PR implements the logic for bonded peer (i.e. any peer deemed bonded).

Actually, there is also a case for Unknown Peer. In that case nil shall be sent back.

aykevl

I don't think this implementation is entirely correct. System attributes are meant to be stored across application restarts in persistent storage (flash). See this diagram:
https://infocenter.nordicsemi.com/index.jsp?topic=%2Fcom.nordic.infocenter.s140.api.v6.0.0%2Fgroup___b_l_e___g_a_t_t_s___s_y_s___a_t_t_r_s___b_o_n_d_e_d___p_e_e_r___m_s_c.html

Therefore, I believe this needs a new API somehow. I'm not entirely sure how that API would look though, especially as these blobs are requested from interrupts.
Without checking, I suspect reading/writing these values might not actually need to happen from within an interrupt:

I believe sd_ble_gatts_sys_attr_set does not need to be called from within the BLE_GATTS_EVT_SYS_ATTR_MISSING event interrupt. As long as it is called reasonably soon after the event.
The documentation for sd_ble_gatts_sys_attr_set explicitly says that system attributes remain valid until the next device connects. But perhaps it's more reliable to read them from within the interrupt, store them in a global array, and then set a flag to call a callback outside the interrupt context (which copies the value to a new heap-allocated area to make sure it won't disappear or get overwritten).
...we don't have a mechanism to queue a callback outside interrupts, though.

One idea for how the API could look like is as a set of callbacks for reading/writing system attributes. If the callback isn't set, it will behave as it does today. This callback will be a no-op on Linux/macOS/Windows, which presumably store the system attributes in the system Bluetooth daemon.

aykevl · 2023-05-01T12:38:10Z

adapter_nrf51.go

+			dataLen := uint16(0)
+			C.sd_ble_gatts_sys_attr_get(gapEvent.conn_handle, nil, &dataLen, 0) // get data length


You are doing a heap allocation here (dataLen escapes to the heap). My suggestion is to make dataLen a global.

aykevl · 2023-05-01T12:50:45Z

adapter_nrf528xx-full.go

+			if int(dataLen) <= cap(DefaultAdapter.systemAttributes) {           // we can not allocate here, so ensure at least data fits the buffer
+				DefaultAdapter.systemAttributes = DefaultAdapter.systemAttributes[:dataLen]
+				C.sd_ble_gatts_sys_attr_get(gapEvent.conn_handle, &DefaultAdapter.systemAttributes[0], &dataLen, 0)
+			}


There is no need to call this function twice: you can set dataLen to the length of the buffer and check the NRF_ERROR_DATA_SIZE return value.

aykevl · 2023-05-01T13:05:53Z

The tricky question though how to know if the peer is bonded or not.

It is if we have information stored about that peer. System attributes are stored per peer, not globally.

aykevl reviewed Nov 11, 2021

View reviewed changes

ysoldak force-pushed the sd-gatts-sys-attr branch from ca3c1e0 to e3b405e Compare January 16, 2022 13:55

ysoldak force-pushed the sd-gatts-sys-attr branch from e3b405e to 571b1bb Compare May 6, 2022 21:55

ysoldak force-pushed the sd-gatts-sys-attr branch from 571b1bb to d0b389c Compare July 11, 2022 13:54

ysoldak force-pushed the sd-gatts-sys-attr branch 2 times, most recently from 54e7cab to d28e8fe Compare April 16, 2023 22:02

ysoldak marked this pull request as draft April 29, 2023 20:04

sd: handle system attibutes on reconnect

7384ef4

ysoldak force-pushed the sd-gatts-sys-attr branch from d28e8fe to 7384ef4 Compare May 1, 2023 00:37

ysoldak changed the title ~~sd: get and set characteristic attributes~~ sd: handle system attibutes on reconnect May 1, 2023

ysoldak commented May 1, 2023

View reviewed changes

ysoldak marked this pull request as ready for review May 1, 2023 12:24

ysoldak mentioned this pull request May 1, 2023

nrf528xx: handle BLE_GAP_EVT_PHY_UPDATE_REQUEST #160

Merged

aykevl reviewed May 1, 2023

View reviewed changes

ysoldak marked this pull request as draft May 26, 2023 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sd: handle system attibutes on reconnect #86

sd: handle system attibutes on reconnect #86

ysoldak commented Nov 10, 2021 •

edited

aykevl left a comment

aykevl commented Nov 11, 2021

ysoldak commented Nov 11, 2021

ysoldak commented Nov 11, 2021

aykevl commented Nov 11, 2021

ysoldak commented Nov 11, 2021

aykevl commented Apr 27, 2023

ysoldak commented Apr 27, 2023

ysoldak commented May 1, 2023 •

edited

ysoldak May 1, 2023

ysoldak commented May 1, 2023

aykevl left a comment

aykevl May 1, 2023

aykevl May 1, 2023

aykevl commented May 1, 2023

		dataLen := uint16(0)
		C.sd_ble_gatts_sys_attr_get(gapEvent.conn_handle, nil, &dataLen, 0) // get data length

sd: handle system attibutes on reconnect #86

Are you sure you want to change the base?

sd: handle system attibutes on reconnect #86

Conversation

ysoldak commented Nov 10, 2021 • edited

aykevl left a comment

Choose a reason for hiding this comment

aykevl commented Nov 11, 2021

ysoldak commented Nov 11, 2021

ysoldak commented Nov 11, 2021

aykevl commented Nov 11, 2021

ysoldak commented Nov 11, 2021

aykevl commented Apr 27, 2023

ysoldak commented Apr 27, 2023

ysoldak commented May 1, 2023 • edited

ysoldak May 1, 2023

Choose a reason for hiding this comment

ysoldak commented May 1, 2023

aykevl left a comment

Choose a reason for hiding this comment

aykevl May 1, 2023

Choose a reason for hiding this comment

aykevl May 1, 2023

Choose a reason for hiding this comment

aykevl commented May 1, 2023

ysoldak commented Nov 10, 2021 •

edited

ysoldak commented May 1, 2023 •

edited