Merge pull request #2085 from stefanrueger/write_byte

Update single byte writes, ie, skip write if data already there
2026-06-02 09:46:34 +03:00 · 2026-04-05 18:53:28 +01:00
parent 051c1e5770 f062636a25
commit 8a527c9bfe
6 changed files with 152 additions and 40 deletions
--- a/src/avr.c
+++ b/src/avr.c
@@ -641,6 +641,46 @@ int avr_bitmask_data(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
  return data;
 }

+// Can pgm->write_byte() be skipped, eg, if the wanted data are already there?
+int avr_can_skip_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
+  unsigned long addr, uint8_t wanted, int *readrc) {
+
+  if(readrc)
+    *readrc = -99;
+
+  /*
+   * Check to see if the write is necessary by reading the existing value and
+   * only write if we are changing the value; we can't use this optimization
+   * for paged addressing nor for IO memory (as it's not necessarily storage).
+   *
+   * For mysterious reasons, on the AT90S1200, this read operation sometimes
+   * causes the high byte of the same word to be programmed to the value of
+   * the low byte that has just been programmed before. Avoid that
+   * optimization on this device.
+   */
+  if(mem->paged || mem_is_io(mem) || (p->flags & AVRPART_IS_AT90S1200))
+    return 0;
+
+  // Don't skip write when programming a bootloader
+  if(is_spm(pgm))
+    return 0;
+
+  // Unclear whether this optimisation would work for TPI parts
+  if(is_tpi(p))
+    return 0;
+
+  unsigned char is;
+  int rc = avr_read_byte_silent(pgm, p, mem, addr, &is);
+  if(readrc)
+    *readrc = rc;
+  if(rc < 0)
+    return 0;
+
+  int bm = avr_mem_bitmask(p, mem, addr);
+
+  return (is & bm) == (wanted & bm);
+}
+
 int avr_write_byte_default(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
  unsigned long addr, unsigned char data) {

@@ -653,7 +693,6 @@ int avr_write_byte_default(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM
  int ready;
  int tries;
  unsigned long start, now;
-  unsigned char b;
  unsigned short caddr;
  OPCODE *writeop;
  int rc;
@@ -730,29 +769,19 @@ int avr_write_byte_default(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM
    goto success;
  }

-  int bm = avr_mem_bitmask(p, mem, addr);
-
-  if(!mem->paged && (p->flags & AVRPART_IS_AT90S1200) == 0) {
-    /*
-     * Check to see if the write is necessary by reading the existing value and
-     * only write if we are changing the value; we can't use this optimization
-     * for paged addressing.
-     *
-     * For mysterious reasons, on the AT90S1200, this read operation sometimes
-     * causes the high byte of the same word to be programmed to the value of
-     * the low byte that has just been programmed before. Avoid that
-     * optimization on this device.
-     */
-    rc = pgm->read_byte(pgm, p, mem, addr, &b);
+  // Check to see if writing could be skipped
+  int rbrc, skip = avr_can_skip_write_byte(pgm, p, mem, addr, data, &rbrc);
+  if(rbrc != -99) {              // read_byte() was executed
+    rc = rbrc;
    if(rc != 0) {
      if(rc != -1) {
        rc = -2;
        goto rcerror;
      }
-      // Read operation is not support on this memory
+      // Read operation is not supported on this memory
    } else {
      readok = 1;
-      if((b & bm) == (data & bm))
+      if(skip)
        goto success;
    }
  }
@@ -796,6 +825,7 @@ int avr_write_byte_default(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM
    goto success;
  }

+  int bm = avr_mem_bitmask(p, mem, addr);
  tries = 0;
  ready = 0;
  while(!ready) {
@@ -882,6 +912,31 @@ rcerror:
  return rc;
 }

+// Update one byte of data at the specified address, ie, write unless already there
+int avr_update_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
+  unsigned long addr, unsigned char data) {
+
+  if(pgm->write_byte == avr_write_byte_default) // avr_write_byte_default() already updates
+    return avr_write_byte(pgm, p, mem, addr, data);
+
+  pmsg_debug("%s(%s, %s, %s, %s, 0x%02x)\n", __func__, pgmid, p->id, mem->desc,
+    str_ccaddress(addr, mem->size), data);
+
+  if(avr_can_skip_write_byte(pgm, p, mem, addr, data, NULL))
+    return 0;
+
+  if(mem_is_readonly(mem) || (pgm->readonly && pgm->readonly(pgm, p, mem, addr))) {
+    pmsg_error("cannot write to %s memory %s of %s\n",
+      mem_is_readonly(mem)? "read-only": "write-protected", mem->desc, p->desc);
+    return -1;
+  }
+
+  if(!(p->prog_modes & (PM_UPDI | PM_aWire))) // Initialise unused bits in classic & XMEGA parts
+    data = avr_bitmask_data(pgm, p, mem, addr, data);
+
+  return pgm->write_byte(pgm, p, mem, addr, data);
+}
+
 // Write a byte of data at the specified address
 int avr_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
  unsigned long addr, unsigned char data) {
@@ -889,13 +944,12 @@ int avr_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
  pmsg_debug("%s(%s, %s, %s, %s, 0x%02x)\n", __func__, pgmid, p->id, mem->desc,
    str_ccaddress(addr, mem->size), data);

-  if(mem_is_readonly(mem)) {
-    unsigned char is;
-
-    if(pgm->read_byte(pgm, p, mem, addr, &is) >= 0 && is == data)
+  if(mem_is_readonly(mem) || (pgm->readonly && pgm->readonly(pgm, p, mem, addr))) {
+    if(avr_can_skip_write_byte(pgm, p, mem, addr, data, NULL))
      return 0;

-    pmsg_error("cannot write to read-only memory %s of %s\n", mem->desc, p->desc);
+    pmsg_error("cannot write to %s memory %s of %s\n",
+      mem_is_readonly(mem)? "read-only": "write-protected", mem->desc, p->desc);
    return -1;
  }

@@ -1182,7 +1236,7 @@ int avr_write_mem(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m, int
      continue;

    if(do_write) {
-      if(avr_write_byte(pgm, p, m, i, data)) {
+      if(avr_update_byte(pgm, p, m, i, data)) {
        msg_error(" *** failed\n");
        led_set(pgm, LED_ERR);
        goto error;
@@ -1254,7 +1308,7 @@ int avr_mem_bitmask(const AVRPART *p, const AVRMEM *mem, int addr) {
  return bitmask;
 }

-// Bitmask for ISP programming (classic parts only)
+// Bitmask for verification after ISP programming (classic parts only)
 static uint8_t get_fuse_bitmask(const AVRMEM *m) {
  int ret = 0xFF;

--- a/src/avrcache.c
+++ b/src/avrcache.c
@@ -76,7 +76,7 @@
 * avr_chip_erase_cached() erases the chip and discards pending writes() to
 * flash or EEPROM. It presets the flash cache to all 0xff alleviating the
 * need to read from the device flash. However, if the programmer serves
- * bootloaders is_spm(pgm) then the flash cache is reset
+ * bootloaders, recognised by is_spm(pgm), then the flash cache is reset
 * instead, necessitating flash memory be fetched from the device on first
 * read; the reason for this is that bootloaders emulate chip erase and they
 * won't overwrite themselves (some bootloaders, eg, optiboot ignore chip
@@ -142,8 +142,20 @@ int avr_has_paged_load(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *me
    mem->size > 0 && mem->size%mem->page_size == 0 && mem_is_paged_type(mem) && !(p && avr_mem_exclude(pgm, p, mem));
 }

-#define fallback_read_byte (pgm->read_byte != avr_read_byte_cached? led_read_byte: avr_read_byte_default)
-#define fallback_write_byte (pgm->write_byte != avr_write_byte_cached? led_write_byte: avr_write_byte_default)
+static int read_byte_error(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem, unsigned long addr, uint8_t *value) {
+  pmsg_error("pgm->read_byte must not be pointing to avr_read_byte_cached()\n");
+  return -1;
+}
+
+static int write_byte_error(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem, unsigned long addr, uint8_t data) {
+  pmsg_error("pgm->write_byte must not be pointing to avr_write_byte_cached()\n");
+  return -1;
+}
+
+// Sanity only: guard against a user setting pgm->xyz_byte to avr_xyz_read_byte_cached (AVRDUDE doesn't)
+#define fallback_read_byte (pgm->read_byte != avr_read_byte_cached? led_read_byte: read_byte_error)
+#define fallback_write_byte (pgm->write_byte != avr_write_byte_cached? led_write_byte: write_byte_error)
+#define fallback_update_byte (pgm->write_byte != avr_write_byte_cached? led_update_byte: write_byte_error)

 /*
 * Read the page containing addr from the device into buf
@@ -670,7 +682,7 @@ int avr_write_byte_cached(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM

  // Use pgm->write_byte() if not flash/EEPROM/bootrow/usersig or no paged access
  if(!avr_has_paged_access(pgm, p, mem))
-    return fallback_write_byte(pgm, p, mem, addr, data);
+    return fallback_update_byte(pgm, p, mem, addr, data);

  // If address is out of range synchronise caches with device and return whether successful
  if(addr >= (unsigned long) mem->size)
--- a/src/leds.c
+++ b/src/leds.c
@@ -196,11 +196,53 @@ int led_chip_erase(const PROGRAMMER *pgm, const AVRPART *p) {
  return rc;
 }

+// Programmer specific update byte function with ERR/PGM LED info (ie, only write if data not there)
+int led_update_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m, unsigned long addr, unsigned char value) {
+  if(pgm->write_byte == avr_write_byte_default) // avr_write_byte_default() already updates
+    return led_write_byte(pgm, p, m, addr, value);
+
+  pmsg_debug("%s(%s, %s, %s, %s, 0x%02x)\n", __func__, pgmid, p->id, m->desc, str_ccaddress(addr, m->size), value);
+
+  if(avr_can_skip_write_byte(pgm, p, m, addr, value, NULL))
+    return 0;
+
+  if(mem_is_readonly(m) || (pgm->readonly && pgm->readonly(pgm, p, m, addr))) {
+    pmsg_error("cannot write to %s memory %s of %s\n",
+      mem_is_readonly(m)? "read-only": "write-protected", m->desc, p->desc);
+    return -1;
+  }
+
+  if(!(p->prog_modes & (PM_UPDI | PM_aWire))) // Initialise unused bits in classic & XMEGA parts
+    value = avr_bitmask_data(pgm, p, m, addr, value);
+
+  led_clr(pgm, LED_ERR);
+  led_set(pgm, LED_PGM);
+
+  int rc = pgm->write_byte(pgm, p, m, addr, value);
+
+  if(rc < 0)
+    led_set(pgm, LED_ERR);
+  led_clr(pgm, LED_PGM);
+
+  return rc;
+}
+
 // Programmer specific write byte function with ERR/PGM LED info
 int led_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m, unsigned long addr, unsigned char value) {
+  pmsg_debug("%s(%s, %s, %s, %s, 0x%02x)\n", __func__, pgmid, p->id, m->desc, str_ccaddress(addr, m->size), value);

-  if(mem_is_readonly(m))
-    return pgm->write_byte(pgm, p, m, addr, value);
+  if(mem_is_readonly(m) || (pgm->readonly && pgm->readonly(pgm, p, m, addr))) {
+    if(avr_can_skip_write_byte(pgm, p, m, addr, value, NULL))
+      return 0;
+
+    pmsg_error("cannot write to %s memory %s of %s\n",
+      mem_is_readonly(m)? "read-only": "write-protected", m->desc, p->desc);
+    return -1;
+  }
+
+  if(pgm->write_byte != avr_write_byte_default)
+    if(!(p->prog_modes & (PM_UPDI | PM_aWire))) // Initialise unused bits in classic & XMEGA parts
+      value = avr_bitmask_data(pgm, p, m, addr, value);

  led_clr(pgm, LED_ERR);
  led_set(pgm, LED_PGM);
@@ -215,13 +257,11 @@ int led_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m, uns
 }

 // Programmer specific read byte function with ERR/PGM LED info
-int led_read_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m,
-  unsigned long addr, unsigned char *valuep) {
-
+int led_read_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m, unsigned long addr, unsigned char *valp) {
  led_clr(pgm, LED_ERR);
  led_set(pgm, LED_PGM);

-  int rc = pgm->read_byte(pgm, p, m, addr, valuep);
+  int rc = pgm->read_byte(pgm, p, m, addr, valp);

  if(rc < 0)
    led_set(pgm, LED_ERR);
--- a/src/libavrdude.h
+++ b/src/libavrdude.h
@@ -1051,7 +1051,7 @@ typedef struct programmer {
  int (*write_byte)(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m,
    unsigned long addr, unsigned char value);
  int (*read_byte)(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m,
-    unsigned long addr, unsigned char *value);
+    unsigned long addr, unsigned char *valp);
  int (*read_sig_bytes)(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m);
  int (*read_sib)(const PROGRAMMER *pgm, const AVRPART *p, char *sib);
  int (*read_chip_rev)(const PROGRAMMER *pgm, const AVRPART *p, unsigned char *chip_rev);
@@ -1168,14 +1168,18 @@ extern "C" {
  uint64_t avr_mstimestamp(void);
  double avr_timestamp(void);
  void init_cx(PROGRAMMER *pgm);
-  int avr_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
-    unsigned long addr, unsigned char data);
  int avr_read_byte_silent(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
    unsigned long addr, unsigned char *datap);
  int avr_bitmask_data(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
    unsigned long addr, unsigned char data);
+  int avr_can_skip_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
+    unsigned long addr, uint8_t wanted, int *readrc);
  int avr_write_byte_default(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
    unsigned long addr, unsigned char data);
+  int avr_update_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
+    unsigned long addr, unsigned char data);
+  int avr_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem,
+    unsigned long addr, unsigned char data);
  int avr_write_mem(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *mem, int size, int auto_erase);
  int avr_write(const PROGRAMMER *pgm, const AVRPART *p, const char *memstr, int size, int auto_erase);
  int avr_signature(const PROGRAMMER *pgm, const AVRPART *p);
@@ -1743,10 +1747,12 @@ extern "C" {
  int led_set(const PROGRAMMER *pgm, int led);
  int led_clr(const PROGRAMMER *pgm, int led);
  int led_chip_erase(const PROGRAMMER *pgm, const AVRPART *p);
+  int led_update_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m,
+    unsigned long addr, unsigned char value);
  int led_write_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m,
    unsigned long addr, unsigned char value);
  int led_read_byte(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m,
-    unsigned long addr, unsigned char *value);
+    unsigned long addr, unsigned char *valp);
  int led_paged_write(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m,
    unsigned int page_size, unsigned int addr, unsigned int n);
  int led_paged_load(const PROGRAMMER *pgm, const AVRPART *p, const AVRMEM *m,
--- a/src/update.c
+++ b/src/update.c
@@ -529,7 +529,7 @@ static int update_avr_write(const PROGRAMMER *pgm, const AVRPART *p, const AVRME

  if(rc < 0)
    return -1;
-  // @@@ has there has been output in the meantime to make the ", x bytes written" look out of place?
+  // @@@ has there been output in the meantime to make the ", x bytes written" look out of place?
  if(pbar && !(flags & UF_VERIFY))
    pmsg_info("%d byte%s of %s written", fs.nbytes, str_plural(fs.nbytes), m_name);
  else if(!pbar)
--- a/tools/test8
+++ b/tools/test8
@@ -14,7 +14,7 @@ Usage() {
 cat <<END
 Syntax: $progname [<opts>] <programmer> <part>
 Function: test AVRDUDE v 8.0 or later with -c programmer -p part;
-  also leaves a file bak-<programmer>-<part.hex for inspection
+  also leaves a file bak-<programmer>-<part>.hex for inspection
 Options:
  -i <n>     Write human-readable contents seeded by <n> to part and exit
  -r <n>     Write random contents seeded by <n> to part and exit