Bug in app_button_is_pushed()?

[Alex Goldenberg]

Within app_button module, code for app_button_is_pushed() seems to have changed from SDK version 5.1 to 6.0. I notice my code has stopped working properly with that update. Debugger makes it apparent that this is the problematic line:

app_button_cfg_t * p_btn = &mp_buttons[pin_no];  // excerpt from app_button_is_pushed SDK 6.0

For instance, if there is only 1 button attached to pin 0.30, I would expect to pass in pin_no=30 (just as I did in version 5.1). But mp_buttons[30] is way out of bounds; only mp_buttons[0] is valid for a single button scenario. The ensuing pointer assignment to p_btn is way off in the weeds.

Is this a bug or am I missing something?

---

For convenience, the 6.0 function call:

uint32_t app_button_is_pushed(uint8_t pin_no, bool * p_is_pushed)
{
    uint32_t err_code;
    uint32_t active_pins;
    
    app_button_cfg_t * p_btn = &mp_buttons[pin_no];

    if (mp_buttons == NULL)
    {
        return NRF_ERROR_INVALID_STATE;
    }

    err_code = app_gpiote_pins_state_get(m_gpiote_user_id, &active_pins);
    
    if (err_code != NRF_SUCCESS)
    {
        return err_code;
    }

    if(p_btn->active_state == APP_BUTTON_ACTIVE_LOW)
    {
        // If the pin is active low, then the pin being high means it is not pushed.
        if(((active_pins >> pin_no) & 0x01))
        {
            *p_is_pushed = false;
        }
        else
        {
            *p_is_pushed = true;  
        }            
    }
    else if(p_btn->active_state == APP_BUTTON_ACTIVE_HIGH)
    {
        // If the pin is active high, then the pin being high means it is pushed.
        if(((active_pins >> pin_no) & 0x01))
        {
            *p_is_pushed = true;
        }
        else
        {
            *p_is_pushed = false;   
        }
    }
    
    return NRF_SUCCESS;
}

and the 5.1 function call:

uint32_t app_button_is_pushed(uint8_t pin_no, bool * p_is_pushed)
{
    uint32_t err_code;
    uint32_t active_pins;
    uint32_t pin_mask    = 0;
    
    if (mp_buttons == NULL)
    {
        return NRF_ERROR_INVALID_STATE;
    }

    err_code = app_gpiote_pins_state_get(m_gpiote_user_id, &active_pins);
    if (err_code != NRF_SUCCESS)
    {
        return err_code;
    }
    
    pin_mask = (1 << pin_no);

    if ((pin_mask & active_pins) == 0)
    {
        *p_is_pushed = ((m_active_low_states_mask & pin_mask) ? true : false);
    }
    else
    {
        *p_is_pushed = ((m_active_high_states_mask & pin_mask) ? true : false);
    }

    return NRF_SUCCESS;
}

[Sam Lin]

such API design is strange, I have to keep 2 enumeration for one button. one is button pin and another one is (trivial) button index of button array. they are totally different, and existing ALL examples wrongly assume they are the same.

[Sam Lin]

I think the API app_button_is_pushed assuming button index is poor. Because the behavior is implicit. you might change button array declare order and the whole thing goes wrong without clue. It's dangerous. Even beacon reference design example made wrong assumption. : From beacon reference code:

err_code = app_button_is_pushed(CONFIG_MODE_BUTTON_PIN, &config_mode);

CONFIG_MODE_BUTTON_PIN is defined as BUTTON_1 (1).

but in fact :

static app_button_cfg_t buttons[] =
{
    {CONFIG_MODE_BUTTON_PIN, false, BUTTON_PULL, button_handler},
    {BOOTLOADER_BUTTON_PIN, false, BUTTON_PULL, button_handler}
};

what it expects is index 0.

This causes mess.