RTL872x-based platforms: supports BLE GATT client, BLE central role and pairing APIs #2542

XuGuohui · 2022-09-23T12:38:52Z

As the title describes.

Features

BLE GATT client APIs
BLE central role APIs
BLE pairing APIs
Set desired ATT MTU

Improvements

GATT server may lose data if characteristic's property is "INDICATE".
Reliable scanning and broadcasting states

Steps to Test

tests/wiring/ble_central_peripheral
tests/wiring/ble_scanner_broadcaster
tests/wiring/no_fixture_ble

References

N/A

Known issues

Failed to get BLE security level
BLE behaves lazily if Wi-Fi is connected due to Wi-Fi/BLE co-existence.

Completeness

User is totes amazing for contributing!
Contributor has signed CLA (Info here)
Problem and Solution clearly stated
Run unit/integration/application tests on device
Added documentation
Added to CHANGELOG.md after merging (add links to docs and issues)

avtolstoy · 2022-09-27T12:48:16Z

user/tests/wiring/ble_scanner_broadcaster/ble_broadcaster/broadcaster.cpp

@@ -52,6 +51,7 @@ test(BLE_00_Prepare) {
    assertTrue(waitFor(getBleTestPeer().isValid, 60 * 1000));
 #endif // PARTICLE_TEST_RUNNER
 }
+#endif // HAL_PLATFORM_NRF52840

 #if HAL_PLATFORM_NRF52840


Is this supposed to be still be disabled for P2?

Yes. The scan API will finally timeout taking the scan semaphore on P2 and it doesn't rely BLE event to give the semaphore. This test case is to simulate the situation that nRF based platforms fail to give the scan semaphore due to missing specific expected BLE events.

avtolstoy · 2022-09-27T12:49:02Z

wiring/src/spark_wiring_ble.cpp

    {
        WiringBleLock lk;
+        int ret = hal_ble_gap_connect(&connCfg, &impl()->connHandle(), nullptr);


Any effects on Gen 3?

We should revert this change, since we have increased the BLE event thread priority to avoid some race conditions. The change here may cause dead lock on Gen3 if there is asynchronous BLE event generated and to be propagated prior to connected event.

I recall that this change was going to fix the race condition that hal_ble_gap_connect() has exited, however, the peer device hasn't been added to wiring cache and a new BLE event is propagated, which results in peer device not found in the wiring event callback. So a new fix is to propagate the connected event to wiring layer followed by adding the peer device to cache, but not notifying (or maybe we can?) user application if user initiate the connection establishedment. @avtolstoy

Can you elaborate on the

but not notifying (or maybe we can?) user application if user initiate the connection establishedment.

?

Currently the BLE.connect() API is designed to not generate connected event for application hook, because this API is a blocking API. So to iron out the race condition as I mentioned above, we can change the current scheme.

Okay, I see. What are the other events that are propagated from HAL to wiring right after hal_ble_gap_connect() ?

E.g. BLE pairing request. It may be generated pretty fast if peer device send the request on connected. I have experienced this many times during the fixture test.

Okay, in that case I don't see a way of resolving this unless we switch over to a CONNECTED event to fill in the necessary data in wiring directly from BLE stack events. So, let's add it. Perhaps to avoid breaking Gen 3 stuff, add a feature flag for now for rtl872x platforms specifically and have this behavior only on those for now until we have a bit more time.

avtolstoy · 2022-09-27T12:49:32Z

user/tests/wiring/ble_central_peripheral/ble_peripheral/peripheral.cpp

+#elif HAL_PLATFORM_RTL872X // RTL872x doesn't support rejecting legacy pairing request
+        assertTrue(BLE.isPaired(peer));
+#else
+        // TODO


Nit: #error Unsupported platform

avtolstoy · 2022-09-27T13:41:54Z

wiring/inc/spark_wiring_ble.h

@@ -167,7 +167,7 @@ enum class BlePairingIoCaps : uint8_t {
 enum class BlePairingAlgorithm : uint8_t {
    AUTO = BLE_PAIRING_ALGORITHM_AUTO,
    LEGACY_ONLY = BLE_PAIRING_ALGORITHM_LEGACY_ONLY,
-    LESC_ONLY = BLE_PAIRING_ALGORITHM_LESC_ONLY
+    LESC_ONLY = BLE_PAIRING_ALGORITHM_LESC_ONLY  // nRF52840-based platforms only


This should be supported with GAP_AUTHEN_BIT_SC_ONLY_FLAG, right?

I've added this in https://github.com/particle-iot/device-os/tree/feature%2Fp2_ble_central-v2, feel free to pull-in if necessary.

avtolstoy · 2022-09-27T14:42:27Z

@XuGuohui Please rebase, this PR is quite out of date with develop.

avtolstoy · 2022-09-27T21:46:28Z

hal/src/rtl872x/ble_hal.cpp

+        // Prefer LESC
+        authFlags = GAP_AUTHEN_BIT_SC_FLAG;
+    } else if (pairingConfig_.algorithm == BLE_PAIRING_ALGORITHM_LESC_ONLY) {
+        authFlags = GAP_AUTHEN_BIT_SC_ONLY_FLAG;


Needs to be GAP_AUTHEN_BIT_SC_ONLY_FLAG | GAP_AUTHEN_BIT_FLAG. I've tested in both roles, works fine.

avtolstoy · 2022-09-27T21:47:07Z

hal/src/rtl872x/ble_hal.cpp

+            BleConnection connection = {};
+            connection.info.version = BLE_API_VERSION;
+            connection.info.size = sizeof(hal_ble_conn_info_t);
+            if (addressEqual(connectingAddr_, peerAddr)) {


This check seems to be failing for me. In central role I'm getting Connected as Peripheral in the log and device crashes afterwards.

avtolstoy · 2022-09-29T00:23:16Z

This should now be passing all the tests without issues in both roles. @XuGuohui and @avtolstoy to re-review the latest commits before merging.

avtolstoy · 2022-09-29T00:26:20Z

user/tests/wiring/ble_central_peripheral/ble_central/central.cpp

@@ -445,11 +448,14 @@ static void pairingTestRoutine(bool request, BlePairingAlgorithm algorithm,
        assertTrue(waitFor([&]{ return !BLE.isPairing(peer); }, 20000));
        assertTrue(BLE.isPaired(peer));
        assertEqual(pairingStatus, (int)SYSTEM_ERROR_NONE);
+// FIXME: le_bond_get_sec_level() failed to retrieve the security level
+#if !HAL_PLATFORM_RTL872X


This is the only issue that still needs to be resolved.

FAE just send mee a patch that has exposed an API to get the security level even if connnection is not bonded.

… data.

…ill overflow. minor fix in ble

…enerated late after some other events

…g iocaps

XuGuohui requested review from avtolstoy and YutingYou September 23, 2022 12:38

avtolstoy reviewed Sep 27, 2022

View reviewed changes

XuGuohui force-pushed the feature/p2_ble_central branch from f607323 to 5fbf565 Compare September 27, 2022 16:49

scott-brust added this to the 5.1.0 milestone Sep 27, 2022

scott-brust added the 5.x label Sep 27, 2022

avtolstoy requested changes Sep 27, 2022

View reviewed changes

avtolstoy force-pushed the feature/p2_ble_central branch from 5e54322 to 9cb1bb7 Compare September 28, 2022 21:27

avtolstoy reviewed Sep 29, 2022

View reviewed changes

XuGuohui and others added 19 commits September 30, 2022 00:52

[rtl872x] hal: makes BLE connections extendable.

2958a65

[rtl872x] hal: implement APIs for getting/setting BLE PPCP.

34b7832

[rtl872x] hal: implements connection related APIs.

344ea64

[rtl872x] hal: implement GATT client related APIs.

cec451f

[rtl872x] hal: fix the issue that BLE GATT server indication may lose…

ab300c1

… data.

[rtl872x] hal: implement BLE pairing APIs

fd8748b

[rtl872x] hal: give BLE connection semaphore only when MTU is updated

161e0eb

[rtl872x] ble: add comments

2be8a0a

[rtl872x] hal: BLE peripheral and broadcaster concurrent roles.

70da17c

[rtl872x] BLE: fixes some race conditions and makes some improvements.

7393c8d

[rtl872x] ble: coexistence workaround, minor LESC stuff

38d50f3

[rtl872x] ble: minor LESC_ONLY fix

12957a9

[rtl872x] fix BLE race conditions.

d6c7949

[test] enable tests of setting BLE address for P2.

2e55fed

[rtl872x] timer thread should have max priority otherwise its queue w…

cacabb3

…ill overflow. minor fix in ble

[rtl872x] ble: fix race condition with GAP_MSG_LE_CONN_STATE_CHANGE g…

4466bd8

…enerated late after some other events

[rtl872x] ble: thread safety for connections_ array

2472a56

[test] wiring/ble_central_peripheral/ble_central: delay when switchin…

7bc980d

…g iocaps

[rtl872x] introduce a new SDK API to get pairing procedure type.

0b37089

keeramis and others added 2 commits September 30, 2022 00:52

Set size to get ppcp params

2fb6d65

[rtl872x] ble: workaround for a BT stack race condition

a002f91

avtolstoy force-pushed the feature/p2_ble_central branch from 5647a51 to a002f91 Compare September 29, 2022 17:54

avtolstoy approved these changes Sep 29, 2022

View reviewed changes

avtolstoy merged commit b64994e into develop Sep 29, 2022

avtolstoy deleted the feature/p2_ble_central branch September 29, 2022 17:55

scott-brust mentioned this pull request Sep 29, 2022

Device OS Release 5.1.0 #2547

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RTL872x-based platforms: supports BLE GATT client, BLE central role and pairing APIs #2542

RTL872x-based platforms: supports BLE GATT client, BLE central role and pairing APIs #2542

XuGuohui commented Sep 23, 2022 •

edited

Loading

avtolstoy Sep 27, 2022

XuGuohui Sep 27, 2022

avtolstoy Sep 27, 2022

XuGuohui Sep 27, 2022

XuGuohui Sep 27, 2022

avtolstoy Sep 27, 2022

XuGuohui Sep 27, 2022

avtolstoy Sep 27, 2022

XuGuohui Sep 27, 2022

avtolstoy Sep 27, 2022

XuGuohui Sep 27, 2022

avtolstoy Sep 27, 2022

avtolstoy Sep 27, 2022

avtolstoy Sep 27, 2022

avtolstoy commented Sep 27, 2022

avtolstoy Sep 27, 2022

avtolstoy Sep 27, 2022

avtolstoy Sep 27, 2022

avtolstoy commented Sep 29, 2022

avtolstoy Sep 29, 2022

XuGuohui Sep 29, 2022

avtolstoy Sep 29, 2022

RTL872x-based platforms: supports BLE GATT client, BLE central role and pairing APIs #2542

RTL872x-based platforms: supports BLE GATT client, BLE central role and pairing APIs #2542

Conversation

XuGuohui commented Sep 23, 2022 • edited Loading

Features

Improvements

Steps to Test

References

Known issues

Completeness

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avtolstoy commented Sep 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avtolstoy commented Sep 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XuGuohui commented Sep 23, 2022 •

edited

Loading