From 01408e8c5ed7da6edb604e03f3ea9d5007a4305f Mon Sep 17 00:00:00 2001 From: Scott Allen Date: Fri, 10 Mar 2017 14:47:18 -0500 Subject: [PATCH 1/4] Fix save/restore of magic key location during reset In the USB CDC code to invoke an auto-reset, the magic key location could be restored before it had actually been saved. The sketch would then have a corrupted value at this location. This fix prevents the value from being restored if it hasn't previously been saved. --- hardware/arduino/avr/cores/arduino/CDC.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/CDC.cpp b/hardware/arduino/avr/cores/arduino/CDC.cpp index 0a743e1ea95..c06b64a02da 100644 --- a/hardware/arduino/avr/cores/arduino/CDC.cpp +++ b/hardware/arduino/avr/cores/arduino/CDC.cpp @@ -119,9 +119,9 @@ bool CDC_Setup(USBSetup& setup) if (1200 == _usbLineInfo.dwDTERate && (_usbLineInfo.lineState & 0x01) == 0) { #if MAGIC_KEY_POS != (RAMEND-1) - // Backup ram value if its not a newer bootloader. + // Backup ram value if its not a newer bootloader and it hasn't already been saved. // This should avoid memory corruption at least a bit, not fully - if (magic_key_pos != (RAMEND-1)) { + if (magic_key_pos != (RAMEND-1) && *(uint16_t *)magic_key_pos != MAGIC_KEY) { *(uint16_t *)(RAMEND-1) = *(uint16_t *)magic_key_pos; } #endif @@ -129,12 +129,14 @@ bool CDC_Setup(USBSetup& setup) *(uint16_t *)magic_key_pos = MAGIC_KEY; wdt_enable(WDTO_120MS); } - else + else if (*(uint16_t *)magic_key_pos == MAGIC_KEY) { // Most OSs do some intermediate steps when configuring ports and DTR can // twiggle more than once before stabilizing. - // To avoid spurious resets we set the watchdog to 250ms and eventually + // To avoid spurious resets we set the watchdog to 120ms and eventually // cancel if DTR goes back high. + // Cancellation is only done if an auto-reset was started, which is + // indicated by the magic key having been set. wdt_disable(); wdt_reset(); From 4c550ce4a53b4138ab1e42e5f3e42dad8122f8ea Mon Sep 17 00:00:00 2001 From: Scott Allen Date: Fri, 10 Mar 2017 15:35:59 -0500 Subject: [PATCH 2/4] Save/restore the watchdog during USB auto-reset The state of the watchdog timer is saved during a USB auto-reset and then restored if the reset is aborted, in case the sketch is using the watchdog. --- hardware/arduino/avr/cores/arduino/CDC.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/hardware/arduino/avr/cores/arduino/CDC.cpp b/hardware/arduino/avr/cores/arduino/CDC.cpp index c06b64a02da..125dc855688 100644 --- a/hardware/arduino/avr/cores/arduino/CDC.cpp +++ b/hardware/arduino/avr/cores/arduino/CDC.cpp @@ -34,6 +34,8 @@ typedef struct static volatile LineInfo _usbLineInfo = { 57600, 0x00, 0x00, 0x00, 0x00 }; static volatile int32_t breakValue = -1; +static u8 wdtcsr_save; + bool _updatedLUFAbootloader = false; #define WEAK __attribute__ ((weak)) @@ -127,6 +129,8 @@ bool CDC_Setup(USBSetup& setup) #endif // Store boot key *(uint16_t *)magic_key_pos = MAGIC_KEY; + // Save the watchdog state in case the reset is aborted. + wdtcsr_save = WDTCSR; wdt_enable(WDTO_120MS); } else if (*(uint16_t *)magic_key_pos == MAGIC_KEY) @@ -138,8 +142,10 @@ bool CDC_Setup(USBSetup& setup) // Cancellation is only done if an auto-reset was started, which is // indicated by the magic key having been set. - wdt_disable(); wdt_reset(); + // Restore the watchdog state in case the sketch was using it. + WDTCSR |= (1< Date: Fri, 10 Mar 2017 15:49:44 -0500 Subject: [PATCH 3/4] Always read key to check for new LUFA bootloader Instead of checking for the NEW_LUFA_SIGNATURE once in program memory and then setting a flag which is used for further checks, a function is used that always checks program memory directly. If a flag is used, there's a slight chance that its location in RAM could fall on MAGIC_KEY_POS. In this case, an aborted USB auto-reset sequence may fail. --- hardware/arduino/avr/cores/arduino/CDC.cpp | 9 ++++++--- hardware/arduino/avr/cores/arduino/USBCore.cpp | 7 ------- hardware/arduino/avr/cores/arduino/USBCore.h | 3 +-- 3 files changed, 7 insertions(+), 12 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/CDC.cpp b/hardware/arduino/avr/cores/arduino/CDC.cpp index 125dc855688..2b0470175e7 100644 --- a/hardware/arduino/avr/cores/arduino/CDC.cpp +++ b/hardware/arduino/avr/cores/arduino/CDC.cpp @@ -36,8 +36,6 @@ static volatile int32_t breakValue = -1; static u8 wdtcsr_save; -bool _updatedLUFAbootloader = false; - #define WEAK __attribute__ ((weak)) extern const CDCDescriptor _cdcInterface PROGMEM; @@ -59,6 +57,11 @@ const CDCDescriptor _cdcInterface = D_ENDPOINT(USB_ENDPOINT_IN (CDC_ENDPOINT_IN ),USB_ENDPOINT_TYPE_BULK,USB_EP_SIZE,0) }; +bool isLUFAbootloader() +{ + return pgm_read_word(FLASHEND - 1) == NEW_LUFA_SIGNATURE; +} + int CDC_GetInterface(u8* interfaceNum) { interfaceNum[0] += 2; // uses 2 @@ -111,7 +114,7 @@ bool CDC_Setup(USBSetup& setup) #if MAGIC_KEY_POS != (RAMEND-1) // For future boards save the key in the inproblematic RAMEND // Which is reserved for the main() return value (which will never return) - if (_updatedLUFAbootloader) { + if (isLUFAbootloader()) { // horray, we got a new bootloader! magic_key_pos = (RAMEND-1); } diff --git a/hardware/arduino/avr/cores/arduino/USBCore.cpp b/hardware/arduino/avr/cores/arduino/USBCore.cpp index 723edb3c828..62f665eb911 100644 --- a/hardware/arduino/avr/cores/arduino/USBCore.cpp +++ b/hardware/arduino/avr/cores/arduino/USBCore.cpp @@ -36,7 +36,6 @@ extern const u8 STRING_PRODUCT[] PROGMEM; extern const u8 STRING_MANUFACTURER[] PROGMEM; extern const DeviceDescriptor USB_DeviceDescriptor PROGMEM; extern const DeviceDescriptor USB_DeviceDescriptorB PROGMEM; -extern bool _updatedLUFAbootloader; const u16 STRING_LANGUAGE[2] = { (3<<8) | (2+2), @@ -815,12 +814,6 @@ void USBDevice_::attach() UDIEN = (1< Date: Fri, 10 Mar 2017 16:18:20 -0500 Subject: [PATCH 4/4] Don't use line coding to trigger USB auto-reset An auto-reset invoked using USB CDC is triggered by the port closing (when set to 1200 baud). Closing of the port is indicated by DTR going inactive. There is no need to have auto-reset invoked by a CDC_SET_LINE_CODING command. Only the CDC_SET_CONTROL_LINE_STATE command, which indicates a change in the state of DTR, should be used. --- hardware/arduino/avr/cores/arduino/CDC.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/hardware/arduino/avr/cores/arduino/CDC.cpp b/hardware/arduino/avr/cores/arduino/CDC.cpp index 2b0470175e7..142f8f66d71 100644 --- a/hardware/arduino/avr/cores/arduino/CDC.cpp +++ b/hardware/arduino/avr/cores/arduino/CDC.cpp @@ -97,10 +97,7 @@ bool CDC_Setup(USBSetup& setup) if (CDC_SET_CONTROL_LINE_STATE == r) { _usbLineInfo.lineState = setup.wValueL; - } - if (CDC_SET_LINE_CODING == r || CDC_SET_CONTROL_LINE_STATE == r) - { // auto-reset into the bootloader is triggered when the port, already // open at 1200 bps, is closed. this is the signal to start the watchdog // with a relatively long period so it can finish housekeeping tasks