Feature/sc 124322/protected error #711

keeramis · 2024-01-11T15:59:13Z

Description

CLI gives an error that device is protected if the device is protected.

How to Test

Prepare your device with a device-os branch that returns a device protected error. I used this diff on device-os 5.6.0

diff --git a/services/inc/system_error.h b/services/inc/system_error.h
index 5f3b9252c..8bb781629 100644
--- a/services/inc/system_error.h
+++ b/services/inc/system_error.h
@@ -24,6 +24,7 @@
 // List of all defined system errors
 #define SYSTEM_ERRORS \
         (NONE, "", 0), /* -999 ... 0: Generic result codes */ \
+        (DEVICE_PROTECTED, "Device protected error", -999), \
         (UNKNOWN, "Unknown error", -100), \
         (BUSY, "Resource busy", -110), \
         (NOT_SUPPORTED, "Not supported", -120), \
diff --git a/system/src/control/storage.cpp b/system/src/control/storage.cpp
index d9438eaa9..d6630a681 100644
--- a/system/src/control/storage.cpp
+++ b/system/src/control/storage.cpp
@@ -84,6 +84,7 @@ void firmwareUpdateCompletionHandler(int result, void* data) {
 } // namespace
 
 int startFirmwareUpdateRequest(ctrl_request* req) {
+    return SYSTEM_ERROR_DEVICE_PROTECTED;
     PB(StartFirmwareUpdateRequest) pbReq = {};
     CHECK(decodeRequestMessage(req, PB(StartFirmwareUpdateRequest_fields), &pbReq));
     cancelFirmwareUpdate(); // Cancel current transfer

Point this particle-cli branch to use this particle-usb PR - Add error code for protected devices particle-usb#97
Attempt to flash a device by running npm start -- flash --local

After testing, make sure you flash the device with a "good" device-os branch so you can resume flashing.

$ npm start -- flash --local --target 5.6.0 /Users/keerthyamisagadda/code/workbench/uhura-tests/

> particle-cli@3.18.1 start
> node ./src/index.js flash --local --target 5.6.0 /Users/keerthyamisagadda/code/workbench/uhura-tests/

Flashing boron e00fce68623fcff480e85e47
Targeting version: 5.6.0

Including:
    ../workbench/uhura-tests/project.properties
    ../workbench/uhura-tests/src/uhura-tests.cpp
    /Users/keerthyamisagadda/code/workbench/uhura-tests/assets/cat.txt
    /Users/keerthyamisagadda/code/workbench/uhura-tests/assets/water.txt
    /Users/keerthyamisagadda/code/workbench/uhura-tests/assets/water2.txt

Compile succeeded and bundle created.

Memory use:
    Flash      RAM
    11144      542

[░░░░░░░░░░░░░░░░░░░░░░░░░] 0% | Flash success!
Operation could not be completed due to device protection.

keerthyamisagadda@loaners-macbook-pro particle-cli (feature/sc-124322/protected-error) $

Related Issues / Discussions

Completeness

keeramis · 2024-01-17T22:15:21Z

src/cmd/keys.js

+				buf = await device.readOverDfu({ altSetting, startAddr, size });
+			} catch (err) {
+				throw new Error('Reading over DFU failed', err);
+			}


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

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

keeramis · 2024-01-17T22:16:18Z

src/lib/flash-helper.js

 	} finally {
-		progress({ event: 'finish' });
+		progress({ event: 'finish', success });


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

monkbroc

Looks good now. Probably simplify the throw statements to just throw error. Update particle-usb dependency then this is good to merge 👍

monkbroc · 2024-01-18T18:27:15Z

src/cmd/keys.js

+			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');


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?

monkbroc · 2024-01-18T18:28:06Z

src/cmd/keys.js

+				buf = await device.readOverDfu({ altSetting, startAddr, size });
+			} catch (err) {
+				throw new Error('Reading over DFU failed', err);
+			}


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

monkbroc · 2024-01-18T18:29:34Z

src/lib/flash-helper.js

+		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');


Same question here. Why use this vs throw error;

monkbroc · 2024-01-18T18:30:01Z

src/lib/flash-helper.js

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


That's a good improvement.

keeramis · 2024-01-19T23:25:19Z

From my local run:

keeramis commented Jan 17, 2024

View reviewed changes

keeramis requested a review from monkbroc January 17, 2024 22:26

keeramis marked this pull request as ready for review January 17, 2024 22:27

monkbroc approved these changes Jan 18, 2024

View reviewed changes

keeramis added 12 commits January 19, 2024 14:56

Add a graceful error for protected devices

37c905b

Add error msg

cfc9a4a

Fix npm-shrinkwrap

6e9dfbe

Modify error msg

2b5b874

Use DeviceProtectionError

92e1e3d

clean up

1ed6f22

minor

3f5a1eb

minor

923b7cc

Minor change in error msgs

cf2b448

lint fix

862197b

delete temp files

f1f7a59

Update particle-usb dep

9e73928

keeramis force-pushed the feature/sc-124322/protected-error branch from ed541c0 to 9e73928 Compare January 19, 2024 22:56

keeramis merged commit f935f3a into master Jan 19, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/sc 124322/protected error #711

Feature/sc 124322/protected error #711

keeramis commented Jan 11, 2024 •

edited

keeramis Jan 17, 2024

monkbroc Jan 18, 2024

keeramis Jan 17, 2024 •

edited

monkbroc left a comment

monkbroc Jan 18, 2024

monkbroc Jan 18, 2024

monkbroc Jan 18, 2024

monkbroc Jan 18, 2024

keeramis commented Jan 19, 2024

Feature/sc 124322/protected error #711

Feature/sc 124322/protected error #711

Conversation

keeramis commented Jan 11, 2024 • edited

Description

How to Test

Related Issues / Discussions

Completeness

keeramis Jan 17, 2024

Choose a reason for hiding this comment

monkbroc Jan 18, 2024

Choose a reason for hiding this comment

keeramis Jan 17, 2024 • edited

Choose a reason for hiding this comment

monkbroc left a comment

Choose a reason for hiding this comment

monkbroc Jan 18, 2024

Choose a reason for hiding this comment

monkbroc Jan 18, 2024

Choose a reason for hiding this comment

monkbroc Jan 18, 2024

Choose a reason for hiding this comment

monkbroc Jan 18, 2024

Choose a reason for hiding this comment

keeramis commented Jan 19, 2024

keeramis commented Jan 11, 2024 •

edited

keeramis Jan 17, 2024 •

edited