search

baerwolf / USBaspLoader

An (V)USB bootloader firmware for AVR-MCUs emulating the popular USBasp for programming itself
Other
178 stars 146 forks source link

Its not enough to call wdt_disable on some devices. #6

Closed coldtobi closed 11 years ago

coldtobi commented 11 years ago

At least on the ATMega644, the MCUSR bit WDRF in the MCUSR needs to be cleared first, otherwise it is not possible to disable the watchdog. (See http://www.atmel.com/Images/doc2593.pdf, p.53)

• Bit 3 - WDE: Watchdog System Reset Enable WDE is overridden by WDRF in MCUSR. This means that WDE is always set when WDRF is set. To clear WDE, WDRF must be cleared first. This feature ensures multiple resets during conditions causing failure, and a safe start-up after the failure.

Otherwise the software cannot disable the wdt.

This line just before wdt_disable will fix this

MCUCSR = ~(1<<WDRF); wdt_disable(); /* main app may have enabled watchdog */

baerwolf commented 11 years ago

Hi coldtobi. Thank you for your remark, but I do not think that this is neccessary or an issue of any kind.

The reason to use high-level programming languages. These abstract hardware specific attributes into common commands. In case of (avr-)glibc the correct code is then selected via MACROS (conditional compilation) at compile time. This is why you always have to define MCU and F_CPU...

So when using "wdt_disable()" your (avr-glibc) library is in responsibility to do the right things for the selected processor. (And as the name might indicate, I want to disable the watchdog and not care about bits to set or reset.) There is also no BUG, looking into the "avr/wdt.h" for ATmega644p unmagics into:

asm volatile ( \ "in tmp_reg, SREG" "\n\t" \ "cli" "\n\t" \ "sts %0, %1" "\n\t" \ "sts %0, zero_reg" "\n\t" \ "out SREG,tmp_reg" "\n\t" \ : /* no outputs */ \ : "M" (_SFR_MEM_ADDR(_WD_CONTROL_REG)), \ "r" ((uint8_t)(_BV(_WD_CHANGE_BIT) | _BV(WDE))) \ : "r0" \ )

And there is an WDE-bit already set...

I am looking forward to your response, until then (or timeout) this issue remains open.

Best regards, Stephan

baerwolf commented 11 years ago

Ah, sorry. I see "WDRF" is missing. Avr-libc BUG ? Would you ask the "http://savannah.nongnu.org/projects/avr-libc/" -team, or should I file an issue?

Regards, Stephan

baerwolf commented 11 years ago

I will add it to the todos to WA broken avr-libcs.

Thanks.

coldtobi commented 11 years ago

Please go ahead and ask the avr-libc team what they think about it.

However, they documented that you need to clear MCUSR here http://www.nongnu.org/avr-libc/user-manual/group__avr__watchdog.html

Saving the value of MCUSR in mcusr_mirror is only needed if the application later wants to examine the reset source, but in particular, clearing the watchdog reset flag before disabling the watchdog is required, according to the datasheet.

So I suppose this is somehow intentional to avoid loosing the information of the reset source.

baerwolf commented 11 years ago

Okay, thank you. It seems to be intention. I will fix it in USBaspLoader within upcoming version.

Best regards, Stephan

baerwolf commented 11 years ago

Again thank you for your report. The issue (6) is now fixed with commit 27f21b1464a7743393bb1ea88f70d687155bbe4b (https://github.com/baerwolf/USBaspLoader/commit/27f21b1464a7743393bb1ea88f70d687155bbe4b) in version v0.96.

Best regards, Stephan