[Tiva] delayMicroseconds unreliable

[igor 163]

Two problems with delayMicroseconds on Tiva (and I believe on CC3200)
 
1) The delay interval is erratic, appearently because delayMicroseconds does not handle rollovers of the systick counter correctly.
 
2) delayMicroseconds can not work for dealys longer that 10,000 microseconds.  (Even though the documentation says that it works up to 16383, and even includes an example.)  
 
Details:
Issue 1)
 
Code to demonstrate problem:
 

#define DELAY_FOR  5000

void setup()
{
  Serial.begin(9600);
  Serial.print("Delay");
  Serial.println(DELAY_FOR);  
  Serial.print("elapsed tics should be: "); Serial.println(F_CPU/1000000);
}


void loop()
{
  unsigned long i = micros();
  unsigned long systicstart = HWREG(NVIC_ST_CURRENT);
  delayMicroseconds(DELAY_FOR);
  unsigned long systicstop = HWREG(NVIC_ST_CURRENT);
  unsigned long p = micros();  
  unsigned long o = p-i;


  Serial.print("Before: ");  Serial.print(i); Serial.print(" Systic: "); Serial.println(systicstart, HEX);
  Serial.print("After: ");  Serial.print(p); Serial.print(" Systic: "); Serial.println(systicstop, HEX);
  Serial.print("After-Before: ");  Serial.print(o);  Serial.print(" Systic: "); Serial.println(systicstart - systicstop, HEX);
  Serial.print("Error: "); Serial.print(((long int)DELAY_FOR) - ((long int)o)); Serial.print("\t Systic: "); Serial.println((long int)((systicstart - systicstop) & 0xFFFFFF)-((long int)(F_CPU/1000000)));
  Serial.println();
  
}

Just loops forever doing a delay, then printing how long the delay took (prints both in micros(), and ticks of the systick counter.  (Note that micros() does not use the systick timer.)
 
 
Example output:

Before: 8470600 Systic: B5751
After: 8475601 Systic: 53C94
After-Before: 5001 Systic: 61ABD
Error: -1  Systic: 399981


Before: 8599763 Systic: 2735
After: 8599893 Systic: C33E4
After-Before: 130 Systic: FFF3F351
Error: 4870  Systic: 15987457


Before: 8734135 Systic: 7055D
After: 8739137 Systic: EAA0
After-Before: 5002 Systic: 61ABD
Error: -2  Systic: 399981

While usually the delay is about 5000 microseconds, often it isn't (in the middle reading above the delay is much too short).
 
 
The code from wiring.c (this is for Tiva, the CC3200 code is somewhat similar).
 

void delayMicroseconds(unsigned int us)
{
volatile unsigned long elapsedTime;
unsigned long startTime = HWREG(NVIC_ST_CURRENT);
do{
elapsedTime = startTime-(HWREG(NVIC_ST_CURRENT) & 0x00FFFFFF);
}
while(elapsedTime <= us * (F_CPU/1000000));
}

 
There are a few suspect things about this code:
 
Since it is subtracting unsigned values, it seems to be the intention to rely on modular arithmatic to get the comparisons right.  (As in discussed in this page http://www.thetaeng.com/TimerWrap.htm ).
However as I understand it, for that to work the timer needs to overflow at the full width of the values being stored.  In this case the timer is at most a 24 bit timer, but the values are 32 bits.
If the full timer was being used, it might be possible to fix that by masking off the top 8 bits.

elapsedTime = (startTime-HWREG(NVIC_ST_CURRENT)) & 0x00FFFFFF;

However in this case the full width of the timer is not being used, it wraps to F_CPU/SYSTICKHZ after reaching 0 (it is a count down timer).
 
So it looks like every time the systick timer rolls over, any delayMicroseconds which is in progress will have an unexpected result.  (In the above output, where the delay was short, not the starting the starting value of NVIC_ST_CURRENT is small, and the ending value is close to C3500, which is F_CPU/SYSTICKHZ on a Tiva launchpad, i.e. the counter rolled over).
 
Small side issue: The code also seems to assume that the upper 8 bits of NVIC_ST_CURRENT will always be 0 (the manual warns against making assumptions about such bits).
A fix for that would be to add & 0x00FFFFFF to where startTime is set.
 
 
I do not have a particular suggestion of how to fix the code.
 
I think this issue also applies to the CC3200 (just from looking at the source code, but I have not checked that out in detail, and I do not have one to test.)
 
--
2) Since the systick counter rolls over every 10000 microseconds, delayMicroseconds can not delay for longer than that.  
.../energia-0101E0013/reference/DelayMicroseconds.html
The documentation says "Currently, the largest value that will produce an accurate delay is 16383."
(There is also an example which uses delays of this length.)
The documentation may be a holdover from Arduino, which says the same thing, even though it does not appear to apply to the Arduino Due, for instance.  (I do not know if this was true of MSP430.)

A possible fix for the delay duration issue would be to add something like the following to the beginning of delayMicroseconds

  if (us > 9000){
    delay( us / 1000);
    us = us % 1000;
  };

(the 9000 was somewhat arbitrary, rather than a constant it should be calculated based on SYSTICKHZ ).
 
Might also be well to clarify the documentation about which platform the 16383 relates to.

 
This is all on Energia 13, but I believe was also present in Energia 12.

[L.R.A 78]

Just quick question. In this version of Energia is the systick now used for the micros()? or still a general purpose timer?

[igor 163]

Just quick question. In this version of Energia is the systick now used for the micros()? or still a general purpose timer?

 

Still a general purpose timer on Tiva/Stellaris (Timer5).

(So in the example program above, the first column of output is using a different timer than the one that delayMicroseconds uses.)

 

On the CC3200, Systick is used for micros. 

http://forum.43oh.com/topic/5902-measure-short-amounts-of-time-with-micros/

 

A few additional notes:

 

If one wants to get the example program to misbehave more, increase the delay, for instance if you use 

#define DELAY_FOR  10000

Then pretty much every time through the loop you get a different delay.

 

 

The above problem means that delay, sleep, and sleepSeconds are also unreliable on Tiva/Stellaris (and probably delay on CC3200), since they work by repeatedly calling delayMicroseconds.

[igor 163]

@@energia Patched version of delayMicroseconds for lm4f/tiva wiring.c
 

void delayMicroseconds(unsigned int us)
{
// Systic timer rolls over every 1000000/SYSTICKHZ microseconds 
  if (us > 1000000UL/SYSTICKHZ/10*8){
    delay( us / 1000);  // delay milliseconds
    us = us % 1000;     // handle remainder of delay
  };

  unsigned long ticks = (unsigned long)us * (F_CPU/1000000UL);

  volatile unsigned long elapsedTime;
  // 24 bit timer - mask off undefined bits
  unsigned long startTime = HWREG(NVIC_ST_CURRENT) & NVIC_ST_CURRENT_M ;  

  if (ticks > startTime) {
    ticks = (ticks + (NVIC_ST_CURRENT_M - (unsigned long)F_CPU / SYSTICKHZ)) & NVIC_ST_CURRENT_M;
  }
  do{
    elapsedTime = (startTime-(HWREG(NVIC_ST_CURRENT) & NVIC_ST_CURRENT_M )) & NVIC_ST_CURRENT_M;
  }
  while(elapsedTime <= ticks);
}

Fixes both the limit on how long a delay can be requested, and handles the rollover issue.

Note that the magic number 0x00FFFFFF is replaced by NVIC_ST_CURRENT_M - the mask corresponding to NVIC_ST_CURRENT (from driverlib).

 

Some of the mask operations might not be strictly necessary.  The mask is used for two things - to get rid of the undefined bits in the counter register (in case they should ever be defined), and used to limit the register values for modular arithmetic.  We are just trying to waste time anyway, so efficiency isn't crucial.  This works, use caution if you try to tweak it.

 

Some of the typecasts may not be necessary (I was trying to be cautious to make sure it worked).

 

I think a similar approach would work on the CC3200.

 

Side note: TestDelays (in lm4f/variants/launchpad_129/tests/TestDelays) could be improved by performing the delays repeatedly.  (Doing delays once with relatively short value much less likely to find rollover issues.)

[spirilis 1,265]

Finally got some time to test this out.  Looking good so far!  Need to put it through a few more tests (using my Saleae Logic16 sampling at 100MHz with a simple GPIO write loop right now) before putting a pull req together.

[spirilis 1,265]

Per chance can you post example code removing some of the mask &'s?  I have noticed a ~1uS or less gain in the total time, which might be accounted by my ROM_GPIOPinWrite() call but could also be accounted for by the code.  I was reading through it but haven't quite thought through each operation yet.

[spirilis 1,265]

Tried delayMicroseconds with 768, 76, 4, and delay with 1, 6 so far ... Looks pretty good, and VERY consistent.  I did a control run with delayMicroseconds(768) before applying the patch and saw how inconsistent and screwed up the waveform looked.

 

Example code BTW-

void setup()
{
  // put your setup code here, to run once:
  pinMode(2, OUTPUT);
  digitalWrite(2, LOW);
}

void loop()
{
  static uint32_t val = 0;
  // put your main code here, to run repeatedly:
  
  while (1) {
    val ^= GPIO_PIN_5;
    ROM_GPIOPinWrite(GPIO_PORTB_BASE, GPIO_PIN_5, val);
    delay(6);
  }
}

Got the Saleae hooked to GND and PB5.

[spirilis 1,265]

Getting wonky results on CC3200, waveform has a short trough (230uS) and correctly-sized peak (769uS, close enough).  The old delayMicroseconds did the same thing though.  Restarting Energia (in case some remnants of the old code were being used) didn't help.

[igor 163]

Per chance can you post example code removing some of the mask &'s?  I have noticed a ~1uS or less gain in the total time, which might be accounted by my ROM_GPIOPinWrite() call but could also be accounted for by the code.  I was reading through it but haven't quite thought through each operation yet.

 

I haven't tested it for how exact it is in number of uS (the existing code varied so wildly I was just focused on something that would consistently give at least close to the desired delay).

I am sure there are other ways to fix it, I tried a couple of approaches and this was the first one that I managed to get to work (i.e. there is probably a better way).

 

Here is a version with the mask &'s that ensure that the upper 8 bits of the counter register will never be a problem removed.  This should still work (as long as the upper 8 bits of the counter register read as 0 - actually should work as long as those upper 8 bits read as a constant).  Not ideal from point of view of future proofing.  Is that what you had in mind?  (I have not tested the below code.)

 

Note that moving setting startTime to before calculating ticks (as I did below) might improve accuracy a little.

Doing away with elapsedTime (just put its value in the while statement) might also reduce overhead a smidge.

void delayMicroseconds(unsigned int us)
{
// Systic timer rolls over every 1000000/SYSTICKHZ microseconds 
  if (us > 1000000UL/SYSTICKHZ/10*8){
    delay( us / 1000);  // delay milliseconds
    us = us % 1000;     // handle remainder of delay
  };

  unsigned long startTime = HWREG(NVIC_ST_CURRENT) ;
  unsigned long ticks = (unsigned long)us * (F_CPU/1000000UL);

  volatile unsigned long elapsedTime;

  if (ticks > startTime) {
    ticks = (ticks + (NVIC_ST_CURRENT_M - (unsigned long)F_CPU / SYSTICKHZ)) & NVIC_ST_CURRENT_M;
  }
  do{
    elapsedTime = (startTime-(HWREG(NVIC_ST_CURRENT) )) & NVIC_ST_CURRENT_M;
  }
  while(elapsedTime <= ticks);
}

[igor 163]

This might improve accuracy - (this is same as the original fix I posted, but moves reading the start time as early as possible.) It will take a little extra overhead on longer delays (i.e. delays more than 8 milliseconds), but should be closer to the old code on short delays.

 

void delayMicroseconds(unsigned int us)
{
  unsigned long startTime = HWREG(NVIC_ST_CURRENT) ;

// Systic timer rolls over every 1000000/SYSTICKHZ microseconds 
  if (us > 1000000UL/SYSTICKHZ/10*8){
    delay( us / 1000);  // delay milliseconds
    startTime = HWREG(NVIC_ST_CURRENT) ;
    us = us % 1000;     // handle remainder of delay
  };

  // 24 bit timer - mask off undefined bits
  startTime &= NVIC_ST_CURRENT_M ;

  unsigned long ticks = (unsigned long)us * (F_CPU/1000000UL);

  volatile unsigned long elapsedTime;

  if (ticks > startTime) {
    ticks = (ticks + (NVIC_ST_CURRENT_M - (unsigned long)F_CPU / SYSTICKHZ)) & NVIC_ST_CURRENT_M;
  }
  do{
    elapsedTime = (startTime-(HWREG(NVIC_ST_CURRENT) & NVIC_ST_CURRENT_M )) & NVIC_ST_CURRENT_M;
  }
  while(elapsedTime <= ticks);
}

[igor 163]

I have noticed a ~1uS or less gain in the total time, which might be accounted by my ROM_GPIOPinWrite() call but could also be accounted for by the code.

If it continues to read consistently 1 us too long (after moving startTime setting), might think about that <= in the while loop (i.e. should it be a less than). I just copied that part of the code, but have not persuaded myself totally which way it should be (< vs <=).

 

I don't have ready access to equipment to independently measure this to that level of accuracy. Thank you for putting this through the wringer.

[spirilis 1,265]

I think that did tighten it up a bit!  I further shortened my GPIO flip into a HWREG operation and now it appears to come in with 0.8uS overhead total (so delayMicroseconds(7) in a while() loop with an XOR'd GPIO_O_DATA register shows ~7.8uS).

 

I like this a lot!  Thanks much @@igor and I am going to get this put into a pull request for the Energia github issue.

[spirilis 1,265]

 Ah one thing I need to vet out though is TM4C1294 support, since that runs at 120MHz.  I need to peek around at the code to see if something needs adjusting for that.

[spirilis 1,265]

These are confusing me a bit-

        // Systick timer rolls over every 1000000/SYSTICKHZ microseconds 

        if (us > 1000000UL/SYSTICKHZ/10*8) {

                delay(us / 1000);  // delay milliseconds

 

If I'm not mistaken, that means if us > 125 (1000000 / 100 / 80) then it tries to do a delay(125/1000), or a delay(0) in other words.  I think I'll just make that if (us > 999) since SYSTICKHZ is 100 anyhow (every 10ms).

edit: er no, that would cause an infinite loop, so it's if (us > 9999) ... that should work.

 

Or programmatically... 1000000UL / SYSTICKHZ - 1

 

Looks pretty good now, issued the pullreq.

[spirilis 1,265]

I think I spot the problem with the CC3200.

[ebrundic@spock cc3200]$ grep -i systick *
grep: driverlib: Is a directory
Energia.h:void initSysTick();
Energia.h:void registerSysTickCb(void (*userFunc)(uint32_t));
HardwareSerial.cpp:#include "driverlib/systick.h"
grep: inc: Is a directory
main.cpp:#include "driverlib/systick.h"
main.cpp:	MAP_SysTickIntEnable();
main.cpp:	MAP_SysTickPeriodSet(F_CPU / 1000);
main.cpp:	MAP_SysTickEnable();
startup_gcc.c:extern void SysTickIntHandler(void);
startup_gcc.c://extern void xPortSysTickHandler(void);
startup_gcc.c:    SysTickIntHandler,                      // The SysTick handler
wiring.c:#include "driverlib/systick.h"
wiring.c:static void (*SysTickCbFuncs[8])(uint32_t ui32TimeMS);
wiring.c:#define SYSTICKMS               (1000 / SYSTICKHZ)
wiring.c:#define SYSTICKHZ               100
wiring.c:#define SYSTICK_INT_PRIORITY    0x80
wiring.c:	return (milliseconds * 1000) + (SysTickValueGet() / (F_CPU/1000000));
wiring.c:	// Systick timer rolls over every 1000000/SYSTICKHZ microseconds 
wiring.c:	if (us > 1000000UL/SYSTICKHZ/10*8) {
wiring.c:		ticks = (ticks + (NVIC_ST_CURRENT_M - (unsigned long)F_CPU / SYSTICKHZ)) & NVIC_ST_CURRENT_M;
wiring.c:void registerSysTickCb(void (*userFunc)(uint32_t))
wiring.c:		if(!SysTickCbFuncs[i]) {
wiring.c:			SysTickCbFuncs[i] = userFunc;
wiring.c:void SysTickIntHandler(void)
wiring.c:		if (SysTickCbFuncs[i])
wiring.c:			SysTickCbFuncs[i](SYSTICKMS);

Main.cpp treats SYSTICKHZ as 1000, hardcoded:

main.cpp: MAP_SysTickPeriodSet(F_CPU / 1000);

 

But wiring.c still has SYSTICKHZ set to 100:

wiring.c:#define SYSTICKMS               (1000 / SYSTICKHZ)

wiring.c:#define SYSTICKHZ               100

wiring.c:#define SYSTICK_INT_PRIORITY    0x80

 

Can't imagine why you'd want 1ms SysTick pulses - @@energia any insight on this?  It would totally break the concept of doing delayMicroseconds(1000) for delay (you'd trip the SysTick overflow guaranteed with unpredictable delays resulting), and would explain why @@igor 's patch for delayMicroseconds blows up there.  If there is no tangible reason why we need 1ms SysTick overflows (e.g. perhaps it's needed for the WiFi subsystem code somehow? haven't looked yet), I am going to redo that so it uses the SYSTICKHZ define somehow, resulting in 10ms ticks and delay code that works identically to the Tiva.