ble_hs_timer_sched() never restarts the timer if a timer is active

Recently, I tried to implement a toggling BLE advertisement. The idea would be to repeat the following sequence indefinitely:

Advertise services A for 7s
Advertise service B for 3s

What I discovered is that the first sequence (A for 7s) works, but the attempt to start B for 3s would actually result in B advertising forever because I never get another BLE_GAP_EVENT_ADV_COMPLETE, even after the 3s expires.

The problem seems to be in this code:

static void
ble_hs_timer_sched(int32_t ticks_from_now)
{
    ble_npl_time_t abs_time;

    if (ticks_from_now == BLE_HS_FOREVER) {
        return;
    }

    /* Reset timer if it is not currently scheduled or if the specified time is
     * sooner than the previous expiration time.
     */
    abs_time = ble_npl_time_get() + ticks_from_now;
    if (!ble_npl_callout_is_active(&ble_hs_timer) ||
            ((ble_npl_stime_t)(abs_time -
                               ble_npl_callout_get_ticks(&ble_hs_timer))) < 0) {
        ble_hs_timer_reset(ticks_from_now);
    }
}

The function it relies on, ble_npl_callout_get_ticks(), always returns zero for the ESP timer:

ble_npl_time_t
npl_freertos_callout_get_ticks(struct ble_npl_callout *co)
{
#if CONFIG_BT_NIMBLE_USE_ESP_TIMER
   /* Currently, esp_timer does not support an API which gets the expiry time for
    * current timer.
    * Returning 0 from here should not cause any effect.
    * Drawback of this approach is that existing code to reset timer would be called
    * more often (since the if condition to invoke reset timer would always succeed if
    * timer is active).
    */

    return 0;
#else
    return xTimerGetExpiryTime(co->handle);
#endif
}

The comment ...existing code to reset timer would be called more often (since the if condition to invoke reset timer would always succeed if timer is active) is wrong; in fact, the condition to invoke reset timer never succeeds if the timer is active. That's because the condition simplifies as follows when ble_npl_callout_get_ticks() returns zero:

!ble_npl_callout_is_active(&ble_hs_timer) || ((ble_npl_stime_t)(abs_time - ble_npl_callout_get_ticks(&ble_hs_timer))) < 0) !ble_npl_callout_is_active(&ble_hs_timer) || ((ble_npl_stime_t)(abs_time - 0)) < 0) !ble_npl_callout_is_active(&ble_hs_timer) || ((ble_npl_stime_t)abs_time) < 0) !ble_npl_callout_is_active(&ble_hs_timer) || false) !ble_npl_callout_is_active(&ble_hs_timer)

Curiously, npl_freertos_callout_remaining_ticks() already has support for esp-idf v5.0. It seems to me the same should have been done for ble_hs_timer_sched(), too.

(In my own code, I can't use esp-idf 5.0, but it was simple to backport just those two ESP timer API functions back into my fork of esp-idf 4.4.3).

espressif / esp-nimble

ble_hs_timer_sched() never restarts the timer if a timer is active #58