micros() is non-monotonic with -1ms glitches on Cortex cores from priority 0 or 1 ISRs

[colintd]

Whilst using micros() to timestamp hardware events on a SAMD21 processor, I've discovered that the function is broken if called from priority 0 or 1 ISRs. Specifically, from time to time, the result is 1ms behind, making the value returned non-monotonic and inaccurate.

Looking at the code, the micros() function has logic to determine if there is a pending SysTick ISR, and adds 1ms() if the SysTick counter has rolled, but the tick increment ISR has not been called. This works fine when the SysTick ISR has priority 0, as it is either pended (if another priority 0 ISR is running already), or runs to completion.

However, pull request #182, related to adding I2S support, reduced the SysTick ISR to priority 2 (from priority 0).

/cores/arduino/wiring.c

NVIC_SetPriority (SysTick_IRQn,  (1 << __NVIC_PRIO_BITS) - 2);  /* set Priority for Systick Interrupt (2nd lowest) */

This means that a priority 0 or 1 ISR (such as that generated by a GPIO interrupt) can preempt an already running SysTick ISR. Under these conditions the logic to add the required 1ms never activates (as the ISR is active not pending ).

If the SysTick ISR has already incremented the tick count, all is fine. If it hasn't, the result of micros() effectively loses 1ms, which makes the result inaccurate and non-monotonic. (Note, even if you checked whether the SysTick ISR is active, it isn't possible to determine whether the tick count has or hasn't been incremented from within micros())

I'm hitting the problem timestamping precision edges, and see the issue whenever an edge aligns with a roll of the SysTick. With 1PulsePerSecond edges I get the incorrect result a few times an hour, as the wander of the main clock leads to the SysTick roll aligning with the external edge.

Alternatively, I suspect that if you use a non-precision 1KHz+ input, you will see frequent 1ms backsteps.

I'm not aware of why it was felt necessary to reduce the priority of SysTick to get I2S to work, but I can confirm that restoring SysTick to priority 0 returns micros() to being monotonic and accurate from ISRs of priorities 0 & 1. I'd suggest that given the critical nature of the micros() function, SysTick should be restored to priority 0, and an alternative solution found for the I2S problem(s).

[colintd]

If I2S breaks with the SysTick interrupt at priority 0, does that mean it will also break if any external GPIO interrupts are generated (or in fact any other priority 0 or 1 interrupts)? If that is the limitation, and it really can't be fixed, perhaps the I2S setup function should dynamically lower the SysTick priority rather than it being a default change?

[colintd]

This sound like the same problem https://forum.arduino.cc/index.php?topic=611189.0

[sslupsky]

@colintd Looking at the commit history for that change it may have something to do with the DMA.

I do not think the SysTick interrupt is broken. Rather, micros() appears to have a bug where it introduces jitter if an interrupt occurs during the call to micros(). I had a look at micros() and it seems to me the correct way to calculate micros() would be to guard the SysTick register and ms counter to ensure coherency during read so that an interrupt cannot cause jitter?

For example:

unsigned long micros( void )
{
  volatile uint32_t ticks;
  volatile uint32_t count;

  noInterrupts();
  ticks  = VARIANT_MCK / 1000 - SysTick->VAL;
  count  = _ulTickCount;
  interrupts();

  return (count * 1000) + ((ticks*(1048576/(VARIANT_MCK/1000000)))>>20) ;
  // this is an optimization to turn a runtime division into two compile-time divisions and
  // a runtime multiplication and shift, saving a few cycles
}

[sslupsky]

The comment at the beginning of the micros() function indicating it is "interrupt compatible" may be misleading. I think someone was thinking micros() could be crafted so that if an interrupt occurs, then they detect it and correct it. This is quite simply impossible because the non atomic register access imposes race conditions. No amount of "looping" around will guarantee that you avoid the race ... I think this is what you have bumped into.

I looked at the commit history and unfortunately, this version of micros() was in the first commit so there is no insight to be gained regarding the history of this function.

[sslupsky]

I updated my code snippet to ensure that "ticks" and "count" are treated as volatile.

There is still one potential race condition though. If the SysTick interrupt is being processed and a higher priority interrupt occurs, if the ISR of the higher priority interrupt calls micros(), then it is possible that the SysTick counter rolled over but the ms tick count has not yet incremented. In this case, the time would appear to briefly jump back by up to 1ms. The interrupt pending indication will only tell you that it was in the SysTick ISR but you do not know where so you cannot know if the ms counter has been incremented yet. There might be a way to compensate if you examined the program counter on the stack so you can derive where it was interrupted. Otherwise, it is probably best to avoid using micros() in an ISR?

[colintd]

Thanks for looking at this.

I agree with some but not all of your analysis.

You're right that if SysTick can be interrupted/pre-empted, then it is impossible to figure out if the tick count has been incremented, and I 100% agree that looping will not resolve the issue.

However, the point is that when SysTick was priority 0, it wasn't possible for the SysTick to be preempted. It would either run to completion or be pended. With the old code any other caller to micros (including from an ISR) would either see the result of a completed update (so get a correct micros value), or would see that the counter had rolled and a SysTick ISR call was pending (in which case there would be a +1ms and they would again get the right value for micros()).

This means that as long as SysTick can't be preempted, then the micros() function is 100% ISR compatible (as suggested by the comment).

The reduction in the SysTick priority introduced the third case of the SysTick ISR starting to run, and then being pre-empted. This is the case which leads to the -1ms steps, and we're agreed that no amount of looping or extra variable can close this condition.

I've done extensive testing with SysTick restored to the old priority, and timestamping from ISRs works perfectly. I can also say from a couple of decades of experience in embedded systems and working with multi-level ISRs (from Z80's all the way up to the largest multi-chip, multi-core NUMA systems), that the wrap related SysTick routines are almost always set to one of the highest level interrupts in the system, and that this is to avoid precisely the kind of problem that I've found when SysTick was reduced in priority.

If it were my choice, I would probably tie SysTick to the NMI vector, because this would ensure micros() and millis() would give seamless timestamping even in long running ISRs or with interrupts masked. I recognize, however, this is a larger change!

I agree that not using micros() from ISR means you won't hit the problem, but there are lots and lots of cases where timestamping external events on interrupts is critical to system operation. You can find thousands of sketches out there which do exactly this. The current priority of SysTick probably makes all of them unreliable, as not only is the value returned by micros() inaccurate, it is also non-monotonic, meaning that comparing timestamps can easily suggest time has gone backwards (or forwards by over an hour)

Given that we used to have a working micros() before the introduction of I2S support, and given that I2S is little used compared to simple timestamping in interrupts, I'd very strongly argue that the default priority for SysTick should be restored such that micros() again works properly.

If this change breaks I2S, then I'd have to assume I2S is also broken if there is any use of GPIO related ISR (which are also bound to priority 0)? In this case, selectively reducing the SysTick() priority if I2S is started would at least limit the extend of the problem, but there should also be a comment saying that use of GPIO ISRs will also break I2S support.

Having said that, the SAMD21 DMA support is generally pretty good, and is designed to work in combination with the NVIC. As such, I'd very much expect that it can be made to work with higher priority interrupts.

Do you have any history on exactly what was "fixed" by reducing the SysTick priority?

[colintd]

Btw, with the SysTick restored to the original priority, there is absolutely no need to have noInterrupt and Interrupt round the access in micros(). As written it is 100% correct, and works perfectly.

[sslupsky]

I tried to figure out what was "fixed" but it is not clear ... its part of some DMA work that was done so that is why I presume it had something to do with the DMA. My "guess" is that the tick was causing hiccups with the DMA transfers but I have no certainty that is the case. @sandeepmistry might be able to shed some light on this.

I do not disagree with increasing the priority level of the SysTick interrupt as it does solve the issue. The SAMD SysTick was poorly designed at the hardware level and there are a few shortcomings. This issue is one of them. Another issue is that SysTick is shut off during sleep which breaks micros() and millis().

If you restored the priority level, which is more efficient? All that pending interrupt looping stuff with additional memory or the atomic read?

[colintd]

I’d very strongly go for the existing code structure, for several reasons:

• you only hit the loop under a very small number of edge conditions when the SysTick interrupts a lower priority caller
• by avoiding the noInterrupt/interrupts calls you avoid normal priority code disturbing the timing of ISR
• you get cleaner behaviour with nested layers of interrupts
• most importantly, it was well tested code with no visible problems until the I2S change, so I rather leave as it.

Does that make sense?

From: sslupsky [email protected] Sent: 24 October 2019 23:40 To: arduino/ArduinoCore-samd [email protected] Cc: Colin Tregenza Dancer [email protected]; Mention [email protected] Subject: Re: [arduino/ArduinoCore-samd] micros() is non-monotonic with -1ms glitches on Cortex cores from priority 0 or 1 ISRs (#463)

I tried to figure out what was "fixed" but it is not clear ... its part of some DMA work that was done so that is why I presume it had something to do with the DMA. My "guess" is that the tick was causing hiccups with the DMA transfers but I have no certainty that is the case. @sandeepmistryhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsandeepmistry&data=02%7C01%7Cctd%40metaswitch.com%7C884991df9f1744f079a108d758d30b37%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637075535758348639&sdata=249dbL1igFYcnBqqpHpN%2Fcev%2BGVT36sVfAsvZNm6zlk%3D&reserved=0 might be able to shed some light on this.

I do not disagree with increasing the priority level of the SysTick interrupt as it does solve the issue. The SAMD SysTick was poorly designed at the hardware level and there are a few shortcomings. This issue is one of them. Another issue is that SysTick is shut off during sleep which breaks micros() and millis().

If you restored the priority level, which is more efficient? All that pending interrupt looping stuff with additional memory or the atomic read?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Farduino%2FArduinoCore-samd%2Fissues%2F463%3Femail_source%3Dnotifications%26email_token%3DABXJWNF2VGVWMBMJBH4RES3QQIP2LA5CNFSM4JBNK5KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECGUWPY%23issuecomment-546130751&data=02%7C01%7Cctd%40metaswitch.com%7C884991df9f1744f079a108d758d30b37%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637075535758348639&sdata=11ln%2BHROUF%2F2cRYwpb95E9Mt11WzzpxqYpLSM6Z8YCE%3D&reserved=0, or unsubscribehttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABXJWNC4UA2PIZF5A4L2KJ3QQIP2LANCNFSM4JBNK5KA&data=02%7C01%7Cctd%40metaswitch.com%7C884991df9f1744f079a108d758d30b37%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637075535758358638&sdata=hwYjC%2Fq8sKBRg%2FCtuXAvOe6dBI3gxKD3GS23VM%2Fkaf8%3D&reserved=0.

[sslupsky]

I think so.

[colintd]

Excellent.

As I said I've been testing for the last couple of weeks, including with high frequency gpio isrs. With the low priority systick I'd hit the problem reliably in under a minute. Restoring to priority 0 I've never see the -1ms jump, and I'm getting ISR timing with RMS jitter under 15us from a precision 1pps source.

The pain is that the low priority systick has now been copied into lots of variant files, but I guess we've got to start somewhere with the fixing.

Do you want to apply the change, or do you need a pull request from me?

Cheers,

Colin.

Sent from my Samsung Galaxy smartphone.

-------- Original message -------- From: sslupsky [email protected] Date: 25/10/2019 00:19 (GMT+00:00) To: arduino/ArduinoCore-samd [email protected] Cc: Colin Tregenza Dancer [email protected], Mention [email protected] Subject: Re: [arduino/ArduinoCore-samd] micros() is non-monotonic with -1ms glitches on Cortex cores from priority 0 or 1 ISRs (#463)

I think so.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Farduino%2FArduinoCore-samd%2Fissues%2F463%3Femail_source%3Dnotifications%26email_token%3DABXJWNGBJ5W5RC7INMW6MDLQQIUQBA5CNFSM4JBNK5KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECGW2MI%23issuecomment-546139441&data=02%7C01%7Cctd%40metaswitch.com%7C5ca6897687114ceba04208d758d89e5e%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637075559707477758&sdata=NCl%2BlbOiOb7bFxrocGDZxEJzYNZikmH7CnHoO4wcYnw%3D&reserved=0, or unsubscribehttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABXJWNGMCCBBGGWX5OL3LTTQQIUQBANCNFSM4JBNK5KA&data=02%7C01%7Cctd%40metaswitch.com%7C5ca6897687114ceba04208d758d89e5e%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637075559707477758&sdata=zM2qBDOiWjbpIDXnPfUXlDR%2FcRALv0LOzA9sqZCn9qc%3D&reserved=0.

[colintd]

Actually in latest testing (a few tweaks to my code), I'm down to ~5us RMS jitter using the micros() code with restored priority.. This is perfectly acceptable performance for most uses.

[sslupsky]

@colintd I am not the maintainer so I cannot merge anything into this repo. Likely either @sandeepmistry or @facchinm will review this but it may take a while. The more information posted here about this the more likely they will ask for a PR. I think it would be a good idea to submit the PR referencing this issue.

[matthijskooijman]

I just read up on this issue and I agree with the analysis (though I haven't looked at the code myself). I also think the proposed solution (restore the systick priority to 0 and fix I2S in another way if possible). I also think the current code with pending interrupt looping should be preserved, since I do not think that the atomic micros() helps in all cases (it won't help when micros() is called from an interrupt that preempted the systick ISR AFAICS).

I'm no maintainer either, but I 2wo

The pain is that the low priority systick has now been copied into lots of variant files, but I guess we've got to start somewhere with the fixing.

Really? Isn't the only changed needed to remove https://github.com/sandeepmistry/ArduinoCore-samd/blob/284bc5a3185756cf6d107c002cc8841d772d07d4/cores/arduino/wiring.c#L66 ?

[colintd]

@matthijskooijman I agree wiring.c is the root change, but doing a quick grep of the installed code on my system for NVIC_SetPriority then SysTick_IRQn I can see this change has propagates in lots of SysTick_Config functions, bootloader and the adafruit cores files (see below).

packages/arduino/hardware/samd/1.8.3/bootloaders/sofia/Bootloader_D21_Sofia_V2.1/src/ASF/thirdparty/CMSIS/Include/core_cm0plus.h:  NVIC_SetPriority (SysTick_IRQn, (1<<__NVIC_PRIO_BITS) - 1);  /* set Priority for Systick Interrupt */
packages/arduino/hardware/samd/1.8.3/bootloaders/mzero/Bootloader_D21/src/ASF/thirdparty/CMSIS/Include/core_cm0plus.h:  NVIC_SetPriority (SysTick_IRQn, (1<<__NVIC_PRIO_BITS) - 1);  /* set Priority for Systick Interrupt */
packages/arduino/hardware/samd/1.8.3/cores/arduino/wiring.c:  NVIC_SetPriority (SysTick_IRQn,  (1 << __NVIC_PRIO_BITS) - 2);  /* set Priority for Systick Interrupt (2nd lowest) */
packages/arduino/tools/CMSIS/4.5.0/CMSIS/Include/core_cm4.h:  NVIC_SetPriority (SysTick_IRQn, (1UL << __NVIC_PRIO_BITS) - 1UL); /* set Priority for Systick Interrupt */
packages/arduino/tools/CMSIS/4.5.0/CMSIS/Include/core_cm3.h:  NVIC_SetPriority (SysTick_IRQn, (1UL << __NVIC_PRIO_BITS) - 1UL); /* set Priority for Systick Interrupt */
packages/arduino/tools/CMSIS/4.5.0/CMSIS/Include/core_sc000.h:  NVIC_SetPriority (SysTick_IRQn, (1UL << __NVIC_PRIO_BITS) - 1UL); /* set Priority for Systick Interrupt */
packages/arduino/tools/CMSIS/4.5.0/CMSIS/Include/core_cm0plus.h:  NVIC_SetPriority (SysTick_IRQn, (1UL << __NVIC_PRIO_BITS) - 1UL); /* set Priority for Systick Interrupt */
packages/arduino/tools/CMSIS/4.5.0/CMSIS/Include/core_sc300.h:  NVIC_SetPriority (SysTick_IRQn, (1UL << __NVIC_PRIO_BITS) - 1UL); /* set Priority for Systick Interrupt */
packages/arduino/tools/CMSIS/4.5.0/CMSIS/Include/core_cm7.h:  NVIC_SetPriority (SysTick_IRQn, (1UL << __NVIC_PRIO_BITS) - 1UL); /* set Priority for Systick Interrupt */
packages/arduino/tools/CMSIS/4.5.0/CMSIS/Include/core_cm0.h:  NVIC_SetPriority (SysTick_IRQn, (1UL << __NVIC_PRIO_BITS) - 1UL); /* set Priority for Systick Interrupt */
packages/adafruit/hardware/samd/1.5.4/bootloaders/sofia/Bootloader_D21_Sofia_V2.1/src/ASF/thirdparty/CMSIS/Include/core_cm0plus.h:  NVIC_SetPriority (SysTick_IRQn, (1<<__NVIC_PRIO_BITS) - 1);  /* set Priority for Systick Interrupt */
packages/adafruit/hardware/samd/1.5.4/bootloaders/mzero/Bootloader_D21/src/ASF/thirdparty/CMSIS/Include/core_cm0plus.h:  NVIC_SetPriority (SysTick_IRQn, (1<<__NVIC_PRIO_BITS) - 1);  /* set Priority for Systick Interrupt */
packages/adafruit/hardware/samd/1.5.4/cores/arduino/wiring.c:  // NVIC_SetPriority (SysTick_IRQn,  (1 << __NVIC_PRIO_BITS) - 2);  /* set Priority for Systick Interrupt (2nd lowest) */

Even in the main ArduinoCore-samd.git repo, I can see 3 places the priority is set:

bootloaders/sofia/Bootloader_D21_Sofia_V2.1/src/ASF/thirdparty/CMSIS/Include/core_cm0plus.h:  NVIC_SetPriority (SysTick_IRQn, (1<<__NVIC_PRIO_BITS) - 1);  /* set Priority for Systick Interrupt */
bootloaders/mzero/Bootloader_D21/src/ASF/thirdparty/CMSIS/Include/core_cm0plus.h:  NVIC_SetPriority (SysTick_IRQn, (1<<__NVIC_PRIO_BITS) - 1);  /* set Priority for Systick Interrupt */
cores/arduino/wiring.c:  NVIC_SetPriority (SysTick_IRQn,  (1 << __NVIC_PRIO_BITS) - 2);  /* set Priority for Systick Interrupt (2nd lowest) */

I'm very tempted to add a

#define SYSTICK_DEFAULT_PRIORITY 0

in one of the top headers, and then change all 3 references to use that value. That will then provide a consistent basis for others to bring their variants into line.

Seems reasonable?

[sslupsky]

Can you tell when the changes to CMSIS occurred? That is troubling.

Perhaps @sandeepmistry made the original changes simply to bring the priority level inline with the CMSIS library?

[sslupsky]

I cannot think of a reason that this change would have an adverse affect on the bootloaders. It would probably be a good idea to ask @ladyada from Adafruit to loop someone into this discussion.

[colintd]

These files never seem to have changed, but the initial commits were after the I2S tweak, so I assume they just copied the “default” from there.

From: sslupsky [email protected] Sent: 28 October 2019 14:07 To: arduino/ArduinoCore-samd [email protected] Cc: Colin Tregenza Dancer [email protected]; Mention [email protected] Subject: Re: [arduino/ArduinoCore-samd] micros() is non-monotonic with -1ms glitches on Cortex cores from priority 0 or 1 ISRs (#463)

Can you tell when the changes to CMSIS occurred? That is troubling.

Perhaps @sandeepmistryhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fsandeepmistry&data=02%7C01%7Cctd%40metaswitch.com%7C40505039e21648c7193308d75bb0284e%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637078684457533993&sdata=ktToRSoQH1XYjXt0dxrMygecDvegHLUZMKX5q7hrSSs%3D&reserved=0 made the original changes simply to bring the priority level inline with the CMSIS library?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Farduino%2FArduinoCore-samd%2Fissues%2F463%3Femail_source%3Dnotifications%26email_token%3DABXJWNC6XZQGWD3HL6AIZKLQQ3WZVA5CNFSM4JBNK5KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECM7XWY%23issuecomment-546962395&data=02%7C01%7Cctd%40metaswitch.com%7C40505039e21648c7193308d75bb0284e%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637078684457533993&sdata=qOVdB1Yc4lLvUZ%2BhbISxHmXu3zjOJ6GqbPGa3K%2FAuho%3D&reserved=0, or unsubscribehttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABXJWNAE5CBYHC5HD4MK7NTQQ3WZVANCNFSM4JBNK5KA&data=02%7C01%7Cctd%40metaswitch.com%7C40505039e21648c7193308d75bb0284e%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637078684457543986&sdata=hAqCtSGoaQgbVC7dxwcLVypoQjdvT%2F9NmNNGFtO9y38%3D&reserved=0.

[matthijskooijman]

I can imagine that all these CMSIS mentions are in code that is not actually used. Perhaps you could check that?

As for the bootloader / CMSIS code, as long as you never use the timer from an interrupt and only read the timer values atomically, I guess having a low priority for the systick ISR is not a problem, so maybe that's why that's the default there?

[colintd]

From the file timings, I suspect these values are just copies of what was in wiring.c at the point they were created, rather than a deliberate decision to use a lower priority.

Similarly, from the history I’m fairly sure the adafruit value is just a merge of the mainline wiring.c change.

My instinct is therefore to change all to use the value of 0, which was the original in wiring.c for a long time.

From: Matthijs Kooijman [email protected] Sent: 28 October 2019 14:26 To: arduino/ArduinoCore-samd [email protected] Cc: Colin Tregenza Dancer [email protected]; Mention [email protected] Subject: Re: [arduino/ArduinoCore-samd] micros() is non-monotonic with -1ms glitches on Cortex cores from priority 0 or 1 ISRs (#463)

I can imagine that all these CMSIS mentions are in code that is not actually used. Perhaps you could check that?

As for the bootloader / CMSIS code, as long as you never use the timer from an interrupt and only read the timer values atomically, I guess having a low priority for the systick ISR is not a problem, so maybe that's why that's the default there?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Farduino%2FArduinoCore-samd%2Fissues%2F463%3Femail_source%3Dnotifications%26email_token%3DABXJWNCEK3XTKKZL6AD2KXTQQ3ZAFA5CNFSM4JBNK5KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECNBX2Q%23issuecomment-546970602&data=02%7C01%7Cctd%40metaswitch.com%7C85d052de15dc489c094708d75bb2c833%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637078695744877008&sdata=4Y%2Fsbm3IWo54As%2F5DGxYDZUFUfX3Wn%2BZz8%2F8vuKwgn8%3D&reserved=0, or unsubscribehttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABXJWNHCRV4THPXFRR7PUE3QQ3ZAFANCNFSM4JBNK5KA&data=02%7C01%7Cctd%40metaswitch.com%7C85d052de15dc489c094708d75bb2c833%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637078695744886955&sdata=pb19wT6ZE8v15xDXoYgAxw5PIpblz9u7zcmFvDHPmRM%3D&reserved=0.

[sslupsky]

As for the bootloader / CMSIS code, as long as you never use the timer from an interrupt and only read the timer values atomically, I guess having a low priority for the systick ISR is not a problem, so maybe that's why that's the default there?

Yes, I think this is probably the underlying issue for the CMSIS people but this is just speculation on my part. Generally speaking, and in my opinion, you would want to use as low a priority as possible simply because in the general case you may not want the Systick to interrupt something else that has a higher timing requirement.

[sslupsky]

Those CMSIS includes are used ... I know for certain the core_cm0plus.h is included by way of Arduino.h. I do not think we (Arduino that is) can just change these files since these are part of the CMSIS library. The maintainer for CMSIS is ARM?

[colintd]

In many ways I’m relaxed about the bootloaders because once the program starts the value is overwritten. I am, however, keen to get the change merged forward into the adafruit code (which was where I actually hit the problem). If we update wiring.c, then I can followup.

From: sslupsky [email protected] Sent: 28 October 2019 14:36 To: arduino/ArduinoCore-samd [email protected] Cc: Colin Tregenza Dancer [email protected]; Mention [email protected] Subject: Re: [arduino/ArduinoCore-samd] micros() is non-monotonic with -1ms glitches on Cortex cores from priority 0 or 1 ISRs (#463)

Those CMSIS includes are used ... I know for certain the core_cm0plus.h is included by way of Arduino.h. I do not think we (Arduino that is) can just change these files since these are part of the CMSIS library. The maintainer for CMSIS is ARM?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Farduino%2FArduinoCore-samd%2Fissues%2F463%3Femail_source%3Dnotifications%26email_token%3DABXJWNGPVG7O553MSPMM73TQQ32EXA5CNFSM4JBNK5KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECNC2VA%23issuecomment-546975060&data=02%7C01%7Cctd%40metaswitch.com%7C89816f7ad3614493b03808d75bb42600%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637078701615963523&sdata=hbxjIMVjCE1aladmDB0yfxjT7xInVRBAcJbOTPayUNs%3D&reserved=0, or unsubscribehttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABXJWNHFWPP3BXXMD6QI22LQQ32EXANCNFSM4JBNK5KA&data=02%7C01%7Cctd%40metaswitch.com%7C89816f7ad3614493b03808d75bb42600%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637078701615973509&sdata=vpSwDI4Upn3zeJMDPeAN9Fy58zdWIMnchHWyllu11qU%3D&reserved=0.

[matthijskooijman]

Those CMSIS includes are used ... I know for certain the core_cm0plus.h is included by way of Arduino.h. Just including does not mean the code actually runs. It might very well be in a function that is not called.

I do not think we (Arduino that is) can just change these files since these are part of the CMSIS library. The maintainer for CMSIS is ARM?

I think so too, I think the CMSIS is a utility library that is intended for ARM cores in general and contains a lot of code that is probably not actually called by Arduino. Given that this issue did not exist before the I2S change (AFAIU), I suspect that the default systick priority is the highest, and the code from CMSIS never runs (and thus also does not need to be modified).

Generally speaking, and in my opinion, you would want to use as low a priority as possible simply because in the general case you may not want the Systick to interrupt something else that has a higher timing requirement.

That is actually a good point. Can we really not find any way to make things work with a low priority systick?

[colintd]

I’d actually argue the opposite way in terms of priority. In my experience systick/timer ISRs almost always run at highest priority to prevent exactly this kind of problem of premption (where you can’t know if the wrap count has incremented) , and to ensure that time “advances” even when a lower priority ISR is running for an extended period. The tick ISRs are typically very short, so have minimal impact on other functions. I’ve worked on a number of systems where the tick timer was actually tied to the NMI vector.

From: Matthijs Kooijman [email protected] Sent: 05 November 2019 17:47 To: arduino/ArduinoCore-samd [email protected] Cc: Colin Tregenza Dancer [email protected]; Mention [email protected] Subject: Re: [arduino/ArduinoCore-samd] micros() is non-monotonic with -1ms glitches on Cortex cores from priority 0 or 1 ISRs (#463)

Those CMSIS includes are used ... I know for certain the core_cm0plus.h is included by way of Arduino.h. Just including does not mean the code actually runs. It might very well be in a function that is not called.

I do not think we (Arduino that is) can just change these files since these are part of the CMSIS library. The maintainer for CMSIS is ARM?

I think so too, I think the CMSIS is a utility library that is intended for ARM cores in general and contains a lot of code that is probably not actually called by Arduino. Given that this issue did not exist before the I2S change (AFAIU), I suspect that the default systick priority is the highest, and the code from CMSIS never runs (and thus also does not need to be modified).

Generally speaking, and in my opinion, you would want to use as low a priority as possible simply because in the general case you may not want the Systick to interrupt something else that has a higher timing requirement.

That is actually a good point. Can we really not find any way to make things work with a low priority systick?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Farduino%2FArduinoCore-samd%2Fissues%2F463%3Femail_source%3Dnotifications%26email_token%3DABXJWNH6T7OWMWO3IZAZVEDQSGWPNA5CNFSM4JBNK5KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDDVI6Q%23issuecomment-549934202&data=02%7C01%7Cctd%40metaswitch.com%7C35c64325f01e4bee296708d76218181a%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637085727933388460&sdata=QfblTT36RLoTL37B0MMVcRkJrDwQMyQHqVv5NA2ZS3k%3D&reserved=0, or unsubscribehttps://nam01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FABXJWNBS2YM4QNUU4KREVB3QSGWPNANCNFSM4JBNK5KA&data=02%7C01%7Cctd%40metaswitch.com%7C35c64325f01e4bee296708d76218181a%7C9d9e56ebf6134ddbb27bbfcdf14b2cdb%7C1%7C0%7C637085727933388460&sdata=hl%2B5Inz7v96J2wM3OQsi0YK5Rywbl9qYRQHL7Ou1XSg%3D&reserved=0.

[PaulStoffregen]

In my experience systick/timer ISRs almost always run at highest priority to prevent exactly this kind of problem

This is how we do things on Teensy. It has worked out very well.

[baekgaard]

Sorry to revive an old issue, but I was just being hit by the same problem here on a CM4 (samd51j19) device:

As the systick handler runs at a lower priority (prio level 5, it seems, in the default adafruit setup (or 7 when using a FreeRTOS layer on top in my case)) I also got a non-monotic clock in my ISR dealing with hardware interrupts; the issue is clearly that my ISR could interrupt a running systick handler when the interrupt was no longer pending but the routine still was in the process of updating the tick count.

The remedy, which was easy enough in my case when I realised the problem, was to lower the priority of my ISR to the same or a lower level than the systick handler. I only found this thread later... I wished I had seen it before :-)

Was this ever resolved by running the systick handler at the highest priority in the standard setup?

If not, perhaps at least a comment in the micros() function could be in place, just to save some time for those that may also run into this issue with non-monotonic clocks.

The micros() code is correct as it is now, but the comment about this being interrupt friendly means that it will return the correct value even when being interrupted in the middle of the execution by the systick interrupt -- but it is NOT currently guaranteed to give monotonically increasing values if called from an ISR that has a higher priority than the systick handler (in fact, it is almost guaranteed to have a -1ms error very frequently (in my case 1-5 times pr. second)).

[colintd]

I don't think there was yet consensus about the change (due to possible impact on the I2S code), but as noted in the comments by multiple people, systick is normally a top priority interrupt, and as it stands micros() is non-monotonic.

Since raising the bug I have also been running with the high priority systick without any issues (and with monotonic time).

So my preference would definitely be to merge in the change...

[mzero]

Sadly, I think there is another problem with micros():

The implementation ends with:

return ((count+pend) * 1000) + (((SysTick->LOAD  - ticks)*(1048576/(VARIANT_MCK/1000000)))>>20) ;

The idea is count + pend gives the correct number of milliseconds, whether the interrupt has occured (and just bumped count) or not (and so pend is 1). Then the number of microseconds in the counter is added in with SysTick->LOAD - ticks.

However, the issue is that the interrupt occurs when the SysTick counter transitions to 0, not when it reloads (on the next clock).

SO- Consider when millis() is called at a moment when the SysTick counter is 0. Either count will have just been bumped or there'll be a pending interrupt. Either way, count + pend is the same. Now, (SysTick->LOAD - 0) will add in almost a full milliseconds worth of microseconds -- even though that millisecond has already been accounted for in count + pend.

As a thought experiment, consider if millis() is called one clock later - count + pend will be the same, but ticks will be the re-load value... and so (SysTick->LOAD - SysTick->LOAD) will add zero microseconds in... The value will have gone backwards.

[rohoog]

I can confirm that using NVIC_SetPriority(EIC_IRQn, 2) after all attachInterrupt() calls solves the 1ms glitches.