Do not rely on Clockstretching for writes

README.md

@@ -3,17 +3,17 @@ twiboot is a simple/small bootloader for AVR MCUs with integrated TWI peripheral
 It was originally created to update I2C controlled BLMCs (Brushless Motor Controller) without an AVR ISP adapter.
 
 twiboot acts as a slave device on a TWI/I2C bus and allows reading/writing of the internal flash memory.
-As a compile time option twiboot also allows reading/writing of the whole internal EEPROM memory.
+As a compile time option (EEPROM_SUPPORT) twiboot also allows reading/writing of the whole internal EEPROM memory.
 The bootloader is not able to update itself (only application flash memory region accessible).
 
 Currently the following AVR MCUs are supported:
 
 AVR MCU | Flash bytes used (.text + .data) | Bootloader region size
 --- | --- | ---
-atmega8 | 724 (0x2D4) | 512 words
-atmega88 | 744 (0x2E8) | 512 words
-atmega168 | 744 (0x2E8) | 512 words
-atmega328p | 744 (0x2E8) | 512 words
+atmega8 | 802 (0x322) | 512 words
+atmega88 | 826 (0x33A) | 512 words
+atmega168 | 826 (0x33A) | 512 words
+atmega328p | 826 (0x33A) | 512 words
 
 (Compiled on Ubuntu 18.04 LTS (gcc 5.4.0 / avr-libc 2.0.0) with EEPROM and LED support)
 
@@ -33,7 +33,7 @@ A TWI/I2C master can use the protocol to
 - write the internal eeprom (byte wise)
 - exit the bootloader and start the application
 
-As a compile time option twiboot can output its state with two LEDs.
+As a compile time option (LED_SUPPORT) twiboot can output its state with two LEDs.
 One LED will flash with a frequency of 20Hz while twiboot is active (including boot wait time).
 A second LED will flash when the bootloader is addressed on the TWI/I2C bus.
 
@@ -74,7 +74,7 @@ Read chip info | **SLA+W**, 0x02, 0x00, 0x00, 0x00, **SLA+R**, {8 bytes}, **STO*
 Read 1+ flash bytes | **SLA+W**, 0x02, 0x01, addrh, addrl, **SLA+R**, {* bytes}, **STO** |
 Read 1+ eeprom bytes | **SLA+W**, 0x02, 0x02, addrh, addrl, **SLA+R**, {* bytes}, **STO** |
 Write one flash page | **SLA+W**, 0x02, 0x01, addrh, addrl, {* bytes}, **STO** | page size as indicated in chip info
-Write 1+ eeprom bytes | **SLA+W**, 0x02, 0x02, addrh, addrl, {* bytes}, **STO** |
+Write 1+ eeprom bytes | **SLA+W**, 0x02, 0x02, addrh, addrl, {* bytes}, **STO** | write 0 < n < page size bytes at once
 
 **SLA+R** means Start Condition, Slave Address, Read Access
 
@@ -82,9 +82,23 @@ Write 1+ eeprom bytes | **SLA+W**, 0x02, 0x02, addrh, addrl, {* bytes}, **STO**
 
 **STO** means Stop Condition
 
+A flash page / eeprom write is only triggered after the Stop Condition.
+During the write process twiboot will NOT acknowledge its slave address.
+
 The multiboot_tool repository contains a simple linux application that uses
 this protocol to access the bootloader over linux i2c device.
 
+The ispprog programming adapter can also be used as a avr910/butterfly to twiboot protocol bridge.
+
+
+## TWI/I2C Clockstretching ##
+While a write is in progress twiboot will not respond on the TWI/I2C bus and the
+TWI/I2C master needs to retry/poll the slave address until the write has completed.
+
+As a compile time option (USE_CLOCKSTRETCH) the previous behavior of twiboot can be restored:
+TWI/I2C Clockstretching is then used to inform the master of the duration of the write.
+Please note that there are some TWI/I2C masters that do not support clockstretching.
+
 
 ## Development ##
 Issue reports, feature requests, patches or simply success stories are much appreciated.
@@ -92,5 +106,4 @@ Issue reports, feature requests, patches or simply success stories are much appr
 
 ## Roadmap ##
 Some ideas that I want to investigate / implement in twiboot:
-- find a way to not rely on TWI clock stretching during write access
 - support AVR TINYs (USI peripheral, no bootloader fuse, no Read-While-Write flash)

main.c

@@ -24,12 +24,15 @@
 #define VERSION_STRING      "TWIBOOT v2.1"
 #define EEPROM_SUPPORT      1
 #define LED_SUPPORT         1
+#define USE_CLOCKSTRETCH    0
 
-/* 25ms @8MHz */
-#define TIMER_RELOAD        (0xFF - 195)
+#define F_CPU               8000000ULL
+#define TIMER_DIVISOR       1024
+#define TIMER_IRQFREQ_MS    25
+#define TIMEOUT_MS          1000
 
-/* 40 * 25ms */
-#define TIMEOUT             40
+#define TIMER_MSEC2TICKS(x)     ((x * F_CPU) / (TIMER_DIVISOR * 1000ULL))
+#define TIMER_MSEC2IRQCNT(x)    (x / TIMER_IRQFREQ_MS)
 
 #if LED_SUPPORT
 #define LED_INIT()          DDRB = ((1<<PORTB4) | (1<<PORTB5))
@@ -56,23 +59,19 @@
 /* SLA+R */
 #define CMD_WAIT                0x00
 #define CMD_READ_VERSION        0x01
-#define CMD_READ_MEMORY         0x02
+#define CMD_ACCESS_MEMORY       0x02
 /* internal mappings */
-#define CMD_READ_CHIPINFO       (0x10 | CMD_READ_MEMORY)
-#define CMD_READ_FLASH          (0x20 | CMD_READ_MEMORY)
-#define CMD_READ_EEPROM         (0x30 | CMD_READ_MEMORY)
-#define CMD_READ_PARAMETERS     (0x40 | CMD_READ_MEMORY)    /* only in APP */
+#define CMD_ACCESS_CHIPINFO     (0x10 | CMD_ACCESS_MEMORY)
+#define CMD_ACCESS_FLASH        (0x20 | CMD_ACCESS_MEMORY)
+#define CMD_ACCESS_EEPROM       (0x30 | CMD_ACCESS_MEMORY)
+#define CMD_WRITE_FLASH_PAGE    (0x40 | CMD_ACCESS_MEMORY)
+#define CMD_WRITE_EEPROM_PAGE   (0x50 | CMD_ACCESS_MEMORY)
 
 /* SLA+W */
 #define CMD_SWITCH_APPLICATION  CMD_READ_VERSION
-#define CMD_WRITE_MEMORY        CMD_READ_MEMORY
 /* internal mappings */
 #define CMD_BOOT_BOOTLOADER     (0x10 | CMD_SWITCH_APPLICATION) /* only in APP */
 #define CMD_BOOT_APPLICATION    (0x20 | CMD_SWITCH_APPLICATION)
-#define CMD_WRITE_CHIPINFO      (0x10 | CMD_WRITE_MEMORY)       /* invalid */
-#define CMD_WRITE_FLASH         (0x20 | CMD_WRITE_MEMORY)
-#define CMD_WRITE_EEPROM        (0x30 | CMD_WRITE_MEMORY)
-#define CMD_WRITE_PARAMETERS    (0x40 | CMD_WRITE_MEMORY)       /* only in APP */
 
 /* CMD_SWITCH_APPLICATION parameter */
 #define BOOTTYPE_BOOTLOADER     0x00    /* only in APP */
@@ -82,7 +81,6 @@
 #define MEMTYPE_CHIPINFO        0x00
 #define MEMTYPE_FLASH           0x01
 #define MEMTYPE_EEPROM          0x02
-#define MEMTYPE_PARAMETERS      0x03    /* only in APP */
 
 /*
  * LED_GN flashes with 20Hz (while bootloader is running)
@@ -130,8 +128,7 @@ const static uint8_t chipinfo[8] = {
 #endif
 };
 
-/* wait 40 * 25ms = 1s */
-static uint8_t boot_timeout = TIMEOUT;
+static uint8_t boot_timeout = TIMER_MSEC2IRQCNT(TIMEOUT_MS);
 static uint8_t cmd = CMD_WAIT;
 
 /* flash buffer */
@@ -147,26 +144,24 @@ static void write_flash_page(void)
     uint8_t size = SPM_PAGESIZE;
     uint8_t *p = buf;
 
-    if (pagestart >= BOOTLOADER_START)
+    if (pagestart < BOOTLOADER_START)
     {
-        return;
+        boot_page_erase(pagestart);
+        boot_spm_busy_wait();
+
+        do {
+            uint16_t data = *p++;
+            data |= *p++ << 8;
+            boot_page_fill(addr, data);
+
+            addr += 2;
+            size -= 2;
+        } while (size);
+
+        boot_page_write(pagestart);
+        boot_spm_busy_wait();
+        boot_rww_enable();
     }
-
-    boot_page_erase(pagestart);
-    boot_spm_busy_wait();
-
-    do {
-        uint16_t data = *p++;
-        data |= *p++ << 8;
-        boot_page_fill(addr, data);
-
-        addr += 2;
-        size -= 2;
-    } while (size);
-
-    boot_page_write(pagestart);
-    boot_spm_busy_wait();
-    boot_rww_enable();
 } /* write_flash_page */
 
 
@@ -174,12 +169,11 @@ static void write_flash_page(void)
 /* *************************************************************************
  * read_eeprom_byte
  * ************************************************************************* */
-static uint8_t read_eeprom_byte(void)
+static uint8_t read_eeprom_byte(uint16_t address)
 {
-    EEARL = addr;
-    EEARH = (addr >> 8);
+    EEARL = address;
+    EEARH = (address >> 8);
     EECR |= (1<<EERE);
-    addr++;
 
     return EEDR;
 } /* read_eeprom_byte */
@@ -195,17 +189,34 @@ static void write_eeprom_byte(uint8_t val)
     EEDR = val;
     addr++;
 
-#if defined (__AVR_ATmega8__)
+#if defined (EEWE)
     EECR |= (1<<EEMWE);
     EECR |= (1<<EEWE);
-#elif defined (__AVR_ATmega88__) || defined (__AVR_ATmega168__) || \
-      defined (__AVR_ATmega328P__)
+#elif defined (EEPE)
     EECR |= (1<<EEMPE);
     EECR |= (1<<EEPE);
+#else
+#error "EEWE/EEPE not defined"
 #endif
 
     eeprom_busy_wait();
 } /* write_eeprom_byte */
+
+
+#if (USE_CLOCKSTRETCH == 0)
+/* *************************************************************************
+ * write_eeprom_buffer
+ * ************************************************************************* */
+static void write_eeprom_buffer(uint8_t size)
+{
+    uint8_t *p = buf;
+
+    while (size--)
+    {
+        write_eeprom_byte(*p++);
+    }
+} /* write_eeprom_buffer */
+#endif /* (USE_CLOCKSTRETCH == 0) */
 #endif /* EEPROM_SUPPORT */
 
 
@@ -222,12 +233,13 @@ static uint8_t TWI_data_write(uint8_t bcnt, uint8_t data)
             switch (data)
             {
                 case CMD_SWITCH_APPLICATION:
-                case CMD_WRITE_MEMORY:
+                case CMD_ACCESS_MEMORY:
                     /* no break */
 
                 case CMD_WAIT:
                     /* abort countdown */
                     boot_timeout = 0;
+                    cmd = data;
                     break;
 
                 default:
@@ -236,8 +248,6 @@ static uint8_t TWI_data_write(uint8_t bcnt, uint8_t data)
                     ack = 0x00;
                     break;
             }
-
-            cmd = data;
             break;
 
         case 1:
@@ -252,19 +262,19 @@ static uint8_t TWI_data_write(uint8_t bcnt, uint8_t data)
                     ack = 0x00;
                     break;
 
-                case CMD_WRITE_MEMORY:
+                case CMD_ACCESS_MEMORY:
                     if (data == MEMTYPE_CHIPINFO)
                     {
-                        cmd = CMD_WRITE_CHIPINFO;
+                        cmd = CMD_ACCESS_CHIPINFO;
                     }
                     else if (data == MEMTYPE_FLASH)
                     {
-                        cmd = CMD_WRITE_FLASH;
+                        cmd = CMD_ACCESS_FLASH;
                     }
 #if (EEPROM_SUPPORT)
                     else if (data == MEMTYPE_EEPROM)
                     {
-                        cmd = CMD_WRITE_EEPROM;
+                        cmd = CMD_ACCESS_EEPROM;
                     }
 #endif /* (EEPROM_SUPPORT) */
                     else
@@ -288,20 +298,41 @@ static uint8_t TWI_data_write(uint8_t bcnt, uint8_t data)
         default:
             switch (cmd)
             {
-                case CMD_WRITE_FLASH:
-                    buf[bcnt -4] = data;
-                    if (bcnt >= sizeof(buf) +3)
-                    {
-                        write_flash_page();
-                        ack = 0x00;
-                    }
-                    break;
-
 #if (EEPROM_SUPPORT)
-                case CMD_WRITE_EEPROM:
+#if (USE_CLOCKSTRETCH)
+                case CMD_ACCESS_EEPROM:
                     write_eeprom_byte(data);
                     break;
+#else
+                case CMD_ACCESS_EEPROM:
+                    cmd = CMD_WRITE_EEPROM_PAGE;
+                    /* fall through */
+
+                case CMD_WRITE_EEPROM_PAGE:
+#endif /* (USE_CLOCKSTRETCH) */
 #endif /* (EEPROM_SUPPORT) */
+                case CMD_ACCESS_FLASH:
+                {
+                    uint8_t pos = bcnt -4;
+
+                    buf[pos] = data;
+                    if (pos >= (sizeof(buf) -2))
+                    {
+                        ack = 0x00;
+                    }
+
+                    if ((cmd == CMD_ACCESS_FLASH) &&
+                        (pos >= (sizeof(buf) -1))
+                       )
+                    {
+#if (USE_CLOCKSTRETCH)
+                        write_flash_page();
+#else
+                        cmd = CMD_WRITE_FLASH_PAGE;
+#endif
+                    }
+                    break;
+                }
 
                 default:
                     ack = 0x00;
@@ -328,18 +359,18 @@ static uint8_t TWI_data_read(uint8_t bcnt)
             data = info[bcnt];
             break;
 
-        case CMD_READ_CHIPINFO:
+        case CMD_ACCESS_CHIPINFO:
             bcnt %= sizeof(chipinfo);
             data = chipinfo[bcnt];
             break;
 
-        case CMD_READ_FLASH:
+        case CMD_ACCESS_FLASH:
             data = pgm_read_byte_near(addr++);
             break;
 
 #if (EEPROM_SUPPORT)
-        case CMD_READ_EEPROM:
-            data = read_eeprom_byte();
+        case CMD_ACCESS_EEPROM:
+            data = read_eeprom_byte(addr++);
             break;
 #endif /* (EEPROM_SUPPORT) */
 
@@ -373,7 +404,6 @@ static void TWI_vect(void)
             if (TWI_data_write(bcnt++, TWDR) == 0x00)
             {
                 control &= ~(1<<TWEA);
-                bcnt = 0;
             }
             break;
 
@@ -389,8 +419,38 @@ static void TWI_vect(void)
 
         /* prev. SLA+W, data received, NACK returned -> IDLE */
         case 0x88:
+            TWI_data_write(bcnt++, TWDR);
+            /* fall through */
+
         /* STOP or repeated START -> IDLE */
         case 0xA0:
+#if (USE_CLOCKSTRETCH == 0)
+            if ((cmd == CMD_WRITE_FLASH_PAGE)
+#if (EEPROM_SUPPORT)
+                || (cmd == CMD_WRITE_EEPROM_PAGE)
+#endif
+               )
+            {
+                /* disable ACK for now, re-enable after page write */
+                control &= ~(1<<TWEA);
+                TWCR = (1<<TWINT) | control;
+
+#if (EEPROM_SUPPORT)
+                if (cmd == CMD_WRITE_EEPROM_PAGE)
+                {
+                    write_eeprom_buffer(bcnt -4);
+                }
+                else
+#endif /* (EEPROM_SUPPORT) */
+                {
+                    write_flash_page();
+                }
+            }
+#endif /* (USE_CLOCKSTRETCH) */
+
+            bcnt = 0;
+            /* fall through */
+
         /* prev. SLA+R, data sent, NACK returned -> IDLE */
         case 0xC0:
             LED_RT_OFF();
@@ -413,7 +473,7 @@ static void TWI_vect(void)
 static void TIMER0_OVF_vect(void)
 {
     /* restart timer */
-    TCNT0 = TIMER_RELOAD;
+    TCNT0 = 0xFF - TIMER_MSEC2TICKS(TIMER_IRQFREQ_MS);
 
     /* blink LED while running */
     LED_GN_TOGGLE();
@@ -483,11 +543,12 @@ int main(void)
     LED_GN_ON();
 
     /* timer0: running with F_CPU/1024 */
-#if defined (__AVR_ATmega8__)
+#if defined (TCCR0)
     TCCR0 = (1<<CS02) | (1<<CS00);
-#elif defined (__AVR_ATmega88__) || defined (__AVR_ATmega168__) || \
-      defined (__AVR_ATmega328P__)
+#elif defined (TCCR0B)
     TCCR0B = (1<<CS02) | (1<<CS00);
+#else
+#error "TCCR0(B) not defined"
 #endif
 
     /* TWI init: set address, auto ACKs */
@@ -501,19 +562,20 @@ int main(void)
             TWI_vect();
         }
 
-#if defined (__AVR_ATmega8__)
+#if defined (TIFR)
         if (TIFR & (1<<TOV0))
         {
             TIMER0_OVF_vect();
             TIFR = (1<<TOV0);
         }
-#elif defined (__AVR_ATmega88__) || defined (__AVR_ATmega168__) || \
-      defined (__AVR_ATmega328P__)
+#elif defined (TIFR0)
         if (TIFR0 & (1<<TOV0))
         {
             TIMER0_OVF_vect();
             TIFR0 = (1<<TOV0);
         }
+#else
+#error "TIFR(0) not defined"
 #endif
     }
 
@@ -521,11 +583,12 @@ int main(void)
     TWCR = 0x00;
 
     /* disable timer0 */
-#if defined (__AVR_ATmega8__)
+#if defined (TCCR0)
     TCCR0 = 0x00;
-#elif defined (__AVR_ATmega88__) || defined (__AVR_ATmega168__) || \
-      defined (__AVR_ATmega328P__)
+#elif defined (TCCR0B)
     TCCR0B = 0x00;
+#else
+#error "TCCR0(B) not defined"
 #endif
 
     LED_OFF();