Skip to content
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

Feature/sc 124322/protected error #711

Merged
merged 12 commits into from
Jan 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions npm-shrinkwrap.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@
"sinon-chai": "^3.3.0"
},
"optionalDependencies": {
"particle-usb": "^2.5.0",
"particle-usb": "^2.5.1",
"serialport": "^9.2.8"
},
"engines": {
Expand Down
19 changes: 17 additions & 2 deletions src/cmd/keys.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const { errors: { usageError } } = require('../app/command-processor');
const UI = require('../lib/ui');
const ParticleApi = require('./api');
const { validateDFUSupport } = require('../lib/flash-helper');
const { DeviceProtectionError } = require('../lib/require-optional')('particle-usb');

/**
* Commands for managing encryption keys.
Expand Down Expand Up @@ -501,16 +502,30 @@ module.exports = class KeysCommand {
}

async _dfuWrite(device, buffer, { altSetting, startAddr, leave, noErase }) {
await device.writeOverDfu(buffer, { altSetting, startAddr, leave, noErase });
try {
await device.writeOverDfu(buffer, { altSetting, startAddr, leave, noErase });
} catch (err) {
if (err instanceof DeviceProtectionError) {
throw new Error('Operation could not be completed due to device protection.');
}
throw new VError(ensureError(err), 'Writing over DFU failed');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in behavior. Previously it would have just thrown the error directly (i.e. throw err). Does this change fix a specific thing you noticed during testing?

}
}

async _dfuRead(device, { altSetting, startAddr, size }) {
let buf;
try {
buf = await device.readOverDfu({ altSetting, startAddr, size });
} catch (err) {
if (err instanceof DeviceProtectionError) {
throw new Error('Operation could not be completed due to device protection.');
}
// FIXME: First time read may fail so we retry
buf = await device.readOverDfu({ altSetting, startAddr, size });
try {
buf = await device.readOverDfu({ altSetting, startAddr, size });
} catch (err) {
throw new VError(ensureError(err), 'Reading over DFU failed');
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might as well throw an error here? So I added this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't look necessary. The previous code will throw an error if it fails already.

}
return buf;
}
Expand Down
25 changes: 21 additions & 4 deletions src/lib/flash-helper.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,24 @@
const _ = require('lodash');
const usbUtils = require('../cmd/usb-util');
const { delay } = require('./utilities');
const VError = require('verror');
const { PLATFORMS, platformForId } =require('./platform');
const { moduleTypeFromNumber, sortBinariesByDependency } = require('./dependency-walker');
const { HalModuleParser: ModuleParser, ModuleInfo } = require('binary-version-reader');
const path = require('path');
const fs = require('fs-extra');
const os = require('os');
const semver = require('semver');
const { DeviceProtectionError } = require('../lib/require-optional')('particle-usb');
const utilities = require('./utilities');
const ensureError = utilities.ensureError;

// Flashing an NCP firmware can take a few minutes
const FLASH_TIMEOUT = 4 * 60000;

async function flashFiles({ device, flashSteps, resetAfterFlash = true, ui }) {
const progress = _createFlashProgress({ flashSteps, ui });
let success = false;
try {
for (const step of flashSteps) {
device = await prepareDeviceForFlash({ device, mode: step.flashMode, progress });
Expand All @@ -25,8 +30,9 @@ async function flashFiles({ device, flashSteps, resetAfterFlash = true, ui }) {
device = await _flashDeviceInDfuMode(device, step.data, { name: step.name, altSetting: altSetting, startAddr: step.address, progress: progress });
}
}
success = true;
} finally {
progress({ event: 'finish' });
progress({ event: 'finish', success });
Copy link
Contributor Author

@keeramis keeramis Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The UI was looking a bit weird as it says Flash Success and then Operation failed due to device protection.

So I added Flash Failed

if (device.isOpen) {
if (resetAfterFlash) {
try {
Expand Down Expand Up @@ -59,7 +65,10 @@ async function _flashDeviceInNormalMode(device, data, { name, progress, checkSki
await device.updateFirmware(data, { progress, timeout: FLASH_TIMEOUT });
return device;
} catch (error) {
// ignore error from attempts to flash to external flash
// ignore other errors from attempts to flash to external flash
if (error instanceof DeviceProtectionError) {
throw new Error('Operation could not be completed due to device protection.');
}
}
}
throw new Error('Unable to flash device');
Expand Down Expand Up @@ -109,7 +118,15 @@ async function _flashDeviceInDfuMode(device, data, { name, altSetting, startAddr
if (progress) {
progress({ event: 'flash-file', filename: name });
}
await device.writeOverDfu(data, { altSetting, startAddr: startAddr, progress });

try {
await device.writeOverDfu(data, { altSetting, startAddr: startAddr, progress });
} catch (error) {
if (error instanceof DeviceProtectionError) {
throw new Error('Operation could not be completed due to device protection.');
}
throw new VError(ensureError(error), 'Writing over DFU failed');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here. Why use this vs throw error;

}
return device;
}

Expand Down Expand Up @@ -177,7 +194,7 @@ function _createFlashProgress({ flashSteps, ui }) {
}
break;
case 'finish':
description = 'Flash success!';
description = payload.success ? 'Flash success!' : 'Flash failed.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good improvement.

if (isInteractive) {
progressBar.update({ description });
progressBar.stop();
Expand Down