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

Add error code for protected devices #97

Merged
merged 12 commits into from
Jan 19, 2024

Conversation

keeramis
Copy link
Contributor

@keeramis keeramis commented Jan 11, 2024

TODO - Change the error id and code per device-os

@keeramis keeramis marked this pull request as ready for review January 17, 2024 15:06
src/device.js Outdated Show resolved Hide resolved
src/dfu.js Outdated Show resolved Hide resolved
@@ -133,7 +139,16 @@ function messageForResultCode(result) {
return (RESULT_CODE_MESSAGES[result] || 'Request error');
}

function errorForRequest(result) {
Copy link
Member

Choose a reason for hiding this comment

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

FYI Andrey, I discussed with Keerthy that for consistency we'd use the DeviceProtectionError for both DFU and control requets so tools like the CLI only have one error to watch out.

@keeramis keeramis force-pushed the feature/sc-124322/protected-error branch from b605b6a to 6639969 Compare January 17, 2024 22:09
src/dfu.js Outdated
Comment on lines 786 to 793
async doUpload({ startAddr, maxSize, progress }) {
const segment = this._getSegment(startAddr);
if (typeof startAddr !== 'number') {
startAddr = this._memoryInfo.segments[0].start;
this._log.warn('Using inferred start address 0x' + startAddr.toString(16));
} else if (this._getSegment(startAddr) === null) {
} else if (segment === null) {
this._log.warn(`Start address 0x${startAddr.toString(16)} outside of memory map bounds`);
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you check what happens when this function is called with startAddr: null? Will it crash?

My thought is that it just doesn't make sense to support calling doUpload without a start address and it would be best to remove this if

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.

Did you mean segment is null?

If the startAddr is null, then it will simply get the start address from the memory segment and segment from that address and gets the data of maxSize bytes from that address. On my Boron, all the bytes read 0xff

If the segment is null, it continues anyway with a warning - segment is not referenced in the function after the warning.

Either way, it does not crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Other than showing a warnings there which is perhaps used in some cases that I am not aware of, the if condition can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's best to have doUpload require startAddr. It feels weird to support uploading without specifiying a source.
I removed the test for doUpload with startAddr null since we don't want to support this use case anymore.

Copy link
Member

@monkbroc monkbroc left a comment

Choose a reason for hiding this comment

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

Looks good. Go ahead and release this package

@keeramis keeramis merged commit 5ca5f9f into main Jan 19, 2024
4 checks passed
@keeramis keeramis deleted the feature/sc-124322/protected-error branch January 19, 2024 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants