1
\$\begingroup\$

I wrote code for determining the push button state whether it is long pressed or not. This function is called by timer interrupt routine every 1ms.

But it seems really dumb. How can I make it shorter and more efficient according to both readability and professional rules? What should I add or change to get double Tap pressing information with this code?

void HAL_TIM_PeriodElapsedCallback(TIM_HandleTypeDef *htim)
{   
    /*button pressed and count*/
    if(!HAL_GPIO_ReadPin(GPIOC, GPIO_PIN_13))
    {
        usTick++;
    }
    /*not pressed*/
    else
    {
        if( usTick > 1000){
            ButtonState.PressedState = LongPressed;
            HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);       
            usTick = 0;
        }
        else if( usTick >350){
            ButtonState.PressedState = ShortPressed;
            HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);               
            usTick = 0;
        }               
        usTick = 0;
    }
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Is pin 13 connected to an interrupt? \$\endgroup\$ – vnp Jan 11 '17 at 17:51
  • \$\begingroup\$ no just a regular Input pin. I just enabled the timer interrupt in order to measure pressed time so that i can decide long or short pressing \$\endgroup\$ – ree Jan 11 '17 at 17:53
  • \$\begingroup\$ Do you really have to poll here? Is there no way for you to receive an interrupt when the button is pressed and when it is released? \$\endgroup\$ – Cody Gray Jan 11 '17 at 19:07
  • \$\begingroup\$ I didnt get it, how I can measure the pressed timing without timer interrupt? \$\endgroup\$ – ree Jan 11 '17 at 21:21
  • \$\begingroup\$ Using interrupts for buttons is a very bad idea, unless you know exactly what you are doing. See this: stackoverflow.com/questions/32646071/… \$\endgroup\$ – Lundin Jul 4 '17 at 11:20
3
\$\begingroup\$

Repeat HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5); is a little bit ugly for me, that would be my approach:

void HAL_TIM_PeriodElapsedCallback(TIM_HandleTypeDef *htim)
{   
    // Button pressed
    if(!READ_PIN_13)
    {
        usTick++;
    }
    else 
    {
        // Not pressed
        if( usTick > 350 )
        {
            ButtonState.PressedState = usTick > 1000 ? LongPressed : ShortPressed;
            HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);               
        }               
        usTick = 0;
    }
}
\$\endgroup\$
3
\$\begingroup\$

... how can I make it shorter and more efficient according to both readability and professional rules?

Calling

usTick = 0;

is superfluous in the else branches sub-conditions. That can be rewritten like:

/*not pressed*/
else
{
    if( usTick > 1000){
        ButtonState.PressedState = LongPressed;
        HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);       
    }
    else if( usTick >350){
        ButtonState.PressedState = ShortPressed;
        HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);       
    }          
    // Calling those statements Once is enough here 
    usTick = 0;
}

Checking the minimum limit and reducing the whole sub-conditions with a ternary condition operator (as mentioned in @David Isla's answer), the whole code could be boiled down to a few lines:

/*not pressed*/
else
{
    if( usTick > 350) {
         ButtonState.PressedState = usTick > 1000 ? LongPressed : ShortPressed;
         HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);       
    }
    usTick = 0;
}

What should I add or change to get double Tap pressing information with this code?

You might consider to put any data updating operations into critical sections (or other non-interruptible code paths):

void HAL_TIM_PeriodElapsedCallback(TIM_HandleTypeDef *htim)
{   
    /*button pressed and count*/
    if(!HAL_GPIO_ReadPin(GPIOC, GPIO_PIN_13))
    {
        START_CRITCAL_SECTION();
        usTick++;
        END_CRITCAL_SECTION();
    }
    /*not pressed*/
    else {
        START_CRITCAL_SECTION();
        if( usTick > 350) {
             ButtonState.PressedState = usTick > 1000 ? LongPressed : ShortPressed;
             HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5);       
        }
        usTick = 0;
        END_CRITCAL_SECTION();
    }
}

where START_CRITCAL_SECTION() disables all interrupts, and END_CRITCAL_SECTION() reenables their former state.

This may help, that any co-routines or threads will always read consistent values for ustick or the GPIO_PIN_5 state.

\$\endgroup\$
  • 2
    \$\begingroup\$ But you can just call HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5); when usTick is greater to 350 \$\endgroup\$ – David Isla Jan 11 '17 at 19:20
  • \$\begingroup\$ @DavidIsla Yes, you're right about that, the logic won't be the same. Fixed it. I hope you're not bothered I've "stolen" the ternary operator from your answer ;-) (gave your answer an upvote). \$\endgroup\$ – πάντα ῥεῖ Jan 11 '17 at 19:25
  • 1
    \$\begingroup\$ of course not! ;-) \$\endgroup\$ – David Isla Jan 11 '17 at 19:31
  • \$\begingroup\$ I think I didn't ask my latest question clearly. Ok I got your answers. But I want to make this script more efficient. Let's assume we got 1 button. Until now I could determine short or long pressing. How can we understand double tap pressing besides long and short pressing? \$\endgroup\$ – ree Jan 11 '17 at 21:25
  • \$\begingroup\$ @ree 1. C programs aren't scripts 2. Define "more efficient" from that context clearly please. Less code? Less blocking in interrupt routines? Or what else? \$\endgroup\$ – πάντα ῥεῖ Jan 11 '17 at 21:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.