Ble extended advertising fixes #8998

pan- · 2018-12-07T00:26:04Z

Description

Second round of fixes discovered with the OOB and our testing.

It fixes: ARMmbed/mbed-os-example-ble#198 and many other bug discovered during our internal testing on the Cordio LL.

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

While good this change breaks some old application that were adding GATT services before the initialisation of BLE. This patch revert temporarily this change for now

Previously, we were passing the event_properties as defined by the Bluetooth spec which is not what DmAdvConfig expect as the advertising type passed to DmAdvConfig is Cordio tailored (and incomplete).

pan- · 2018-12-07T00:27:51Z

@ARMmbed/mbed-os-maintainers Fix for ARMmbed/mbed-os-example-ble#200 will be included in that branch.

cmonr

LGTM, just a couple of questions.

cmonr · 2018-12-07T03:10:03Z

features/FEATURE_BLE/ble/gap/Events.h

@@ -63,7 +63,7 @@ struct AdvertisingReportEvent {
        advertising_sid_t SID,
        advertising_power_t txPower,
        rssi_t rssi,
-        periodic_interval_t periodicInterval,
+        uint16_t periodicInterval,


Odd. Why not replace/update the constructor instead?

As you can see, that constructor is not part of the documentation because it is not meant to be used by user code. We cannot use friend in this case because friend cannot be inherited and we don't know the vendor file that will use it. There is no reason for us to keep non usable constructor if it is not part of the public API.

To give a bit more context, the periodic interval returned in an advertising event can be equal to 0 if there is no periodic interval or a value comprised between [0x0006 : 0xFFFF]. The type periodic_interval_t is not able to represent the absence of value as it normalizes values passed in input.

cmonr · 2018-12-07T03:12:41Z

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

        DmScanStart(
            scanning_phys.value(),
            DM_DISC_MODE_NONE,
            extended_scan_type,
-            filter_duplicates.value(), // TODO: cordio API incomplete ???
+            filter_duplicates.value(),


Any more TODOs to update? 😜

I'm not sure why I removed it, the comment still stands 😆 . These TODO's and FIXME are important in our implementations and internal files when we work with APIs that do not implement all Bluetooth requirements or when we deliver subsets of BT features.

donatieng

Looks good to me!

donatieng · 2018-12-07T11:48:16Z

...s/FEATURE_BLE/targets/TARGET_CORDIO/stack/cordio_stack/ble-host/sources/stack/dm/dm_adv_ae.c

  {
    pMsg->hdr.event = DM_ADV_PER_MSG_API_SET_DATA;
    pMsg->advHandle = advHandle;
    pMsg->op = op;
    pMsg->len = len;
-    pMsg->pData = pData;
+    memcpy(pMsg->pData, pData, len);


Let's make sure we upstream this

adbridge · 2018-12-07T11:51:45Z

@cmonr are you happy with Vincent's responses ? We should get this into ci anyway I think

…et sequence.

The number of advertising sets supported is the minimum of advertising sets supported beween the host and the controller.

adbridge · 2018-12-07T15:50:24Z

CI started

adbridge · 2018-12-07T16:08:39Z

CI stopped again awaiting Vincent's last fixes

mbed-ci · 2018-12-07T16:11:11Z

Test run: FAILED

Summary: 6 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-IAR
jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_mbed2-build-ARM
jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
jenkins-ci/mbed-os-ci_mbed2-build-IAR

adbridge · 2018-12-07T20:03:36Z

@donatieng could you please re-review

pan- · 2018-12-07T20:06:38Z

@adbridge There's an issue with the libs generated, they will be re generated shortly.

cmonr · 2018-12-07T20:47:49Z

@pan- Ping @ARMmbed/mbed-os-maintainers when this is ready for CI again.

0xc0170 · 2018-12-10T08:09:17Z

CI started

0xc0170 · 2018-12-10T10:48:20Z

CI aborted, will wait for @ARMmbed/mbed-os-pan to confirm this is ready for CI (noticed the last commit after CI started)

paul-szczepanek-arm · 2018-12-10T10:55:18Z

sorry, still not ready we're have to figure out the ARM compiler problem first

mbed-ci · 2018-12-10T11:05:20Z

Test run: FAILED

Summary: 2 of 11 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test
jenkins-ci/mbed-os-ci_exporter

paul-szczepanek-arm · 2018-12-10T13:05:09Z

ARM cc fix incoming within the hour

paul-szczepanek-arm · 2018-12-10T13:47:53Z

wait

paul-szczepanek-arm · 2018-12-10T13:55:17Z

good to go

0xc0170 · 2018-12-10T14:11:38Z

CI started

mbed-ci · 2018-12-10T16:32:30Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 3
Build artifacts

kjbracey · 2018-12-12T13:34:24Z

What actually was the problem here? Compiler deducing that ble_cordio_get_hci_driver didn't return by looking at the weak version?

Was initially puzzled because assert failures are also noreturn, but then this assert is true so doesn't do anything.

I would have thought the code should be issuing an error in the calling code if ble_cordio_get_hci_driver returns NULL (whether it is because it fell through to the weak code, or because a real implementation had some other init problem). That's the pattern we've used in all the get_default_instance patterns - any base weak version is just a plain "return NULL".

pan- · 2018-12-12T13:50:44Z

What actually was the problem here? Compiler deducing that ble_cordio_get_hci_driver didn't return by looking at the weak version?

Yes, a straight call to error seems to taint the weak function as a no return function and that attributes propagates seems to be propagated by the linker to the non weak version. More investigation would be required to find out the issue details but we didn't had the time to conduct that full investigation as we were urged to find a solution.

As you pointed out, the assert statement in the weak function is not correct as it always return true. The pattern you described seems suited for our use and may be a long term fix (not sure of the behaviour if the weak factory and the non weak accessor are in the same source file ...).

kjbracey · 2018-12-12T14:00:36Z

Your pattern is almost totally equivalent to what we've done in networking and other areas, so it would be nice to align it.

After a couple of slightly varying iterations (1 layer/2 layer, pointer/reference), the final refined form I would suggest for you is

MBED_WEAK static BLEInstanceBase *BLEInstanceBase::get_default_instance()
{
     return get_target_default_instance();
}

MBED_WEAK static BLEInstanceBase *BLEInstanceBase::get_target_default_instance()
{
     return NULL;
}

Code gated on TARGET_XXX (only) gets to override the get_target_default_instance().

Application or plug-in module code has the option of overriding get_default_instance(), and the extra layer ensures that whatever the target is doing doesn't interfere with that. A library acting as a module plugin driver like ESP8266 would provide a JSON option xxx.provide-default to do that override.

pan- and others added 11 commits December 6, 2018 11:21

BLE: Revert change in stack setup initialisation.

d9d4a21

While good this change breaks some old application that were adding GATT services before the initialisation of BLE. This patch revert temporarily this change for now

BLE: Fix conversion of advertising type

0543442

Previously, we were passing the event_properties as defined by the Bluetooth spec which is not what DmAdvConfig expect as the advertising type passed to DmAdvConfig is Cordio tailored (and incomplete).

BLE: Set advertising random address when appropriate.

c83dccf

fix return value

d372f16

check return value of DmSyncStart

58c7c38

fix swapped errors

5735456

always set filter policy

22a117a

copy periodic payload

df443c2

BLE: Allow null value for periodic interval in advertising report event.

df95a1f

BLE: Fix address type allowed to create a periodic sync.

698447b

BLE: Fix recursion in ble::advertising_data_status_t raw constructor.

1c71713

pan- requested a review from donatieng December 7, 2018 00:28

cmonr added needs: review release-version: 5.11.0-rc3 labels Dec 7, 2018

cmonr reviewed Dec 7, 2018

View reviewed changes

BLE: Revert changes introduced by debugging.

0d398bc

donatieng approved these changes Dec 7, 2018

View reviewed changes

adbridge added needs: CI and removed needs: review labels Dec 7, 2018

pan- added 5 commits December 7, 2018 12:33

BLE: Add an option to inject the random static address during the res…

a36b04f

…et sequence.

BLE: Inject random static address during reset sequence.

20e0cd1

BLE: Set the number of supported phy by the host to 3.

57b79d9

BLE: Set the number of the advertising sets supported by the host to 3.

4e5240b

BLE: Fix the number of advertising sets supported

e7f81fe

The number of advertising sets supported is the minimum of advertising sets supported beween the host and the controller.

baseband clock rate increased to 1 000 000

c13dcf3

cmonr added needs: work and removed needs: CI labels Dec 7, 2018

Donatien Garnier added 4 commits December 7, 2018 22:04

Remove duplicate symbols in libcordio_stack.a

674ff28

Remove use of GPIOs (LEDs and diag pins) from Cordio LL for Nordic

3f00595

ARMCC only likes armar

db6b09a

Replace ARMCC libs

6f94339

working libs for GCC and IAR

642b2df

paul-szczepanek-arm added 2 commits December 10, 2018 13:46

working ARM lib

97df8f5

fixed ARM compilation problem caused by noreturn

c998287

remove noreturn error function to avoid compilation issues

a545da7

0xc0170 approved these changes Dec 10, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Dec 10, 2018

cmonr merged commit 23022dd into ARMmbed:master Dec 10, 2018

cmonr removed the needs: CI label Dec 10, 2018

Ble extended advertising fixes #8998

Ble extended advertising fixes #8998

Uh oh!

Conversation

pan- commented Dec 7, 2018

Description

Pull request type

Uh oh!

pan- commented Dec 7, 2018

Uh oh!

cmonr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

donatieng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbridge commented Dec 7, 2018

Uh oh!

adbridge commented Dec 7, 2018

Uh oh!

adbridge commented Dec 7, 2018

Uh oh!

mbed-ci commented Dec 7, 2018

Uh oh!

adbridge commented Dec 7, 2018

Uh oh!

pan- commented Dec 7, 2018

Uh oh!

cmonr commented Dec 7, 2018

Uh oh!

0xc0170 commented Dec 10, 2018

Uh oh!

0xc0170 commented Dec 10, 2018

Uh oh!

paul-szczepanek-arm commented Dec 10, 2018

Uh oh!

mbed-ci commented Dec 10, 2018

Uh oh!

paul-szczepanek-arm commented Dec 10, 2018

Uh oh!

paul-szczepanek-arm commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paul-szczepanek-arm commented Dec 10, 2018

Uh oh!

0xc0170 commented Dec 10, 2018

Uh oh!

mbed-ci commented Dec 10, 2018

Uh oh!

kjbracey commented Dec 12, 2018

Uh oh!

pan- commented Dec 12, 2018

Uh oh!

kjbracey commented Dec 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

paul-szczepanek-arm commented Dec 10, 2018 •

edited

Loading

kjbracey commented Dec 12, 2018 •

edited

Loading