Fix avrdude 6.3 fuses inconsistency

[facchinm]

New avrdude build with applied patch https://savannah.nongnu.org/patch/index.php?8996 (not yet upstreamed).
With this patch applied, all inconsistencies between unused fuse bit / programmers etc should be gone.

THIS PATCH IS NOT BACKWARDS COMPATIBLE WITH AVRDUDE 6.0.1
This shouldn't be a problem because we merged #5199

Contains and supersedes #5182

Please test with any programmer you may have in house (personally tested with Arduino as ISP (stk500v1) and AVRISP mkII (stk500v2) on all Arduino boards MCUs).

3rd party core could need to adjust their fuse setting if this patch gets merged

The new revision uses a completely different approach, patching avrdude so it ignores differences between fuses if the offending bits are marked as reserved.

This approach is backwards-compatible with existing boards.txt and avrdude 6.0.1

Based on arduino/avrdude-build-script@7eab60d

@per1234 @descampsa @mikaelpatel @matthijskooijman @damellis @markaswift @aethaniel

[per1234]

I could certainly be wrong on this but shouldn't this(and all other ATmega32U4 entries) be 0xEF? http://eleccelerator.com/fusecalc/fusecalc.php?chip=atmega32u4&LOW=FF&HIGH=D8&EXTENDED=CB&LOCKBIT=2F shows the original lock bits setting(0x2F) is:

• Application protection mode 1
• Boot Loader protection mode 2
• Mode 1: No memory lock features enabled

http://eleccelerator.com/fusecalc/fusecalc.php?chip=atmega32u4&LOW=FF&HIGH=D8&EXTENDED=CB&LOCKBIT=CF shows 0xCF is:

• Application protection mode 1
• Boot Loader protection mode 3
• Mode 1: No memory lock features enabled

http://eleccelerator.com/fusecalc/fusecalc.php?chip=atmega32u4&LOW=FF&HIGH=D8&EXTENDED=CB&LOCKBIT=EF shows 0xEF is the same as the original setting:

• Application protection mode 1
• Boot Loader protection mode 2
• Mode 1: No memory lock features enabled

EDIT: formatting

[facchinm]

Thanks for pointing this out.
I believed that protecting bootloader with mode3 (like all the other boards) shouldn't have any side effect but I was wrong, reading the bootloader section from application space is used here https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/cores/arduino/USBCore.cpp#L819
Reverting these fuses to 0xEF in next commit

[facchinm]

@per1234 , the problems you exposed and the fact that merging this one would be a nightmare for 3rd party cores (unless we implement the coreDependency field on the json) makes me think that the problem must be solved on avrdude side. Working on this today, I'll keep you posted.

[facchinm]

@per1234 , could you test again using the new @ArduinoBot build? Thanks!

[ArduinoBot]

✅ Build completed.

Please test this code using one of the following:

⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5202-BUILD-587-linux32.tar.xz
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5202-BUILD-587-linux64.tar.xz
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5202-BUILD-587-windows.zip
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5202-BUILD-587-macosx.zip
⬇️ http://downloads.arduino.cc/javaide/pull_requests/arduino-PR-5202-BUILD-587-linuxarm.tar.xz

ℹ️ The linuxarm build is still experimental and may not be always available.

[descampsa]

I understand that backward compatibility is an issue, but in my opinion, fixing this problem by only ignoring differences in unused bits is not the proper way to fix it. This will encourage users to keep using the previous value, and we will end up with some users using the old way, and some the new way.
This is bad for at least two reasons:

1. It is confusing for the users to have different possible value for the same configuration, to have value that do not match with what the datasheet says, and to write a value and get a different one readed back.
2. Correct me if i am missleaded, but i think that there is a reason why those bits must be writed to one : it is that Atmel may decide to use them in a future revision for a new feature. If this is happen and many users are still using the old value, THAT will be a nightmare.

My suggestion to solve this is the following.:
On the Arduino side, fix all fuse value in boards.txt (extended and lock) , and tell all 3rd party core maintainers you know to do the same.
On the avrdude side,
-apply https://savannah.nongnu.org/patch/index.php?8996 , to fix the lock fuse bug
-apply you fix to accept any value for the unused bits, BUT with a visible warning that this is deprecated behaviour, and that future version won't allow that anymore. The warning can even include the new value to be used. This way, it won't break anything, but (hopefully) encourage the 3rd party cores to adopt the new convention.

In the future, when you are confident that most users have seen the warning and have cared about it, that will become an error.

Of course, all change need to happen simultaneously, and you may have to decide to temporarily revert avrdude to a previous version, until all fixes are ready and tested, but in the long term, imho, it is the best option.

What do you think about it?

[matthijskooijman]

@descampsa, sounds like a sane argument you make, and adding the warning you suggest sounds good to me. This is certainly something to also talk about with avrdude upstream, see what they think about it.

[descampsa]

Hey, here is a version of @facchinm patch that implement the warning, and also take @matthijskooijman comments into account, if you want to test it :
https://github.com/descampsa/avrdude-build-script/blob/fix_unused_fuse_bits/avrdude-6.3-patches/80-Avoid-failing-fuse-check-if-different-bits-are-reserved.patch

[facchinm]

Hi @descampsa ,
I really like your patch, if you don't mind I'd propose it in avrdude devlist.
I'd only change https://github.com/descampsa/avrdude-build-script/blob/fix_unused_fuse_bits/avrdude-6.3-patches/80-Avoid-failing-fuse-check-if-different-bits-are-reserved.patch#L90 that should read m = avr_locate_mem(p, "fuse");

[descampsa]

Sure, go ahead and propose it.
Indeed, that was an error, it is fixed now.

[matthijskooijman]

I added one more nitpick coment to that patch.

Also, one other issue I discussed with @facchinm on Skype, but I now see I didn't mention here as well, is about how get_fuse_bitmask() works. Right now, it only looks at the LSB of the command, and includes any bits in the comparison that are not IGNORE when reading and are not VALUE when writing and are not IGNORE when writing. However, it seems to me that it would be sufficient to just include the bytes that are actually read, without even looking at the write command? Also, I think the mask should be based on all four command bytes, not just the LSB, since that is also how the reading process works. E.g. see the avrdude source (bitno is the bit number within the current byte).

Based on the source above, I would propose:

uint8_t get_fuse_bitmask(AVRMEM * m) {
  uint8_t bitmask = 0;
  int i, j;

  if (!m || m->size > 1) {
    // not a fuse, compare bytes directly
    return 0xFF;
  }

  for (i=0; i<AVR_OP_MAX; i++) {
    if (m->op[i] && i == AVR_OP_READ) {
      for (j=0; i<32; i++) {
        if (m->op->bit[i].type == AVR_CMDBIT_OUTPUT) {
          bitmask |= 1 << (j % 8);
        }
      }
    }
  }

  return bitmask;
}

This includes the nitpick mentioned above. The code is completely untested.

[descampsa]

Hey @matthijskooijman, i have taken a second look at the code after your last comment.

I don't think you can base the bitmask on the read command only, because the origin of the problem is that new avrdude.conf now read the complete byte, including unused bits (so all height bits are OUTPUT). However, i agree that the current logic is probably more complex than necessary. What we could do is maybe include only bits that are INPUT in the write command, which would mean that we check only the bits that we actually write based on the user input?

Also, i may be missing something, or it is just a strange coding convention of avrdude, but the first for loop over i doesn't seem to do anything usefull. Why not accessing m->op[AVR_OP_READ] directly for example?

[matthijskooijman]

I don't think you can base the bitmask on the read command only, because the origin of the problem is that new avrdude.conf now read the complete byte, including unused bits (so all height bits are OUTPUT). However, i agree that the current logic is probably more complex than necessary. What we could do is maybe include only bits that are INPUT in the write command, which would mean that we check only the bits that we actually write based on the user input?

Ah, I missed that part. Your proposal sounds good, then. In any case, getting feedback from upstream would help here, I'm sure that Joerg has a better overview of the code than either of us :-)

Also, i may be missing something, or it is just a strange coding convention of avrdude, but the first for loop over i doesn't seem to do anything usefull. Why not accessing m->op[AVR_OP_READ] directly for example?

Hm, now that you mention it... Perhaps this is a leftover from an older version where the operation type was stored as a string or something like that? The loop is indeed quite pointless as it is now...

[facchinm]

Superseeded by #5374