Store MCUSR into R2 on bootup

[De-Backer]

this makes it possible to find the MCU reset caused in run code. because the MCUSR is being erased by the bootloader.

[crycode-de]

Thank you for this PR. Good idea.

The register is only saved on controllers with the MCUSR register. I think it should handle MCUCSR as well.

Maybe there should be an option in config.h to disable this, because I don't know if it could affect the main program in any unwanted way?

[De-Backer]

If you can wait a little longer I'll make some more adjustments.

• option in config.h
• handle MCUCSR as well
• review datasheets of the AVR's
• did i forget something?

ETA: I think for Friday (I'm still busy with other things).

[crycode-de]

Sounds good. Take all the time you need. :-)

[De-Backer]

• review datasheets of the AVR's => ok

ok: ATmega328P MCUSR GPIOR0 ATmega644P MCUSR GPIOR0 ATmega1284P MCUSR GPIOR0 ATmega640/V MCUSR GPIOR0 ATmega1280/V MCUSR GPIOR0 ATmega1281/V MCUSR GPIOR0 ATmega2560/V MCUSR GPIOR0 ATmega2561/V MCUSR GPIOR0

• handle MCUCSR as well => GPIOR0 is missing

Niet: ATmega32 MCUCSR missing ATmega32L MCUCSR missing ATmega64 MCUCSR missing ATmega64L MCUCSR missing ATmega128 MCUCSR missing ATmega128L MCUCSR missing

Todo: option in config.h

[De-Backer]

is there an alternative to the GPIOR0 ?

• handle MCUCSR as well => GPIOR0 is missing ATmega32 MCUCSR missing ATmega32L MCUCSR missing ATmega64 MCUCSR missing ATmega64L MCUCSR missing ATmega128 MCUCSR missing ATmega128L MCUCSR missing

should we use the RAM? a ref: http://amichalec.net/2015/06/18/xmem-mcucsr/ Or should we mention that store MCUCSR is not workable.

[crycode-de]

I've took a look at how Optiboot (the Arduino bootloader) handles this. They store the value of MCUSR into the R2 register here. R2 should be available on all AVRs.

Maybe we should adopt this and store the original value of MCUSR/MCUCSR in R2 too?

[De-Backer]

This sketch demonstrates retrival of the Reset Cause register: https://github.com/arduino/Arduino/issues/5346#issuecomment-244935316

[crycode-de]

I think we should use the R2 register instead of GPIOR0 to store the information and maybe add some description in the readme (after the "CAN bus communication" description). Would be nice if you could adjust your PR. :-)

[De-Backer]

Would be nice if you could adjust your PR. :-)

yes it would, I can't test the bootloader code for the moment. I need to see how Optiboot works first. Then documentation must be written.

And a test.=> my problem.

[crycode-de]

Unfortunately, I don't have much time for this at the moment . Maybe next week or so. Imho we just need to do something like this in MCP-CAN-Boot to store the MCUSR register into R2: (untested)

__asm__ __volatile__ ("  mov r2, %0\n" :: "r" (MCUSR));

and for controllers with MCUCSR:

__asm__ __volatile__ ("  mov r2, %0\n" :: "r" (MCUCSR));

[De-Backer]

code bootloader uint8_t ch;

ch = MCUSR; /* * save the reset flags in the designated register * This can be saved in a main program by putting code in .init0 (which * executes before normal c init code) to save R2 to a global variable. */ __asm__ __volatile__ (" mov r2, %0\n" :: "r" (ch));

code main prog. //copy the MCUSR value saved in r2 by Optiboot to a global variable. uint8_t mcusr __attribute__ ((section (".noinit"))); void getMCUSR(void) __attribute__((naked)) __attribute__((section(".init0")));

void getMCUSR(void) { __asm__ __volatile__ ( "mov %0, r2 \n" : "=r" (mcusr) : ); }

[De-Backer]

Unfortunately, I don't have much time for this at the moment . Maybe next week or so. Imho we just need to do something like this in MCP-CAN-Boot to store the MCUSR register into R2: (untested)

__asm__ __volatile__ ("  mov r2, %0\n" :: "r" (MCUSR));

and for controllers with MCUCSR:

__asm__ __volatile__ ("  mov r2, %0\n" :: "r" (MCUCSR));

looks good let's give it a try

[De-Backer]

for de test : I have an ATmega1284P still in its packaging now a breadboard, and a Mcp2515

[De-Backer]

first test zend reboot (Watchdog time out) (1622578808.024106) can0 TX - - 4FF [2] 04 04 '..' ack reboot (1622578808.025234) can0 RX - - 0100FFFF [8] 01 00 FF 64 FF FF 40 40 '...d..@@' bootloader (1622578808.120979) can0 RX - - 1FFFFF01 [8] FF FF 02 00 1E 97 05 01 '........' main prog (1622578808.373201) can0 RX - - 0100FFFF [8] 01 00 FF 64 FF FF 08 00 '...d....' 08 ==MCU Status Register => Watchdog Reset Flag

[De-Backer]

when updating firmware=> overwrites the R2 register

[crycode-de]

when updating firmware=> overwrites the R2 register

What do you mean with "when updating firmware"? The current code of your PR should update R2 on each bootloader init.

[De-Backer]

yes that works, but if there is a firmware update the R2 register will be overwritten eg:

Flash and verify done in 5637 ms. Starting the app on the MCU ... MCU ist starting the app. :-)

MCUSR == random

[crycode-de]

Sorry for the late response...

Maybe we need to store MCUSR into a variable first and then set R2 in the startApp function right before the gotoApp call?

[De-Backer]

Funny thing, I tried that but I couldn't get it right. it was something like that: unint8_t var=0;//global var __asm__ __volatile__(" sts %0, %1\n"::"r"var,"r"(MCUSR));//Store Direct to SRAM and __asm__ __volatile__(" lds r2, %0\n"::"r"var);//Load Direct from SRAM

my asm is probably wrong.

[De-Backer]

and i also tried this: unint8_t var=0;//global var var=MCUSR; __asm__ __volatile__(" ldi r2, %0\n"::"r"var);//Load Immediate

[De-Backer]

what would help: if i could see the compiler asm (*.S file's) in Cmake I can add this compile_options: -save-temps # Do not delete intermediate files.

but i can't do it for platformio :/

[crycode-de]

You can add the following to the build_flags in platformio.ini to get the .s and .ii files.

  -save-temps
  -fverbose-asm

[De-Backer]

thanks got it working. ToDo: now the documentation in the code

[crycode-de]

Nice, but why are you moving the registers so often? Shouldn't it be enough to store the current value in get_mcusr into a global variable like

#if MCUSR_TO_R2
  uint8_t mcusr = 0;
#endif
// [...]
void get_mcusr(void) {
 // [...]
  #if MCUSR_TO_R2
    mcusr = MCUSR;
  #endif
  MCUSR = 0;
// [...]

and add the move to R2 in startApp like this?

void startApp () {
  // [...]
  #if MCUSR_TO_R2
    __asm__ __volatile__("  mov r2, %0\n" ::"r"(mcusr));
  #endif

  // jump to main application
  gotoApp();
}

[De-Backer]

yes i tried that too see: https://github.com/crycode-de/mcp-can-boot/pull/2#issuecomment-856112455 I always got 0x00 and for the registers movements: Yes always 1: mov r2, MCUSR Yes always 2: mov reset_caused_by,r2 start the main application if we are not in bootloading mode and run into timeout

when bootloading 3: mov r2, reset_caused_by

2 times when starting normal => 2 1 Clocks = 2 3 times when bootloading => 3 1 Clocks = 3

note: http://ww1.microchip.com/downloads/en/Appnotes/doc1497.pdf Eighteen Hints to Reduce Code Size: 2. Use local variables whenever possible

Local variables are preferably assigned to a register when they are declared. The local variable is kept in the same register until the end of the function, or until it is not referenced further. Global variables must be loaded from the SRAM into the working registers before they are accessed.

Variable Code Size (Bytes) Global 10 Local 2 VariableExecution Time (Cycles) Global 5 Local 1

so if you read and write a Global variable you are at 10 Cycles min.

[crycode-de]

Thank you for your explanation. Now it makes sense to me. :-)

One problem... the part

  #ifndef MCUSR  // Backward compatability with old AVRs
    #define MCUSR MCUCSR
  #endif

won't work for ATmega128 and throw an error:

src\bootloader.cpp:50:11: error: attempt to use poisoned "MCUSR"
   #ifndef MCUSR  // Backward compatability with old AVRs

From iom128.h definition:

/* MCU Status Register */
#define MCUSR     _SFR_IO8(0x34)
#define MCUCSR    _SFR_IO8(0x34) /* new name in datasheet (2467E-AVR-05/02) */
// [...]
#pragma GCC poison MCUSR

I've updated the GitHub workflow to run the check action on pull requests too. So if you push something new, all check should be done automatically.

[De-Backer]

there is one more thing I want to test, this is the code in the normal program

uint8_t mcusr __attribute__ ((section (".noinit")));//<= the MCU Status Register void getMCUSR(void) __attribute__((naked)) __attribute__((section(".init0"))); void getMCUSR(void) { __asm__ __volatile__ ( "mov %0, r2 \n" : "=r" (mcusr) : ); }

wat als we call __asm__ __volatile__ ( "mov %0, r2 \n" : "=r" (mcusr) : ); at start of main ( Use local variables )

test Watchdog, ok got Watchdog reset from MCU Status

[crycode-de]

Looks like this feature is ready to be merged, right?

[De-Backer]

yes, it works stable here, note: Brown-out reset from MCU Status works too. (my breadboard will start to fail) :/ next one (a new Pull request): make it work for Standard Data Frame (I thought someone was already working on it) see https://github.com/Doc-Smimble/mcp-can-boot/blob/19ccdd2c76b7572da630dfe4a644d3b728fadc05/src/config.h#L77

[crycode-de]

Brown-out reset from MCU Status works too

👍

Thank you for your contribution! I'll merge this PR.

make it work for Standard Data Frame

This may be done by adding a simple config option. I'll have a look at it tomorrow.