Trouble setting watchdog timer period on ATmega32M1

[HwAoRrDk]

I have some existing AVR code that uses the watchdog timer to asynchronously flash an LED at varying rates (e.g. 1Hz, 2Hz, etc.). I've used it before (with minor adaptations) on ATmega328P, ATtiny, etc. But I'm having trouble trying to get it to work on an ATmega32M1.

Here's the code I'm using:

Code: [Select]

#define LED_FLASH_1_HZ (_BV(WDP2) | _BV(WDP1))
#define LED_FLASH_2_HZ (_BV(WDP2) | _BV(WDP0))
#define LED_FLASH_4_HZ (_BV(WDP2))
#define LED_FLASH_8_HZ (_BV(WDP1) | _BV(WDP0))

void led_flash_start(uint8_t frequency) {
frequency &= _BV(WDP0) | _BV(WDP1) | _BV(WDP2) | _BV(WDP3); // Mask WDPn bits
if(frequency > (_BV(WDP0) | _BV(WDP3))) frequency = (_BV(WDP0) | _BV(WDP3)); // Ensure value is not in 'reserved' range

ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
WDTCSR |= _BV(WDCE) | _BV(WDE);
WDTCSR = _BV(WDIE) | frequency;
}
}

ISR(WDT_vect) {
PINB |= _BV(PINB3); // Toggle LED.
}

As far as I can see, the 32M1's watchdog timer works in the same way as the 328P's, so the code above is exactly the same as that I've successfully used on an Arduino board. But yet, it doesn't work as expected. It just goes into a reset loop at the point (or immediately after) that led_flash_start() is called. "Well, duh! Sounds like the watchdog is causing the reset!", I hear you say. But I'm not leaving it in reset mode! WDE should be back to zero after I set WDIE and the prescaler bits, so therefore it should be operating in interrupt-only mode.

If I change the code to avoid setting WDE in the first place (i.e. setting WDCE only), the prescaler bit changes don't take effect - the timer stays at the default of 16ms. So it seems that both WDCE and WDE need to both be set to change things. But yet setting WDE somehow triggers the resetting behaviour.

So I have reached an impasse and I don't know how I'm supposed to get this to work on this MCU. Help!

By the way, one other bizarre thing I observed is that the reset-loop behaviour persists even after re-flashing with the offending code commented out, and cycling power! Although, I suspect this might be due to also having a USB UART adapter connected that may perhaps be inadvertently powering the MCU through the IO pins when the main supply is disconnected. When I disconnected everything from the board, normal operation was resumed.

[HwAoRrDk]

I think I figured out what the problem was. I believe it is a problem with the compiler generating machine code that doesn't quite do things in the same order as the C code.

The assembly code generated for the led_flash_start() function code as previously given was as follows:

Code: [Select]

000000ca <led_flash_start>:
  ca: 87 72        andi r24, 0x27 ; 39
  cc: 2f b7        in r18, 0x3f ; 63
  ce: f8 94        cli
  d0: 90 91 60 00 lds r25, 0x0060 ; 0x800060 <__TEXT_REGION_LENGTH__+0x7e0060>
  d4: 98 61        ori r25, 0x18 ; 24
  d6: 90 93 60 00 sts 0x0060, r25 ; 0x800060 <__TEXT_REGION_LENGTH__+0x7e0060>
  da: 82 32        cpi r24, 0x22 ; 34
  dc: 08 f0        brcs .+2      ; 0xe0 <led_flash_start+0x16>
  de: 81 e2        ldi r24, 0x21 ; 33
  e0: 80 64        ori r24, 0x40 ; 64
  e2: 80 93 60 00 sts 0x0060, r24 ; 0x800060 <__TEXT_REGION_LENGTH__+0x7e0060>
  e6: 2f bf        out 0x3f, r18 ; 63
  e8: 08 95        ret

I don't really know what I'm doing with assembly, but I can recognise what I think are the instructions writing to the WDTCSR register (sts 0x0060... - 0x60 being the address of the reg), and there are 4 instructions between the two instances. Given that the datasheet specifies that the changes to the watchdog configuration must be done within four clock cycles, this code would appear to overrun that timing requirement!

I'm not 100% sure what the stuff in-between is, but it looks like (because of the branch instruction) it might be the if() statement that ensures the frequency variable is not a reserved value.

If I comment that if() line out and re-compile, the generated assembly code is drastically different:

Code: [Select]

000000ca <led_flash_start>:
  ca: 2f b7        in r18, 0x3f ; 63
  cc: f8 94        cli
  ce: e0 e6        ldi r30, 0x60 ; 96
  d0: f0 e0        ldi r31, 0x00 ; 0
  d2: 90 81        ld r25, Z
  d4: 98 61        ori r25, 0x18 ; 24
  d6: 90 83        st Z, r25
  d8: 87 72        andi r24, 0x27 ; 39
  da: 80 64        ori r24, 0x40 ; 64
  dc: 80 83        st Z, r24
  de: 2f bf        out 0x3f, r18 ; 63
  e0: 08 95        ret

I don't quite recognise what's going on - the only reference to the memory address of the WDTCSR register (0x60) is in loading that value to another register - but hazarding a guess that the two st instructions are the ones writing to WDTCSR, there are now only two instructions separating them, which now complies with the timing requirements. And, this code actually works!

Bloody compilers, re-arranging my code! I think I'll just remove the if() statement, as it's not strictly necessary, just a sanity check against if one were to pass some random value into the function.

I also think I'll add a line to reset the watchdog at the beginning of the atomic section, as the datasheet notes that when changing the timer period to a shorter value, you might get a premature time-out, and resetting prior will mitigate that.

[cv007]

If I change the code to avoid setting WDE in the first place (i.e. setting WDCE only), the prescaler bit changes don't take effect

Just as the datasheet says.

You could make an enum to limit the values passed without having to check for valid values, something like-

Code: [Select]

typedef enum { WDTO_10MS = 0, ..., WDTO_8S = 0x21 } WDTO_t;
//the WDIE bit could also be incorporated into each enum, if only using wd irq

void led_flash_start(WDTO_t frequency){
ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
                __asm__ __volatile__("wdr"); //or something already defined in headers
                //clear wdif, set wdce,wde, prescaler bits unchanged (protected), wdie also cleared
WDTCSR = _BV(WDIF) | _BV(WDCE) | _BV(WDE);
               //now set wdie and prescaler, clear wde (irq mode)
WDTCSR = _BV(WDIE) | frequency;
}
}

By the way, one other bizarre thing I observed is that the reset-loop behaviour persists even after re-flashing with the offending code commented out, and cycling power!

WDRF flag overrides WDE. Maybe the recycling power didn't really get you a power on reset (power to micro never really dropped far enough- ext reset only). Maybe not the case, but if no one is taking care of the WDRF flag, then enabling change to watchdog but not meeting time/cycle requirement wde will remain enabled which gets you a real watchdog reset after the first watchdog irq reset (wdie not protected, so still will get set). After watchdog reset, WDRF is set so WDE is also set. No one touches WDRF, so- Loop.

[HwAoRrDk]

You could make an enum to limit the values passed without having to check for valid values

Thanks for the suggestion - I already did that, actually. Don't really know why I did it with macros in the first place.

Good point about clearing WDIF too, I will do that. Can't hurt to take a belt-and-braces approach.

WDRF flag overrides WDE. Maybe the recycling power didn't really get you a power on reset (power to micro never really dropped far enough- ext reset only). Maybe not the case, but if no one is taking care of the WDRF flag, then enabling change to watchdog but not meeting time/cycle requirement wde will remain enabled which gets you a real watchdog reset after the first watchdog irq reset (wdie not protected, so still will get set). After watchdog reset, WDRF is set so WDE is also set. No one touches WDRF, so- Loop.

Doh! I forgot about WDRF persisting in MCUSR! That explains what was going on.

I should have read the datasheet more thoroughly, as the example code given clears WDRF. I'll add that to my code too.