2

I am trying to read the PWM duty cycle set for pin 6 on the Arduino Wifi Rev2.

By default, the timer used is TCB0, used in 8-bit PWM mode. The 16-bit Compare channel CCMP is used as two 8-bit registers, the lowest byte CCMPL is for the period, while the highest byte CCMPH is for the duty cycle.

However, something weird happens when I try to read the Compare channel register, the value of CCMPH is unexpecteadly copied into CCMPL.


// PWM on pin 6, timer TCB0
//TCB_t* timer_B0 = (TCB_t *)&TCB0; // pointer on timer structure

void setup() {
  pinMode(6, OUTPUT);
  Serial.begin(9600);
}

void loop() {
  setPWMValueOnPin6(20);
  setPWMValueOnPin6(40);
  setPWMValueOnPin6(60);
  setPWMValueOnPin6(80);
}

void printCompareChannelValues()
{
  Serial.print("Compare channel = ");
  unsigned int r = TCB0_CCMP;
  //byte r = timer_B0->CCMP;
  Serial.println(r);
 
  Serial.print("Compare channel L = ");
  byte l = TCB0_CCMPL;
  //byte l = timer_B0->CCMPL;
  Serial.println(l);
 
  Serial.print("Compare channel H = ");
  byte h = TCB0_CCMPH;
  //byte h = timer_B0->CCMPH;
  Serial.println(h);
}

void setPWMValueOnPin6(byte value)
{
  analogWrite(6, value);
  printCompareChannelValues();
  delay(2000);
}

Here is the serial monitor output:

Compare channel = 5375
Compare channel L = 255
Compare channel H = 20
Compare channel = 10260
Compare channel L = 20
Compare channel H = 40
Compare channel = 15400
Compare channel L = 40
Compare channel H = 60
Compare channel = 20540
Compare channel L = 60
Compare channel H = 80
Compare channel = 5200
Compare channel L = 80
Compare channel H = 20
Compare channel = 10260
Compare channel L = 20
Compare channel H = 40
Compare channel = 15400
Compare channel L = 40
Compare channel H = 60
Compare channel = 20540
Compare channel L = 60
Compare channel H = 80
Compare channel = 5200
Compare channel L = 80
Compare channel H = 20

Problem occurs even when removing the serial functions (by watching at a led intensity on pin 6). The same behavior occurs on pin 3 (timer TCB1).

Does someone has any idea what is going on? Thanks for your help.

1

Arguable bug location

The problem appears to be here in the analogWrite() code:

    timer_B->CCMPH = val;

I suppose it could be argued that you're using the timer in an way unintended by the Arduino megaavr core, but I think it's reasonable to consider this as a bug that should be fixed in the core.

16-bit hardware register access and analogWrite() internals

According to the "Accessing 16-Bit Registers" of the ATMega4809 datasheet (among others no doubt):

The AVR data bus has a width of eight bits, and so accessing 16-bit registers requires atomic operations. These registers must be byte accessed using two read or write operations. 16-bit registers are connected to the 8-bit bus and a temporary register using a 16-bit bus.

For a write operation, the low byte of the 16-bit register must be written before the high byte. The low byte is then written into the temporary register. When the high byte of the 16-bit register is written, the temporary register is copied into the low byte of the 16-bit register in the same clock cycle.

For a read operation, the low byte of the 16-bit register must be read before the high byte. When the low byte 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. The high byte will now be read from the temporary register.

This ensures that the low and high bytes of 16-bit registers are always accessed simultaneously when reading or writing the register. Interrupts can corrupt the timed sequence if an interrupt is triggered and accesses the same 16-bit register during an atomic 16-bit read/write operation. To prevent this, interrupts can be disabled when writing or reading 16-bit registers. The temporary registers can be read and written directly from user software.

So in effect, timer_B->CCMPH = val; is not just writing the real CCMPH hardware register, it is also writing the real CCMPL register with whatever is in this anonymous temporary register that was filled by either the last read or write operation.

So here's what's happening in your code:

  • You execute byte l = TCB0_CCMPL; in printCompareChannelValues() which both reads TCB0_CCMPL and loads the temporary register with the real CCMPH register's value, which as you'll see is important.

  • You execute byte h = TCB0_CCMPH; which is reading the temporary register rather than the real CCMPH hardware register from which the temporary register was loaded in your read from TCB0_CCMPL above. Note: technically this statement doesn't play a role in problem. I'm mentioning it only to drive home the behaviour of the temporary register.

  • You exit printCompareChannelValues() and exit-and-reenter setPWMValueOnPin6() which calls analogWrite() which inside runs timer_B->CCMPH = val;, which stores val in the real CCMPH with and stores the temporary register (containing the previous CCMPH value) into the real CCMPL.

So yeah, on each time through the previous CCMPH becomes the new CCMPL value.

Patching the core

Arguably what the core should contain, instead of timer_B->CCMPH = val;, is something like:

{
    const uint8_t saved_SREG = SREG;
    cli();

    uint16_t temp  = timer_B->CCMP;
    temp          &= 0xFF;
    temp          |= (uint16_t)val << 8;
    timer_B->CCMP  = temp;

    SREG = saved_SREG;
}

Here we're doing full 16-bit register reads. The load from CCMP reads the CCMPL first which primes the temporary register with the real CCMPH. Then the second half of the load from CCMP tries to read CCMPH, but is really reading the temporary register correctly containing CCMPH's value. And now there are no unpleasant surprises when this plays out in reverse.

The statement that assigns CCMP writes the CCMPL (really the temp register) then writes to the real CCMPH and simultaneously commits the temp register to the real CCMPL.

You can patch the above snipped over the timer_B->CCMPH = val; in your PATH-TO-ARDUINO15-DIRECTORY/packages/arduino/hardware/megaavr/CORE-VERSION/cores/arduino/wiring_analog.c

I have not spent a pile of time thinking about that patch, so use a bit of caution. I may well have forgotten something important.

Priming the temporary register instead

If you don't want to mess with the core or you can't for some reason or another, a ugly hack would be to turn your simple analogWrite(6, value); call into something like the following:

{
  const uint8_t saved_SREG = SREG; cli();
  TCB0_CCMPL = TCB0_CCMPL;
  analogWrite(6, value);
  SREG = saved_SREG;
}

Besides turning off interrupt and then restoring them, the only thing happening here is this somewhat ridiculous looking statement:
TCB0_CCMPL = TCB0_CCMPL;

All this is doing is priming the temporary register for you. On the right-hand side of the assignment we read the real CCMPL register. This is also loading the temporary register with the real CCMPH, but we don't care about that. Then on the left-hand side of the assignment, we attempt to write into CCMPL, but what we're in fact doing is writing to the temporary register. And now your temporary register has the correct CCMPL value for when you run the timer_B->CCMPH = val; in the original analogWrite() code.

So, I suppose if you want to do this and you're unable/unwilling to modify your core, you could do this, potentially wrapping with an #if #else based on the Arduino megaavr core version, should they update the core in the future with patch given above or something equivalent.

Core patch using the same priming technique

The same priming technique could be applied inside the patch as follows. This is maybe more confusing but slightly more efficient than the comparatively normal looking first patch.

{
    const uint8_t saved_SREG = SREG; cli();
    timer_B->CCMPL = timer_B->CCMPL; // Prime temporary register
    timer_B->CCMPH = val;
    SREG = saved_SREG;
}
6
  • Many thanks @timemage for this very detailed and great answer ! I really like the option of priming the temporary register. I would like to submit this bug to the Arduino team on github so that they take care about correcting the core, and give them the link to this page for patch reference. Are you ok with this?
    – Wind
    Dec 11 '20 at 6:22
  • Sure, you can try it. Pretty sure I couldn't stop you even if I wanted. =) For what it's worth it affects the "Nano Every" board as well. Basically both of the only two boards currently in the arduino:megaavr package.
    – timemage
    Dec 11 '20 at 14:21
  • I submitted this as a bug to be fixed on Arduino github (github.com/arduino/ArduinoCore-megaavr/issues/91). I was asked to do a Pull Request. @timemage Would you be interested to do this PR for your proper attribution?
    – Wind
    Dec 23 '20 at 20:22
  • That is something that will take me a long time to research, decide, and do if that's what I decide. In short, I don't know. If you want to talk about it though you can find me on freenode under this name.
    – timemage
    Dec 23 '20 at 21:03
  • I am not on freenode, sorry @timemage. As I would really need this to be fixed, I think the core patch using the priming technique could be a first proposal for the PR, then discussion about what to do or to change could be done on github with the Arduino team.
    – Wind
    Dec 24 '20 at 3:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.