feat(flash): Add "flash crc" command#46
Conversation
JakubVanek
changed the title
JakubVanek
force-pushed
the
feature/crc
branch
4 times, most recently
from
2385844 to
58dfbfc
Compare
JakubVanek
changed the title
|
I have verified on a real brick that the modified version still works. |
JakubVanek
force-pushed
the
feature/crc
branch
3 times, most recently
from
2a69231 to
9b98858
Compare
| u8 *firmware = calloc(FLASH_SIZE, 1); | ||
| int read_bytes = fread(firmware, 1, FLASH_SIZE, fp); |
There was a problem hiding this comment.
Should check these for error return.
There was a problem hiding this comment.
I was indeed a bit lazy, I tried to fix this by checking for ferror() and by modifying the returned byte count condition.
| if (sector_number < 0x04) { // 0x000000 - 0x03FFFF | ||
| explanation = "U-Boot"; | ||
| } else if (sector_number == 0x04) { // 0x040000 - 0x04FFFF | ||
| explanation = "U-Boot environment"; | ||
| } else if (sector_number < 0x25) { // 0x050000 - 0x24FFFF | ||
| explanation = "Linux kernel"; | ||
| } else if (sector_number < 0xCB) { // 0x250000 - 0xCAFFFF | ||
| explanation = "Root file system"; | ||
| } else if (sector_number < 0xFA) { // 0xCB0000 - 0xF9FFFF | ||
| explanation = "User data"; | ||
| } |
There was a problem hiding this comment.
The first 256k is pretty much always going to be U-boot. But the rest is flexible and depends on the firmware.
You can build custom firmware already with Buildroot that has a different layout and at Pybricks, we're working on an alternate firmware that doesn't have Linux at all. So ideally, this would be configurable instead of hard-coded.
There was a problem hiding this comment.
I was somewhat aware of this possibility, but I didn't know how to handle these cases. Which of these do you think would be a better solution?
- Removing the flash area descriptions as they cannot be reliably inferred from the flash image and may thus be misleading,
- Adding a new command line parameter that would set the partition table (e.g. "LMS2012", "Pybricks")
- Adding a new environment variable that would override the partition table name in the same way
I would slightly prefer the first option, because I think that plumbing an additional argument for the partition table name is going to be a bit involved when flash crc <file>, flash crc <file> <from sector> <nr of sectors> and flash crc <file> <from sector> <nr of sectors> v are all accepted syntaxes. Alternatively, I may also try to simplify the current flash crc command line interface.
There was a problem hiding this comment.
I have dropped the flash-area-description-introducing commit from the PR, but I can recover it from reflog if needed.
There was a problem hiding this comment.
If you want to bring that back, we could require a flash memory map for 3rd-party firmware as a separate file or use a hash to detect the official LEGO firmware files so that a separate file isn't required in that case.
There was a problem hiding this comment.
I think that it might not be worth the effort. I don't expect the ev3duder flash crc command to be commonly used and the main advantage of flash area descriptions is in explaining why sector 4 and the user-data sectors are reported as corrupted. I will try to document this peculiarity elsewhere; I will mention there that these sector ranges only apply to the stock Lego firmware.
| if (sector_number == 4) { | ||
| // U-Boot environment | ||
| mutable = true; | ||
| } else if (sector_number >= 0xCB) { | ||
| // User data | ||
| mutable = true; | ||
| } |
There was a problem hiding this comment.
Since the sectors aren't fixed, it would make more sense to look up these by name or have a sector info struct that contains both the name and a mutable bool flag.
There was a problem hiding this comment.
Hmm, agreed; I was initially thinking that the table would be an unnecessary complication of the code, but it may be a bit cleaner after all.
"flash crc" gives the user various ways of comparing the CRC of a file to the CRC reported by the EV3 bootloader. Can do that for the full image, or a group of sectors, or sector-by-sector.
3 participants
"flash crc" gives the user various ways of comparing the CRC of a file to the CRC reported by the EV3 bootloader. Can do that for the full image, or a group of sectors, or sector-by-sector.
Credits go to @gtminch and so I have left him as the commit author. I have only picked the code up from JakubVanek#1, cleaned it up slightly and separated it from the other flashing code changes.