Interrupts: use of the "volatile" keyword with a structure pointer for button debounce

Question

I've written a small sketch targeted at the Arduino Uno (ATmega328P) to debounce a mechanical pushbutton using the summing/integration technique:

#include <IntegratingDebounce.h>
#define PIN_BTN 4
IntegratingDebounce *btn;
ISR (TIMER2_OVF_vect) {
 ideb_poll(btn);
} // end timer2 overflow ISR
void set_debounce_timer() {
 TCNT2 = 0; // Clear timer
 TCCR2A &= ~( bit( WGM21 ) | bit( WGM20 ) ); // Set waveform generation mode to normal
 TCCR2B &= ~bit( WGM22 ); 
 TIMSK2 |= bit( TOIE2 ); // Enable interrupt on overflow
 TCCR2B |= 7; // Start clocking the timer
} // set_debounce_timer()
void setup() {
 Serial.begin(9600);
 pinMode(PIN_BTN, INPUT_PULLUP);
 ideb_init(&btn, PIN_BTN, 4);
 set_debounce_timer();
} // setup()
void loop() {
 while (ideb_read(btn) == HIGH); // wait until the button pin goes low
 Serial.println("Pressed!");
 delay(100);
} // loop()

An IntegratingDebounce structure stores the data needed to perform the debounce. I keep a pointer to the structure, btn, in the global memory space.

The timer2 overflow ISR periodically polls the state of the button pin, updates the internal integrator, and sets the (also internal) debounced output accordingly. The ideb_read() function reads the debounced output out of the structure.

I'm having trouble determining where the volatile keyword should be placed in the declaration for the btn structure pointer. First I declared it as:

volatile IntegratingDebounce *btn;

I thought this would make all of the fields in the structure pointed to by btn volatile, but button presses are not detected in loop(). Declaring btn as a volatile pointer rather than a pointer to a volatile structure generates the expected output:

IntegratingDebounce *volatile btn;

This seems backwards. Why does this work?

IntegratingDebounce.h

#ifndef __INTEGRATING_DEBOUNCE_H__
#define __INTEGRATING_DEBOUNCE_H__
#include <Arduino.h>
#include <stdint.h>
extern "C" {
typedef struct IntegratingDebounce_S IntegratingDebounce;
typedef uint8_t ideb_size_t;
// Create a debounce structure and initialize.
void ideb_init(
 IntegratingDebounce **ideb,
 uint8_t pin, // I/O pin to debounce
 ideb_size_t clamp // debounce clamp; the maximum value the
 // internal sum is able to take on
);
// Poll the state of the pin, integrate, and update the debounced output.
void ideb_poll(IntegratingDebounce *ideb);
// Read the debounced state of the pin.
uint8_t ideb_read(IntegratingDebounce *ideb);
// Reset the internal integrator to either the LOW or HIGH state.
void ideb_reset(IntegratingDebounce *ideb, uint8_t state);
} // extern C
#endif

IntegratingDebounce.c

#include "IntegratingDebounce.h"
struct IntegratingDebounce_S {
 uint8_t pin; // pin to debounce
 uint8_t output; // debounced output
 ideb_size_t clamp; // debounce clamp
 ideb_size_t integrator; // clamped running sum
};
void ideb_init(IntegratingDebounce **ideb, uint8_t pin, uint8_t clamp) {
 *ideb = (IntegratingDebounce *)malloc(sizeof(IntegratingDebounce));
 (*ideb)->pin = pin;
 (*ideb)->clamp = clamp;
 (*ideb)->output = 0;
 (*ideb)->integrator = 0;
} // ideb_init()
void ideb_poll(IntegratingDebounce *ideb) {
 // update the integrator based on the present state of the pin:
 // - if LOW, decrement the integerator; if HIGH, increment the integrator
 // - constrain the integrator to values between 0 and 'clamp'
 if (digitalRead(ideb->pin) == LOW) {
 if (ideb->integrator > 0) ideb->integrator--;
 } // if
 else {
 if (ideb->integrator < ideb->clamp) ideb->integrator++;
 } // else
 // update the output based on the value of the integrator; the output
 // will not change until the integrator reaches a limiting value
 if (ideb->integrator == 0 && ideb->output == HIGH) {
 ideb->output = LOW;
 } // if
 else if (ideb->integrator == ideb->clamp && ideb->output == LOW) {
 ideb->output = HIGH;
 } // else if
} // ideb_poll()
uint8_t ideb_read(IntegratingDebounce *ideb) {
 return ideb->output;
} // ideb_read()
void ideb_reset(IntegratingDebounce *ideb, uint8_t state) {
 if (state == LOW) {
 ideb->integrator = 0;
 ideb->output = LOW;
 } // if
 else {
 ideb->integrator = ideb->clamp;
 ideb->output = HIGH;
 } // else
} // ideb_reset()

Answer 1

I think, that the problem is in ideb_read, which expects "a pointer to (non-volatile) structure" and so it does not care about re-reading ideb->output if it can it optimize out somehow (say putting it in free register).

IMHO the ideb_read should have parameter declared as (volatile IntegratingDebounce *ideb) (to know, that structure pointed by ideb is volatile) or use global variable btn (which is pointer to volatile structure) instead of pointer parametr to non-volatitle structure.

---

And why the second works would be because it forces re-read the parameter of ideb_read and so it cannot be saved out somewhere and the function is indirectly forced to expect changed pointer, so it have to use its 'new value' and read the structure (as it is simpler, than ensure, that the ideb have the same value as it had last call and so the whole structure there (which is declared as non-volatile in header of ideb_read) could be cached)

---

Another wording of the above:

Declaring volatile IntegratingDebounce *btn; the main program in loop simply takes the value of btn (which is non-volatile pointer to volatile structure) and copy that (non-volatile) value to ideb parametr od ideb_read, which is non-volatile pointer to non-volatile structure. ideb_read use it that way and compiler somehow was able to cache all of that and so not read ideb->output again, as it knows, that it is part of non-volatile struct. (So it does not work)

Declaring IntegratingDebounce *volatile btn; you made the btn volatile, so it gets read each time ideb_read is called and the 'new actual value' is passed to (non-volatile) ideb parametr (which is this way given 'new value' and so cannot be cached between calls) and it results to reading (non-volatile) date from "new" place pointed by ideb. So it works (by mistake).

Right solution would be declare correctly volatile IntegratingDebounce *btn; in main program AND declare correctly uint8_t ideb_read(volatile IntegratingDebounce *ideb) so the ideb_read function is forced read value pointed by ideb every time again (which is the right thing to do)

And it should be used for all other ideb_** functions too.

And I think, that it would be simpler to declare some typedef for this volatile structure (something like typedef struct { ... } volatile ideb_t; ) and use the new type everywhere.

• Gutenberg: That is making sense, and your solution works! Thanks. So passing a volatile IntegratingDebounce * as an argument to a function that expects an IntegratingDebounce * is an implicit type conversion to non-volatile, allowing the structure to be cached?
• gilhad: Yes, "pointer as pointer, it just points somewhere to memory" :) After all you can convert any pointer to void* and then convert it to any (other) pointer.
• gilhad: Also keep in mind, that the IntegratedDebounce.* can be (and usually is) compiled (long) before the main.* and (as any library after all) does not know much, what would use/call it and how. (and the ideb_read is so small and easy, that compiler probably would it convert it into inline where possible, as the function body is lot smaller (in asm), than the preffix and suffix of the function and comparable with length of calling sequence.

Answer 2

Getting pointer declarations right can be anything but obvious, especially where const and volatile are concerned (which are syntactically equivalent). Your problem stemmed from the confusion between "volatile pointer to struct" and "pointer to volatile struct". See the difference? In words it's fairly clear. In C, it has you - and me - running to your C manual's Table of Precedence.

I keep a copy of Dan Saks' article, "Const T vs. T Const" on hand for just exactly this case - it seems I always need to look it up.

And while you're there, check out some of his other articles as well. I learn something every time I read one.

• Gutenberg: The article is very helpful, particularly in the suggestion to place const and volatile to the right of other type specifiers to improve the right-to-left reading. Thanks!
• gilhad: Also interesting and simple insight is here - unixwiz.net/techtips/reading-cdecl.html