BLE Fix read and write callbacks for descriptors not firing if characteristic is not readable or writable #13293

paul-szczepanek-arm · 2020-07-14T16:02:45Z

Summary of changes

The write and read callback for gatt attributes that are used as descriptors were not firing because they were tied to characteristic properties which might be different than descriptor. This removes that limitation and allows the callbacks for fire on read and write.

Impact of changes

Migration actions required

Documentation

none

Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[] Covered by existing mbed-os tests (Greentea or Unittest)
[x] Tests / results supplied as part of this PR

Reviewers

paul-szczepanek-arm · 2020-07-14T16:03:26Z

Tested in the new pytest suite:

============================= test session starts ==============================
platform linux -- Python 3.8.2, pytest-4.6.9, py-1.8.1, pluggy-0.13.0 -- /usr/bin/python3.8
cachedir: .pytest_cache
rootdir: /home/paul/gh/tests-paul/ble-tests, inifile: pytest.ini
plugins: mock-1.10.4, pep8-1.0.6
collecting ... collected 1 item

test_descriptor_read_write.py::test_server_catch_write_to_descriptor 
------------------------------ live log teardown -------------------------------
16:54:52.394   INFO   -->|peripheral| ble shutdown
16:54:52.400   INFO   <--|peripheral| {"status": 0}
16:54:52.400   INFO   <--|peripheral| retcode: 0
16:54:52.401   INFO   -->|peripheral| echo on
16:54:52.404   INFO   <--|peripheral| ECHO is on
16:54:52.405   INFO   <--|peripheral| retcode: 0
16:54:52.405   INFO   -->|peripheral| set --vt100 on
16:54:52.409   INFO   <--|peripheral| 
16:54:52.410   INFO   <--|peripheral| retcode: 0
16:54:52.411   INFO   Stopping "peripheral" runner...
16:54:52.412   INFO   -->|peripheral| set retcode false
16:54:52.447   INFO   <--|peripheral| />set retcode false
16:54:52.521   INFO   -->|central| ble shutdown
16:54:52.527   INFO   <--|central| {"status": 0}
16:54:52.528   INFO   <--|central| retcode: 0
16:54:52.529   INFO   -->|central| echo on
16:54:52.534   INFO   <--|central| ECHO is on
16:54:52.534   INFO   <--|central| retcode: 0
16:54:52.535   INFO   -->|central| set --vt100 on
16:54:52.539   INFO   <--|central| 
16:54:52.541   INFO   <--|central| retcode: 0
16:54:52.541   INFO   Stopping "central" runner...
16:54:52.542   INFO   -->|central| set retcode false
16:54:52.580   INFO   <--|central| />set retcode false


=========================== 1 passed in 3.85 seconds ===========================

Process finished with exit code 0

-------------------------------- live log setup --------------------------------
16:54:48.899   INFO   Starting runner threads for "central"
16:54:49.420   INFO   <--|central| ARM Ltd
16:54:50.016   INFO   -->|central| set --retcode true
16:54:50.052   INFO   <--|central| />set --retcode true
16:54:50.054   INFO   <--|central| retcode: 0
16:54:50.054   INFO   -->|central| echo off
16:54:50.068   INFO   <--|central| />echo off
16:54:50.070   INFO   <--|central| ECHO is off
16:54:50.071   INFO   <--|central| retcode: 0
16:54:50.071   INFO   -->|central| set --vt100 off
16:54:50.075   INFO   <--|central| retcode: 0
16:54:50.075   INFO   -->|central| ble init
16:54:50.093   INFO   <--|central| {"status": 0}
16:54:50.094   INFO   <--|central| retcode: 0
16:54:50.096   INFO   Starting runner threads for "peripheral"
16:54:50.616   INFO   <--|peripheral| ARM Ltd
16:54:51.209   INFO   -->|peripheral| set --retcode true
16:54:51.246   INFO   <--|peripheral| />set --retcode true
16:54:51.247   INFO   <--|peripheral| retcode: 0
16:54:51.248   INFO   -->|peripheral| echo off
16:54:51.261   INFO   <--|peripheral| />echo off
16:54:51.263   INFO   <--|peripheral| ECHO is off
16:54:51.263   INFO   <--|peripheral| retcode: 0
16:54:51.264   INFO   -->|peripheral| set --vt100 off
16:54:51.267   INFO   <--|peripheral| retcode: 0
16:54:51.268   INFO   -->|peripheral| ble init
16:54:51.286   INFO   <--|peripheral| {"status": 0}
16:54:51.287   INFO   <--|peripheral| retcode: 0
-------------------------------- live log call ---------------------------------
16:54:51.289   INFO   -->|peripheral| gattServer declareService 65531
16:54:51.295   INFO   <--|peripheral| {"status": 0}
16:54:51.296   INFO   <--|peripheral| retcode: 0
16:54:51.297   INFO   -->|peripheral| gattServer declareCharacteristic 57007
16:54:51.304   INFO   <--|peripheral| {"status": 0}
16:54:51.305   INFO   <--|peripheral| retcode: 0
16:54:51.305   INFO   -->|peripheral| gattServer setCharacteristicProperties notify indicate
16:54:51.313   INFO   <--|peripheral| {"status": 0}
16:54:51.314   INFO   <--|peripheral| retcode: 0
16:54:51.315   INFO   -->|peripheral| gattServer declareDescriptor 57005
16:54:51.322   INFO   <--|peripheral| {"status": 0}
16:54:51.323   INFO   <--|peripheral| retcode: 0
16:54:51.323   INFO   -->|peripheral| gattServer setDescriptorValue 00112233
16:54:51.330   INFO   <--|peripheral| {"status": 0}
16:54:51.332   INFO   <--|peripheral| retcode: 0
16:54:51.332   INFO   -->|peripheral| gattServer setDescriptorVariableLength false
16:54:51.339   INFO   <--|peripheral| {"status": 0}
16:54:51.340   INFO   <--|peripheral| retcode: 0
16:54:51.341   INFO   -->|peripheral| gattServer commitService
16:54:51.375   INFO   <--|peripheral| {"status": 0,"result": {"UUID": 65531,"handle": 11,"characteristics": [{"UUID": 57007,"value_handle": 13,"properties": ["notify","indicate"],"length": 0,"max_length": 0,"has_variable_length": true,"value": "","descriptors": [{"UUID": 57005,"handle": 14,"length": 4,"max_length": 4,"has_variable_length": false,"value": "00112233"}]}]}}
16:54:51.376   INFO   <--|peripheral| retcode: 0
16:54:51.376   INFO   -->|peripheral| advDataBuilder clear
16:54:51.381   INFO   <--|peripheral| {"status": 0}
16:54:51.382   INFO   <--|peripheral| retcode: 0
16:54:51.382   INFO   -->|peripheral| advDataBuilder setFlags LE_GENERAL_DISCOVERABLE
16:54:51.391   INFO   <--|peripheral| {"status": 0}
16:54:51.393   INFO   <--|peripheral| retcode: 0
16:54:51.393   INFO   -->|peripheral| advDataBuilder setFlags BREDR_NOT_SUPPORTED
16:54:51.400   INFO   <--|peripheral| {"status": 0}
16:54:51.401   INFO   <--|peripheral| retcode: 0
16:54:51.402   INFO   -->|peripheral| gap applyAdvPayloadFromBuilder 0
16:54:51.408   INFO   <--|peripheral| {"status": 0}
16:54:51.409   INFO   <--|peripheral| retcode: 0
16:54:51.410   INFO   -->|peripheral| advParams setType CONNECTABLE_UNDIRECTED
16:54:51.417   INFO   <--|peripheral| {"status": 0}
16:54:51.418   INFO   <--|peripheral| retcode: 0
16:54:51.418   INFO   -->|peripheral| gap setAdvertisingParameters 0
16:54:51.425   INFO   <--|peripheral| {"status": 0}
16:54:51.427   INFO   <--|peripheral| retcode: 0
16:54:51.428   INFO   -->|peripheral| gap getAddress
16:54:51.441   INFO   <--|peripheral| {"status": 0,"result": {"address_type": "RANDOM","address": "D3:A6:95:6A:3B:56"}}
16:54:51.442   INFO   <--|peripheral| retcode: 0
16:54:51.443   INFO   -->|peripheral| gap startAdvertising 0 0 0
16:54:51.451   INFO   <--|peripheral| {"status": 0}
16:54:51.451   INFO   <--|peripheral| retcode: 0
16:54:51.452   INFO   -->|central| gap connect RANDOM D3:A6:95:6A:3B:56
16:54:51.453   INFO   -->|peripheral| gap waitForConnection 10000
16:54:52.121   INFO   <--|central| {"status": 0,"result": {"status": "BLE_ERROR_NONE","peer_address_type": "RANDOM","peer_address": "D3:A6:95:6A:3B:56","interval": 56,"latency": 0,"supervision_timeout": 100,"connection_handle": 1,"own_role": "CENTRAL","master_clock_accuracy": 0}}
16:54:52.123   INFO   <--|central| retcode: 0
16:54:52.130   INFO   <--|peripheral| {"status": 0,"result": {"status": "BLE_ERROR_NONE","peer_address_type": "RANDOM","peer_address": "D8:E2:FB:7F:0A:A8","interval": 56,"latency": 0,"supervision_timeout": 100,"connection_handle": 1,"own_role": "PERIPHERAL","master_clock_accuracy": 0}}
16:54:52.132   INFO   <--|peripheral| retcode: 0
16:54:52.136   INFO   <--|peripheral| <<< {"type": "event","name": "advertising_end","value": {"advertising_handle": 0,"completed_events": 0,"is_connected": true,"connection_handle": 0}}
16:54:52.225   INFO   -->|peripheral| gattServer waitForDataWritten 1 14 5000
16:54:52.226   INFO   -->|central| gattClient writeCharacteristicDescriptor 1 14 11223344
16:54:52.324   INFO   <--|peripheral| {"status": 0,"result": {"connection_handle": 1,"attribute_handle": 14,"offset": 0,"length": 4,"write_operation_type": "OP_WRITE_REQ","data": "11223344"}}
16:54:52.325   INFO   <--|peripheral| retcode: 0
16:54:52.391   INFO   <--|central| {"status": 0,"result": {"connection_handle": 1,"attribute_handle": 14,"offset": 0,"length": 0,"write_operation_type": "OP_WRITE_REQ","data": ""}}
16:54:52.392   INFO   <--|central| retcode: 0
PASSED                                                                   [100%]

paul-szczepanek-arm · 2020-07-14T16:07:48Z

features/FEATURE_BLE/targets/TARGET_CORDIO/source/CordioGattServer.cpp

@@ -510,7 +510,7 @@ ble_error_t GattServer::insert_descriptor(
 #endif // BLE_FEATURE_SECURITY
        }

-        if (properties & READ_PROPERTY && !(attribute_it->settings & ATTS_SET_CCC)) {
+        if (!(attribute_it->settings & ATTS_SET_CCC)) {


the real condition to check is descriptor->isReadAllowed() which is already checked above

paul-szczepanek-arm · 2020-07-14T16:07:58Z

features/FEATURE_BLE/targets/TARGET_CORDIO/source/CordioGattServer.cpp

@@ -543,7 +543,7 @@ ble_error_t GattServer::insert_descriptor(
 #endif // BLE_FEATURE_SECURITY
        }

-        if (properties & WRITABLE_PROPERTIES && !(attribute_it->settings & ATTS_SET_CCC)) {
+        if (!(attribute_it->settings & ATTS_SET_CCC)) {


same as above

ciarmcom · 2020-07-14T17:00:31Z

@paul-szczepanek-arm, thank you for your changes.
@ARMmbed/mbed-os-pan @ARMmbed/mbed-os-maintainers please review.

pan-

Thanks @paul-szczepanek-arm it works well.

adbridge · 2020-07-17T15:06:24Z

Ci started

mbed-ci · 2020-07-17T16:02:36Z

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 1
Build artifacts

callbacks to descriptors should not based on characteristic properties

aa42aeb

paul-szczepanek-arm commented Jul 14, 2020

View reviewed changes

ciarmcom requested review from a team July 14, 2020 17:00

ciarmcom added the needs: review label Jul 14, 2020

pan- approved these changes Jul 15, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Jul 15, 2020

adbridge added the release-type: patch Indentifies a PR as containing just a patch label Jul 17, 2020

adbridge approved these changes Jul 17, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Jul 17, 2020

0xc0170 merged commit 7ef1dbc into ARMmbed:master Jul 20, 2020

mergify bot removed the ready for merge label Jul 20, 2020

mbedmain added release-version: 6.2.1 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Aug 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

BLE Fix read and write callbacks for descriptors not firing if characteristic is not readable or writable #13293

BLE Fix read and write callbacks for descriptors not firing if characteristic is not readable or writable #13293

Uh oh!

paul-szczepanek-arm commented Jul 14, 2020

Uh oh!

paul-szczepanek-arm commented Jul 14, 2020

Uh oh!

paul-szczepanek-arm Jul 14, 2020

Uh oh!

paul-szczepanek-arm Jul 14, 2020

Uh oh!

ciarmcom commented Jul 14, 2020

Uh oh!

pan- left a comment

Uh oh!

adbridge commented Jul 17, 2020

Uh oh!

mbed-ci commented Jul 17, 2020

Uh oh!

Uh oh!

BLE Fix read and write callbacks for descriptors not firing if characteristic is not readable or writable #13293

BLE Fix read and write callbacks for descriptors not firing if characteristic is not readable or writable #13293

Uh oh!

Conversation

paul-szczepanek-arm commented Jul 14, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

paul-szczepanek-arm commented Jul 14, 2020

Uh oh!

paul-szczepanek-arm Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

paul-szczepanek-arm Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

ciarmcom commented Jul 14, 2020

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

adbridge commented Jul 17, 2020

Uh oh!

mbed-ci commented Jul 17, 2020

Test run: SUCCESS

Uh oh!

Uh oh!