app_timer2 library new bug

[CHATAR]

Hi,

I recently discover a new bug in the app_timer2 lib of the lastest SDK (17.1.0) using the softdevice s132_7.2.0

Here are the steps to reproduce the bug:

1- create a repeated timer in main().

APP_TIMER_DEF(TIMER1);

int main()
{
    ..
    ..
    ..
    ..
    uint32_t err_code = app_timer_create(&TIMER1, APP_TIMER_MODE_REPEATED, TIMER1_IRQ_Handle);

    ..
    ..
    ..
}

2- start the timer with 2000ms of interval.

app_timer_start(TIMER1, APP_TIMER_TICKS(2000), NULL);

3- when the timer expires, stop it in its interrupt handler and start it again with the same interval (2000ms)

static void TIMER1_IRQ_Handle(void * p_context)
{
    app_timer_stop(TIMER1);
    uint32_t err_code = app_timer_start(TIMER1, APP_TIMER_TICKS(2000), NULL);

    NRF_LOG_INFO("TIMER1_IRQ_Handle app_timer_start err =%d\n",err_code)
}

the BUG : the timer will expire in 4000ms the second time and the in next others  and not in 2000ms

it seems that the timer always expires in a double interval than the one given to it in the interrupt handler

Do you have any workaround for this bug ?.

Attached a simple .c example to directly reproduce the bug.

Fullscreen 8270.main.c Download

/**
 * Copyright (c) 2014 - 2021, Nordic Semiconductor ASA
 *
 * All rights reserved.
 *
 * Redistribution and use in source and binary forms, with or without modification,
 * are permitted provided that the following conditions are met:
 *
 * 1. Redistributions of source code must retain the above copyright notice, this
 *    list of conditions and the following disclaimer.
 *
 * 2. Redistributions in binary form, except as embedded into a Nordic
 *    Semiconductor ASA integrated circuit in a product or a software update for
 *    such product, must reproduce the above copyright notice, this list of
 *    conditions and the following disclaimer in the documentation and/or other
 *    materials provided with the distribution.
 *
 * 3. Neither the name of Nordic Semiconductor ASA nor the names of its
 *    contributors may be used to endorse or promote products derived from this
 *    software without specific prior written permission.
 *
 * 4. This software, with or without modification, must only be used with a
 *    Nordic Semiconductor ASA integrated circuit.
 *
 * 5. Any software provided in binary form under this license must not be reverse
 *    engineered, decompiled, modified and/or disassembled.
 *
 * THIS SOFTWARE IS PROVIDED BY NORDIC SEMICONDUCTOR ASA "AS IS" AND ANY EXPRESS
 * OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
 * OF MERCHANTABILITY, NONINFRINGEMENT, AND FITNESS FOR A PARTICULAR PURPOSE ARE
 * DISCLAIMED. IN NO EVENT SHALL NORDIC SEMICONDUCTOR ASA OR CONTRIBUTORS BE
 * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
 * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE
 * GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
 * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
 * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
 * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 *
 */
/** @file
 *
 * @defgroup ble_sdk_app_beacon_main main.c
 * @{
 * @ingroup ble_sdk_app_beacon
 * @brief Beacon Transmitter Sample Application main file.
 *
 * This file contains the source code for an Beacon transmitter sample application.
 */

#include <stdbool.h>
#include <stdint.h>
#include "nordic_common.h"
#include "bsp.h"
#include "nrf_soc.h"
#include "nrf_sdh.h"
#include "nrf_sdh_ble.h"
#include "ble_advdata.h"
#include "app_timer.h"
#include "nrf_pwr_mgmt.h"

#include "nrf_log.h"
#include "nrf_log_ctrl.h"
#include "nrf_log_default_backends.h"

APP_TIMER_DEF(TIMER1);


/**@brief Callback function for asserts in the SoftDevice.
 *
 * @details This function will be called in case of an assert in the SoftDevice.
 *
 * @warning This handler is an example only and does not fit a final product. You need to analyze
 *          how your product is supposed to react in case of Assert.
 * @warning On assert from the SoftDevice, the system can only recover on reset.
 *
 * @param[in]   line_num   Line number of the failing ASSERT call.
 * @param[in]   file_name  File name of the failing ASSERT call.
 */
void assert_nrf_callback(uint16_t line_num, const uint8_t * p_file_name)
{
    app_error_handler(0xDEADBEEF, line_num, p_file_name);
}



/**@brief Function for initializing the BLE stack.
 *
 * @details Initializes the SoftDevice and the BLE event interrupt.
 */
static void ble_stack_init(void)
{
    ret_code_t err_code;

    err_code = nrf_sdh_enable_request();
    APP_ERROR_CHECK(err_code);

    // Configure the BLE stack using the default settings.
    // Fetch the start address of the application RAM.
    uint32_t ram_start = 0;
    err_code = nrf_sdh_ble_default_cfg_set(0xFF, &ram_start);
    APP_ERROR_CHECK(err_code);

    // Enable BLE stack.
    err_code = nrf_sdh_ble_enable(&ram_start);
    APP_ERROR_CHECK(err_code);
}


/**@brief Function for initializing logging. */
static void log_init(void)
{
    ret_code_t err_code = NRF_LOG_INIT(NULL);
    APP_ERROR_CHECK(err_code);

    NRF_LOG_DEFAULT_BACKENDS_INIT();
}



/**@brief Function for initializing timers. */
static void timers_init(void)
{
    ret_code_t err_code = app_timer_init();
    APP_ERROR_CHECK(err_code);
}


/**@brief Function for initializing power management.
 */
static void power_management_init(void)
{
    ret_code_t err_code;
    err_code = nrf_pwr_mgmt_init();
    APP_ERROR_CHECK(err_code);
}


/**@brief Function for handling the idle state (main loop).
 *
 * @details If there is no pending log operation, then sleep until next the next event occurs.
 */
static void idle_state_handle(void)
{
    if (NRF_LOG_PROCESS() == false)
    {
        nrf_pwr_mgmt_run();
    }
}

static void TIMER1_IRQ_Handle(void * p_context)
{
    app_timer_stop(TIMER1);
    uint32_t err_code = app_timer_start(TIMER1, APP_TIMER_TICKS(2000), NULL);

    NRF_LOG_INFO("TIMER1_IRQ_Handle app_timer_start err =%d\n",err_code)
}


/**
 * @brief Function for application main entry.
 */
int main(void)
{
    // Initialize.
    log_init();
    timers_init();
    power_management_init();
    ble_stack_init();

    uint32_t err_code = app_timer_create(&TIMER1, APP_TIMER_MODE_REPEATED, TIMER1_IRQ_Handle);
    
    NRF_LOG_INFO("main app_timer_create err=%d \n",err_code);
    
    err_code = app_timer_start(TIMER1, APP_TIMER_TICKS(2000), NULL);
    
    NRF_LOG_INFO("main app_timer_start err=%d \n",err_code);
    
    // Enter main loop.
    for (;; )
    {
        idle_state_handle();
    }
}


/**
 * @}
 */

[Hieu]

Hi CHATAR,

Let me try to reproduce and debug this and I will get back to you soon.

Meanwhile, may I ask why you start the timer in repeat mode if you would stop and restart it in the handler?

Also, you might already know this, but the app timers are not the actual TIMER peripheral, so you might want to avoid naming the instance and the handler in that way. It could be confusing.

Hieu

[CHATAR]

Hi Hieu,

this is just an example to show you the bug, in our program we have no choice but to use repeated mode and stop in the inerrupt in our complex behavior is mandatory, believe me. So in the previous version of sdk (17.0.2) the lib was working fine but now I have this bug.

[Hieu]

Hi CHATAR,

I think I found the "bug." It lies in the timer_expire() function of app_timer2.c.

The details of the issue are provided below, but first are two alternatives you can consider:

- Use APP_TIMER_MODE_SINGLE_SHOT and always restart the timer in the handler
- Defer the call to app_timer_start() out of the handler. This can be done, for example, with a boolean flag.

Next, I would like to explain the technical details:

Here is a snippet of code from the timer_expire() function. I marked the critical lines at the beginning of them:

    #if APP_TIMER_CONFIG_USE_SCHEDULER
            app_timer_event_t timer_event;

            timer_event.timeout_handler = p_timer->handler;
            timer_event.p_context       = p_timer->p_context;
            uint32_t err_code = app_sched_event_put(&timer_event,
                                                    sizeof(timer_event),
                                                    scheduled_timeout_handler);
            APP_ERROR_CHECK(err_code);
    #else
            NRF_LOG_DEBUG("Timer expired (context: %d)", (uint32_t)p_timer->p_context)
/**/        p_timer->handler(p_timer->p_context);
    #endif
            CRITICAL_REGION_ENTER();
            /* check active flag as it may have been stopped in the user handler */
            if (p_timer->repeat_period && !APP_TIMER_IS_IDLE(p_timer))
            {
/**/            p_timer->end_val += p_timer->repeat_period;
                cont = true;
            }
            else
            {
                cont = false;
            }
            CRITICAL_REGION_EXIT();

Let's keep in mind that: In the library, a timer is set as inactive by having the end value field set to an invalid value.

In your use case, each time the timer expires, the following things happen:

• app_timer_stop() is called, stopping the timer. end_val is set to APP_TIMER_IDLE_VAL.
• app_timer_start() is called. end_val is set to <now> + repeat_period.
• In timer_expire(), after the application handler, the driver check:
  • If the timer is in repeat mode (repeat_period != 0) and the timer is not stopped (end_val != APP_TIMER_IDLE_VAL)
    • end_val is set to <now> + repeat_period

If app_timer_start() is not called in the handler, then the timer_expire() logic above would be correct.

However, in your use case, app_timer_start() is used and thus the logic causes repeat_period to be added to end_val twice.

Whether or not this is a "bug" depends on whether or not the design is expected to handle app_timer_start() in the handler. I can imagine your use case being logical, so I may agree this is a bug.

Nonetheless, both the workaround I propose above should work, so unfortunately this issue will not be fixed.

I am sorry for the inconvenience. If the workarounds don't work for you for any reasons, please let me know.

Hieu