Go Down

Topic: Bug in generated AVR code.. report? (Read 269 times) previous topic - next topic

Fox65535

Feb 29, 2016, 09:48 pm Last Edit: Mar 01, 2016, 05:04 pm by Fox65535
I got bit by a bug IMO. Reading the high byte of 16-bit AVR registers does not follow the [poorly described] required protocol. This includes reads to UBRR1H, UBRR0H, OCR3BH, OCR3AH, ICR3H, TCNT3H, OCR1BH, OCR1AH, ICR1H, TCNT1H, and  EEARH.

I presume there is a parallel bug writing to the corresponding low bytes, but I have not tried that. In each case, the byte read of the high byte or the write to the low byte requires a dummy access first.

I understand there can be different opinions about what the tools should do and what the individual programmer should do to match architectural quirks.

There is also a lesser potential rare problem with the 16-bit access to special registers UBRR1, UBRR0, OCR3B, OCR3A, ICR3, TCNT3, OCR1B, OCR1A, ICR1, TCNT1, or EEAR getting corrupted by an untimely interrupt between the two compiler-generated byte accesses.

If somebody want more info, or even just wants to comment that that is the programmer's job, say please say so.  I know how to deal for my project now; I know the workaround.

kowalski

I believe this is well documented in the AVR product descriptions for each MCU. AVR is little endian and 8-bit internal architecture. Reading a 16-bit register requires transfer to an intermediate register. LSB must be read first. As this is an internal transfer and potential two 8-bit reads the operation is not atomic. It has to be protected from possible interleaving with ISR (that may also use the intermediate registers), i.e. turning off interrupt handling during the access of the 16-bit register.

If I understand you correctly you are expecting the compiler to resolve this for you? Both the order of the register read (LSB/MSB) but also keeping it atomic??

Cheers!

Fox65535

#2
Mar 01, 2016, 02:33 am Last Edit: Mar 01, 2016, 05:06 pm by Fox65535
I believe this is well documented in the AVR product descriptions for each MCU. AVR is little endian and 8-bit internal architecture. Reading a 16-bit register requires transfer to an intermediate register. LSB must be read first. As this is an internal transfer and potential two 8-bit reads the operation is not atomic. It has to be protected from possible interleaving with ISR (that may also use the intermediate registers), i.e. turning off interrupt handling during the access of the 16-bit register.
Not well documented in my opinion. Can you read this and discern that doing an isolated 8-bit read of TCNT0H needs any special action?

13.3 Accessing 16-bit Registers
The TCNT1, OCR1A/B, and ICR1 are 16-bit registers that can be accessed by the AVR CPU via
the 8-bit data bus. The 16-bit register must be byte accessed using two read or write operations.
Each 16-bit timer has a single 8-bit register for temporary storing of the high byte of the 16-bit
access. The same temporary register is shared between all 16-bit registers within each 16-bit
timer. Accessing the low byte triggers the 16-bit read or write operation. When the low byte of a
16-bit register is written by the CPU, the high byte stored in the temporary register, and the low
byte written are both copied into the 16-bit register in the same clock cycle. When the low byte of
a 16-bit register is read by the CPU, the high byte of the 16-bit register is copied into the temporary
register in the same clock cycle as the low byte is read.
Not all 16-bit accesses uses the temporary register for the high byte. Reading the OCR1A/B 16-
bit registers does not involve using the temporary register.
To do a 16-bit write, the high byte must be written before the low byte. For a 16-bit read, the low
byte must be read before the high byte


The read of bytes/char TCNT0H or the write of TCNT0L always requires a two-byte access. That sure did not come through to me when reading the above text. Sure seems like something a compiler (or library) would be particularly good at.

If I understand you correctly you are expecting the compiler to resolve this for you? Both the order of the register read (LSB/MSB) but also keeping it atomic??

Cheers!
No, not expecting the atomic thing. It would cost some execution time if it were automatic. Just musing, how about a warning if -Wall or -Wextra was turned on? By the way, is there an easy way to pass such switches to the Arduino IDE?

And thank you very much for your perspective. I infer this has been discussed quite a bit before. And I suspect those discussions were often triggered by those who were bit before. Thanks again.

Edit: For reading TCNT0H, reading TCNT1H_READ generated good code:
Code: [Select]
#define TCNT1H_READ ((unsigned char)(TCNT1 >>8))

pert

how about a warning if -Wall or -Wextra was turned on? By the way, is there an easy way to pass such switches to the Arduino IDE?
In Arduino IDE 1.6.4+ you can do:
File > Preferences > Compiler Warnings: More for -Wall
File > Preferences > Compiler Warnings: All for -Wall -Wextra
These are configured in platform.txt.

Fox65535

In Arduino IDE 1.6.4+ you can do:
File > Preferences > Compiler Warnings: More for -Wall
File > Preferences > Compiler Warnings: All for -Wall -Wextra
These are configured in platform.txt.
I am not familiar with Arduino IDE 1.6.4+. Google searches ignore "+" I think.

In Arduino IDE 1.6.4 I did did edit platform.txt compiler.warning_level=all, and I was rewarded with warnings... and I cleaned some stuff up as a result. Thanks.

pert

I am not familiar with Arduino IDE 1.6.4+.
Sorry, I just meant Arduino IDE 1.6.4 and all versions after that.

In Arduino IDE 1.6.4 I did did edit platform.txt compiler.warning_level=all, and I was rewarded with warnings... and I cleaned some stuff up as a result. Thanks.
When I switched to using Arduino IDE 1.5.x from 1.0.x I went for a while with no warnings because they were disabled with -w and I hadn't gotten around to editing platform.txt to put the warnings back on some of the versions. Then when 1.6.4 came out and I got the warnings back I was amazed to see all the problems with my code I had written during that time. It's crazy that they're disabled by default because they're so useful.

westfw

Quote
The read of bytes/char TCNT0H or the write of TCNT0L always requires a two-byte access.
Interesting.  Presumably only if you want correct results.  The bootloader only writes UBRR0L when the high byte is zero (always, essentially), and that seems to work (because the high-holding register is reset to zero?)


Quote
Sure seems like something a compiler (or library) would be particularly good at.
I don't see how the compiler can be expected to know that some pointers are "magic" and need you to read/write some other location first before accessing them.  It apparently DOES contain some code to properly handle 16bit reads and writes to registers (even though the datapath is 8bits), and the "avr/io.h" include normally includes 16-bit definitions for those registers (UBRR0 is a 16bit IO register, for instance.)  See also http://www.nongnu.org/avr-libc/user-manual/FAQ.html#faq_16bitio

If it were actually useful to read only the high byte, I expect there would be a library function to do so.  As things are, you might as well just read the 16bit register.


Go Up
 


Please enter a valid email to subscribe

Confirm your email address

We need to confirm your email address.
To complete the subscription, please click the link in the email we just sent you.

Thank you for subscribing!

Arduino
via Egeo 16
Torino, 10131
Italy