ArduinoCore-avr icon indicating copy to clipboard operation
ArduinoCore-avr copied to clipboard

Fix stk500v2 READ_EEPROM to properly read EEPROM

Open mlasevich opened this issue 7 years ago • 6 comments

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 https://github.com/arduino/Arduino/pull/6456 before the code was split off of that repo. Re-submitting so that it goes against right repo)

mlasevich avatar Jul 27 '18 19:07 mlasevich

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.

facchinm avatar Nov 26 '18 08:11 facchinm

@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 :-)

mlasevich avatar Nov 30 '18 02:11 mlasevich

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Apr 09 '21 16:04 CLAassistant

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

  • https://github.com/avrdudes/avrdude/issues/515

mcuee avatar Jun 03 '22 14:06 mcuee

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

  • https://github.com/avrdudes/avrdude/issues/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 avatar Jun 04 '22 02:06 mcuee

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

Ref: run log and hex file here.

  • https://github.com/avrdudes/avrdude/issues/986#issuecomment-1146741271

mcuee avatar Jun 05 '22 04:06 mcuee