mihaigalos / miniboot

🏗️ An I2C bootloader for Arduino.
GNU General Public License v3.0
65 stars 18 forks source link

Issue with large programs #7

Closed Wellhamster closed 5 years ago

Wellhamster commented 5 years ago

Hi Mihai,

I have an issue with flashing larger sketches of up to 28kB. I did all the testing with sketch sizes of around 2kB and everything works just fine. But with the larger files I had some trouble. I'm sure that the header and the program is stored correctly on the EEPROM. I changed the bootloader that way: while the CRC is checked a green LED is flashing and while writeFlashFromI2C is executed, a red LED is flashing. With toggling every 128byte I would expect ca. 109 on/off cycles with a 28kB program. It appears to me that the program is only partially checked. However, the crc == expected_crc appears the be true because it goes to writeFlashFromI2C afterwards. At the end the LED is turned off and nothing happens afterwards. The application does not start. Any suggestion on what could be the problem?

Thanks a lot! Felix

Fuses: Hi Fuse: 0xD8 Lo Fuse: 0xD2

mihaigalos commented 5 years ago

Hi Felix, I've tested miniboot with ~10kB programs, no problem. Didn't try with large programs yet. I had a look at the code again. Can't find anything buggy at first sight. Can you please count the number of led flashes? If it's too fast, you may need to recompile miniboot and add delays to the LED_TOGGLE() macro.

M

Wellhamster commented 5 years ago

I changed it to flash every 512 bytes. With a program size of 25520 it flashes 25 times.

mihaigalos commented 5 years ago

Humm. That would mean it writes 12800 bytes to the flash and it stops. The value doesn't ring a bell.. So the error must be in the interation count in writeFlashFromI2C. In this function, try to find the value of length, this variable represents the numeber of bytes to write. Ensure that this matches your application length.

There might be a rounding(?) error when putting the file to flash in the external I2C memory. I am suspecting a faulty length there..

Wellhamster commented 5 years ago

ok. Let me try to check the length. But wouldn't you multiply the 12800 by 2 because the LED_TOGGLE represents twice the number of bytes?

mihaigalos commented 5 years ago

no, you said you have 512 bytes per led flash. So you have 512*25=12800 bytes successfully flashed. The last page (each page is defined by SPM_PAGE_SIZE as 128 bytes on <=64kB flash architectures) may be flashed without a led indication, but I don't think this is important.

Wellhamster commented 5 years ago

I changed: uint16_t toggle_led_every_x_bytes = 512; The LED_TOGGLE() switch the LED on or off, depending on the current state of the LED. For every 512 bytes ON there is 512 bytes LED OFF, right?

Edit: I know that it isCrcOk is true because otherwise it wouldn't go into writeFlashFromI2C. Since the getDataLength(i2c_address); is used in both functions it should be the same value. A wrong length and a correct CRC is very unlikely.

mihaigalos commented 5 years ago

arf. Fell into the old trap again. So you need to differentiate between the computing of the crc and reflashing part. You cannot know in which exact one the toggling of the leds happen, since both toggle it!

See line 69 and 94.

Wellhamster commented 5 years ago

I use a green LED at 69 and a red at 94. That way I know :-)

mihaigalos commented 5 years ago

check the computed CRC, you could try to output it via UART. I really suspect that the reflashing doesn't even start because of a CRC mismatch.

mihaigalos commented 5 years ago

ok then. Where is the problem? On the red one, the writeFlashFromI2C? Output the length via UART and check that it's correct.

Fabiancj81 commented 5 years ago

Hi friend   thanks for your answer, since I have the same problem, I have loaded small programs and everything works fine, but when loading a program on 20Kb this records the flash of the arduino but the program is not started. I have reviewed the flash memory map and it is correct (it only has byte number 2 that is redirecting to bootloader). my main program has a size of 22Kb. best regards Fabian

mihaigalos commented 5 years ago

Interesting. What do you mean by "I have reviewed the flash memory map"? Is the information you read from the microcontroller flash the same as the original hex file, apart from the 2 bytes that redirect to the bootloader?

mihaigalos commented 5 years ago

I think the computing of the address of the application was wrong. I have corrected this with c81ad2bbad5477165adb54de06ca3af53f9655a1. Can you please git pull and try again? Many thanks!

Fabiancj81 commented 5 years ago

Hello   I check the hexadecimal from the ICSP with an external programmer, and compare with the same program with the arduino bootloader.   and if, miniboot performs 2 periods of blinking and then i check that it has loaded reading with an external programmer.   I have verified that miniboot works with a simple program like Blink and it works ok.   thank you check the last commit and I will comment again. best regards Fabian

Fabiancj81 commented 5 years ago

Hello again I tried the update and the main program still does not work (but it works with small programs), despite the fact that miniboot records it in flash. Apparently as you commented, the program start address is not indicated correctly

mihaigalos commented 5 years ago

So we know there is a problem in the construction of the address where the program start address is. In io.h I define the jump opcode for jumping anywhere from 0..0xFFFF by setting it to 0x940c, which is 1001 0100 0000 1100 in binary and conforms to the opcode format in the AVR Spec. This opcode gets put at address 0 (also known as the reset vector) by miniboot in writeToFlash(). Address 2 and 3 contain the rest of the opcode argument, the actual address where to jump to the program start! These bytes are also put at address 2 and 3 by writeToFlash().

I unfortunately have no hardware I can debug on anymore. So the closest I can offer is to have a look at your original .hex and the .hex you get by dumping your entire microcontroller flash memory after miniboot had reflashed it. I can then at least narrow the issue down, by looking how bytes 0..3 in these files look like.

mihaigalos commented 5 years ago

I think I have found something. I just pushed 040a35013ec3131d4d320cd61c72f42b19bd8277, which should fix at least part of the problem.

writes was an uint8_t, so 255 maximum values. This got multiplied with SPM_PAGE_SIZE which is 128. This means that the result is recast to uint8_t meaning at most 255. Everything occurs in writeFlashFromI2C, just before writeToFlash() is called. While this is ok for small programs, eventually the uint8_t variable will overflow and that means the argument for address passed to writeToFlash() is 0. This function then thinks it's time to patch the reset vector again, wrongfully.

Can you please retest with the latest commit? If the problem still occurs, I need the two hex files mentioned earlier. Many thanks!

Fabiancj81 commented 5 years ago

Thank you again!   I also think that the problem is there, I was trying to change the size of some variables due to overflow suspicion, but clearly I have not been successful, I also thought it was something related to the watchdog since miniboot deactivates it and my program needs the active watchdog, but neither is that. I loaded another program of 15kbyte and miniboot work correctly, the problem is already presented with a program larger than that size.   On Monday I will try out the new commit and I'll tell you how it goes, since it's already weekend, a greeting and thanks again.

Fabiancj81 commented 5 years ago

Hello again    I have recorded the new miniboot and it still does not run the main program. I have attached the hex files of the main program (named Slave) with the arduino bootloader (which works fine) and the same with miniboot bootloader after it has been burned in flash memory.   I will be attentive to your comments. best regards Fabian Slave_bootloaderArduino.hex.zip Slave_bootloaderMiniboot.hex.zip

mihaigalos commented 5 years ago

I have mocked miniboot for offline testing and pushed everything to a new branch : testing_experimental. You can consult a diff or just check out the branch and try it yourself. As far as I can see, the initial address of the program start (0x1103) is left unaltered after miniboot finishes. So far so good.

Your Slave_bootloaderMiniboot.hex.zip represents a hex dump of your microcontroller flash memory after miniboot has finished. Which leads me to think that the error may already be in the external i2c flash memory or the way this address is fetched and reflashed by miniboot.

avr-objcopy -I ihex Slave_bootloaderMiniboot.hex -O binary Slave_bootloaderMiniboot.bin
clear && g++ unit.cpp -o unit && ./unit
application_start: 1103
mihaigalos commented 5 years ago

what is the SPM_PAGE_SIZE for your unit? If you're using atmega328p, it's 128. But may be different for your architecture and is also a potential diff.

Fabiancj81 commented 5 years ago

Hi friend   I am using the atmega328p, and an eeprom memory 24c256. To record the external memory I use the method get_metadata and avr-objcopy to compile binary. I have carefully followed the instructions of the Readme. regards fabian

mihaigalos commented 5 years ago

ok. Can you perform a binary dump of the external eeprom memory(24c256)? We need to find out if the information there is correct.

Fabiancj81 commented 5 years ago

cutecom.log I could not read the eeprom directly, for as long as I sent a log file of the eeprom records. I'm sure I have an old JDM programmer in a drawer with which if I can get a hex as soon as I find it, I send it to him. Thank you

Fabiancj81 commented 5 years ago

I forgot to comment that to burn the miniboot I use a USBasp programmer, but it only works for Avr devices and not eeprom

mihaigalos commented 5 years ago

The cutecom.log shows 0x11 at byte 36 and 0x03 at byte 37 in the external eeprom, which means 0x1103, the correct application start address. You can try to modify miniboot and print the flash location at address 2 and 3 (1 word) after each page which gets written. Something like :

#include <avr/pgmspace.h>
...
Serial.println(static_cast<uint16_t>(pgm_read_word(2)));
...
Fabiancj81 commented 5 years ago

Hello sorry for the delay, but I have not been able to return to return the topic, tomorrow I will try to print the message from miniboot. regards

Fabiancj81 commented 5 years ago

Dear    I failed to send the current page index by serial, due to the uart libraries, I tried several but all of them printed out clearly incorrect data for me to use the itoat () function. sorry.   it gives me the impression that after the bootloader records correctly in the flash and then restarts, the bootloader does not calculate the start of the application correctly (lines 153 and up) best regards

} else { uint16_t address_in_external_eeprom = getWordFromSource( source_i2c_address_for_program, application_start_address_byte_offset);

application_start = address_in_external_eeprom >> 8;
application_start |= static_cast<uint8_t>(address_in_external_eeprom);

} leaveBootloader(application_start);

mihaigalos commented 5 years ago

This looks wrong. It performs an endianness change here. I corrected it with commit a64cbff3078e206f584409342931cfa7879bab6e which is now in master. Can you please retest with the latest master? Many thanks for your help!

Fabiancj81 commented 5 years ago

fantastic¡¡   now miniboot has successfully started the program after the recording of the flash, I have tried it with different program sizes and so far all ok. I thank you for sharing this library, thank you very much again and have a good day. best regards

mihaigalos commented 5 years ago

Awesome. I am closing this bug issue.

Wellhamster commented 5 years ago

I think the problem is still present in certain cases. It doesn't work if application_start is calculated in: writeToFlash() But if you restart by disconnecting power supply and reconnect the application_start is calculated the way you fixed it. It starts normal in this case. I did a quick check and placed part: uint16_t address_in_external_eeprom = getWordFromSource(source_i2c_address_for_program,application_start_address_byte_offset); application_start = address_in_external_eeprom >> 8; application_start |= static_cast<uint16_t>(static_cast<uint8_t>(address_in_external_eeprom))<<8; right before leaveBootloader(application_start); That way the application_start is calculated the fixed way. It works fine with this modification. That is why I thing there is an issue within writeToFlash(). I was also not able to find where the problem within the writeToFlash() actually is up to this point. I'll try to track the error down.