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 new/missing defmt log format options #90

Merged
merged 7 commits into from
May 22, 2024
Merged

Conversation

bugadani
Copy link
Contributor

@@ -318,6 +326,20 @@
],
"default": "String"
},
"mode": {
"type": "string",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just hoping here that omitting this will result in None on the Rust side...

Copy link
Contributor

@noppej noppej left a comment

Choose a reason for hiding this comment

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

Yes, omitting will result in a None on the rust side.

Some observations:

  • It feels odd to have defmtLogFormat as a peer to rttChannelFormats. Can we not somehow incorporate it into `rttChannelFormats.dataFormat ... or something sane?
  • Why is channelName a number?

@bugadani
Copy link
Contributor Author

Why is channelName a number?

Copy-paste-forget bug, sorry

Can we not somehow incorporate it into

This is a side effect of what cargo embed does: there is an outer option, that serves as a default for unconfigured channels, and inner ones have the same option to override this default. This is probably excessive...

@noppej
Copy link
Contributor

noppej commented Apr 16, 2024

@bugadani I just remembered ... we used to have a channel name, and then removed it, because it can be specified in the target application when the channels are configured. Since we would have to arbitrarily choose which one takes priority, it seemed to make more sense to just let the naming happen on the rust side, and then name the client channels based on that.

@noppej
Copy link
Contributor

noppej commented Apr 16, 2024

@bugadani I'm not sure I like having the outer and inner option. It adds code complexity and maintenance, for something that the user configures once in a launch.json file, and probably very seldom modified after that.

Does it really add value to have both?

@bugadani
Copy link
Contributor Author

Honestly I don't know, but the option is there in Rust - with the same question attached. I guess we can sack it from cargo embed, too, and forget the idea of a default set of configuration altogether.

@noppej
Copy link
Contributor

noppej commented Apr 16, 2024

I don't want to propose changing cargo-embed ... it will impact existing users. I am just suggesting that we don't bring the unnecessary complexity to the dap-server.

Especially since there can only be one defmt channel, what is the point of having an outer setting? Either set it for the one channel you have, or don't. Or am I missing something?

@bugadani
Copy link
Contributor Author

Well then I just added a bunch of code to delete it :)

cargo embed's config format has already been changed in not a small way, so I think twisting it once more shouldn't be too bad. Also, as you've said, this should be a one time annoyance.

package.json Outdated Show resolved Hide resolved
package.json Outdated
@@ -488,6 +520,10 @@
"type": "boolean",
"default": true,
"description": "Enable the inclusion of Defmt location information in the RTT output for `dataFormat=Defmt`."
},
"defmtLogFormat": {
Copy link
Contributor

Choose a reason for hiding this comment

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

@bugadani The probe-rs CLI, uses

      --log-format <LOG_FORMAT>
          The default format string to use for decoding defmt logs

Does it make sense for us to be consistent in the naming of these options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depends on where probe-rs/probe-rs#2442 ends up at, I'm not opposed.

@bugadani
Copy link
Contributor Author

Last commit enables squigglies on typos:

image

Relevant (but not really helpful?) docs: https://json-schema.org/draft/2020-12/json-schema-core#section-10.3.2.3

Copy link
Contributor

@noppej noppej left a comment

Choose a reason for hiding this comment

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

You are my hero!!! Thank you for always going the extra mile and making things so much better.

@bugadani bugadani marked this pull request as ready for review May 22, 2024 20:49
@noppej noppej merged commit c666cbd into probe-rs:master May 22, 2024
3 checks passed
@bugadani bugadani deleted the rtt branch May 22, 2024 21:02
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