Skip to content

Commit

Permalink
save RAM content overridden by bootloader magic
Browse files Browse the repository at this point in the history 
and restore it in case of aborted reboot
use RAMEND-1 as suggested by @yyyc514 in PR arduino#2474

of course it's not a real solution but we cannot force everyone to update the bootloader using an external programmer
  • Loading branch information
@facchinm
facchinm committed Jul 1, 2015
1 parent b275d9c commit 95b1550
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion hardware/arduino/avr/cores/arduino/CDC.cpp
View file
Expand Up @@ -95,6 +95,7 @@ bool CDC_Setup(USBSetup& setup)
// We check DTR state to determine if host port is open (bit 0 of lineState).
if (1200 == _usbLineInfo.dwDTERate && (_usbLineInfo.lineState & 0x01) == 0)
{
*(uint16_t *)(RAMEND-1) = *(uint16_t *)0x0800;
*(uint16_t *)0x0800 = 0x7777;
wdt_enable(WDTO_120MS);
}
Expand All @@ -107,7 +108,7 @@ bool CDC_Setup(USBSetup& setup)

wdt_disable();
wdt_reset();
*(uint16_t *)0x0800 = 0x0;
*(uint16_t *)0x0800 = *(uint16_t *)(RAMEND-1);
}
}
return true;
Expand Down

6 comments on commit 95b1550

@joshgoebel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, this is a neat trick, and an improvement, but not a real solution as you mention.

Is the USB stuff not interrupt driven? Is there a reason we can't just delay(125) after the watchdog? Freezing the running program would prevent the memory corruption from being seen while we're not sure if a reboot is going to happen... if we could swap the memory in and out + delay I think that would be a pretty solid fix.

@matthijskooijman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would mean you need to re-enable interrupts inside an interrupt handler, which is possible but might make things more complex than you'd want (if the host quickly toggles the baudrate or dtr, the stack could overflow, I think).

@joshgoebel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, because we need the timer for delay.

@matthijskooijman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And because you want to allow cancelling the reset again, which needs an interrupt?

@joshgoebel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems you could pair this fix with a user-space pause inside loop that was something like this (pseudo code):

while (magic_location == 0x7777 and WDT_enabled? )
  { 
  NOP 
  or just nothing
  or delay(x); (would allow yield, if yield didn't depend on the magic location)   
}

This would effectively "pause" user space while a reboot was possibly happening and prevent bad behavior or memory corruption. Would this be a reasonable recommendation for someone who wanted absolute stability and who was compiling with a patched CDC that was preserving memory?

@joshgoebel
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Obviously this would be program specific... but all my issues stem from that location being active memory... and either it's value MATTERS, or it's being changed too rapidly by user space and the reboot never even happens because by the time the WDT fires the memory location is no longer 7777.

Please sign in to comment.