-
Notifications
You must be signed in to change notification settings - Fork 39
ev3: Fix issues with flashing (some?) EV3s. #2352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
EDIT: I'll collect some notes in the issue thread instead. |
d1681a4 to
9cd1a76
Compare
Flash the EV3 all in one go rather than sector-by-sector. This appears to resolve issues we had with flashing (some?) EV3s, and mirrors what we do in pybricksdev.
9cd1a76 to
e9e075f
Compare
| ); | ||
|
|
||
| const firmware = new Uint8Array(firmwareBase.length + 4); | ||
| const [checksumFunc, checksumSize] = (function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const [checksumFunc, checksumSize] = (function () { | |
| const [checksumFunc, checksumExtraLength] = (function () { |
As to not confuse with metadata['checkum-size'].
| })(); | ||
|
|
||
| if (checksum === undefined) { | ||
| if (checksumFunc === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would make sense to move this to fail as early as possible (line 366).
| ); | ||
|
|
||
| const firmware = new Uint8Array(firmwareBase.length + 4); | ||
| const [checksumFunc, checksumSize] = (function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const [checksumFunc, checksumSize] = (function () { | |
| const [checksumFunc, checksumSize] = (() => { |
This code base typically uses lambda syntax.
| true, | ||
| )}, HW version: ${version.getUint32(4, true)}`, | ||
| ); | ||
| // For reasons that we do not currently understand, some EV3s do not return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we fail the get the version correctly, then can't we expect other responses to fail?
Seems like this needs more investigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact the other responses do not fail. I agree it warrants further investigation. Perhaps a new issue, though, since the method of flashing the firmware does not depend on the version?
| for (let i = 0; i < action.firmware.byteLength; i += sectorSize) { | ||
| const sectorData = action.firmware.slice(i, i + sectorSize); | ||
| assert(sectorData.byteLength <= sectorSize, 'sector data too large'); | ||
| console.info(`Firmware size: ${action.firmware.byteLength} bytes`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| console.info(`Firmware size: ${action.firmware.byteLength} bytes`); | |
| console.debug(`Firmware size: ${action.firmware.byteLength} bytes`); |
Don't really need to spam the logs.
| erasePayload.setUint32(0, 0, true); // start address | ||
| erasePayload.setUint32(4, numSectors * sectorSize, true); // size | ||
| console.debug( | ||
| `Erasing bytes [0x${(0).toString(16)}, 0x${(numSectors * sectorSize).toString( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would also be useful to log the number of sectors that we calculated.
| erasePayload.setUint32(0, 0, true); // start address | ||
| erasePayload.setUint32(4, numSectors * sectorSize, true); // size | ||
| console.debug( | ||
| `Erasing bytes [0x${(0).toString(16)}, 0x${(numSectors * sectorSize).toString( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a - 1 on the end address?
| erasePayload.setUint32(0, 0, true); // start address | ||
| erasePayload.setUint32(4, numSectors * sectorSize, true); // size | ||
| console.debug( | ||
| `Erasing bytes [0x${(0).toString(16)}, 0x${(numSectors * sectorSize).toString( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, we also have a util function to add leading zeros in hex numbers.
| // Apparently, erasing a span of the flash creates some sort of record in | ||
| // the EV3, and we can only write within a given erase span. Writes that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I though that is what we were doing before. Erase one sector - write the entire one sector - erase the next sector - write one sector - and so on...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There has been quite a bit of testing and discussion about this already. Erasing the required space in one go was found to be more reliable. It's also what we've been testing with Pybricksdev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // will hang on the last write we send. | ||
| const firmware = action.firmware; | ||
| for (let i = 0; i < firmware.byteLength; i += maxPayloadSize) { | ||
| const writeLength = Math.min(maxPayloadSize, firmware.byteLength - i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure this is redundant since slice doesn't pad thing but rather returns a partial if i + maxPayloadSize exceeds the firmware size.
|
It seems like this is fixing at least 3 different things. So would be better to split it up into 3 commits.
|
Flash the EV3 all in one go rather than sector-by-sector. This appears to resolve issues we had with flashing (some?) EV3s, and mirrors what we do in pybricksdev.
Fixes pybricks/support#2375.