Fix stk500v2 READ_EEPROM to properly read EEPROM

[mlasevich]

Fix for #23. READ_EEPROM was not reading EEPROM correctly, assuming each EEPROM address points to a 16bit location, but reading only 8 bit, so it would return every other byte.

This fixes it to read every byte correctly.

(This was originally submitted as arduino/Arduino#6456 before the code was split off of that repo. Re-submitting so that it goes against right repo)

[matthijskooijman]

I'm a bit confused with this change, which seems to somehow do the inverse of what you describe at first. Digging deeper, I think the problem here is:

• LOAD_ADDRESS doubles the passed address before putting it into the address variable.
• This doubling makes sense for flash, since that passes word addresses to LOAD_ADDRESS and this doubling makes it a byte address.
• This doubling makes no sense for eeprom, since there you already pass a byte address. Effectively, your patch undoes the doubling, by halving the address before using it.

I wonder if this is the right fix, since it makes things complicated. IMHO it could be better to instead not do the doubling in LOAD_ADDRESS, but only when access flash, so it is only applied when needed.

Also, I'm not sure if this particular bootloader is still being updated. I can imagine that if a new bootloader version is going to be used for the Mega2560 (Which, I think, is the board that uses this bootloader), it will switch to optiboot instead (which has support for the 2560 since a while).

@facchinm, perhaps you can comment about that?

[facchinm]

We normally update bootloaders on the repo only when the boards are being factory flashed with that bootloader.
The validation phase usually requires quite a long time and it's rather unusual to "live patch" a board except for special cases (like Nano's WDT problem).
Moreover, I'd say that we'd probably switch to optiboot for the 2560 if we change the BL in production.

[mlasevich]

@matthijskooijman I am by no means intimately familiar with this bootloader code. I simply came across what seemed like a pretty severe (to me, at least) bug and tried to fix it with minimum code changes to minimize potential impact to other things. I would not be surprised if there is a better way to fix this, and what you describe makes prefect sense, I was just not comfortable enough to be making larger scale changes, since I was not sure what I might break...

@facchinm I understand your point, but it has been a year and a half since I reported this, and still, a bootloader that is not capable of reading EEPROM without corrupting it is shipped on every new 2560 board(though I am not sure where that comes from), as well as is included with Arduino IDE/SDK.

That said, this has been out for years and years, and in all that time I do not think I have seen anyone else mention or be concerned about this problem - so perhaps nobody besides me really wants to read EEPROM via bootloader or cares that this is broken - so maybe this is all moot :-)

[CLAassistant]

All committers have signed the CLA.

[mcuee]

This may be related. It does not seem to be an avrdude issue but rather the issue of the bootloader.

• [bug #61169] Bad EEPROM read on mega2560 R3 Arduino board avrdudes/avrdude#515

[mcuee]

Ref: yes the following is exactly because of the long standing bug in the bootloader.

• [bug #61169] Bad EEPROM read on mega2560 R3 Arduino board avrdudes/avrdude#515

Moreover, I'd say that we'd probably switch to optiboot for the 2560 if we change the BL in production.

This was mentioned on 26-Nov-2018, and now it is June 2022. So it seems this will not happen any time soon.

[mcuee]

@mlasevich @facchinm Yes I can confirm that this pull request fixed the EEPROM read issue.

Ref: run log and hex file here.

• avrdude EEPROM read/write problem with optiboot and stk500v2 bootloader avrdudes/avrdude#986 (comment)