An alternative way to load peripherals information

[asimgunes]

Hi,

This implementation contains a new ability to extend SVD Viewer to support not only SVD file format but also different formats. With this implementation the retrieving logic of the peripherals information separated from PeripheralTreeProvider class and extracted to a different class. In addition, a new parameter getPeripherals introduced, which enables svd-viewer to load the peripherals information from a different VSCode extension during the debug session by invoking the VSCode command defined in the getPeripherals launch parameter.

We believe this implementation provide a new ability to svd-viewer and we are kindly asking for review for this pull request. We also extend the documentation and provide a basic guideline for users to extend the svd-viewer by implementing their own VSCode plugins.

Kind regards.
Asim Gunes

[thegecko]

@asimgunes this is a great idea!

Please be aware that this extension has now been upstreamed back to the original which has been separated at:

https://github.com/mcu-debug/peripheral-viewer

Both extensions still live in the marketplace, but I intend to deprecate this one once this issue is resolved:

mcu-debug/peripheral-viewer#8

Let me know if you want to keep this PR here, re-open it on peripheral-viewer or open it on both. I'm keen to not review it twice :)

Thanks!

[asimgunes]

Hi @thegecko,

I am preparing a new pull request for https://github.com/mcu-debug/peripheral-viewer and planning to send it soon.

You can ignore this one.

Kind regards.

[thegecko]

@asimgunes

I've re-opened this to get it merged here.

Did you consider exposing an API in this extension to hook into rather than accepting a command? That feels more structured, but I can see how your approach allows more control by the user (e.g. per-project)

[asimgunes]

Hi @thegecko,

Thanks for returning me back, I think I didnot get the idea of providing an API here. Actually when I started I was trying somehow registering a loader and then try to use that loader here, however I wasn't successful then return back to command invocation as implemented here. But I am open for suggestions and changes here if you can explain a little bit more about your idea.

Some notes while this request was closed:

1. I remember I optimised/fixed some parts of the code in peripheral-viewer request which are not reflected to this pull request, if you plan to merge this, let me change that part too.
2. I think you are also moving this project to eclipse cdt, if you want I can provide the update to cdt project too. I can also provcide some sample code and after the migration we wouldnt need to make changes of the sample code too.

[thegecko]

... if you can explain a little bit more about your idea.

For example, this extension returns an API you can interact with already:

https://github.com/eclipse-cdt-cloud/vscode-svd-viewer/blob/main/src/browser/extension.ts#L26

One approach could be to offer an extension point in that API for other file types and write an extension for your file type. e.g. expose in the API:

    ...
    registerFileParser(fileExtension: string, (filePath: Uri) => Promise<PeripheralNode[]>)),
    ...

which can be called with:

    ...
    <svdViewerApi>.registerFileParser('myEsvdExtension', async uri => {
        // do something with contents or uri and return node array
    });
    ...

I remember I optimised/fixed some parts of the code in peripheral-viewer request which are not reflected to this pull request, if you plan to merge this, let me change that part too.

yes please

I think you are also moving this project to eclipse cdt, if you want I can provide the update to cdt project too. I can also provcide some sample code and after the migration we wouldnt need to make changes of the sample code too.

already done (check the URL above!)

[asimgunes]

Hi @thegecko,

Ok, let me check the api suggestion while checking the pull request again. it seems a better solution.

[asimgunes]

Hi @thegecko,

I re-visit my previous implementation and changed the integration points to work over the extension api. Could you please re-check the implementation when you are available?

[thegecko]

Thanks @asimgunes let me check it out!

Do you have an example non-svd file I can test?

[thegecko]

This works well for me, nothing seems broken!

I've added a few comments about structure and exposing the API and a few styling nits.

Could you also check your formatting and indentation for the new/modified files?

[asimgunes]

Hi @thegecko,

I made changed related to your suggestions. There could be slight differences related to implementation details. Could you check the latest implementation in your suitable time?

This is what Arm do for the cmsis extension:

https://www.npmjs.com/package/@arm-software/vscode-cmsis-csolution

Also, example about the "cmsis extension" is interesting and I can contribute for similar approach for this extension, but we need a new repository for this implementation. Additonally, it might be logical to move the type definitions and some utility functions might be useful for users, and then reference the package in svd-viewer to avoid duplication as you mentioned in this code review. But I also prefer merging the changes up to now and leaving that work for another pull request. Is it suitable for you?

[thegecko]

I started renaming SVD Viewer and svd-viewer to their new equivalents but gave up after so many comments. Can you do this in the PR? :)

Please also rebase on main and check all still compiles and works for you

[asimgunes]

Hi @thegecko,

I rebased and update the request as the name of the package changed. Could you please review again in your suitable time?

You may observe that I have additional updates. There is an additional api.ts at the root folder and types are now imported from 'peripheral-inspector/api' as you can also see in the documentation. This change comes because previously I missed the scenario that if the package installed directly from Github, there is no dist folder in it, which is causing import problem. For a workaround, I added api.ts file into the root, but still keep the actual definition file under the src folder. In long term, I think we need a separate package that includes API information like you previously mentioned about the ARM cmsis extension, but I like to leave this work to another (follow-up) update, and if you agree, merge this with the current state. Please let me know your thoughts, I am looking forward to hear them.

[thegecko]

Some indentations are wrong, but that's a minor niggle which can be addressed separately.
I also think we can make the API better by using GitHubs own npm registry. I'll craft a follow on PR to discuss that.
I'll merge this once I've done a final test locally.

[thegecko]

OK, so this fails to run with svd files. please accept the suggestions and all should be well. You could also check the indentation in the api-types file ;)

[asimgunes]

Hi @thegecko, thanks for your comments, I am going to check the suggestions soon and planning to return you by tomorrow. Kind regards.

[asimgunes]

Hi @thegecko,

I made the changes, could you please check it again when you are suitable?

Thanks.

[thegecko]

LGTM

[thegecko]

Released in v1.5.0