volatile elapsedMillis on Teensy 4.0?

J

jonahpearl

New member
Hi all -- I'm hoping to have an elapsedMillis variable be reset inside of an interrupt callback, like this:

Code: 
const int trigger_pin = 17; // listens for trigger
volatile elapsedMillis sinceTrigger;
volatile long int trigger_count = 0;

const int min_offset_from_trigger_msec = 10;
const int max_offset_from_trigger_msec = 30;
const bool various_conditions = true;

void setup() {

  Serial.begin(9600);
  pinMode(trigger_pin, INPUT);
  digitalWrite(trigger_pin, HIGH);
  attachInterrupt(digitalPinToInterrupt(trigger_pin), countTriggerOne, RISING);

}

void loop() {
  // put your main code here, to run repeatedly:

  if (
    (various_conditions) and 
    (sinceTrigger >= min_offset_from_trigger_msec) and 
    (sinceTrigger <= max_offset_from_trigger_msec)
    ){
    do_something();
  }
}

void do_something(){
  Serial.println("foo");
}

void countTriggerOne()
{
  sinceTrigger = 0;
  trigger_count++;
}

When I try to declare the elapsedMillis as volatile, I get an error that seems to indicate that comparison operation isn't defined for this combination of variable types.

Code: 
volatile_elapsedMillis_example:24: error: no match for 'operator>=' (operand types are 'volatile elapsedMillis' and 'const int')
     (sinceTrigger >= min_offset_from_trigger_msec) and 
                   ^
/Users/jonahpearl/Documents/Arduino/volatile_elapsedMillis_example/volatile_elapsedMillis_example.ino:24:19: note: candidate: operator>=(long unsigned int, int) <built-in>
/Users/jonahpearl/Documents/Arduino/volatile_elapsedMillis_example/volatile_elapsedMillis_example.ino:24:19: note:   conversion of argument 1 would be ill-formed:
/Users/jonahpearl/Documents/Arduino/volatile_elapsedMillis_example/volatile_elapsedMillis_example.ino:24:22: warning: passing 'volatile elapsedMillis' as 'this' argument discards qualifiers [-fpermissive]
     (sinceTrigger >= min_offset_from_trigger_msec) and 
                      ^
In file included from /private/var/folders/x7/04jny8k55mv6gr_jtz4kh05c0000gn/T/AppTranslocation/72E6703A-E80D-43F8-985A-CA1173953BBF/d/Teensyduino.app/Contents/Java/hardware/teensy/avr/cores/teensy3/WProgram.h:71:0,
                 from /var/folders/x7/04jny8k55mv6gr_jtz4kh05c0000gn/T/arduino_build_371018/pch/Arduino.h:6:
/private/var/folders/x7/04jny8k55mv6gr_jtz4kh05c0000gn/T/AppTranslocation/72E6703A-E80D-43F8-985A-CA1173953BBF/d/Teensyduino.app/Contents/Java/hardware/teensy/avr/cores/teensy3/elapsedMillis.h:42:2: note:   in call to 'elapsedMillis::operator long unsigned int() const'
  operator unsigned long () const { return millis() - ms; }

Would it be an easy fix to define the relevant operations, or is there something more deeply complicated about defining ops for volatile vars / is there something wrong about the way I've declared the vars?

(I suspect that declaring the elapsedMillis as volatile is overkill for my particular application, i.e. since I give it many msec of buffer anyways, it won't matter if it gets updated one loop iteration late. But I'm curious because I could imagine cases where it might matter to be able to quickly update a timer based on an incoming trigger!)
 
Thanks Paul! Appreciate the help.
s-l1600.jpg
 
I'm not seeing `volatile` in teensy4/elapsedMillis.h (or in teensy3's version). Am I missing something?

I was going to suggest these other alternatives:
1. C++'s const_cast<>() can remove volatility, but knowing what you're doing with that is important.
2. Use a `volatile uint32_t` with `millis()` instead of using `elapsedMillis`, and then do the subtraction comparison yourself.
 
shawn said:
Am I missing something?
Click to expand...

In delay.c

Code: 
[COLOR="#FF0000"]volatile[/COLOR] uint32_t systick_millis_count = 0;
[COLOR="#FF0000"]volatile[/COLOR] uint32_t systick_cycle_count = 0;
[COLOR="#FF0000"]volatile[/COLOR] uint32_t scale_cpu_cycles_to_microseconds = 0;

and in core_pins.h

Code: 
extern [COLOR="#FF0000"]volatile[/COLOR] uint32_t F_CPU_ACTUAL;
extern [COLOR="#FF0000"]volatile[/COLOR] uint32_t F_BUS_ACTUAL;
extern [COLOR="#FF0000"]volatile[/COLOR] uint32_t scale_cpu_cycles_to_microseconds;
extern [COLOR="#FF0000"]volatile[/COLOR] uint32_t systick_millis_count;
 
I don’t agree that elapsedMillis is volatile-safe unless its internal `unsigned long ms` variable is also volatile. It’s true that `millis()` is volatile-safe, but elapsedMillis is not.

Both its assignment operators and its retrieval operators use the internal `ms` variable, and that could, in theory, be a cached value.

The p#2 note doesn’t provide an example, and the p#5 note only addresses `millis()` internals and not elapsedMillis internals. An expression that uses a volatile variable doesn’t necessarily make the whole expression volatile; that’s what p#2 implies and is what I’m disagreeing with. This statement: "code that relies on p#5 volatile vars" doesn't make the code itself volatile-safe.

Example:
Code: 
int a = 3;
volatile int b = 2;  // Similar to the volatile millis() variable, 'systick_millis_count'

...
Interrupt changes non-volatile 'a' to 7 <-- Similar to an interrupt changing the non-volatile elapsedMillis::ms
...

int c = a + b;  // c might contain 5 and not necessarily 9

Source: All elapsedMillis functions rely on a non-volatile `unsigned long ms` variable:
https://github.com/PaulStoffregen/cores/blob/master/teensy4/elapsedMillis.h

The code for retrieving the value:
Code: 
return millis() - ms;

The `ms` part of that expression is non-volatile, and hence might contain a cached copy.

The code for setting the value:
Code: 
ms = millis() - val; return *this;

This stores the value into the non-volatile `ms`, in your case in an interrupt. Its retrieval from the non-interrupt context might not detect the change.
 
Last edited:
I think Shawn is right. Below a simple example demonstrating the problem. A 1s timer is resetting the elapsedMillis variable `stopwatch` in its callback. In a tight loop the LED is switched on when the stopwatch value gets larger than 500ms. If the code uses the actual value of the stopwatch, the LED should blink with 1Hz. If it doesn't recognize that the stopwatch was resetted, the LED should be permantently on after 500ms.

Code: 
IntervalTimer t1;
elapsedMillis stopwatch;

void reset()
{
    stopwatch = 0;
}

void setup()
{
    pinMode(LED_BUILTIN, OUTPUT);
    t1.begin(reset, 1'000'000); // reset the stopwatch every second
}

void loop()
{
    while (1)
    {
        digitalWriteFast(LED_BUILTIN, stopwatch > 500); // should blink with 50% duty cycle but doesn't
        //Serial.println(stopwatch);                      // uncomment to make it work
    }
}

Result: Looks like the compiler is super conservative and tends to read out the correct value of the stopwatch whenever something "complicated" happens between reads (e.g. if you print out the value, or do a delay etc). If you just use the stopwatch value in a tight loop, the generated code does not notice the resetting in the background (as expected, since ms in elapsedMillis is not volatile). So, for "normal" cases it seems to behave as if 'ms' was volatile, but you can certainly write code which fails.

If I delclare "ms" as "volatile unsigned long" in elapsedMillis.h it works in all cases.
 
Last edited:
luni said:
Result: Looks like the compiler is super conservative and tends to read out the correct value of the stopwatch whenever something "complicated" happens between reads (e.g. if you print out the value, or do a delay etc). If you just use the stopwatch value in a tight loop, the generated code does not notice the resetting in the background (as expected, since ms in elapsedMillis is not volatile). So, for "normal" cases it seems to behave as if 'ms' was volatile, but you can certainly write code which fails.
Click to expand...

stopwatch is a public (non-static) global variable so any external function call (digitalWriteFast is inlined and defined in a header, println is not) could potentially modify it, meaning it can't be cached in that case. Do you get the same behavior if it's declared static?
 
stopwatch is a public (non-static) global variable so any external function call (digitalWriteFast is inlined and defined in a header, println is not) could potentially modify it, meaning it can't be cached in that case.
Click to expand...
This is certainly a better definition than "something complicated" :)
But, even if I delcare stopwartch static, the println will trigger a reload of ms. Anyway, I think the example shows that elapsedMillis is not "implicitely volatile" as Shawn pointed out.
 
luni said:
This is certainly a better definition than "something complicated" :)
But, even if I delcare stopwartch static, the println will trigger a reload of ms. Anyway, I think the example shows that elapsedMillis is not "implicitely volatile" as Shawn pointed out.
Click to expand...

Maybe, but it could be argued that they're not using elapsedMillis in the intended way either - better to only read from it in the interrupt handler and assign to a volatile variable for later comparison, since other possible operations on elapsedMillis (e.g. += or -=) are not going to be atomic even if ms is volatile.
 
I have absolutely no stakes in it. I was just interested if Pauls statement from #2 is correct. If this behaviour of elapsedMillis should be fixed / documentend / ignored is another question which needs to be decided by others.
 
luni said:
I have absolutely no stakes in it. I was just interested if Pauls statement from #2 is correct. If this behaviour of elapsedMillis should be fixed / documentend / ignored is another question which needs to be decided by others.
Click to expand...

It sounds like making "ms" volatile is the safer way to go.
 
Making a structure’s or class’s members volatile disables the ability to use the members as non-volatile; this is not desired. The correct approach is to add volatile-labelled functions, which tells the compiler to treat internal members as volatile. This would approximately double the number of functions, however.

I don’t think there’s a problem to fix here. If you really want to use elapsedMillis in a volatile-requiring way, then either:
1. Add volatile versions of all the needed elapsedMillis functions, or
2. Don't use elapsedMillis; use your own `volatile uint32_t` to measure `milis()` and elapsed time differences yourself.

Also highlighting @jmarsh's "atomic" comment:
jmarsh said:
other possible operations on elapsedMillis (e.g. += or -=) are not going to be atomic even if ms is volatile.
Click to expand...

Voltaile doesn't imply atomic, unfortunately.

Volatile struct/class example

An example of a volatile function (in elapsedMillis) follows; this would be in addition to the non-volatile version, as both are needed:
Code: 
class elapsedMillis {
  // ...
  elapsedMillis & operator = (unsigned long val) { ms = millis() - val; return *this; }
  elapsedMillis & operator = (unsigned long val) [B]volatile[/B] { ms = millis() - val; return *this; }
  // ...
};

If an elapsedMillis variable were marked volatile, then the volatile versions of the functions would be used. For example:
Code: 
elapsedMillis x;
volatile elapsedMillis y;

x = 0;  // <-- Uses non-volatile assignment operator
y = 0;  // <-- Uses volatile assignment operator
 
Last edited:
My comment about the non-atomic operations was to point out that even with volatile, there's still going to be a problem lurking waiting to bite someone if they insist on modifying elapsedMillis in a situation where they really shouldn't (e.g. an interrupt handler).
Consider a case where the main execution path contains "ms += 5" and an interrupt handler contains "ms=0":
Code: 
   MAIN THREAD                         INTERRUPT ROUTINE
loads ms from memory to register
*** Interrupt occurs *** --------->
                                      clears a register
                                      store register to ms memory
                         <--------    *** Interrupt ends ***
add 5 to register
store register to ms memory

In this case the modification of ms by the interrupt handler would be completely disregarded, even though it's being correctly treated as volatile.

tl,dr; altering elapsedMillis to be volatile would just be giving a false sense of security.
 
Its always a better approach I think to leave the main clock alone and take snapshots of it if you want relative timings. That way different libraries and bits of code can share the clock without any contention/crosstalk.

The exception might be for running unit-tests over clock-dependent code.
 
You must log in or register to reply here.
Back
Top