BLE: Fix GattServer callbacks not being copied to GattServer instance of the service #14084

paul-szczepanek-arm · 2020-12-23T12:07:14Z

Summary of changes

When adding services to GattServer, they are allocated new memory and any changes made to original characteristics will not take effect after adding them - all operations must be performed on the GattServer version. This is correct behaviour.

At the same time we are requesting the user keeps hold of the old characteristics, wasting space for no reason. This is because gattserver makes references to the original characteristics. There is no need for this.

This PR copies the values being used through reference (authorisation callbacks) and updates the docs to remove the requirement to keep the characteristics after adding. The behaviour does not change and existing code will run the same way but it is now allowed to free the characteristics used to build the service and thus free up memory.

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)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@ithinuel

paul-szczepanek-arm · 2020-12-23T12:17:39Z

connectivity/FEATURE_BLE/source/cordio/source/GattServerImpl.cpp

@@ -941,7 +964,8 @@ ble_error_t GattServer::reset(ble::GattServer* server)
    currentHandle = 0;
    cccd_cnt = 0;

-    _auth_char_count = 0;
+    _auth_callbacks_count = 0;
+    _auth_callbacks = nullptr;


memory is freed elsewhere (line 954)

ithinuel

Despite the nitpicking on class/pointer to bool auto conversion in if/while statements, it LGTM.

ithinuel · 2020-12-23T12:10:41Z

connectivity/FEATURE_BLE/source/cordio/source/GattServerImpl.cpp

-    GattCharacteristic *auth_char = getInstance().get_auth_char(handle);
-    if (auth_char && auth_char->isReadAuthorizationEnabled()) {
+    char_auth_callback *auth_cb = getInstance().get_auth_callback(handle);
+    if (auth_cb && auth_cb->read_cb) {


I'm not a bit fan of pointer to bool coersion.
I'd prefer a

Suggested change

if (auth_cb && auth_cb->read_cb) {

if ((auth_cb != NULL) && (auth_cb->read_cb != NULL)) {

ithinuel · 2020-12-23T12:15:31Z

connectivity/FEATURE_BLE/source/cordio/source/GattServerImpl.cpp

-        GattAuthCallbackReply_t ret = auth_char->authorizeRead(&read_auth_params);
-        if (ret != AUTH_CALLBACK_REPLY_SUCCESS) {
-            return ret & 0xFF;
+        auth_cb->read_cb.call(&read_auth_params);


I don't know what FunctionPointerWithContext does, but the test above followed by this by-value access to the member is a bit confusing.

ciarmcom · 2020-12-23T12:30:16Z

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

adbridge · 2020-12-23T12:33:17Z

CI started

mbed-ci · 2020-12-23T13:12:17Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️

paul-szczepanek-arm · 2021-01-04T08:52:06Z

There's something wrong with CI, the failed test is complaining about errors in mbed tools?

0xc0170 · 2021-01-04T16:57:30Z

There's something wrong with CI, the failed test is complaining about errors in mbed tools?

This fixes the issue #14108. This PR needs to be rebased to the latest master.

paul-szczepanek-arm · 2021-01-04T17:11:40Z

thanks, rebased

0xc0170 · 2021-01-04T17:13:27Z

Ci restarted

mbed-ci · 2021-01-04T17:35:24Z

Jenkins CI Test : ❌ FAILED

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	❌
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️

paul-szczepanek-arm · 2021-01-04T19:42:26Z

Build failing because of unrelated issue causing link error on ARM:
#14112

jamesbeyond · 2021-01-05T01:05:48Z

Build failing because of unrelated issue causing link error on ARM:
#14112

Hi, @paul-szczepanek-arm
The failure is caused by recent(31th Dec) changes happened to socket example :
https://github.com/ARMmbed/mbed-os-example-sockets/commits/development
The failure is consistent and reproduceable across both mbed-OS nighty and PR tests

paul-szczepanek-arm · 2021-01-05T08:57:49Z

Build failing because of unrelated issue causing link error on ARM:
#14112

Hi, @paul-szczepanek-arm
The failure is caused by recent(31th Dec) changes happened to socket example :
https://github.com/ARMmbed/mbed-os-example-sockets/commits/development
The failure is consistent and reproduceable across both mbed-OS nighty and PR tests

That was the socket defaulting to tls and is already fixed.

0xc0170 · 2021-01-05T10:41:10Z

CI restarted

mbed-ci · 2021-01-05T11:12:13Z

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs	Status
jenkins-ci/mbed-os-ci_unittests	✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM	✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-ARM	✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM	✔️
jenkins-ci/mbed-os-ci_greentea-test	✔️
jenkins-ci/mbed-os-ci_cmake-example-test	✔️

paul-szczepanek-arm commented Dec 23, 2020

View reviewed changes

mergify bot added the needs: work label Dec 23, 2020

ithinuel approved these changes Dec 23, 2020

View reviewed changes

ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label Dec 23, 2020

ciarmcom requested review from ithinuel and a team December 23, 2020 12:30

ciarmcom added the needs: review label Dec 23, 2020

ithinuel approved these changes Dec 23, 2020

View reviewed changes

mergify bot removed the needs: review label Dec 23, 2020

paul-szczepanek-arm added 2 commits January 4, 2021 17:11

copy authorsation callbacks to gattserver

cdc8845

correct the documentation for GattServer::addService

5e99ea8

paul-szczepanek-arm force-pushed the gatt-copy-callbacks branch from 3ddab77 to 5e99ea8 Compare January 4, 2021 17:11

0xc0170 added needs: CI and removed needs: work labels Jan 5, 2021

0xc0170 approved these changes Jan 5, 2021

View reviewed changes

0xc0170 merged commit ce825bc into ARMmbed:master Jan 5, 2021

mergify bot added ready for merge and removed needs: CI ready for merge labels Jan 5, 2021

mbedmain added release-version: 6.7.0 Release-pending and removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BLE: Fix GattServer callbacks not being copied to GattServer instance of the service #14084

BLE: Fix GattServer callbacks not being copied to GattServer instance of the service #14084

paul-szczepanek-arm commented Dec 23, 2020

paul-szczepanek-arm Dec 23, 2020

ithinuel left a comment

ithinuel Dec 23, 2020

ithinuel Dec 23, 2020

ciarmcom commented Dec 23, 2020

adbridge commented Dec 23, 2020

mbed-ci commented Dec 23, 2020

paul-szczepanek-arm commented Jan 4, 2021

0xc0170 commented Jan 4, 2021

paul-szczepanek-arm commented Jan 4, 2021

0xc0170 commented Jan 4, 2021

mbed-ci commented Jan 4, 2021

paul-szczepanek-arm commented Jan 4, 2021

jamesbeyond commented Jan 5, 2021

paul-szczepanek-arm commented Jan 5, 2021

0xc0170 commented Jan 5, 2021

mbed-ci commented Jan 5, 2021

	if (auth_cb && auth_cb->read_cb) {
	if ((auth_cb != NULL) && (auth_cb->read_cb != NULL)) {

BLE: Fix GattServer callbacks not being copied to GattServer instance of the service #14084

BLE: Fix GattServer callbacks not being copied to GattServer instance of the service #14084

Conversation

paul-szczepanek-arm commented Dec 23, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

paul-szczepanek-arm Dec 23, 2020

Choose a reason for hiding this comment

ithinuel left a comment

Choose a reason for hiding this comment

ithinuel Dec 23, 2020

Choose a reason for hiding this comment

ithinuel Dec 23, 2020

Choose a reason for hiding this comment

ciarmcom commented Dec 23, 2020

adbridge commented Dec 23, 2020

mbed-ci commented Dec 23, 2020

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

paul-szczepanek-arm commented Jan 4, 2021

0xc0170 commented Jan 4, 2021

paul-szczepanek-arm commented Jan 4, 2021

0xc0170 commented Jan 4, 2021

mbed-ci commented Jan 4, 2021

Jenkins CI Test : ❌ FAILED

Build Number: 2 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

paul-szczepanek-arm commented Jan 4, 2021

jamesbeyond commented Jan 5, 2021

paul-szczepanek-arm commented Jan 5, 2021

0xc0170 commented Jan 5, 2021

mbed-ci commented Jan 5, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 3 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts