From 4babe183dae107fc249d4f853faa61b0653d8baa Mon Sep 17 00:00:00 2001 From: Stefan Rueger Date: Sun, 24 Jul 2022 18:20:35 +0100 Subject: [PATCH 1/5] Initialise memory before avr_set_bits() calls in stk500.c and stk500v2.c --- src/stk500.c | 5 ++--- src/stk500v2.c | 11 +++++++++-- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/stk500.c b/src/stk500.c index cb270036..fa013012 100644 --- a/src/stk500.c +++ b/src/stk500.c @@ -212,7 +212,6 @@ static int stk500_chip_erase(PROGRAMMER * pgm, AVRPART * p) pgm->pgm_led(pgm, ON); memset(cmd, 0, sizeof(cmd)); - avr_set_bits(p->op[AVR_OP_CHIP_ERASE], cmd); pgm->cmd(pgm, cmd, res); usleep(p->chip_erase_delay); @@ -745,8 +744,8 @@ static int stk500_loadaddr(PROGRAMMER * pgm, AVRMEM * mem, unsigned int addr) if (lext != NULL) { ext_byte = (addr >> 16) & 0xff; if (ext_byte != PDATA(pgm)->ext_addr_byte) { - /* Either this is the first addr load, or a 64K word boundary is - * crossed, so set the ext addr byte */ + /* Either this is the first addr load, or a different 64K word section */ + memset(buf, 0, 4); avr_set_bits(lext, buf); avr_set_addr(lext, buf, addr); stk500_cmd(pgm, buf, buf); diff --git a/src/stk500v2.c b/src/stk500v2.c index 523d6539..b0ccaad2 100644 --- a/src/stk500v2.c +++ b/src/stk500v2.c @@ -991,6 +991,7 @@ static int stk500v2_chip_erase(PROGRAMMER * pgm, AVRPART * p) buf[0] = CMD_CHIP_ERASE_ISP; buf[1] = p->chip_erase_delay / 1000; buf[2] = 0; // use delay (?) + memset(buf+3, 0, 4); avr_set_bits(p->op[AVR_OP_CHIP_ERASE], buf+3); result = stk500v2_command(pgm, buf, 7, sizeof(buf)); usleep(p->chip_erase_delay); @@ -1121,8 +1122,8 @@ retry: buf[5] = p->bytedelay; buf[6] = p->pollvalue; buf[7] = p->pollindex; + memset(buf+8, 0, 4); avr_set_bits(p->op[AVR_OP_PGM_ENABLE], buf+8); - buf[10] = buf[11] = 0; rv = stk500v2_command(pgm, buf, 12, sizeof(buf)); @@ -1957,12 +1958,12 @@ static int stk500isp_read_byte(PROGRAMMER * pgm, AVRPART * p, AVRMEM * mem, buf[0] = CMD_READ_SIGNATURE_ISP; } - memset(buf + 1, 0, 5); if ((op = mem->op[AVR_OP_READ]) == NULL) { avrdude_message(MSG_INFO, "%s: stk500isp_read_byte(): invalid operation AVR_OP_READ on %s memory\n", progname, mem->desc); return -1; } + memset(buf+2, 0, 4); avr_set_bits(op, buf + 2); if ((pollidx = avr_get_output_index(op)) == -1) { avrdude_message(MSG_INFO, "%s: stk500isp_read_byte(): cannot determine pollidx to read %s memory\n", @@ -2314,6 +2315,7 @@ static int stk500v2_paged_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, progname, p->desc); return -1; } + memset(cmds, 0, sizeof cmds); avr_set_bits(m->op[AVR_OP_LOADPAGE_LO], cmds); commandbuf[5] = cmds[0]; @@ -2322,6 +2324,8 @@ static int stk500v2_paged_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, progname, p->desc); return -1; } + + memset(cmds, 0, sizeof cmds); avr_set_bits(m->op[AVR_OP_WRITEPAGE], cmds); commandbuf[6] = cmds[0]; @@ -2335,6 +2339,7 @@ static int stk500v2_paged_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, progname, p->desc); return -1; } + memset(cmds, 0, sizeof cmds); avr_set_bits(wop, cmds); commandbuf[5] = cmds[0]; commandbuf[6] = 0; @@ -2346,6 +2351,7 @@ static int stk500v2_paged_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, progname, p->desc); return -1; } + memset(cmds, 0, sizeof cmds); avr_set_bits(rop, cmds); commandbuf[7] = cmds[0]; @@ -2549,6 +2555,7 @@ static int stk500v2_paged_load(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, progname, p->desc); return -1; } + memset(cmds, 0, sizeof cmds); avr_set_bits(rop, cmds); commandbuf[3] = cmds[0]; From 7310df030f19d8e11586fc53f02f667031b78c54 Mon Sep 17 00:00:00 2001 From: Stefan Rueger Date: Sun, 24 Jul 2022 18:48:22 +0100 Subject: [PATCH 2/5] Check stk500_recv() actually worked before accepting SYNC byte --- src/stk500.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/stk500.c b/src/stk500.c index fa013012..208fb1a3 100644 --- a/src/stk500.c +++ b/src/stk500.c @@ -91,8 +91,6 @@ int stk500_getsync(PROGRAMMER * pgm) int attempt; int max_sync_attempts; - /* - * get in sync */ buf[0] = Cmnd_STK_GET_SYNC; buf[1] = Sync_CRC_EOP; @@ -119,11 +117,11 @@ int stk500_getsync(PROGRAMMER * pgm) usleep(50*1000); stk500_drain(pgm, 0); } + stk500_send(pgm, buf, 2); - stk500_recv(pgm, resp, 1); - if (resp[0] == Resp_STK_INSYNC){ + if(stk500_recv(pgm, resp, 1) >= 0 && resp[0] == Resp_STK_INSYNC) break; - } + avrdude_message(MSG_INFO, "%s: stk500_getsync() attempt %d of %d: not in sync: resp=0x%02x\n", progname, attempt + 1, max_sync_attempts, resp[0]); } From 535004ee3da1d73858e71fb48ac8867964425264 Mon Sep 17 00:00:00 2001 From: Stefan Rueger Date: Sun, 24 Jul 2022 19:27:07 +0100 Subject: [PATCH 3/5] Consolidate error messages for stk500.c --- src/stk500.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/stk500.c b/src/stk500.c index 208fb1a3..b3ed3980 100644 --- a/src/stk500.c +++ b/src/stk500.c @@ -119,6 +119,7 @@ int stk500_getsync(PROGRAMMER * pgm) } stk500_send(pgm, buf, 2); + resp[0] = 0; if(stk500_recv(pgm, resp, 1) >= 0 && resp[0] == Resp_STK_INSYNC) break; @@ -202,8 +203,8 @@ static int stk500_chip_erase(PROGRAMMER * pgm, AVRPART * p) } if (p->op[AVR_OP_CHIP_ERASE] == NULL) { - avrdude_message(MSG_INFO, "chip erase instruction not defined for part \"%s\"\n", - p->desc); + avrdude_message(MSG_INFO, "%s: chip erase instruction not defined for part \"%s\"\n", + progname, p->desc); return -1; } @@ -779,13 +780,12 @@ static int stk500_loadaddr(PROGRAMMER * pgm, AVRMEM * mem, unsigned int addr) if (stk500_recv(pgm, buf, 1) < 0) return -1; - if (buf[0] == Resp_STK_OK) { + if (buf[0] == Resp_STK_OK) return 0; - } - avrdude_message(MSG_INFO, "%s: loadaddr(): (b) protocol error, " + avrdude_message(MSG_INFO, "%s: stk500_loadaddr(): (b) protocol error, " "expect=0x%02x, resp=0x%02x\n", - progname, Resp_STK_INSYNC, buf[0]); + progname, Resp_STK_OK, buf[0]); return -1; } @@ -876,9 +876,9 @@ static int stk500_paged_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, if (stk500_recv(pgm, buf, 1) < 0) return -1; if (buf[0] != Resp_STK_OK) { - avrdude_message(MSG_INFO, "\n%s: stk500_paged_write(): (a) protocol error, " + avrdude_message(MSG_INFO, "\n%s: stk500_paged_write(): (b) protocol error, " "expect=0x%02x, resp=0x%02x\n", - progname, Resp_STK_INSYNC, buf[0]); + progname, Resp_STK_OK, buf[0]); return -5; } } @@ -970,7 +970,7 @@ static int stk500_paged_load(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, } else { if (buf[0] != Resp_STK_OK) { - avrdude_message(MSG_INFO, "\n%s: stk500_paged_load(): (a) protocol error, " + avrdude_message(MSG_INFO, "\n%s: stk500_paged_load(): (b) protocol error, " "expect=0x%02x, resp=0x%02x\n", progname, Resp_STK_OK, buf[0]); return -5; @@ -1152,7 +1152,7 @@ static int stk500_getparm(PROGRAMMER * pgm, unsigned parm, unsigned * value) return -3; } else if (buf[0] != Resp_STK_OK) { - avrdude_message(MSG_INFO, "\n%s: stk500_getparm(): (a) protocol error, " + avrdude_message(MSG_INFO, "\n%s: stk500_getparm(): (b) protocol error, " "expect=0x%02x, resp=0x%02x\n", progname, Resp_STK_OK, buf[0]); return -3; @@ -1211,9 +1211,9 @@ static int stk500_setparm(PROGRAMMER * pgm, unsigned parm, unsigned value) return -3; } else { - avrdude_message(MSG_INFO, "\n%s: stk500_setparm(): (a) protocol error, " + avrdude_message(MSG_INFO, "\n%s: stk500_setparm(): (b) protocol error, " "expect=0x%02x, resp=0x%02x\n", - progname, Resp_STK_INSYNC, buf[0]); + progname, Resp_STK_OK, buf[0]); return -3; } } From 29c6645abc92441f204f04e6a220e628fed9aab6 Mon Sep 17 00:00:00 2001 From: Stefan Rueger Date: Sun, 24 Jul 2022 19:41:42 +0100 Subject: [PATCH 4/5] Resolve signed/unsigned comparisons in stk500.c and stk500v2.c --- src/stk500.c | 3 ++- src/stk500v2.c | 33 ++++++++++++++++----------------- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/stk500.c b/src/stk500.c index b3ed3980..084bbec8 100644 --- a/src/stk500.c +++ b/src/stk500.c @@ -1034,7 +1034,8 @@ static int stk500_set_fosc(PROGRAMMER * pgm, double v) static unsigned ps[] = { 1, 8, 32, 64, 128, 256, 1024 }; - int idx, rc; + size_t idx; + int rc; prescale = cmatch = 0; if (v > 0.0) { diff --git a/src/stk500v2.c b/src/stk500v2.c index b0ccaad2..a50fda4c 100644 --- a/src/stk500v2.c +++ b/src/stk500v2.c @@ -392,9 +392,7 @@ static int stk500v2_send_mk2(PROGRAMMER * pgm, unsigned char * data, size_t len) static unsigned short get_jtagisp_return_size(unsigned char cmd) { - int i; - - for (i = 0; i < sizeof jtagispcmds / sizeof jtagispcmds[0]; i++) + for (size_t i = 0; i < sizeof jtagispcmds / sizeof jtagispcmds[0]; i++) if (jtagispcmds[i].cmd == cmd) return jtagispcmds[i].size; @@ -481,7 +479,6 @@ static int stk500v2_jtag3_send(PROGRAMMER * pgm, unsigned char * data, size_t le static int stk500v2_send(PROGRAMMER * pgm, unsigned char * data, size_t len) { unsigned char buf[275 + 6]; // max MESSAGE_BODY of 275 bytes, 6 bytes overhead - int i; if (PDATA(pgm)->pgmtype == PGMTYPE_AVRISP_MKII || PDATA(pgm)->pgmtype == PGMTYPE_STK600) @@ -500,12 +497,13 @@ static int stk500v2_send(PROGRAMMER * pgm, unsigned char * data, size_t len) // calculate the XOR checksum buf[5+len] = 0; - for (i=0;i<5+len;i++) + for (size_t i=0; i<5+len; i++) buf[5+len] ^= buf[i]; DEBUG("STK500V2: stk500v2_send("); - for (i=0;ifd, buf, len+6) != 0) { avrdude_message(MSG_INFO, "%s: stk500_send(): failed to send command to serial port\n",progname); @@ -551,9 +549,9 @@ static int stk500v2_jtagmkII_recv(PROGRAMMER * pgm, unsigned char *msg, progname); return -1; } - if (rv - 1 > maxsize) { + if ((size_t) rv - 1 > maxsize) { avrdude_message(MSG_INFO, "%s: stk500v2_jtagmkII_recv(): got %u bytes, have only room for %u bytes\n", - progname, (unsigned)rv - 1, (unsigned)maxsize); + progname, (unsigned) rv - 1, (unsigned) maxsize); rv = maxsize; } switch (jtagmsg[0]) { @@ -597,9 +595,9 @@ static int stk500v2_jtag3_recv(PROGRAMMER * pgm, unsigned char *msg, implementation of JTAGICE3, as they always request a full 512 octets from the ICE. Thus, only complain at high verbose levels. */ - if (rv - 1 > maxsize) { + if ((size_t) rv - 1 > maxsize) { avrdude_message(MSG_DEBUG, "%s: stk500v2_jtag3_recv(): got %u bytes, have only room for %u bytes\n", - progname, (unsigned)rv - 1, (unsigned)maxsize); + progname, (unsigned) rv - 1, (unsigned) maxsize); rv = maxsize; } if (jtagmsg[0] != SCOPE_AVR_ISP) { @@ -814,13 +812,13 @@ retry: static int stk500v2_command(PROGRAMMER * pgm, unsigned char * buf, size_t len, size_t maxlen) { - int i; int tries = 0; int status; DEBUG("STK500V2: stk500v2_command("); - for (i=0;i 0.0) { @@ -2781,7 +2780,7 @@ static int stk500v2_set_fosc(PROGRAMMER * pgm, double v) fosc = (unsigned)v; for (idx = 0; idx < sizeof(ps) / sizeof(ps[0]); idx++) { - if (fosc >= STK500V2_XTAL / (256 * ps[idx] * 2)) { + if ((unsigned) fosc >= STK500V2_XTAL / (256 * ps[idx] * 2)) { /* this prescaler value can handle our frequency */ prescale = idx + 1; cmatch = (unsigned)(STK500V2_XTAL / (2 * fosc * ps[idx])) - 1; @@ -2828,7 +2827,7 @@ static double avrispmkIIfreqs[] = { static int stk500v2_set_sck_period_mk2(PROGRAMMER * pgm, double v) { - int i; + size_t i; for (i = 0; i < sizeof(avrispmkIIfreqs) / sizeof(avrispmkIIfreqs[0]); i++) { if (1 / avrispmkIIfreqs[i] >= v) From 3d06457a1614a2247efc846b8e73788cb8585978 Mon Sep 17 00:00:00 2001 From: Stefan Rueger Date: Sun, 24 Jul 2022 20:18:15 +0100 Subject: [PATCH 5/5] Deprecate original STK500 v1 protocol in favour of optiboot and Arduino as ISP For paged read/write early AVRDUDE implementations of the STK500 v1 protocol communicated a word address (below a_div=2) or byte address (a_div=1) based on the following code irrespective of which memories were used: if(m->op[AVR_OP_LOADPAGE_LO] || m->op[AVR_OP_READ_LO]) a_div = 2; else a_div = 1; This turned out to be a bug: it really should have been a_div=2 for flash and a_div=1 for eeprom. At the time presumably no one noted because Atmel was at the cusp of replacing their FW 1.x with FW 2 (and the STK500 v2 protocol). It seems that the world (optiboot, Arduino as ISP, ...) has compensated for the bug by assuming AVRDUDE sends *all* eeprom addresses as word addresses. Actually these programmers overcompensated for the bug because for six out of the 146 known SPI programmable parts with eeprom and page size > 1, AVRDUDE would still send the eeprom addresses as byte addresses (ATmega8 ATmega8A ATmega64 ATmega64A ATmega128 ATmega128A) owing to above code. It makes no sense to correct the bug now seeing that virtually no one uses the old 2005 STK 500 v1 firmware. This commit now follows optiboot, Arduino as ISP and other projects, and simply sends all addresses for paged read or write as word addresses. There are no longer (little known) exceptions for ATmega8 et al that surprised some optiboot etc users. --- src/stk500.c | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/stk500.c b/src/stk500.c index 084bbec8..27334297 100644 --- a/src/stk500.c +++ b/src/stk500.c @@ -805,19 +805,20 @@ static int stk500_paged_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, if (strcmp(m->desc, "flash") == 0) { memtype = 'F'; - } - else if (strcmp(m->desc, "eeprom") == 0) { + a_div = 2; + } else if (strcmp(m->desc, "eeprom") == 0) { memtype = 'E'; - } - else { + /* + * The STK original 500 v1 protocol actually expects a_div = 1, but the + * v1.x FW of the STK500 kit has been superseded by v2 FW in the mid + * 2000s. Since optiboot, arduino as ISP and others assume a_div = 2, + * better use that. See https://github.com/avrdudes/avrdude/issues/967 + */ + a_div = 2; + } else { return -2; } - if ((m->op[AVR_OP_LOADPAGE_LO]) || (m->op[AVR_OP_READ_LO])) - a_div = 2; - else - a_div = 1; - n = addr + n_bytes; #if 0 avrdude_message(MSG_INFO, "n_bytes = %d\n" @@ -825,7 +826,7 @@ static int stk500_paged_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, "a_div = %d\n" "page_size = %d\n", n_bytes, n, a_div, page_size); -#endif +#endif for (; addr < n; addr += block_size) { // MIB510 uses fixed blocks size of 256 bytes @@ -872,7 +873,7 @@ static int stk500_paged_write(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, progname, Resp_STK_INSYNC, buf[0]); return -4; } - + if (stk500_recv(pgm, buf, 1) < 0) return -1; if (buf[0] != Resp_STK_OK) { @@ -899,19 +900,20 @@ static int stk500_paged_load(PROGRAMMER * pgm, AVRPART * p, AVRMEM * m, if (strcmp(m->desc, "flash") == 0) { memtype = 'F'; - } - else if (strcmp(m->desc, "eeprom") == 0) { + a_div = 2; + } else if (strcmp(m->desc, "eeprom") == 0) { memtype = 'E'; - } - else { + /* + * The STK original 500 v1 protocol actually expects a_div = 1, but the + * v1.x FW of the STK500 kit has been superseded by v2 FW in the mid + * 2000s. Since optiboot, arduino as ISP and others assume a_div = 2, + * better use that. See https://github.com/avrdudes/avrdude/issues/967 + */ + a_div = 2; + } else { return -2; } - if ((m->op[AVR_OP_LOADPAGE_LO]) || (m->op[AVR_OP_READ_LO])) - a_div = 2; - else - a_div = 1; - n = addr + n_bytes; for (; addr < n; addr += block_size) { // MIB510 uses fixed blocks size of 256 bytes