atmega328p keeps restarting loop with watchdog

Go To Last Post
19 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I have AVR Atmega328p which I want to blink for 5 secs every 16 secs (to test the watchdog interrupts).
I have the following code, which keeps restarting, and the last line is:

test_blink(2);

Which I know because the led on PC2 is always on. Where did I wrong? I re-checked the WD settings for 10s times already..

 

#define F_CPU 128000
// LED Definitions
#define LED_PORT PORTD
#define LED_PIN PD6
#define LED_DDR DDRD
#define LED_PWM_PCNT 50
// Watchdog definitions
// counter = 2*8 = 16 seconds
#define WD_COUNTER_MAX 2
#include <avr/io.h>
#include <avr/interrupt.h>
#include <avr/sleep.h>
#include <stdint.h>
#include <util/delay.h>
#include <avr/wdt.h>
// Global watchdog counter (for wake-up counts)
volatile uint8_t watchdog_counter;
// Function declarations [may split to files]
void reset_pins();
void led_blink();
void sleep_me();
void setup_init();
void test_blink(int pin);
void setup_watchdog(uint8_t state);
void led_blink()
{
    LED_DDR = (1<<LED_PIN);
    LED_PORT = (1<<LED_PIN);
    _delay_ms(5000);
}
void test_blink(int pin)
{
    reset_pins();
    DDRC |= (1<<pin);
    PORTC |= (1<<pin);
    _delay_ms(300);
    reset_pins();
}
void reset_pins()
{
    DDRB = 0;
    DDRC = 0;
    DDRD = 0;
    PORTB = 0;
    PORTC = 0;
    PORTD = 0;
}
void setup_watchdog(uint8_t state)
{
    // Watchdog:
    // Clear WD reset flag
    MCUSR = 0;
    WDTCSR = (1 << WDCE) | (1 << WDE);
    if (state == 0) {
        WDTCSR = 0;
    } else {
        WDTCSR = (1<<WDP3) | (1<<WDP0) | (1<<WDIE);
    }
    wdt_reset();
    test_blink(2);
}
void sleep_me()
{
    ADCSRA = 0;
    reset_pins();
    PRR = 0xff;
    // Set sleep mode to POWER DOWN
    SMCR |= (1 << SM1);
    // Enable SLEEP
    SMCR |= (1<<SE);
    // Enable global interrupts.
    sei();
    // Disable pull-up resistors
    SMCR |= (1<<PUD);
    test_blink(2);
    setup_watchdog(1);
  sleep_cpu();
}
void setup_init()
{
    watchdog_counter = 0;
    setup_watchdog(1);
}
// watchdog interrupt
ISR(WDT_vect) {
    setup_watchdog(0);
}
int main(void)
{
    cli();
    setup_init();
    test_blink(4);
  while(1)
  {
        cli();
        // Disable SLEEP
        SMCR &= ~(1<<SE);
        if (watchdog_counter >= WD_COUNTER_MAX) led_blink();
        else sleep_me();
        watchdog_counter++;
  }
}

 

I am using the following fuses, and avrdude command:

avrdude -B 250 -c usbtiny -P usb -p m328p -U lfuse:w:0xD3:m -U hfuse:w:0xDF:m -U efuse:w:0x07:m -U flash:w:watchdog.hex:a

Thanks

This topic has a solution.
Last Edited: Wed. Jun 14, 2017 - 04:13 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Did you see this page in the user manual? ...

 

http://www.nongnu.org/avr-libc/u...

 

In particular the "Note that for newer devices (ATmega88 and newer,..."

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hi, Thanks for the reply.

 

I changed my code, but it still keeps rebooting and not getting into the ISR:

 

#define LED_PORT PORTD
#define LED_PIN PD6
#define LED_DDR DDRD
#define LED_PWM_PCNT 50

#define WD_COUNTER_MAX 1

#include <avr/io.h>
#include <avr/interrupt.h>
#include <stdint.h>
#include <util/delay.h>
#include <avr/wdt.h>
#include <avr/sleep.h>

volatile uint8_t watchdog_counter;

void led_blink()
{
	LED_DDR = (1<<LED_PIN);
	LED_PORT = (1<<LED_PIN);
	_delay_ms(1000);
	LED_DDR = 0;
	LED_PORT = 0;
	watchdog_counter = 0;
}

void test_blink(int pin)
{
//	reset_pins();
//	DDRC |= (1<<pin);
	PORTC |= (1<<pin);
	_delay_ms(100);
	PORTC &= ~(1<<pin);

//	reset_pins();
}

void reset_pins()
{
	DDRB = 0;
	DDRC = 0;
	DDRD = 0;
	PORTB = 0;
	PORTC = 0;
	PORTD = 0;
}

void setup_watchdog()
{
	MCUSR = 0;
	WDTCSR = (1 << WDCE) | (1 << WDE);
	WDTCSR = (1<<WDP3) | (1<<WDP0) | (1<<WDIE);

//	__asm__ __volatile__ ("wdr");
	wdt_reset();
	test_blink(2);
//	__asm__ __volatile__ ("wdr");
	wdt_reset();
}

void sleep_me()
{
	setup_watchdog();
//	wdt_enable(WDTO_1S);

//	__asm__ __volatile__ ("sleep");
	sei();
	sleep_enable();
	sleep_cpu();
}

void setup_init()
{
//	CLKPR=(1<<CLKPCE);
//	CLKPR=8;   // CLKPS[3:0] = 1000

	ACSR = 0;
	ADCSRA = 0;

	watchdog_counter = 0;
	DDRC = 0xff;
	set_sleep_mode(SLEEP_MODE_PWR_DOWN);
}

// watchdog interrupt
ISR(WDT_vect) {
	wdt_disable();
	sleep_disable();
//	test_blink(3);
	PORTC ^= (1<<3);
	watchdog_counter+=1;
}

int main(void)
{
	MCUSR = 0;
	wdt_disable();

	test_blink(4);
	setup_init();
    while(1)
    {
    	cli();

    	if (watchdog_counter >= WD_COUNTER_MAX) led_blink();

    	sleep_me();
    }
}

 

any idea what I did wrong here?

 

Thanks!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I find the issue to be in setting the watchdog prescaler and interrupts enabled:

 

WDTCSR = (1<<WDP3) | (1<<WDP0) | (1<<WDIE);

 

Instead, I am doing this, which works fine: [97 = WDIE | WDP3 | WDP0]

__asm__ __volatile__ ("sts %0,%1" "\n\t" : : "M" (_SFR_MEM_ADDR(WDTCSR)), "r" (97));

 

What am i missing here?!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

This also works.....

 

WDTCSR = (1 << WDCE) | (1 << WDE);
asm volatile("LDI r16,0x47");
asm volatile("STS 0x60,r16");

I am starting to think my avr-gcc is broken,

although i have:

#define __AVR_ATmega328P__

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

gabikz wrote:
although i have:
Why on earth would you ever need such a line?!?!? You clearly are not using the compiler correctly if you think you need to make such a #define manually.

 

You should be invoking the compiler with:

avr-gcc -mmcu=atmega328p ....

That ensures that -mmcu=atmega328p is passed to both the compiler and the linker (both need to know that it is a 328). When the switch goes to the C compiler it has a look up table that converts all the possible -mmcu= values into one of the __AVR_ATxxxxx__ options. Later on, when you '#include the <avr/io.h> file it uses:

#if defined (__AVR_AT94K__)
#  include <avr/ioat94k.h>
#elif defined (__AVR_AT43USB320__)
#  include <avr/io43u32x.h>
#elif defined (__AVR_AT43USB355__)
#  include <avr/io43u35x.h>
#elif defined (__AVR_AT76C711__)
#  include <avr/io76c711.h>
#elif defined (__AVR_AT86RF401__)
#  include <avr/io86r401.h>
#elif defined (__AVR_AT90PWM1__)
#  include <avr/io90pwm1.h>
#elif defined (__AVR_AT90PWM2__)
#  include <avr/io90pwmx.h>
#elif defined (__AVR_AT90PWM2B__)
#  include <avr/io90pwm2b.h>
#elif defined (__AVR_AT90PWM3__)
<and tons more where this came from!!!>

to then pull in the right ioXXX.h for the device. So if you find this only works if you:

#define __AVR_ATmega328P__

then it's because -mmcu= is not set correctly and that will have all sorts of other very subtle side effects.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

1)  Did you follow Cliff's link? 

2)  Does your toolchain's generated code for the WDTCSR sequence, with your chosen optimization settings, follow the four-cycle limit?

    WDTCSR = (1 << WDCE) | (1 << WDE);
    if (state == 0) {
        WDTCSR = 0;
    } else {
        WDTCSR = (1<<WDP3) | (1<<WDP0) | (1<<WDIE);
    }

I doubt much whether that first fragment does.  The second will depend on your optimization settings; post the generated code.

 

There have been a number of threads on watchdog-interrupt setup on this AVR series.  See if you can search out some pertinent ones.

 

This is my working sequence with my toolchain that I've used in a number of apps.  [I know that the 4-cycle is OK]  The only difference to yours is that I set the period in both writes.  I seem to recall some discussion on that; I don't know if it is really needed or not.

WDTCSR = BIT_WDTCSR_WDCE | BIT_WDTCSR_WDE | WDT_8_SEC;
WDTCSR = BIT_WDTCSR_WDIE | WDT_8_SEC;

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Few updates [still not working]

 

1) removed the __AVR_Atmega328P__ define

2) this is the avr-gcc command I am using:

avr-gcc -Wall -g2 -gstabs -O0 -fpack-struct -fshort-enums -ffunction-sections -fdata-sections -std=gnu99 -funsigned-char -funsigned-bitfields -mmcu=atmega328p -DF_CPU=128000UL -MMD -MP -MF"main.d" -MT"main.o" -c -o "main.o" "../main.c"
avr-gcc -Wl,-Map,only_watchdog.map -mmcu=atmega328p -o "only_watchdog.elf"  ./main.o
avr-objcopy -R .eeprom -R .fuse -R .lock -R .signature -O ihex only_watchdog.elf  "only_watchdog.hex"

I am using Eclipse with AVR-GCC, so it is the default settings

 

3) Tried to add the prescaled to the WDCE set, no help neider.. 

WDTCSR = (1 << WDCE) | (1 << WDE) | 7;

 

4) tried to compile and build it manually.. no help.

 

But I cannot get it, the gcc commands are the same as I am using the asm() command, and the asm command works..

 

My latest code:

 

#define LED_PORT PORTD
#define LED_PIN PD6
#define LED_DDR DDRD

#define WD_COUNTER_MAX 1

#include <avr/io.h>
#include <avr/interrupt.h>
#include <stdint.h>
#include <util/delay.h>
#include <avr/wdt.h>
#include <avr/sleep.h>

volatile uint8_t watchdog_counter;



void setup_watchdog()
{
	MCUSR = 0;
	wdt_reset();

	WDTCSR = (1 << WDCE) | (1 << WDE) | 7;
	WDTCSR = 71;

//	__asm__ __volatile__ ("sts %0,%1" "\n\t" : : "M" (_SFR_MEM_ADDR(WDTCSR)), "r" (71));
//	asm volatile("LDI r16,0x47");
//	asm volatile("STS 0x60,r16");

	test_blink(2);
	wdt_reset();
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

gabikz wrote:
2) this is the avr-gcc command I am using: avr-gcc -Wall -g2 -gstabs -O0 ...

 

theusch wrote:
The second will depend on your optimization settings; post the generated code.

It is very unlikely that -O0 will generate the correct sequence (i.e. within the 4-cycle limit).

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

gabikz wrote:
WDTCSR = 71;

???  So now we need to go and decode that?

 

WDIE+WDCE+WDP3+WDP0

 

I don't think you want a WDCE in that line.

 

Oh, 71 decimal; sorry.  Lessee -- WDIE+WDP2+WDP1+WDP0  Looks reasonable.

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

Last Edited: Wed. Jun 14, 2017 - 02:46 PM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1
#include <avr/io.h>
#include <avr/interrupt.h>
int main(void)
{
WDTCSR = (1 << WDCE) | (1 << WDE);
WDTCSR = (1<<WDP3) | (1<<WDP0) | (1<<WDIE);

	while(1)
	{
	}
}

Built with -O0:

int main(void)
{
  7a:	cf 93       	push	r28
  7c:	df 93       	push	r29
  7e:	cd b7       	in	r28, 0x3d	; 61
  80:	de b7       	in	r29, 0x3e	; 62
WDTCSR = (1 << WDCE) | (1 << WDE);
  82:	80 e6       	ldi	r24, 0x60	; 96
  84:	90 e0       	ldi	r25, 0x00	; 0
  86:	28 e1       	ldi	r18, 0x18	; 24
  88:	fc 01       	movw	r30, r24
  8a:	20 83       	st	Z, r18
WDTCSR = (1<<WDP3) | (1<<WDP0) | (1<<WDIE);
  8c:	80 e6       	ldi	r24, 0x60	; 96
  8e:	90 e0       	ldi	r25, 0x00	; 0
  90:	21 e6       	ldi	r18, 0x61	; 97
  92:	fc 01       	movw	r30, r24
  94:	20 83       	st	Z, r18

	while(1)
	{

 

Four-cycle limitation not met.  Built with -O1:

 

int main(void)
{
WDTCSR = (1 << WDCE) | (1 << WDE);
  7a:	e0 e6       	ldi	r30, 0x60	; 96
  7c:	f0 e0       	ldi	r31, 0x00	; 0
  7e:	88 e1       	ldi	r24, 0x18	; 24
  80:	80 83       	st	Z, r24
WDTCSR = (1<<WDP3) | (1<<WDP0) | (1<<WDIE);
  82:	81 e6       	ldi	r24, 0x61	; 97
  84:	80 83       	st	Z, r24

 

>>Now<< you can watchdog interrupt with impunity.  With -Os:

int main(void)
{
WDTCSR = (1 << WDCE) | (1 << WDE);
  7a:	88 e1       	ldi	r24, 0x18	; 24
  7c:	80 93 60 00 	sts	0x0060, r24	; 0x800060 <__TEXT_REGION_LENGTH__+0x7e0060>
WDTCSR = (1<<WDP3) | (1<<WDP0) | (1<<WDIE);
  80:	81 e6       	ldi	r24, 0x61	; 97
  82:	80 93 60 00 	sts	0x0060, r24	; 0x800060 <__TEXT_REGION_LENGTH__+0x7e0060>

Different approach; four-cycle limitation met.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

Last Edited: Wed. Jun 14, 2017 - 02:54 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

How did you reversed from the compiles hex back to asm?

 

I am going to try -O1 now

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

gabikz wrote:
How did you reversed from the compiles hex back to asm?

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

What does Cliff say?  Something like "No-one should every use -O0 in an AVR app.  It is there primarily for the compiler writers.  Studio shouldn'e even preset it as an option".

 

But you aren't using Studio.  The IDE's -O0 default is just plain ugly.  You will have to hunt up the .LSS on your own in your IDE/toolchain installation.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are right, I am using Eclipse.. it is Mac laptop :(

I WISH they had Mac option for AVR studio... 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Just to note that:

#include <avr/io.h>
#include <avr/wdt.h>

int main(void) {
	wdt_enable(WDTO_8S);
}

I know that's not much use in this case as it does not provide for setting WDIE however I thought it was illuminating to look at the code it generates:

avr-gcc -mmcu=atmega328p -g -Os avr.c -o avr.elf
  80:   99 e2           ldi     r25, 0x29       ; 41
  82:   88 e1           ldi     r24, 0x18       ; 24
  84:   0f b6           in      r0, 0x3f        ; 63
  86:   f8 94           cli
  88:   a8 95           wdr
  8a:   80 93 60 00     sts     0x0060, r24
  8e:   0f be           out     0x3f, r0        ; 63
  90:   90 93 60 00     sts     0x0060, r25

One issue it is dealing with there is the state of I in SREG.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
it does not provide for setting WDIE ...

A topic in some of the past discussions.  Once the correct magic incantations for watchdog-interrupt on that series were figured out, I've always been surprised that no-one amended the wdt.h stuff for watchdog interrupt.

 

clawson wrote:
One issue it is dealing with there is the state of I in SREG.

IME nearly all apps don't fuss with WD during operation, and at startup the first SEI wouldn't be done.  A good point for the thread, though.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

theusch wrote:
I've always been surprised that no-one amended the wdt.h stuff for watchdog interrupt.

Yup, I guess this whole open source thing is only driven when someone has the need for some missing feature then, after doing the work, generously pushes it back upstream. Presumably either no one has seen the need to expand wdt.h out for WDIE or those who had the need did not think about contributing?

 

There are, of course, always Atmel themselves - they push changes into some of the LibC stuff from time to time - perhaps they might update it one day? OTOH perhaps they maybe see ASF3/ASF4/Start as the way to go for that kind of thing? (which would then raise the question of what ASF/Start has to offer for Watchdog support in various AVRs?)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I seem to recall that one of the gurus might have posted an appropriate macro here in past related discussions.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.