Stopwatch program written in C, using Pico W

Question

I've a raspberry Pico W and developed a simple stopwatch program in C. I configured GPIO pin 15 as an input and enabled the internal pull-up resistor. If the state is high, I will start the timer and print every second, if the state is low, I will stop the timer and reset the counter to 0. This is my code. However, I know that for embedded C coding standard, we should try not to use global variables. Hence, how can I improve my code to use less global variables?

#include <stdio.h>
#include "pico/stdlib.h"
#include "hardware/gpio.h"
#define BTN_PIN 15
#define TIME_INTERVAL_MS 1000
#define DEBOUNCE_TIME_MS 50
volatile bool timer_running = false;
volatile uint32_t elapsed_time = 0;
volatile bool button_state = false; // Current debounced button state
volatile uint32_t last_debounce_time_ms = 0; // Timestamp of the last state change
struct repeating_timer timer;
// Debounce function to filter out button state changes caused by noise
bool debounce(bool new_state)
{
 // Get the current time in milliseconds
 uint32_t current_time_ms = to_ms_since_boot(get_absolute_time());
 // Check if enough time has passed since the last state change
 if (current_time_ms - last_debounce_time_ms >= DEBOUNCE_TIME_MS) // Adjust the debounce time as needed
 {
 // Update the last state change timestamp
 last_debounce_time_ms = current_time_ms;
 // Update the debounced state
 button_state = new_state;
 }
 return button_state;
}
bool repeating_timer_callback(struct repeating_timer *t)
{
 if (timer_running)
 {
 elapsed_time++;
 printf("Elapsed Time: %d seconds\n", elapsed_time);
 }
 return true;
}
void gpio_irq_handler(uint gpio, uint32_t events)
{
 // Read the status of the button and debounce it to reduce noise
 bool debounced_state = debounce(gpio_get(BTN_PIN));
 if (debounced_state)
 {
 // If state is high, start timer
 if (!timer_running)
 {
 timer_running = true;
 printf("Timer started\n");
 add_repeating_timer_ms(TIME_INTERVAL_MS, repeating_timer_callback, NULL, &timer);
 }
 }
 else
 {
 // If state is low (GND), stop timer
 if (timer_running)
 {
 timer_running = false;
 elapsed_time = 0;
 cancel_repeating_timer(&timer);
 printf("Timer Stopped\n");
 }
 }
}
int main()
{
 stdio_init_all();
 // Set GPIO pin 15 as input and enable its internal pull - up resistor
 gpio_set_dir(BTN_PIN, GPIO_IN);
 gpio_set_pulls(BTN_PIN, true, false);
 // Enable interrupts on GPIO pin 15 for falling and rising edges (button pressed)
 gpio_set_irq_enabled_with_callback(BTN_PIN, GPIO_IRQ_EDGE_RISE | GPIO_IRQ_EDGE_FALL, true, &gpio_irq_handler);
 while (1)
 ;
 return 0;
}

• chux: Why print uint32_t with "%d"? Save time and enable all warnings.

Answer 1

No need for volatile

There is no need to make any of your variables volatile. This is only required when using pointers to hardware registers, or when you are signaling something from an interrupt context to the main thread. However, you are not doing either.

Removing global variables

Removing global variables is good practice in general, not just for embedded programming. In fact, it's even less of an issue in embedded systems such as your Pico. First, always check if variables need to be used by multiple functions. If not, just limit them to one function. For example, button_state and last_debounce_time_ms are only used by debounce(), so you can write:

bool debounce(bool new_state)
{
 static bool button_state = false;
 static int last_debounce_time_ms = 0;
 ...
}

Note that the static here only means these variables will outlive the function call, but they are no longer in the global namespace.

You can do the same for timer_running (just print unconditionally in repeating_time_callback(), so it's only used in gpio_irq_handler()).

Very weak debouncing

Your debounce() function doesn't debounce much at all, it only prevents button_state from changing more than 20 times per second. However, if there is any noise that would temporarily cause the IO line to switch state, it could still treat that as a button press.

A much better way to debounce is to check new_state every time debounce() is called, and only update button_state if new_state has the same value for many calls in a row, or perhaps even require the same value for the whole duration of DEBOUNCE_TIME_MS.

It could be that your physical button gives a clean enough signal so that you don't have to worry about this, but I recommend implementing more robust debouncing anyway.

Go to a low power mode in the main loop

It's very good that you are using interrupts to handle timer ticks and IO line changes. However, now you have nothing left to do in the main loop. Doing while(1); will cause it to use CPU cycles though. The Pico can go into a low(er) power sleep mode and will wake up automatically if there is an interrupt. Check the SDK you are using for which functions you can use for this. Especially if you want to run your Pico from batteries, this can make the difference between hours and months of battery life.

• chux: Disagree about solution to debounce() problem. IMO, additional changes are to be inhibited for 50ms after a change. This way the change is recognized right away and not delayed 50ms as you suggest.
• G. Sliepen: Ah true. But still, it doesn't do anything about noise. It just samples at most at 20 Hz, but it doesn't filter anything.
• Isakkiii: @G.Sliepen Hi, thanks for the reply! What about struct repeating_timer timer & uint32_t elapsed_time = 0; Is it possible to not declare them as global variables?
• G. Sliepen: You can indeed make them both local variables of gpio_irq_handler, and then use the user_data field of struct repeating_timer and have it point to the local elapsed_time, that way repeating_timer_callback() can access it as well.
• Isakkiii: @G.Sliepen Hi, just an update, I have successful make repeating_timer timer a static local variable. Now, only elapsd_time is a global variable. Is there any solution to this?

Answer 2

@G. Sliepen well explains how to move some global variables into static ones inside debounce(). That is OP's review goal.

A non-review idea:
Below is an alternative debounce code that reports a change immediately and then debounces (inhibits change for 50 ms while the switch is bouncing).

bool debounce(bool new_state) {
 static int button_state = -1; // Current debounced button state, start at -1
 static uint32_t next_allowable_change_time_ms = 0;
 if (button_state == new_state) {
 return button_state;
 }
 uint32_t current_time_ms = to_ms_since_boot(get_absolute_time());
 if (current_time_ms >= next_allowable_change_time_ms) {
 next_allowable_change_time_ms = current_time_ms + DEBOUNCE_TIME_MS;
 button_state == new_state;
 }
 return button_state;
}