-
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -351,7 +351,20 @@ function* loadFirmware( | |||||
| 'Expected metadata to be v2.x', | ||||||
| ); | ||||||
|
|
||||||
| const firmware = new Uint8Array(firmwareBase.length + 4); | ||||||
| const [checksumFunc, checksumSize] = (function () { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This code base typically uses lambda syntax. |
||||||
| switch (metadata['checksum-type']) { | ||||||
| case 'sum': | ||||||
| return [sumComplement32, 4]; | ||||||
| case 'crc32': | ||||||
| return [crc32, 4]; | ||||||
| case 'none': | ||||||
| return [null, 0]; | ||||||
| default: | ||||||
| return [undefined, 0]; | ||||||
| } | ||||||
| })(); | ||||||
|
|
||||||
| const firmware = new Uint8Array(firmwareBase.length + checksumSize); | ||||||
| const firmwareView = new DataView(firmware.buffer); | ||||||
|
|
||||||
| firmware.set(firmwareBase); | ||||||
|
|
@@ -361,22 +374,7 @@ function* loadFirmware( | |||||
| firmware.set(encodeHubName(hubName, metadata), metadata['hub-name-offset']); | ||||||
| } | ||||||
|
|
||||||
| const checksum = (function () { | ||||||
| switch (metadata['checksum-type']) { | ||||||
| case 'sum': | ||||||
| return sumComplement32( | ||||||
| firmwareIterator(firmwareView, metadata['checksum-size']), | ||||||
| ); | ||||||
| case 'crc32': | ||||||
| return crc32(firmwareIterator(firmwareView, metadata['checksum-size'])); | ||||||
| case 'none': | ||||||
| return null; | ||||||
| default: | ||||||
| return undefined; | ||||||
| } | ||||||
| })(); | ||||||
|
|
||||||
| if (checksum === undefined) { | ||||||
| if (checksumFunc === undefined) { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||||||
| // FIXME: we should return error/throw instead | ||||||
| yield* put( | ||||||
| didFailToFinish( | ||||||
|
|
@@ -391,8 +389,12 @@ function* loadFirmware( | |||||
| throw new Error('unreachable'); | ||||||
| } | ||||||
|
|
||||||
| if (checksum !== null) { | ||||||
| firmwareView.setUint32(firmwareBase.length, checksum, true); | ||||||
| if (checksumFunc !== null) { | ||||||
| firmwareView.setUint32( | ||||||
| firmwareBase.length, | ||||||
| checksumFunc(firmwareIterator(firmwareView, metadata['checksum-size'])), | ||||||
| true, | ||||||
| ); | ||||||
| } | ||||||
|
|
||||||
| return { firmware, deviceId: metadata['device-id'] }; | ||||||
|
|
@@ -1219,73 +1221,114 @@ function* handleFlashEV3(action: ReturnType<typeof firmwareFlashEV3>): Generator | |||||
|
|
||||||
| defined(version); | ||||||
|
|
||||||
| console.debug( | ||||||
| `EV3 bootloader version: ${version.getUint32( | ||||||
| 0, | ||||||
| true, | ||||||
| )}, HW version: ${version.getUint32(4, true)}`, | ||||||
| ); | ||||||
| // For reasons that we do not currently understand, some EV3s do not return | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? |
||||||
| // anything to our GetVersion command. We don't actually use the version | ||||||
| // for anything so we will just ignore this error. | ||||||
| try { | ||||||
| console.debug( | ||||||
| `EV3 bootloader version: ${version.getUint32( | ||||||
| 0, | ||||||
| true, | ||||||
| )}, HW version: ${version.getUint32(4, true)}`, | ||||||
| ); | ||||||
| } catch (err) { | ||||||
| console.error(`Failed to parse ev3 version response: ${ensureError(err)}`); | ||||||
| } | ||||||
|
|
||||||
| // FIXME: should be called much earlier. | ||||||
| yield* put(didStart()); | ||||||
|
|
||||||
| const sectorSize = 64 * 1024; // flash memory sector size | ||||||
| const maxPayloadSize = 1018; // maximum payload size for EV3 commands | ||||||
|
|
||||||
| 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`); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Don't really need to spam the logs. |
||||||
|
|
||||||
| // 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 | ||||||
|
Comment on lines
+1246
to
+1247
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| // cross the boundary will hang. To avoid this, we erase the whole firmware | ||||||
| // range at once. | ||||||
| const numSectors = Math.floor(action.firmware.byteLength / sectorSize); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we need to round up rather than down?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The direction doesn't matter here because it is implicitly testing that we're getting an integer result, which is in turn asserted below. Maybe we could make that more explicit instead.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I see. So really we want to check that If it isn't, we should show an error to the user. |
||||||
| assert( | ||||||
| action.firmware.byteLength === numSectors * sectorSize, | ||||||
| 'Firmware size is required to be a round multiple of sector size.', | ||||||
| ); | ||||||
|
|
||||||
| const erasePayload = new DataView(new ArrayBuffer(8)); | ||||||
| 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( | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing a
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||
| 16, | ||||||
| )})`, | ||||||
| ); | ||||||
|
|
||||||
| yield* put( | ||||||
| alertsShowAlert( | ||||||
| 'firmware', | ||||||
| 'flashProgress', | ||||||
| { | ||||||
| action: 'erase', | ||||||
| progress: undefined, | ||||||
| }, | ||||||
| firmwareBleProgressToastId, | ||||||
| true, | ||||||
| ), | ||||||
| ); | ||||||
|
|
||||||
| const [, eraseError] = yield* sendCommand( | ||||||
| 0xf0, | ||||||
| new Uint8Array(erasePayload.buffer), | ||||||
| ); | ||||||
|
|
||||||
| if (eraseError) { | ||||||
| yield* put( | ||||||
| alertsShowAlert('alerts', 'unexpectedError', { | ||||||
| error: eraseError, | ||||||
| }), | ||||||
| ); | ||||||
| // FIXME: should have a better error reason | ||||||
| yield* put(didFailToFinish(FailToFinishReasonType.Unknown, eraseError)); | ||||||
| yield* put(firmwareDidFailToFlashEV3()); | ||||||
| yield* cleanup(); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const erasePayload = new DataView(new ArrayBuffer(8)); | ||||||
| erasePayload.setUint32(0, i, true); | ||||||
| erasePayload.setUint32(4, sectorData.byteLength, true); | ||||||
| const [, eraseError] = yield* sendCommand( | ||||||
| 0xf0, | ||||||
| new Uint8Array(erasePayload.buffer), | ||||||
| // If we don't write an exact multiple of the sector size, the flash process | ||||||
| // 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); | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
| const payload = firmware.slice(i, i + writeLength); | ||||||
| console.debug( | ||||||
| `Programming bytes [0x${i.toString(16)}, 0x${(i + writeLength).toString( | ||||||
| 16, | ||||||
| )})`, | ||||||
| ); | ||||||
|
|
||||||
| if (eraseError) { | ||||||
| const [, sendError] = yield* sendCommand(0xf2, new Uint8Array(payload)); | ||||||
| if (sendError) { | ||||||
| yield* put( | ||||||
| alertsShowAlert('alerts', 'unexpectedError', { | ||||||
| error: eraseError, | ||||||
| error: sendError, | ||||||
| }), | ||||||
| ); | ||||||
| // FIXME: should have a better error reason | ||||||
| yield* put(didFailToFinish(FailToFinishReasonType.Unknown, eraseError)); | ||||||
| yield* put(didFailToFinish(FailToFinishReasonType.Unknown, sendError)); | ||||||
| yield* put(firmwareDidFailToFlashEV3()); | ||||||
| yield* cleanup(); | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| for (let j = 0; j < sectorData.byteLength; j += maxPayloadSize) { | ||||||
| const payload = sectorData.slice(j, j + maxPayloadSize); | ||||||
|
|
||||||
| const [, sendError] = yield* sendCommand(0xf2, new Uint8Array(payload)); | ||||||
| if (sendError) { | ||||||
| yield* put( | ||||||
| alertsShowAlert('alerts', 'unexpectedError', { | ||||||
| error: sendError, | ||||||
| }), | ||||||
| ); | ||||||
| // FIXME: should have a better error reason | ||||||
| yield* put(didFailToFinish(FailToFinishReasonType.Unknown, sendError)); | ||||||
| yield* put(firmwareDidFailToFlashEV3()); | ||||||
| yield* cleanup(); | ||||||
| return; | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| yield* put( | ||||||
| didProgress((i + sectorData.byteLength) / action.firmware.byteLength), | ||||||
| ); | ||||||
| const progress = (i + writeLength) / firmware.byteLength; | ||||||
| yield* put(didProgress(progress)); | ||||||
|
|
||||||
| yield* put( | ||||||
| alertsShowAlert( | ||||||
| 'firmware', | ||||||
| 'flashProgress', | ||||||
| { | ||||||
| action: 'flash', | ||||||
| progress: (i + sectorData.byteLength) / action.firmware.byteLength, | ||||||
| progress: progress, | ||||||
| }, | ||||||
| firmwareBleProgressToastId, | ||||||
| true, | ||||||
|
|
||||||
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.
As to not confuse with
metadata['checkum-size'].