Why doesn't analogWrite() protect its writes to 16bit registers?

DaveX · February 20, 2022, 3:21pm

Continuing the discussion from What is the inverse of analogWrite(pin,val)?:

Do you need to protect reads and writes to 16bit timer registers, such as OCR1A, TCNT1, ICR1 ) from interrupts?

The Arduino analog core code at ArduinoCore-avr/wiring_analog.c at master · arduino/ArduinoCore-avr · GitHub writes to 16 bit timer registers like:

...
case TIMER1A:
	// connect pwm to pin on timer 1, channel A
	sbi(TCCR1A, COM1A1);
	OCR1A = val; // set pwm duty
	break;
#endif
...

Why do they not protect the 16 bit writes as in http://ww1.microchip.com/downloads/en/Appnotes/doc1493.pdf or ATmega48/V / 88/V / 168/V or this section 15.3 from the Uno's datasheeet at https://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-7810-Automotive-Microcontrollers-ATmega328P_Datasheet.pdf

This is a portion of the avr-objdump -Sj disassembly for a "void setup(){analogWrite(9,127);} void loop(){;}" sketch:

                        #if defined(TCCR1A) && defined(COM1A1)
                        case TIMER1A:
                                // connect pwm to pin on timer 1, channel A
                                sbi(TCCR1A, COM1A1);
 2de:   80 91 80 00     lds     r24, 0x0080     ; 0x800080 <__DATA_REGION_ORIGIN__+0x20>
 2e2:   80 68           ori     r24, 0x80       ; 128
 2e4:   80 93 80 00     sts     0x0080, r24     ; 0x800080 <__DATA_REGION_ORIGIN__+0x20>
/Users/xxx/Library/Arduino15/packages/arduino/hardware/avr/1.8.4/cores/arduino/wiring_analog.c:145
                                OCR1A = val; // set pwm duty
 2e8:   8f e7           ldi     r24, 0x7F       ; 127
 2ea:   90 e0           ldi     r25, 0x00       ; 0
 2ec:   90 93 89 00     sts     0x0089, r25     ; 0x800089 <__DATA_REGION_ORIGIN__+0x29>
 2f0:   80 93 88 00     sts     0x0088, r24     ; 0x800088 <__DATA_REGION_ORIGIN__+0x28>
 2f4:   e7 cf           rjmp    .-50            ; 0x2c4 <main+0x150>
/Users/xxx/Library/Arduino15/packages/arduino/hardware/avr/1.8.4/cores/arduino/wiring_analog.c:176
                        #endif

johnwasser · February 20, 2022, 4:08pm

The answer seems to be "Yes, if both the main code and an enabled ISR write to or read from 16-bit registers for that timer." The ISR doesn't have to worry about it since it is running with interrupts disabled.

DrDiettrich · February 21, 2022, 1:01am

Because the writes are buffered?

Interrupts should be disabled in setup() until a module is fully configured.

johnwasser · February 21, 2022, 2:01am

A) In the Arduino runtime there are no ISR's writing to or reading from Timer1 registers.

B) The value passed to analogWrite() is supposed to be between 0 and 255 so when that value is written to OCR1A or OCR1B the high byte is always zero. If you put an analogWrite() in an ISR the worst it could do is accidentally set the high byte to 0 when the main code was going to set it to 0 anyway.

DaveX · February 21, 2022, 2:27pm

So analogWrite(pin,val) doesn't need to worry about it for 8 bit timers, since those aren't 16 bits, and for the 16 bit ones in 8bit the default WGM=1, 8-bit phase correct modes, they're safe because the datasheet says "Note that when using fixed TOP values, the unused bits are masked to zero when any of the
OCR1x registers are written. "

Wokwi looks like it has a bug where it doesn't do this masking and allows OCR1A to be set higher than 255

Try this script on a real Mega and on Wokwi Mega:

void setup() {
  // put your setup code here, to run once:

Serial.begin(115200);
// Uno_OC1A=0 Mega_OC1A=11, Leo_OC1A=9
// Uno https://docs.arduino.cc/hacking/hardware/PinMapping168
// Mega https://docs.arduino.cc/hacking/hardware/PinMapping2560
// Leonardo https://docs.arduino.cc/hacking/hardware/PinMapping32u4
//analogWrite(13,0xffff); //ocr0a
//bitClear(TCCR1A,WGM10); //  Change WGM 0 normal 
//bitSet(TCCR1B,WGM12); //  Change WGM 1 phase correct, to wgm 5 fastpwm 
analogWrite(11,0xffff); //ocr1a
//digitalWrite(9,0);
Serial.print("TCCR0A 0b");
Serial.print(TCCR0A,BIN);
Serial.print(" TCCR0B 0b");
Serial.print(TCCR0B,BIN);
Serial.print(" OCR0A ");
Serial.print(OCR0A,DEC);
Serial.print(" OCR0B ");
Serial.print(OCR0B,DEC);
Serial.println();

Serial.print("TCCR1A 0b");
Serial.print(TCCR1A,BIN);
Serial.print(" TCCR1B 0b");
Serial.print(TCCR1B,BIN);
Serial.print(" TCCR1C 0b");
Serial.print(TCCR1C,BIN);
Serial.print(" OCR1A ");
Serial.print(OCR1A,DEC);
Serial.print(" OCR1B ");
Serial.print(OCR1B,DEC);
Serial.println();

Serial.println("On real mega, OCR1A is 255");
Serial.println("On Wokwi mega, OCR1A is 65535");
}

void loop() {
  // put your main code here, to run repeatedly:
}

The only way that a timer's TEMP buffer can screw up a write is if an ISR happens to corrupt it in the middle of other code by reading that timer's TCNTx or writing one of the same timer's other OCRnx registers.

I guess the reason why it doesn't protect TEMP is a tradeoff between depending on the default configuration and speed--it doesn't matter for fixed-TOP 8-bit modes, or when there are no ISRs that write timer registers. But if speed really mattered, you might depend on the 8bit config and only write the low byte like OCR1AL and save a couple bytes & cycles.

It's also interesting that analogWrite will work (** sort of -- 255 is still a full on digitalWrite!) to set pwm values higher than 255 in other modes. (e.g. 0-1023 resolution in 10 bit mode).

johnwasser · February 21, 2022, 2:41pm

Remember that writing to OCR1xL causes OCR1xH to be written from the TEMP register. Do we know if the masking happens on the way into the TEMP register or on the way out? One way to check is to put the WGM into 10-bit PWM mode, write 0x0000 to OCR1A, write 0xFFFF to ICR1 and then write 0xAA into OCR1AL. Does the OCR1A read back as 0xFFAA or 0x03AA (or maybe 0x00AA by some magic)?

DaveX · February 21, 2022, 2:55pm

I guess we can't relay on magic:

void setup() {
  // put your setup code here, to run once:

Serial.begin(115200);
// Uno_OC1A=0 Mega_OC1A=11, Leo_OC1A=9
// Uno https://docs.arduino.cc/hacking/hardware/PinMapping168
// Mega https://docs.arduino.cc/hacking/hardware/PinMapping2560
// Leonardo https://docs.arduino.cc/hacking/hardware/PinMapping32u4
//analogWrite(13,0xffff); //ocr0a
//bitClear(TCCR1A,WGM10); //  Change WGM 0 normal 
//bitSet(TCCR1B,WGM12); //  Change WGM 1 phase correct, to wgm 5: 8bit fastpwm 

bitSet(TCCR1A,WGM11); //  Change WGM 3 10 bit phase correct
analogWrite(11,0x0000); //ocr1a
ICR1 = 0xFFFF;
OCR1AL = 0xAA;

//analogWrite(11,0xffff); //ocr1a
//digitalWrite(9,0);
Serial.print("TCCR0A 0b");
Serial.print(TCCR0A,BIN);
Serial.print(" TCCR0B 0b");
Serial.print(TCCR0B,BIN);
Serial.print(" OCR0A ");
Serial.print(OCR0A,DEC);
Serial.print(" OCR0B ");
Serial.print(OCR0B,DEC);
Serial.println();

Serial.print("TCCR1A 0b");
Serial.print(TCCR1A,BIN);
Serial.print(" TCCR1B 0b");
Serial.print(TCCR1B,BIN);
Serial.print(" TCCR1C 0b");
Serial.print(TCCR1C,BIN);
Serial.print(" OCR1A ");
Serial.print(OCR1A,DEC);
Serial.print(" 0x");
Serial.print(OCR1A,HEX);
Serial.print(" OCR1B ");
Serial.print(OCR1B,DEC);
Serial.println();

Serial.println("On real mega, OCR1A is 255");
Serial.println("On Wokwi mega, OCR1A is 65535");
}

void loop() {
  // put your main code here, to run repeatedly:
}

gets:

TCCR0A 0b10000011 TCCR0B 0b11 OCR0A 255 OCR0B 0
TCCR1A 0b11 TCCR1B 0b11 TCCR1C 0b0 OCR1A 65450 0xFFAA OCR1B 0 TCNT1 3A0 ICR1 FFFF
On real mega, OCR1A is 255
On Wokwi mega, OCR1A is 65535

(Edited code and result-- WGM10 vs WGM11 typo and missed a bit)

alto777 · February 21, 2022, 3:22pm

I don't know if Urish and the wokwi boys are tuned in to these fora, but I can assure you that they will be very interested to see anything that makes the simulation less than faithful.

As I have said elsewhere, the tool chain for the code is identical, and the code is run on an AVR simulator.

Since you @DaveX discovered this and have it cornered, so to speak, I wonder if you would enjoy to take the time to bring it to their attention?

If not, I will. I'm in the club and although I haven't recently, use discord to ask questions… they never seem to sleep so I am sure we could get a quick answer.

a7

DaveX · February 21, 2022, 3:38pm

I posted an issue at Simulator not masking 16 bit timer register writes in fixed-TOP modes? · Issue #117 · wokwi/avr8js · GitHub

johnwasser · February 21, 2022, 3:38pm

Looks like the value is masked on the way into the TEMP register. This is what I get with a 10-bit PWM mode on an UNO:

 OCR1A = 0xFF11; -> 0x311
 ICR1 = 0xFFFF;
 OCR1AL = 0x22; OCR1A -> 0xFF22

If I set OCR1A to a 16-bit value, the high byte gets masked. If I set TEMP by writing a 16-bit value to ICR1 and then write an 8-bit value to OCR1AL, the unmasked TEMP register gets saved in OCR1AH.

alto777 · February 21, 2022, 3:56pm

@DaveX Nice.

Looks like it has been fixed! Or will be, anyway.

Obvsly if you have any kind of dance for circumstances like this, it is time to perform it.

a7

DaveX · February 21, 2022, 4:10pm

I sent a thank you: Simulator not masking 16 bit timer register writes in fixed-TOP modes? · Issue #117 · wokwi/avr8js · GitHub

alto777 · February 21, 2022, 4:17pm

Haha, I meant like a "root dance", that self-congratulatory physical exhibition of joy and accomplishment reserved for getting root access to a protected system.

About which I will say no more.

Since to a first approximation these things are not found to be flaws in the compiler, the code or the hardware, but rather ID10T or PEBKACs, you can deserve to be proud of yourself.

a7

DaveX · February 21, 2022, 5:16pm

Well, besides the wokwi digression, I think the answer to my question about why they do not protect the writes, it that it is a balance between performance and safety.

The safety issues only come up if you are fooling around with writing or reading registers in interrupt contexts that may corrupt a particular timer's Temp register. It is interesting that reading OCR1x is not affected by/does not affect the TEMP register, because the read access is direct to the buffered or non-buffered OCRnxH byte.

If you do use or make use of interrupt code that might affect TEMP, you should protect your writes and reads, and analogWrite might not perform as expected. In particular, a corrupted TEMP could set OCRnx above TOP and the pwm would turn off.

Here's some sample code where that would happen on a Mega:

// Arduino Mega code for https://forum.arduino.cc/t/why-doesnt-analogwrite-protect-its-writes-to-16bit-registers/961470/14
// demonstrating that analogWrite is unprotected.
// With the noInterrupts(); line commented
// the reading of ICR1 in the interrupt will 
// corrupt the OCR1A value above TOP and 
// the PWM will shut off.  The code will detect 
// this condition, turn on the LED and enter 
// an endless loop.
// 
// With the noInterrupts() line uncommented, 
// the analogWrite() command  is protected from 
// the corrupting influence of the ICR1 read in the interrupt.

ISR(TIMER0_COMPA_vect) {
  int saveICR1 = ICR1;
}

void setup() {
  // put your setup code here, to run once:
  pinMode(LED_BUILTIN, OUTPUT);
  ICR1 = 0xFFFF;
  // Setup a millisecond interrupt
  OCR0A = 127; // 180° out of phase with millis();
  TIMSK0 |= (1 << OCIE0A);
}

void loop() {
  // put your main code here, to run repeatedly:
  //noInterrupts();  // uncomment to protect analogWrite from the interrupt
  analogWrite(11, 127);
  interrupts();
  if (OCR1A > 255 ) { // check for error
    digitalWrite(LED_BUILTIN, 1);
    while (1); // trap
  }
}

(Edited to add explanation.)

DrDiettrich · February 21, 2022, 6:28pm

Which compiler settings are required that this code is not optimized out?

DaveX · February 21, 2022, 6:59pm

Nothing special, Arduino 1.5 on my mac compiled it with:

/Users/drf/Library/Arduino15/packages/arduino/tools/avr-gcc/7.3.0-atmel3.6.1-arduino7/bin/avr-g++ -c -g -Os -w -std=gnu++11 -fpermissive -fno-exceptions -ffunction-sections -fdata-sections -fno-threadsafe-statics -Wno-error=narrowing -MMD -flto -mmcu=atmega2560 -DF_CPU=16000000L -DARDUINO=10813 -DARDUINO_AVR_MEGA2560 -DARDUINO_ARCH_AVR -I/Users/drf/Library/Arduino15/packages/arduino/hardware/avr/1.8.4/cores/arduino -I/Users/drf/Library/Arduino15/packages/arduino/hardware/avr/1.8.4/variants/mega /var/folders/7g/2b2t5vv15cg3yrp46fm2yv7h0000gt/T/arduino_build_190836/sketch/UnprotectedAnalogWrite.ino.cpp -o /var/folders/7g/2b2t5vv15cg3yrp46fm2yv7h0000gt/T/arduino_build_190836/sketch/UnprotectedAnalogWrite.ino.cpp.o

Which with an avr-objdump -Sl gives

...
/Users/drf/Documents/Arduino/UnprotectedAnalogWrite/UnprotectedAnalogWrite.ino:5
  int saveICR1= ICR1;
 81a:   80 91 86 00     lds     r24, 0x0086     ; 0x800086 <__TEXT_REGION_LENGTH__+0x700086>
 81e:   90 91 87 00     lds     r25, 0x0087     ; 0x800087 <__TEXT_REGION_LENGTH__+0x700087>
/Users/drf/Documents/Arduino/UnprotectedAnalogWrite/UnprotectedAnalogWrite.ino:6
}
...

DrDiettrich · February 21, 2022, 7:11pm

Is -Os the default optimization in the IDE?

DaveX · February 21, 2022, 7:51pm

I don't think I've done any customization of my IDE other than Preferences/Show verbose output. I like the verbose output so I can see the commands where things are compiled. I use that output in order to grab the avr-size command with paths and replace it with avr-objdump -Sl to see what the complier did.

amazed · February 22, 2022, 5:05pm

I can't say, I fully understood what you guys are talking about, but definately made me curious about timers on register level.

johnwasser:

Looks like the value is masked on the way into the TEMP register. This is what I get with a 10-bit PWM mode on an UNO:
 OCR1A = 0xFF11; -> 0x311
 ICR1 = 0xFFFF;
 OCR1AL = 0x22; OCR1A -> 0xFF22
If I set OCR1A to a 16-bit value, the high byte gets masked. If I set TEMP by writing a 16-bit value to ICR1 and then write an 8-bit value to OCR1AL, the unmasked TEMP register gets saved in OCR1AH.

@johnwasser you wrote that this was in 10-bit PWM mode (where TOP is fixed at 0x03FF). So basically ICR1 = 0xFFFF; set was just a workaround method to force TEMP register (it's high byte exactly) to a desired value?

johnwasser · February 22, 2022, 5:22pm

Yes. I write ICR1only to get a non-masked value into TEMP.

Topic		Replies	Views
Problem reading/writing 16-bit Timer1 registers Programming	10	3105	May 6, 2021
16 bit arduino timer reading Programming	6	1841	May 5, 2021
Unable to write to 16bit register Programming	17	1502	May 6, 2021
Bug of AnalogWrite(pin,val) Programming	3	759	May 5, 2021
truncated 16bit timer registers Suggestions for the Arduino Project	9	10565	May 6, 2021

Why doesn't analogWrite() protect its writes to 16bit registers?

Related topics