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;
    }
}
share|improve this question
1  
Is pin 13 connected to an interrupt? – vnp 17 hours ago
    
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 – ree 17 hours ago
    
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? – Cody Gray 16 hours ago
    
I didnt get it, how I can measure the pressed timing without timer interrupt? – ree 14 hours ago

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;
    }
}
share|improve this answer

... 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.

share|improve this answer
2  
But you can just call HAL_GPIO_TogglePin(GPIOA,GPIO_PIN_5); when usTick is greater to 350 – David Isla 16 hours ago
    
@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). – πάντα ῥεῖ 16 hours ago
1  
of course not! ;-) – David Isla 16 hours ago
    
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? – ree 14 hours ago
    
@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? – πάντα ῥεῖ 14 hours ago

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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