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 definitionPath attribute to 'gdbtarget' JSON schema. #19

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

Conversation

FlorentVSTM
Copy link

This PR adds the 'definitionPath' attribute by extending the 'gdbtarget' JSON schema from cdt-gdb-vscode, helping users and avoiding the yellow squiggly line.

Note this PR was first proposed in the cdt-gdb-vscode repository, but after discussion with Colin, it was decided that it is better to make the change here: eclipse-cdt-cloud/cdt-gdb-vscode#116

@thegecko
Copy link
Contributor

Thanks for the PR @FlorentVSTM

I would prefer to see this fixed in a more generic way for two reasons:

  • this fix is specific to one debugger. I would prefer debugger types don't bleed into this project nor have to add many more entries as we see similar issues in other debuggers
  • different debuggers may have this path available as a different config item, therefore its overridable in the settings. If a user changes this, your squiggles reappear

There may be a better way of fixing this using a DiagnosticCollection or similar

cc @colin-grant-work

@colin-grant-work
Copy link

It does look like the way this extension handles this field creates a challenge for JSON schema documentation, because it seems to say that it will assign special significance to whatever field of a config is set in this configuration. Since that isn't something that the debugger extension itself is guaranteed to know - as in the case of GDB, where it can be used without this extension and doesn't use the peripheral fields for itself - it doesn't make sense to document it as a feature of a particular debugger. On the other hand, since it's dynamic, it doesn't even make sense to add it to the schema here, because we don't know what value that field could take, or whether some debugger actually might already know and care about that field have documented it itself, which I assume is why the actual value of the field name is configurable, to allow it to be adjusted in case a debug adapter already defines it.

@FlorentVSTM that leaves you in a difficult position, because it doesn't seem like the functionality you're looking for quite belongs in either upstream project. If you have a mechanism of distributing a fork to your users, you could apply your fix to either extension downstream. Alternatively, we could raise a feature request on VSCode to allow extensions to contribute general schema additions, so that this extension could say 'any debug configuration may include field X' without specifying the specific debug types to which it applies. Even then, the dynamic nature of the key as its defined here would likely make a hard sell for the feature.

@jreineckearm
Copy link
Contributor

jreineckearm commented Jan 11, 2025

I've been thinking about this and did some research as I do see benefit in defining additional launch/attach configuration items for debug view extensions that can be used with any debugger type.

I found that VS Code supports the * debug type. Which extends any debugger type with configuration attributes defined through this. If a debugger type has such items already then it ignores them. I believe this may be the way to allow the functionality intended by this PR.

We however may want to define items with names that are more specific to this extension to avoid confusion with debug adapter extensions that make already use of definitionPath and the likes. It could be either by "namespacing" options, for example peripheral-inspector.svdPath / peripheral-inspector.definitionPath. Or by defining a single object to scope peripheral inspector options like

  "contributes": {
    "debuggers": [
      {
        "type": "*",
        "configurationAttributes": {
          "launch": {
            "properties": {
              "peripheral-inspector": {
                "description": "Peripheral Inspector configuration",
                "type": "object",
                "properties": {
                  "definitionPath": {
                    "type": "string",
                    "description": "SVD file definition path"
                  }
                }
              }
            }
          }
. . . 

Some rethinking around the various existing ways to define the path and the option name (in the extensions settings) might be necessary. But I believe this would help us going forward to make the solution more scalable.

Note that it's also possible to register DebugConfigurationProvider for the * debugger type. In case we have to worry about additional validation of launch configurations. As per VS Code API spec, all of those providers are chained and called in arbitrary order before a debug session starts.

Thoughts are welcomed.

@thegecko
Copy link
Contributor

This is a much cleaner approach. Perhspas we could also add some other common names used for this path e.g. svd and svdPath?

@jreineckearm
Copy link
Contributor

This is a much cleaner approach. Perhspas we could also add some other common names used for this path e.g. svd and svdPath?

I'd even propose to not reuse definitionPath as part of the new approach. The above was an example how to define things. definitionPath is very generic and the examples you proposed would be clearer. Unless that contradicts in any way with the API usage for other formats than SVD.

@thegecko
Copy link
Contributor

I think definitionPath was added as the default when this extension became more generic than just showing svd files

@jreineckearm
Copy link
Contributor

I think definitionPath was added as the default when this extension became more generic than just showing svd files

OK, in that case it shall of course remain. :-)

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.

4 participants