Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is use of LDREXW & STREXW in micros() correct? #609

Open
PaulStoffregen opened this issue Sep 14, 2021 · 15 comments
Open

Is use of LDREXW & STREXW in micros() correct? #609

PaulStoffregen opened this issue Sep 14, 2021 · 15 comments

Comments

@PaulStoffregen
Copy link
Owner

The systick_isr() and systick_isr_with_timer_events() functions don't access the systick_safe_read variable which micro() is using for synchronization.

@Defragster
Copy link
Contributor

Any ref info for correct use or note on the direction of the failure?

It is odd that that single var resides isolated in a big block (2K?) of RAM based on Frank B's posting of linker data.

@FrankBoesing
Copy link
Contributor

Paul: Yes, because it is not needed for the T4.

Tim: That is a completely different topic

@Defragster
Copy link
Contributor

Not this topic? in delay.c :: uint32_t micros(void)
do { __LDREXW(&systick_safe_read); smc = systick_millis_count; scc = systick_cycle_count; } while ( __STREXW(1, &systick_safe_read));

Some means of atomic read is needed?

@FrankBoesing
Copy link
Contributor

No, the linkage issue with large unused RAM areas is a different topic:)

Paul: It's (still) a interrupt-detector. The loop repeats when an interrupt happened. Not more, not less. It's completely OK for the Teensy 4 (even if the variable would be local) . If you want to use it different, you need to rewrite the code - more than this.

@FrankBoesing
Copy link
Contributor

There are way more important things. Things that are broken. This is not broken.

@Defragster
Copy link
Contributor

Quick look I don't see any other suggestions for use where this {__LDREXW... __STREXW} is broken. Seems it is deprecated in 'mainstream' ARM beyond M4/M7. Ref notes found show use for a single value - here we need two volatile DWORDS.

If attention were given here - then the RAM issue might be resolved - yes perhaps ref address to a 'local' var? - and from OP not clear it wasn't part of the concern at hand.

Indeed in between {__LDREXW... __STREXW} as tested any interrupt causes it to repeat, as without an interrupt the values should be safely copied?

@FrankBoesing
Copy link
Contributor

It works very reliable is has way less impact than disabling interrups. 

Tim: Local var: #594

@PaulStoffregen
Copy link
Owner Author

As mentioned on the forum, I created this issue mainly as a reminder to look at this more carefully at some point in the future. I absolutely agree many much more important things are broken and need to be investigated & fixed before this. I did not intend to start a discussion, but here we are...

Found this today which specifically mentions Cortex M7's behavior.

https://developer.arm.com/documentation/dui0646/c/the-cortex-m7-processor/memory-model/synchronization-primitives

It does say the exclusive access tag is cleared if an exception occurs. Since systick is an ARM exception rather than a normal interrupt, I'm pretty sure that means this does indeed work.

@Defragster
Copy link
Contributor

Just wondered what triggered your concern - thanks for the link - seems that is maybe a new look at it.

Would be interesting to find the systick faux interrupt didn't count - but given the millions of iterations in testing seems something odd would have show up.

I can see writing a test to confirm it triggering in 'that while' and repeating on systick update.

@FrankBoesing
Copy link
Contributor

I think i posted  a test on the beta thread

@PaulStoffregen
Copy link
Owner Author

The 1000+ message thread in 2019, right?

Just wondered what triggered your concern

No LDREX or CLREX in the interrupt. Most documentation on these instructions doesn't mention the exception behavior to clear the exclusive access flag.

Still not clear to me if the clear of exclusive access applies to interrupts or only to ARM exceptions, though in this case systick definitely is an ARM exception. Eventually I would like to use this in other places where we're regularly disabling interrupts...

Again, not my intention to turn this into a lengthy discussion.

@FrankBoesing
Copy link
Contributor

I think we found 2019 that it triggers on any interrupt.

@PaulStoffregen
Copy link
Owner Author

Added a comment to the code. f452eb4

@Defragster
Copy link
Contributor

Wrote a sketch that showed me what I expected to see - more 'testy' than anything I wrote before.
Cloned cores micros() to MYmicros(), with only change being to increment a inMicros inside the while to detect when it repeats.
Before calling recorded micros and millis
After calling recorded micros and millis
And incremented another toMicros on each call that should match inMicros, unless it repeats the while.

NOTE: Test results unchanged with Global or local copy of: uint32_t MYsystick_safe_read; // micros() synchronization

  • Suggesting it could be a local variable as below

Output: Pauses 10 secs every 1M loops()

  • when (toMicros != inMicros) shows interrupt across the atomic read
  • when millis before call to MYmicros() not the same as on return

Results: Seems to be working as expected

  • some 4283 times an interrupt caused while() repeat in 641911 millis
  • some many more uncounted times the millis changed across the call to MYmicros()
  • But changes are in batches small and large with no pattern as loop ran 509,872,986 times
  • Only rare oddity is last digit of micros() '9' before and after and not incremented to '0' with updated millis()
  • I should have captured and printed result from MYmicros() to see if they agree?

`void setup() {
// C:\T_Drive\tCode\Interrupts\Micros_IsrDetect\Micros_IsrDetect.ino
Serial.begin(115200);
while (!Serial && millis() < 4000 );
Serial.println("\n" FILE " " DATE " " TIME);

}

uint32_t inMicros = 0;
uint32_t toMicros = 0;
uint32_t lpCnt = 0;
uint32_t MWloop = 0;
void loop() {
lpCnt++;
uint32_t bMillis, aMillis;
uint32_t bMicros, aMicros;
bMicros = micros();
bMillis = millis();
MYmicros();
aMicros = micros();
aMillis = millis();
toMicros++;
if ( toMicros != inMicros ) {
if ( bMillis != aMillis ) {
MWloop++;
Serial.printf("\n Off CNT and Changed millis() b=%u .vs. a=%u", bMillis, aMillis );
Serial.printf("\t :: micros() b=%u .vs. a=%u", bMicros, aMicros );
Serial.printf("\n\t Off CNT :: to=%u .vs. in=%u >IntTrig=%lu in %u loop()s", toMicros, inMicros, MWloop, lpCnt );
}
}
else if ( bMillis != aMillis ) {
Serial.printf("\n Changed millis() b=%u .vs. a=%u > in %u loop()s", bMillis, aMillis, lpCnt );
Serial.printf("\t :: micros() b=%u .vs. a=%u", bMicros, aMicros );
}

inMicros = toMicros;
if ( !(lpCnt%10000000) ) delay (10000);
}

#include "arm_math.h" // micros() synchronization
extern volatile uint32_t systick_millis_count;
extern volatile uint32_t systick_cycle_count;
uint32_t MYmicros(void)
{
uint32_t MYsystick_safe_read; // micros() synchronization
uint32_t smc, scc;
do {
__LDREXW(&MYsystick_safe_read);
inMicros++;
smc = systick_millis_count;
scc = systick_cycle_count;
} while ( __STREXW(1, &MYsystick_safe_read));
uint32_t cyccnt = ARM_DWT_CYCCNT;
asm volatile("" : : : "memory");
uint32_t ccdelta = cyccnt - scc;
uint32_t frac = ((uint64_t)ccdelta * scale_cpu_cycles_to_microseconds) >> 32;
if (frac > 1000) frac = 1000;
uint32_t usec = 1000 * smc + frac;
return usec;
}
`

@Defragster
Copy link
Contributor

minor code edits - oddity explained by code call order where millis() was changing between calls

See : https://forum.pjrc.com/threads/68180-how-to-debug-problem-caused-by-micros()-overrunning-and-going-back-to-zero?p=288543&viewfull=1#post288543

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants