Merge pull request #14672 from paul-szczepanek-arm/fix-advertising-start

BLE: Fix advertising start and stop

connectivity/FEATURE_BLE/include/ble/Gap.h

@@ -747,7 +747,9 @@ public:
      * @param handle Advertising set handle.
      * @param maxDuration Max duration for advertising (in units of 10ms) - 0 means no limit.
      * @param maxEvents Max number of events produced during advertising - 0 means no limit.
-     * @return BLE_ERROR_NONE on success.
+     * @return BLE_ERROR_NONE on success. This does not guarantee the set has started if
+     * extended advertising is enabled. Register an event handler and wait for onAdvertisingStart
+     * event. An (unlikely) failed start the status of the event will contain an error.
      *
      * @see EventHandler::onAdvertisingStart when the advertising starts.
      * @see EventHandler::onScanRequestReceived when a scan request is received.
@@ -765,7 +767,9 @@ public:
      *  which will not be affected.
      *
      * @param handle Advertising set handle.
-     * @return BLE_ERROR_NONE on success.
+     * @return BLE_ERROR_NONE on success. For extented advertising this does not guarantee
+     * the set is stopped if. Register an event handler and wait for onAdvertisingEnd event.
+     * An (unlikely) failed stop the event status will contain the error code.
      */
     ble_error_t stopAdvertising(advertising_handle_t handle);
 

connectivity/FEATURE_BLE/include/ble/gap/Events.h

@@ -586,9 +586,10 @@ struct AdvertisingStartEvent {
     /** Create an advertising start event.
      *
      * @param advHandle Advertising set handle.
+     * @param status Advertising set start command status.
      */
-    AdvertisingStartEvent(advertising_handle_t advHandle) :
+    AdvertisingStartEvent(advertising_handle_t advHandle, ble_error_t status = BLE_ERROR_NONE) :
-        advHandle(advHandle)
+        advHandle(advHandle), status(status)
     {
     }
 
@@ -600,8 +601,15 @@ struct AdvertisingStartEvent {
         return advHandle;
     }
 
+    /** Get status of operation. */
+    ble_error_t getStatus() const
+    {
+        return status;
+    }
+
 private:
     advertising_handle_t advHandle;
+    ble_error_t status;
 };
 
 /**
@@ -610,7 +618,8 @@ private:
  * @see ble::Gap::EventHandler::onAdvertisingEnd().
  *
  * @note The connection handle, connected flag and completed_event fields are
- * valid if the flag legacy is not set to true.
+ * valid if the flag legacy is not set to true. If status is different from BLE_ERROR_NONE
+ * the completed_events field is not valid and the set may still be active.
  */
 struct AdvertisingEndEvent {
 #if !defined(DOXYGEN_ONLY)
@@ -621,18 +630,21 @@ struct AdvertisingEndEvent {
      * @param connection Connection handle.
      * @param completed_events Number of events created during before advertising end.
      * @param connected True if connection has been established.
+     * @param status Error code if stop command failed.
      */
     AdvertisingEndEvent(
         advertising_handle_t advHandle,
         connection_handle_t connection,
         uint8_t completed_events,
-        bool connected
+        bool connected,
+        ble_error_t status = BLE_ERROR_NONE
     ) :
         advHandle(advHandle),
         connection(connection),
         completed_events(completed_events),
         connected(connected),
-        legacy(false)
+        legacy(false),
+        status(status)
     {
     }
 
@@ -643,7 +655,8 @@ struct AdvertisingEndEvent {
         connection(),
         completed_events(0),
         connected(false),
-        legacy(true)
+        legacy(true),
+        status(BLE_ERROR_NONE)
     {
     }
 
@@ -683,12 +696,20 @@ struct AdvertisingEndEvent {
         return legacy;
     }
 
+    /** Get the result of the stop advertising event. If the status is not BLE_ERROR_NONE the set
+     * may still be active. */
+    ble_error_t getStatus() const
+    {
+        return status;
+    }
+
 private:
     advertising_handle_t advHandle;
     connection_handle_t connection;
     uint8_t completed_events;
     bool connected;
     bool legacy;
+    ble_error_t status;
 };
 
 /**

connectivity/FEATURE_BLE/mbed_lib.json

@@ -108,6 +108,11 @@
             "value": 16,
             "macro_name": "BLE_GAP_HOST_PRIVACY_RESOLVED_CACHE_SIZE"
         },
+        "ble-gap-host-max-advertising-start-commands": {
+            "help": "There can only be one outstanding advertising set command sent to the controller. This determines how many advertising set start commands can be queued on the host. Must be between 1 and BLE_GAP_MAX_ADVERTISING_SETS. If privacy is used, this must be at equal or higher than then number of simultaneously active sets with a private address.",
+            "value": 4,
+            "macro_name": "BLE_GAP_HOST_MAX_OUTSTANDING_ADVERTISING_START_COMMANDS"
+        },
 
         "ble-passkey-display-reversed-digits-deprecation": {
           "help": "This option is part of the deprecation process. Set this to false to remove the deprecation notice. When set to true, the digits in the SecurityManager passkeyDiplay event are reversed. When set to false the digits are in the correct order.",

connectivity/FEATURE_BLE/source/generic/GapImpl.cpp

@@ -373,6 +373,9 @@ Gap::Gap(
     , _scan_parameters_set(false)
 #endif // BLE_ROLE_OBSERVER
 {
+#if BLE_FEATURE_EXTENDED_ADVERTISING
+    _advertising_enable_command_params.number_of_handles = 0;
+#endif //BLE_FEATURE_EXTENDED_ADVERTISING
     _pal_gap.initialize();
 
     _pal_gap.when_gap_event_received(
@@ -1212,8 +1215,7 @@ ble_error_t Gap::reset()
 
     _event_handler = nullptr;
     _initiating = false;
-    set_scan_state(ScanState::idle);
+
-    _scan_requested = false;
 #if BLE_FEATURE_PRIVACY
     _privacy_initialization_pending = false;
 #if BLE_GAP_HOST_BASED_PRIVATE_ADDRESS_RESOLUTION
@@ -1224,50 +1226,27 @@ ble_error_t Gap::reset()
 #endif // BLE_FEATURE_PRIVACY
 
 #if BLE_ROLE_OBSERVER
+    set_scan_state(ScanState::idle);
+    _scan_requested = false;
     _scan_parameters_set = false;
     _scan_timeout.detach();
 #endif
 
 #if BLE_ROLE_BROADCASTER
-#if BLE_FEATURE_EXTENDED_ADVERTISING
-    if (is_extended_advertising_available()) {
-        /* stop all advertising sets */
-        for (size_t i = 0; i < BLE_GAP_MAX_ADVERTISING_SETS; ++i) {
-            if (_active_sets.get(i)) {
-                _pal_gap.extended_advertising_enable(
-                    /* enable */ false,
-                    /* number of advertising sets */ 1,
-                    (advertising_handle_t *) &i,
-                    nullptr,
-                    nullptr
-                );
-            }
-#if BLE_FEATURE_PERIODIC_ADVERTISING
-            if (_active_periodic_sets.get(i)) {
-                _pal_gap.periodic_advertising_enable(
-                    /* enable */ false,
-                    (advertising_handle_t) i
-                );
-            }
-            _active_periodic_sets.clear();
-#endif // BLE_FEATURE_PERIODIC_ADVERTISING
-        }
-
-        /* clear state of all advertising sets */
-        _existing_sets.clear();
-
     /* clear advertising set data on the controller */
     _pal_gap.clear_advertising_sets();
-    } else
+#if BLE_FEATURE_EXTENDED_ADVERTISING
-#else // BLE_FEATURE_EXTENDED_ADVERTISING
+    /* reset pending advertising sets */
-    {
+    _advertising_enable_command_params.number_of_handles = 0;
-        if (_active_sets.get(LEGACY_ADVERTISING_HANDLE)) {
+    _process_enable_queue_pending = false;
-            _pal_gap.advertising_enable(false);
+    _process_disable_queue_pending = false;
-        }
+    _existing_sets.clear();
-    }
+#if BLE_FEATURE_PERIODIC_ADVERTISING
+    _active_periodic_sets.clear();
+#endif // BLE_FEATURE_PERIODIC_ADVERTISING
 #endif // BLE_FEATURE_EXTENDED_ADVERTISING
-
     _active_sets.clear();
+    _pending_stop_sets.clear();
     _pending_sets.clear();
     _address_refresh_sets.clear();
     _interruptible_sets.clear();
@@ -2413,22 +2392,30 @@ ble_error_t Gap::startAdvertising(
             _pal_gap.set_advertising_set_random_address(handle, *random_address);
         }
 
-        error = _pal_gap.extended_advertising_enable(
+        /* check we haven't run out of space to remember parameters */
-            /* enable */ true,
+        if (_advertising_enable_command_params.number_of_handles == BLE_GAP_HOST_MAX_OUTSTANDING_ADVERTISING_START_COMMANDS) {
-            /* number of advertising sets */ 1,
+            /* try to process early */
-            &handle,
+            tr_debug("Processing enable queue early as run out of queue space");
-            maxDuration.storage(),
+            process_enable_queue();
-            &maxEvents
+            /* if the processing didn't clear the handles we cannot continue */
-        );
+            if (_advertising_enable_command_params.number_of_handles) {
-
+                tr_warn("Cannot enable set as no memory to record the parameters");
-        if (error) {
+                return BLE_ERROR_NO_MEM;
-            return error;
+            }
         }
 
-        if (maxDuration.value() || maxEvents) {
+        /* remember the parameters that will be enabled in the delayed processing */
-            _interruptible_sets.clear(handle);
+        const uint8_t i = _advertising_enable_command_params.number_of_handles;
-        } else {
+        _advertising_enable_command_params.handles[i] = handle;
-            _interruptible_sets.set(handle);
+        _advertising_enable_command_params.max_durations[i] = maxDuration;
+        _advertising_enable_command_params.max_events[i] = maxEvents;
+        _advertising_enable_command_params.number_of_handles++;
+
+        /* we delay the processing to gather as many calls as we can in one go */
+        if (!_process_enable_queue_pending) {
+            _process_enable_queue_pending = _event_queue.post([this] {
+                process_enable_queue();
+            });
         }
 
     } else
@@ -2439,12 +2426,14 @@ ble_error_t Gap::startAdvertising(
             return BLE_ERROR_INVALID_PARAM;
         }
 
+#if BLE_ROLE_OBSERVER
         // Address can be updated if the device is not scanning or advertising
         if ((_scan_state == ScanState::idle) && !_active_sets.get(LEGACY_ADVERTISING_HANDLE)) {
             _pal_gap.set_random_address(*random_address);
         } else {
             tr_error("could not update address, device scanning/advertising");
         }
+#endif // BLE_ROLE_OBSERVER
 
         error = _pal_gap.advertising_enable(true);
         if (error) {
@@ -2461,14 +2450,65 @@ ble_error_t Gap::startAdvertising(
         }
 
         _interruptible_sets.set(LEGACY_ADVERTISING_HANDLE);
-    }
 
         _pending_sets.set(handle);
+    }
 
     return error;
 }
 #endif
 
+#if BLE_FEATURE_EXTENDED_ADVERTISING
+void Gap::process_enable_queue()
+{
+    tr_info("Evaluating pending advertising sets to be started");
+    if (!_advertising_enable_command_params.number_of_handles) {
+        /* no set pending to be enabled */
+        tr_debug("No sets to be enabled");
+        return;
+    }
+
+    for (size_t i = 0; i < BLE_GAP_MAX_ADVERTISING_SETS; ++i) {
+        if (_pending_sets.get(i)) {
+            /* we have to wait until nothing is pending */
+            tr_info("Cannot enable sets as pending sets present, will retry later");
+            return;
+        }
+    }
+
+    ble_error_t error = _pal_gap.extended_advertising_enable(
+        /* enable */ true,
+        _advertising_enable_command_params.number_of_handles,
+        _advertising_enable_command_params.handles,
+        (uint16_t*)&_advertising_enable_command_params.max_durations,
+        _advertising_enable_command_params.max_events
+    );
+
+    if (error) {
+        tr_error("Failed to start advertising set with error: %s", to_string(error));
+        if (_event_handler) {
+            for (size_t i = 0; i < _advertising_enable_command_params.number_of_handles; ++i) {
+                _pending_sets.clear(_advertising_enable_command_params.handles[i]);
+                _event_handler->onAdvertisingStart(
+                    AdvertisingStartEvent(_advertising_enable_command_params.handles[i], error)
+                );
+            }
+        }
+    } else {
+        for (size_t i = 0; i < _advertising_enable_command_params.number_of_handles; ++i) {
+            if (_advertising_enable_command_params.max_durations[i].value() || _advertising_enable_command_params.max_events[i]) {
+                _interruptible_sets.clear(_advertising_enable_command_params.handles[i]);
+            } else {
+                _interruptible_sets.set(_advertising_enable_command_params.handles[i]);
+            }
+            _pending_sets.set(_advertising_enable_command_params.handles[i]);
+        }
+    }
+
+    _advertising_enable_command_params.number_of_handles = 0;
+    _process_enable_queue_pending = false;
+}
+#endif //BLE_FEATURE_EXTENDED_ADVERTISING
 
 #if BLE_ROLE_BROADCASTER
 ble_error_t Gap::stopAdvertising(advertising_handle_t handle)
@@ -2499,17 +2539,16 @@ ble_error_t Gap::stopAdvertising(advertising_handle_t handle)
 
 #if BLE_FEATURE_EXTENDED_ADVERTISING
     if (is_extended_advertising_available()) {
-        status = _pal_gap.extended_advertising_enable(
+        _pending_stop_sets.set(handle);
-            /*enable ? */ false,
+        /* delay execution of command to accumulate multiple sets */
-            /* number of advertising sets */ 1,
+        if (!_process_disable_queue_pending) {
-            &handle,
+            _process_disable_queue_pending = _event_queue.post([this] {
-            nullptr,
+                process_disable_queue();
-            nullptr
+            });
-        );
-
-        if (status) {
-            return status;
         }
+
+        status = BLE_ERROR_NONE;
+
     } else
 #endif // BLE_FEATURE_EXTENDED_ADVERTISING
     {
@@ -2525,12 +2564,60 @@ ble_error_t Gap::stopAdvertising(advertising_handle_t handle)
         }
 
         _advertising_timeout.detach();
-    }
 
         _pending_sets.set(handle);
+    }
 
     return status;
 }
+
+#if BLE_FEATURE_EXTENDED_ADVERTISING
+void Gap::process_disable_queue()
+{
+    advertising_handle_t sets[BLE_GAP_MAX_ADVERTISING_SETS];
+    uint8_t number_of_handles = 0;
+    // refresh for address for all connectable advertising sets
+    for (size_t i = 0; i < BLE_GAP_MAX_ADVERTISING_SETS; ++i) {
+        if (_pending_stop_sets.get(i)) {
+            sets[number_of_handles] = i;
+            number_of_handles++;
+            _pending_stop_sets.clear(i);
+        }
+    }
+
+    if (number_of_handles) {
+        ble_error_t error = _pal_gap.extended_advertising_enable(
+            /* enable */ false,
+             number_of_handles,
+             (advertising_handle_t*)&sets,
+             nullptr,
+             nullptr
+        );
+
+        if (error) {
+            for (size_t i = 0; i < number_of_handles; ++i) {
+                _event_handler->onAdvertisingEnd(
+                    AdvertisingEndEvent(
+                        (advertising_handle_t)sets[i],
+                        0/*connection*/,
+                        0/*completed_events*/,
+                        false/*connected*/,
+                        error
+                    )
+                );
+                tr_error("Could not stop advertising set %u, error: %s", i, to_string(error));
+            }
+        } else {
+            for (size_t i = 0; i < number_of_handles; ++i) {
+                _pending_sets.set(sets[i]);
+            }
+        }
+    }
+
+    _process_disable_queue_pending = false;
+}
+#endif // BLE_FEATURE_EXTENDED_ADVERTISING
+
 #endif
 
 
@@ -3334,7 +3421,11 @@ void Gap::on_legacy_advertising_stopped()
     _pending_sets.clear(LEGACY_ADVERTISING_HANDLE);
 
     // restart advertising if it was stopped to refresh the address
-    if (_address_refresh_sets.get(LEGACY_ADVERTISING_HANDLE) && (_scan_state == ScanState::idle)) {
+    if (_address_refresh_sets.get(LEGACY_ADVERTISING_HANDLE)
+#if BLE_ROLE_OBSERVER
+        && (_scan_state == ScanState::idle)
+#endif //BLE_ROLE_OBSERVER
+    ) {
         _address_refresh_sets.clear(LEGACY_ADVERTISING_HANDLE);
         startAdvertising(LEGACY_ADVERTISING_HANDLE);
         _adv_started_from_refresh.set(LEGACY_ADVERTISING_HANDLE);
@@ -3365,6 +3456,13 @@ void Gap::on_advertising_set_started(const mbed::Span<const uint8_t>& handles)
             );
         }
     }
+
+    /* delay processing to minimise churn (if multiple events are pending that would trigger it) */
+    if (!_process_enable_queue_pending) {
+        _process_enable_queue_pending = _event_queue.post([this] {
+            process_enable_queue();
+        });
+    }
 }
 
 void Gap::on_advertising_set_terminated(
@@ -3394,6 +3492,13 @@ void Gap::on_advertising_set_terminated(
         return;
     }
 
+    /* postpone as other events may still be pending */
+    if (!_process_disable_queue_pending) {
+        _process_disable_queue_pending = _event_queue.post([this] {
+            process_disable_queue();
+        });
+    }
+
     if (!_event_handler) {
         return;
     }
@@ -4129,7 +4234,11 @@ bool Gap::is_advertising() const
 }
 
 bool Gap::is_radio_active() const {
-    return _initiating || (_scan_state != ScanState::idle) || is_advertising();
+    return _initiating ||
+#if BLE_ROLE_OBSERVER
+        (_scan_state != ScanState::idle) ||
+#endif // BLE_ROLE_OBSERVER
+        is_advertising();
 }
 
 void Gap::update_advertising_set_connectable_attribute(
@@ -4194,12 +4303,14 @@ const address_t *Gap::get_random_address(controller_operation_t operation, size_
     // it to the address to use to determine if the address is correct or not.
     if (_initiating) {
         address_in_use = &resolvable_address;
+#if BLE_ROLE_OBSERVER
     } else if (_scan_state != ScanState::idle) {
         if (central_non_resolvable) {
             address_in_use = &non_resolvable_address;
         } else {
             address_in_use = &resolvable_address;
         }
+#endif //BLE_ROLE_OBSERVER
     } else if (advertising_use_main_address && (_active_sets.get(set_id) || _pending_sets.get(set_id))) {
         if (!_set_is_connectable.get(set_id) && peripheral_non_resolvable) {
             address_in_use = &non_resolvable_address;

connectivity/FEATURE_BLE/source/generic/GapImpl.h

@@ -576,6 +576,11 @@ private:
     ~Gap();
 
 #if BLE_ROLE_BROADCASTER
+#if BLE_FEATURE_EXTENDED_ADVERTISING
+    void process_enable_queue();
+    void process_disable_queue();
+#endif // BLE_FEATURE_EXTENDED_ADVERTISING
+
     ble_error_t setAdvertisingData(
         advertising_handle_t handle,
         Span<const uint8_t> payload,
@@ -912,6 +917,7 @@ private:
 #endif // BLE_FEATURE_PRIVACY
     ble::address_t _random_static_identity_address;
 
+#if BLE_ROLE_OBSERVER
     enum class ScanState : uint8_t {
         idle,
         scan,
@@ -928,6 +934,7 @@ private:
     scan_period_t _scan_requested_period = scan_period_t(0);
 
     bool _scan_requested = false;
+#endif // BLE_ROLE_OBSERVER
 
 #if BLE_GAP_HOST_BASED_PRIVATE_ADDRESS_RESOLUTION
     enum class ConnectionToHostResolvedAddressState : uint8_t {
@@ -994,6 +1001,9 @@ private:
     };
 
     BitArray<BLE_GAP_MAX_ADVERTISING_SETS> _existing_sets;
+#if BLE_FEATURE_EXTENDED_ADVERTISING
+    BitArray<BLE_GAP_MAX_ADVERTISING_SETS> _pending_stop_sets;
+#endif // BLE_FEATURE_EXTENDED_ADVERTISING
     BitArray<BLE_GAP_MAX_ADVERTISING_SETS> _active_sets;
     BitArray<BLE_GAP_MAX_ADVERTISING_SETS> _active_periodic_sets;
     BitArray<BLE_GAP_MAX_ADVERTISING_SETS> _connectable_payload_size_exceeded;
@@ -1003,6 +1013,21 @@ private:
     BitArray<BLE_GAP_MAX_ADVERTISING_SETS> _interruptible_sets;
     BitArray<BLE_GAP_MAX_ADVERTISING_SETS> _adv_started_from_refresh;
 
+#if BLE_FEATURE_EXTENDED_ADVERTISING
+#if BLE_GAP_HOST_MAX_OUTSTANDING_ADVERTISING_START_COMMANDS < 1 || BLE_GAP_HOST_MAX_OUTSTANDING_ADVERTISING_START_COMMANDS > BLE_GAP_MAX_ADVERTISING_SETS
+#error "BLE_GAP_HOST_MAX_OUTSTANDING_ADVERTISING_START_COMMANDS must be at least 1 and not bigger than BLE_GAP_MAX_ADVERTISING_SETS"
+#endif
+    struct AdvertisingEnableCommandParams_t {
+        adv_duration_t max_durations[BLE_GAP_HOST_MAX_OUTSTANDING_ADVERTISING_START_COMMANDS];
+        advertising_handle_t handles[BLE_GAP_HOST_MAX_OUTSTANDING_ADVERTISING_START_COMMANDS];
+        uint8_t max_events[BLE_GAP_HOST_MAX_OUTSTANDING_ADVERTISING_START_COMMANDS];
+        uint8_t number_of_handles;
+    };
+
+    AdvertisingEnableCommandParams_t _advertising_enable_command_params;
+    bool _process_enable_queue_pending = false;
+    bool _process_disable_queue_pending = false;
+#endif // BLE_FEATURE_EXTENDED_ADVERTISING
 
     bool _user_manage_connection_parameter_requests;
 #if BLE_ROLE_OBSERVER

connectivity/FEATURE_BLE/source/pal/PalEventQueue.h

@@ -52,6 +52,10 @@ public:
      * BLEInstanceBase::process
      */
     virtual bool post(const mbed::Callback<void()>& event) = 0;
+
+    /** Remove all pending events.
+     */
+    virtual void clear() = 0;
 };
 
 } // namespace ble