Bluetooth: settings: CCC not stored on device reset

[Qbicz]

The only situation where CCC is stored is on Bluetooth disconnection - bt_gatt_store_ccc() is only called in bt_gatt_disconnected().

However, if the device resets, there is no Bluetooth disconnection event and the CCC is not stored. Because of this fact, on reconnection with bonded peer the CCC is incorrect: e.g. HID device cannot send HID input.

I think the CCC should be stored to persistent memory on every CCC change or maybe periodically.

[Qbicz]

This can be reproduced on HIDS peripheral sample for latest Zephyr master with Android peer. Windows peer writes the CCC to the Bluetooth HID peripheral with every connection establishment, so the issue is not visible there.

When the bonded peer disconnects, CCC is saved correctly and loaded after reconnection:

[bt] [DBG] bt_gatt_disconnected: (0x20003b94) conn 0x2000241c
[bt] [DBG] disconnected_cb: (0x20003b94) ccc 0x20006bf0 reseted
[bt] [DBG] ccc_save: (0x20003b94) Storing CCCs handle 0x001c value 0x0001
[bt] [DBG] bt_settings_encode_key: (0x20003b94) Encoded path bt/ccc/XX27ea307e830
[bt] [DBG] bt_gatt_store_ccc: (0x20003b94) Stored CCCs for XX:27:ea:30:7e:83 (public) (bt/ccc/XX27ea307e830) val HAABAA==
Disconnected from XX:27:ea:30:7e:83 (public) (reason 19)
[bt] [DBG] bt_gatt_connected: (0x20003b94) conn 0x2000241c
[bt] [DBG] gatt_ccc_changed: (0x20003b94) ccc 0x20006bf0 value 0x0001
Connected XX:27:ea:30:7e:83 (public)
Security changed: XX:27:ea:30:7e:83 (public) level 3
[bt] [DBG] bt_gatt_attr_read: (0x20003b94) handle 0x0001 offset 0 length 2
[bt] [DBG] bt_gatt_attr_read: (0x20003b94) handle 0x0008 offset 0 length 2
(...)
[bt] [DBG] bt_gatt_attr_read: (0x20003b94) handle 0x001d offset 0 length 2
[bt] [DBG] bt_gatt_attr_write_ccc: (0x20003b94) handle 0x001c value 1

However, when the device is reset, the CCC is not stored, and after connect re-establishment the bt_gatt_attr_write_ccc() is not called for CCC.

[Qbicz]

cc @carlescufi

[Vudentz]

@Qbicz What is the device reset procedure? Don't you have to disconnect, or have some event before shutting down? That could then be used to cleanup the connection and then save the states otherwise even if we do save on every write there is a risk that the device is reset while storing so you will need a cleanup procedure anyway.

[Qbicz]

@Vudentz if we can gracefully shutdown that's great. I'm concerned about scenarios where the battery life ends or user just removes the batteries. In such case, we should provide a correct behaviour after restart of system and reconnection to peer. That's why I think the CCC should be stored earlier, maybe just after it is changed.

[Qbicz]

@Vudentz I decided to fix this behaviour, but wanted to ask if you think it's a good idea to store CCC in persistent memory on the peripheral side after CCC change, instead of on disconnection only.

[carlescufi]

@joerchan @nvlsianpu can you please advise on this one? There was a conversation on IRC about this in #zephyr-bt, hopefully you have the logs.

[nvlsianpu]

I see no obstacles for saving this just after case. In normal device life storing CCC Is expected to be rare event -like paring is.. In case a device might subscribe to more than one characteristic it is better to save all CCC at once (this will decrees wear levering - understand that we don't know how many subscription request will received, so nice to have only). We disused this together with @Qbicz off-chanel as well.

[Qbicz]

As discussed with @Vudentz and @nvlsianpu, I'm adding an optional behaviour: store CCC to flash after it has been changed (in bt_gatt_attr_write_ccc()). I already have the storing and restoring CCC from flash working. This requires either increasing BT RX stack size or deferring to system work queue (connected to a question @Vudentz asked on IRC).

However there is another bug in loading from settings which holds me back: in bt_settings_decode_key() after decoding all CCC values, base64_decode() tries to decode some garbage byte and fails. It can be observed even on master. I'll report it as a separate issue.

[JoeHut]

@Qbicz did you report the base64_decode() issue? I cannot find it and we are facing it as well.

[Qbicz]

@JoeHut no, I didn't create the issue. I stopped seeing it after implementing #10452 which ensures that I always have correct CCC stored. If you can reproduce the decoding issue (especially for something different than CCC), please create a bug report.

[JoeHut]

@Qbicz ok, thanks. I will check with your PR once merged 👍
@dhananjaygj FYI