sim- / tgy

tgy -- Open Source Firmware for ATmega-based Brushless ESCs
http://0x.ca/tgy/
686 stars 388 forks source link

bug in bootloader boot_clear_flash #72

Closed 4712 closed 9 years ago

4712 commented 9 years ago

If using CMD_CHIP_ERASE_ISP the flash memory will only be erased from 0x0000 to 0x0E00 word-address.

Please have a look at https://github.com/sim-/tgy/pull/71

Fix: multiplied BOOTSTART and PAGESIZE by 2 to get byte-address ;.db "AVRISP2" ; stk500v2 signature (changed '' to '' to get a chance to identify - AVRdude does not care)

sim- commented 9 years ago

Hello! Thanks for this! ...I'm a bit dazed at the moment, and something seems to be eating the asterisks in your comment, but I believe I originally followed http://www.atmel.com/images/doc2591.pdf when implementing this, and it seems to list AVRISP_2 there. What did you find that expects AVRISP*2?

The other change looks right other than the nop gapping. I wonder why it doesn't end up erasing itself...I suppose it probably does erase the page containing boot_spm, then can't do any more damage? Good times.. :)

4712 commented 9 years ago

Hello Simon. Yes the asterisk is eaten up ;) I know, AVRISP_2 or STK500 is the right CMD_SIGN_ON answer, but neither AVRDude nor any other software I know cares about the answer (AVR Studio would not work with USBLinker anyway, I guess). The purpose would be to identify the state of the bootloader in BLHeliSuite (or other) after CMD_SIGN_ON. One could suggest an update to the user, or simply customize the erase method.

The nop gapping is correct, as the last page is not changed, but the code in front increased because of the split of sbiw ZL, PAGESIZE into subi ZL, low(PAGESIZE<<1) sbci ZH, high(PAGESIZE<<1)

Size compiles here to 1024 bytes. I tested the self updating successfully... Have a nice day! Achim

sim- commented 9 years ago

Hi Achim,

Do you mean that you want to identify that the chip erase is now fixed, or you just want it to identify at all in some way?

By "nop gapping" I meant purely the \n\n in the source. No worries.

4712 commented 9 years ago

Hi Simon, yes, I would like to check, if the chip erase is fixed or if I need to do a manual erase by overriding with 0xFF. Beside of this a version numbering would be useful anyway... must not be an asterisk sign ;) Uupps, haven't seen the \n\n in the code...

4712 commented 9 years ago

or use PARAM_BUILD_NUMBER (low and high) vor version numbering ?

sim- commented 9 years ago

Hello! Sorry for delay, back from a family emergency. Yeah, I'd much rather use a parameter than to just randomly fake the identification string. I can't even remember now where I got the values from that it is currently returning for those parameters, but they are certainly easy to change. Maybe i was trying to emulate something? I'll dig around after work.

It's also probably possible to just check if chip erase worked or not (by seeing if a page it should have clobbered is still there), but I was going to instrument this to confirm.

4712 commented 9 years ago

Hello Simon, no problem! I like to change the identification string much much more, because it is very easy to identify the fixed revision by the start call. Otherwise I would have to integrate another call sequence for the GetParameter command. On the PC not a problem, but in the external serial interfaces it costs bytes ;) ...

sim- commented 9 years ago

Hi Achim,

I've pushed the base fix fow this now (basically your patch), but as for the header change, I'd really rather not do it that way. I think I tried AVR Studio, but I can't remember now if it worked.

Anyway, let me better understand your application. In reality, chip erase isn't actually needed, since pages will be erased as they are reprogrammed. I just checked this with

avrdude -c stk500v2 -b 9600 -P /dev/ttyUSB0 -u -p m8 -U flash:w:fat.hex:i avrdude -c stk500v2 -b 9600 -P /dev/ttyUSB0 -u -p m8 -U flash:r:1.hex:i avrdude -c stk500v2 -b 9600 -P /dev/ttyUSB0 -u -p m8 -D -U flash:w:afro_nfet.hex:i avrdude -c stk500v2 -b 9600 -P /dev/ttyUSB0 -u -p m8 -U flash:r:2.hex:i

...where fat.hex is entirely nops except for the boot loader (copied to http://0x.ca/sim/tmp/fat.hex for reference). The second write and verification and subsequent read to 2.hex shows that afro_nfet.hex was flashed successfully, even with chip erase disable (-D).

What do you think?

sim- commented 9 years ago

ping :)

sim- commented 9 years ago

I'll close this for now. Let me know if you need anything else to help.

4712 commented 9 years ago

Hi Simon, was problem was a different one. Many people told, that flashing failed on the first try and always succeeded on the second try. I could reproduced it, it failed after tgy was flashed and the throttle calibration was done (EEprom has data). When flashing all pages and after it writing to EEprom (BLHeli needs this), the EEprom write failed. Not only with BLheliSuite Stk500v2, also using AVRDude stk500v2 . Using chip erase resolved this issue (Why?... good question... ). Anyway. I cannot assume that chip erase worked in all parts, so I need also to override at least the flash from 0x0E00 to top once more with 0xFF.

All in all, good to have eliminated one bug more...

sim- commented 9 years ago

But to try to be completely clear, avrdude already skips what it thinks is "blank" (by the page) even for verification, so the problem should be totally transparent except when the program is bigger than 4k and the program explicitly reads it back after a chip erase.

Programming anything of any size with or without a chip erase will still work and verify, but the full hex may contain stale data beyond 4k if and there was program code beyond 4k, a chip erase issued, and read the whole flash (unless a program equal or bigger than last time is written after the chip erase).

But the only thing this could really break is a nop-sled to the boot loader during an interrupted flash operation.