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;
}

[RK]

That looks like a huge bug to me, I agree that that array is an array of buttons which exist, not an array of pins and that's just not going to work in most cases. The pointer is only checked for NULL on the next line as well, which is rather strange.

I'd suggest filing a support case with this one but perhaps report back after that's concluded.

[Leif-Alexandre ASCHEHOUG]

Hi Actually, the input parameter of the app_button_is_pushed has changed and documentation is not reflecting this change, which is misleading. It should be the actual pin number being passed to the function but the index of the button in the array of configured buttons (which has been given to the init function).

[Charles Custon]

This is still an issue, as the function also uses:

    // 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;
    }

pin_no is used to index an array that has BUTTON_COUNT number of elements, yet is also being used to logically shift a 32 bit register.

We've modified the functions for our purposes as follows, with the first argument being the button number, instead of pin number:

uint32_t app_button_is_pushed(uint8_t button_no, bool * p_is_pushed)
{
uint32_t err_code;
uint32_t active_pins;

app_button_cfg_t * p_btn = &mp_buttons[button_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 >> p_btn->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 >> p_btn->pin_no) & 0x01))
    {
        *p_is_pushed = true;
    }
    else
    {
        *p_is_pushed = 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.