1
\$\begingroup\$

I want to start and stop timers from the button state. I am using an STM32F407VG.

This it the circuit I designed:

enter image description here

When the button is pushed there is 0.45 V. When I release the button there will be 4.99 V.

enter image description here

This is the code block which I wrote to achieve my goal:

  uint8_t buttonState=0;
  uint8_t oldState=0;
  buttonState=HAL_GPIO_ReadPin(GPIOB, GPIO_PIN_10);
  if(oldState==0 && buttonState==1)
  {
      HAL_TIM_PWM_Start(&htim1, TIM_CHANNEL_3); // TIM1_CH3 start
      HAL_TIM_PWM_Start(&htim1, TIM_CHANNEL_4); // TIM1_CH4 start
      oldState=1;
  }

  else if(oldState==1 && buttonState==0)
  {
      HAL_TIM_PWM_Stop(&htim1, TIM_CHANNEL_3); // TIM1_CH3 stop
      HAL_TIM_PWM_Stop(&htim1, TIM_CHANNEL_4); // TIM1_CH4 stop
      oldState=0;
  }

I want to start and stop the timers from the button state. I first read the button state. I want to start the timers only when button is pushed (like in the first picture.) When I release the button (like in the second picture,) I want the timers to stop.

When I debug the project, the code never goes into the else if statement. PWM always runs and never stops.

Is there a problem in my circuit or software?

\$\endgroup\$
1
  • \$\begingroup\$ oldState seems irrelevant \$\endgroup\$
    – jsotola
    Sep 24, 2021 at 23:51

3 Answers 3

1
\$\begingroup\$

I'm not familiar with STM or the compiler you're using but is having the variable declarations and initialisations in the main control loop resetting oldState to 0 on every pass? I would expect the first two lines to be outside the main loop.

int main(void) {
  uint8_t buttonState=0;   // Do once.
  uint8_t oldState=0;      // Do once.

  while (1) {              // loop forever ...
    buttonState=HAL_GPIO_ReadPin(GPIOB, GPIO_PIN_10);
    if(oldState == 0 && buttonState == 1) {
        HAL_TIM_PWM_Start(&htim1, TIM_CHANNEL_3); // TIM1_CH3 start
        HAL_TIM_PWM_Start(&htim1, TIM_CHANNEL_4); // TIM1_CH4 start
        oldState = 1;
    } else if(oldState == 1 && buttonState == 0) {
        HAL_TIM_PWM_Stop(&htim1, TIM_CHANNEL_3); // TIM1_CH3 stop
        HAL_TIM_PWM_Stop(&htim1, TIM_CHANNEL_4); // TIM1_CH4 stop
        oldState = 0;
    }
  }
}

Can you please explain deeper what you mean when you said resetting oldState to 0 every pass?

Follow your original code:

Button has been off but is now pressed:

  • Line 2 sets oldState = 0.
  • Line 3 detects and sets buttonState = 1.
  • Line 8 sets oldState = 1.

Program loops back to start.

  • Line 2 sets oldState = 0 even though the button is still pressed. You need to move lines 1 and 2 out of the main execution loop.

You could probably also do

} else if(buttonState == 0) {

to turn off the PWM and oldState if the button is released regardless of the condition of oldState.

\$\endgroup\$
4
  • \$\begingroup\$ I use STM32CubeIDE and there is int main() function and while(1) loop. I do the all the stuff in int main() function. \$\endgroup\$
    – Bowman
    Sep 24, 2021 at 20:52
  • \$\begingroup\$ @Kırambor, thanks. Have I got it right now? \$\endgroup\$
    – Transistor
    Sep 24, 2021 at 21:03
  • \$\begingroup\$ Yes, you have. Can you please explain deeper what you mean when you said resetting oldState to 0 every pass ? \$\endgroup\$
    – Bowman
    Sep 24, 2021 at 21:05
  • \$\begingroup\$ See the update. \$\endgroup\$
    – Transistor
    Sep 24, 2021 at 21:13
1
\$\begingroup\$

Don't use 0 and 1.

It's better to use GPIO_PIN_RESET, GPIO_PIN_SET which are the preferred return values of the HAL_GPIO_ReadPin function. The return type of HAL_GPIO_ReadPin function is either 0 or non zero; non-zero is not necessarily 1. The function just checks if the return type of IDR register masked with that particular pin is 0 or not. It is either 0, or a value representing the IDR masked with that pin. e.g. 1,2,4,8,16,32...

Take a look at the HAL_GPIO_ReadPin definition:

GPIO_PinState HAL_GPIO_ReadPin(GPIO_TypeDef* GPIOx, uint16_t GPIO_Pin)
{
  GPIO_PinState bitstatus;

  /* Check the parameters */
  assert_param(IS_GPIO_PIN(GPIO_Pin));

  if((GPIOx->IDR & GPIO_Pin) != (uint32_t)GPIO_PIN_RESET)
  {
    bitstatus = GPIO_PIN_SET;
  }
  else
  {
    bitstatus = GPIO_PIN_RESET;
  }
  return bitstatus;
}

and the definition of GPIO_PIN_SET and GPIO_PIN_RESET:

typedef enum
{
  GPIO_PIN_RESET = 0,
  GPIO_PIN_SET
}GPIO_PinState;

The second problem:

When checking PB10, the return value of the HAL_GPIO_ReadPin is either 0 or (1<<10) as mentioned above. You try to store it in a uint8_t variable, which overflows. You have to use a bigger variable, or use the GPIO_PinState typedef. I would rewrite the code provided, just correcting the errors I found, not neccesarily the main problem:

  GPIO_PinState buttonState=0;
  GPIO_PinState oldState=0;
  buttonState=HAL_GPIO_ReadPin(GPIOB, GPIO_PIN_10);
  if(oldState == GPIO_PIN_RESET && buttonState == GPIO_PIN_SET)
  {
      HAL_TIM_PWM_Start(&htim1, TIM_CHANNEL_3); // TIM1_CH3 start
      HAL_TIM_PWM_Start(&htim1, TIM_CHANNEL_4); // TIM1_CH4 start
      oldState = GPIO_PIN_SET;
  }

  else if(oldState == GPIO_PIN_SET && buttonState == GPIO_PIN_RESET)
  {
      HAL_TIM_PWM_Stop(&htim1, TIM_CHANNEL_3); // TIM1_CH3 stop
      HAL_TIM_PWM_Stop(&htim1, TIM_CHANNEL_4); // TIM1_CH4 stop
      oldState = GPIO_PIN_RESET ;
  }
\$\endgroup\$
1
  • \$\begingroup\$ Thanks. Your answer gave me a good lead. \$\endgroup\$
    – Bowman
    Sep 24, 2021 at 21:17
0
\$\begingroup\$

Let's quickly run over what your code does.

I will assume you're doing polling - just checking the state of the button in the forever loop again and again and again. When you press the button, your loop will detect state change (not pressed became pressed - old state and new state become 0 and 1 respectively), so it will start the timers. When the next polling round comes, which is probably immediately, and while your button is still pressed, you have no state change, so no of the conditions are met, which is OK, timers already activated, we don't need to do anything anyway, so we're good.

Things get a little hairy when the button is released, your old state is actually always ZERO, because every time you re-declare your old state variable to be zero. So your old and new state will always be zero and no if conditions are ever met.

I suggest you create a variable, say, TIMERS_ACTIVATED boolean outside the polling loop (before it will do, no need for globals), which, you guessed it, will be 1 when timers are started and will be 0 when they're off. Your conditions will have to change accordingly, but not much. You will only need to check for current state of the button and this new variable (which is effectively the same thing as your "old state", to be honest, the same logic, just more descriptive name). So your polling checks everything just the same, it will simply check if "button state pressed AND timers off" then turn timers on AND (!!!) update TIMERS_ACTIVATED state, and "if button state not pressed AND timers on" then turn timers off AND updates TIMERS_ACTIVATED state.

Since TIMERS_ACTIVATED is declared before the loop and updated manually every time you enable/disable timers, its state won't be reset after every loop like in your piece of code.

\$\endgroup\$

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.