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

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

}