Skip to content

An alternative way to load peripherals information (with peripherals) #11

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

asimgunes
Copy link

Hi,

We recently discuss the method here: eclipse-cdt-cloud/vscode-peripheral-inspector#8, and this request is the modified version of the same request for peripheral-viewer.

This implementation contains a new ability to extend Peripheral 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 peripherals introduced, which enables peripheral-viewer to load the peripherals information from a different VSCode extension during the debug session by invoking the VSCode command defined in the peripherals launch parameter.

We believe this implementation provide a new ability to peripheral-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 peripheral-viewer by implementing their own VSCode plugins.

Kind regards.
Asim Gunes

@asimgunes
Copy link
Author

Hi @thegecko, @haneefdm,

I hope you are all well.

I couldn't get any feedback yet, I wonder how this update sounds to you. Also, is there any plan to review and merge this update soon.

Please let me know if you have additional ideas.

Kind regards.
Asim

@haneefdm
Copy link
Contributor

Thank you for submitting the PR. Good idea. It is quite a few changes and I will review them over the weekend.

In the meantime, please remove the use of forEach and use for ... of. Better when single stepping and also has performance benefits. Same goes for map in case you used those.

Could you create a repo with a test extension that would use this feature? This is a must. If there is no way for me to try it out, I will have to decline the PR.

@thegecko
Copy link
Collaborator

@asimgunes apologies I missed your initial PR.

@haneefdm let me know if you want me to check this after you have.

@asimgunes
Copy link
Author

Hi @haneefdm,

Thanks for your feedback, I will provide an update for forEach logics of the code. Please review usage of map (it is used in one place), and it sound me weird to rewrite it as for ... of instead of map at this point. But, if you are sure about the change, I can implemented that with for logic too.

You are right about a test extension. Let me look for what can I provide for you in terms of testing, it might be a little bit complicated since the example is going to be device specific implementation.

Kind regards.

@asimgunes
Copy link
Author

asimgunes commented Jun 20, 2023

@asimgunes apologies I missed your initial PR.

@haneefdm let me know if you want me to check this after you have.

Hi @thegecko, no worries, thanks for your feedback now.

@haneefdm
Copy link
Contributor

You can keep the .map but I just don't like them as I value my time for debug convenience much more. Trumps code density ... for me.

Yeah, I was never notified of this PR when it was initially submitted.

haneefdm and others added 5 commits June 23, 2023 11:24
In this commit we are changing the foreach logic with for ... of logic.
And a minor fix in cache key logic.
@asimgunes
Copy link
Author

asimgunes commented Jun 23, 2023

Hi @haneefdm,

I updated the foreach logic to for ... of logic as you mentioned.

Unfortunately, I am not able to provide you a test example for now. But we have an ongoing process, which is going to be a new extension published to Microsoft Extension Marketplace soon. After this, I am going to have a change to share a test project with you.

I planning to return back you soon about the test project.

Kind regards.

@asimgunes
Copy link
Author

Hi @haneefdm,

You can find a sample VSCode extension that implements the new update here:
https://github.com/asimgunes/dynamic-peripherals-extension

I also generate a sample project and put the step by step description to run the sample project in the README file.

I think this update is ready to go for your review, (including sample projects to test them). Please let me know about your observations and do not hesitate to contact me for further updates.

Kind regards.

@haneefdm
Copy link
Contributor

haneefdm commented Jul 7, 2023

Thanks. I will look at it over the weekend.

@asimgunes
Copy link
Author

Hi @haneefdm,

Is there any update or plan about this request for soon?

Kind regards

@haneefdm
Copy link
Contributor

haneefdm commented Aug 8, 2023

Sorry, I was in the hospital for many days, just got out. It is a non-trivial merge and testing effort which is why I couldn't get around to it... No promises but I will have to wait until the weekend to re-look at everything.

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.

3 participants