GPIOR registers used by core contrary to the documentation
alexandrevoilab opened this issue · 4 comments
Hi, thank you for this awesome core and the extensive documentation.
The Ref_Interrupts.md documentation says:
[...] you can use one of the general purpose registers: GPIOR0/1/2/3. The only place the core uses any of those is at the very beginning of execution, when the reset cause is stashed in GPIOR0 [...]
I use them for my interupts and statemachine.
After quite a long time tracking some very wierd gremlings, I found out that the values are changed by the core.
I tracked down some writes in wiring_analog.c:
Line 973: GPIOR2
Line 978: GPIOR3
Line 980: GPIOR1
They seem to be debugging attempts since the values are never used nor read.
The USERSIG library sets them also, which I found out grep'ing for GPIOR on the sources.
See USERSIG.h lines 197 to 246
Are thoses occurences only mistakes or should the documentation be updated?
Thank you these will be removed promptly, I thought all of these cases had been removed.
They're leftover debugging flags - it's hard enough to get data out of the function nested three levels of calls down when it's written in C and isn't perverse.
(see, I don't use hardware debuggers, the tools involved in using them have never seemed worth the time one must invest to learn to use it. And I have to use Microchip's software? No thanks
And then it once works and I forget all about the debug code and it gets left in (at minimal cost, as each of those GPIOR write lines that I use for debugging is a single instruction single clock operation. - it's the non-physical equivalent of flipping pins within a port via the VPORTs (VPORTA.OUT |= 1 << n; // where 8 > n >= 0 is 1 word 1 clock. PORTA.OUT |= 1 << n; is 5 words and 6 clocks. Even PORTA.OUT = 1; //this takes 3 words 3 clocks!
I ought to make a test that greps *.c and *.h in the core directories and fails any file with matches for the regex.... uh, off the top of my head, like:
([ \n\(\[\{])GP(IO)?R?\d[^@]
Wouldn't that catch any usage of GPIOn, GPIORn (some devices spelled it this way), as well as the GPR.GPRn form wouldn't it?
And the last bit is there so that you can put a comment later on the line to suppress the error, eg,
GPIOR0 |= 1<<5; // @ This is **intended**
Then the on-checkin unit tests could also catch forgotten debug code of this sort.
GPIOR0 is the only GPR that the core is ever "intended" to write to on it's own. All such writes occur prior to execution of user code (most notably, the 6 lowest bits of GPIOR0 contain the reset flags that the chip saw upon most recent reset - we have to clear these flags (we cannot rely on user code to reset RSTFR - code that does that is almost unheard of in Arduino land), but we can't do that if doing so took away functionality (in this case the ability to determine what caused the previous reset)
Sure, I could use some other register, but it's slower, and depending on what construction you use, you will get different implementations from the compiler. Correctly done bit-tests and bit-writes, in contrast, will always compile to the same thing (sbi or cbi, for write, sbis/sbic for read). They're also rare enough in other compiler output that you could probably search compiler output for them as signposts.... heh....
But in any case, yeah, it's just the most convenient way for me to debug something buried like that.
Most of the places you cite were fixed a year ago. Only one of the USERSIG files still seemed to have any issues there...
Thank you for the fix!