Fix load hex file - #175
Conversation
jreineckearm
left a comment
jreineckearm
left a comment
There was a problem hiding this comment.
Apologies for the long delay in the review.
Implementation looks good. See a few questions/suggestions.
How much of the standard does it support? IIRC, other packages we looked at in the past usually missed one or the other part.
Still need to take it for a spin.
| const memory = createMemoryFromRead(memoryResponse); | ||
| const memoryMap = new MemoryMap({ [Number(memory.address)]: memory.bytes }); | ||
| await vscode.workspace.fs.writeFile(outputFile, new TextEncoder().encode(memoryMap.asHexString())); | ||
| const encodedContent = IntelHEX.encode({ address: memory.address, bytes: memory.bytes }); |
There was a problem hiding this comment.
Nit: maybe rename to hexEncodedContent. Made me look twice seeing it fed into another encoder.
| const memoryBlocks = IntelHEX.decode(new TextDecoder().decode(byteContent)); | ||
| for (const memory of memoryBlocks) { | ||
| memoryReference = toHexStringWithRadixMarker(memory.address); | ||
| count = memory.bytes.length; |
There was a problem hiding this comment.
Should that be accumulated?
| return value.toString(16).toUpperCase().padStart(2, '0'); | ||
| } | ||
|
|
||
| function parseHexByte(value: string, context: string): number { |
There was a problem hiding this comment.
Assume it's callers responsibility to ensure it's only two digits? Some commentary would be helpful.
| @@ -1,2 +1,2 @@ | |||
| /******************************************************************************** | |||
| * Copyright (C) 2024 EclipseSource. | |||
There was a problem hiding this comment.
Maybe extend the copyright header with ", Arm Limited and others". This is a significant change to the original implementation from EclipseSource. And this makes it clearer they are not the only point of contact for an implementation that comes from Arm.
Any objections to that, @planger , @martin-fleck-at ?
| import { expect } from 'chai'; | ||
| import { IntelHEX } from './intel-hex'; | ||
|
|
||
| describe('intel-hex', () => { |
There was a problem hiding this comment.
Nit: Could extend the tests with some real world captures and test snapshots. Maybe even generated with the previous implementation as a reference.
2 participants
What it does
Fixes the issue
#172
Removes dependancy nrf-intel-hex
How to test
Review checklist
Reminder for reviewers