Add file config support for ravedude #522

Creative0708 · 2024-03-18T13:44:43Z

Followup PR to #521

What the title says. This adds a board named custom to ravedude that reads from Ravedude.toml from the project directory.

Works with this config (in Ravedude.toml):

name = "ATtiny85 through AVRISP"

[reset]
automatic = true

[avrdude]
programmer = "avrisp"
partno = "t85"
baudrate = 19200
do_chip_erase = true

And this cargo runner:

runner = "/path/to/ravedude custom -P /dev/ttyUSB0"

Minor detail but this PR also changes get_board to return anyhow::Result<Box<dyn Board>>, if that matters.

Anyways, format and implementation of custom board support in ravedude is up for discussion. If we reach a consensus about what would be best, this PR will be marked as ready for review.

Creative0708 · 2024-03-18T14:19:24Z

Also, some further notes:

Leveraging the power of serde and the toml crate, we get nice error messages whenever a field is missing:

# [...]

[avrdude]
programmer = "avrisp"
# partno = "t85"
baudrate = 19200
do_chip_erase = true

Produces

Error: TOML parse error at line 7, column 1
  |
7 | [avrdude]
  | ^^^^^^^^^
missing field `partno`

"Dumping" a built-in board configuration to toml is not yet done. Due to how the Board trait is implemented, it's impossible to get the board ports from a dyn Board or the like. Most likely, we'll just transcribe the source of board.rs into several BoardConfigs.
This is fully backwards-compatible with previous ravedude configurations, as it just adds a board named "custom". Now that I've got some chance to think, this feels a bit... clunky.
I'd rather have a mandatory Ravedude.toml in the project root for all boards. My reasoning for this is that a TOML file with all the keys already present would be more intuitive/familiar and less confusing than modifying the runner in .cargo/config.toml with flag options only discoverable by running ravedude -h.
Additionally, if someone wants to modify their board setup, they'll have to generate the config for their board, then modify it. This is an extra step in what I believe should be an easy modification of... something.
While breaking every past user's project in a new version is off the table, maybe we could display a warning like Using a board argument is deprecated. Run `ravedude --dump-toml uno > Ravedude.toml` to generate a new config, then change the runner to `ravedude -P /dev/ttyUSB0`. We could then change cargo-generate to use Ravedude.toml configured with the correct board in the setup process.
Obviously, this is my personal opinion. This is up for discussion of course.

Rahix · 2024-03-19T01:03:10Z

Leveraging the power of serde and the toml crate, we get nice error messages whenever a field is missing:

nice! 👍

This is fully backwards-compatible with previous ravedude configurations, as it just adds a board named "custom". Now that I've got some chance to think, this feels a bit... clunky.

Feel free to drop the entire Board trait abstraction. I was assuming that you were going to do that anyway :D I see no reason to stick to the existing architecture here - it was mostly written this way to quickly get something running.

Whatever we end up doing with the "builtin boards", they should also simply get encoded in the new TOML format somewhere, as you suggested.

I'd rather have a mandatory Ravedude.toml in the project root for all boards.

Interesting idea. My original design approach was to model ravedude after probe-run. A simple tool to be dropped into the cargo config as a project runner.

Your suggestion reminds me of the approach of cargo-embed which also has a more or less mandatory config file for all its parameters.

I don't see a big reason why not to go down this route, although I think we should consider the specifics carefully:

First of all, I would then suggest to drop a lot of the commandline arguments and push them into Ravedude.toml instead. Something like -P should stay, but also get a corresponding setting in the file.
We suggest building new projects using the avr-hal-template which could also generate an initial Ravedude.toml without problems.
I'm not a fan of the suggestion that people will always encode all board settings in their own Ravedude.toml rather than referencing a "well known board". The reason is that this prevents us from rolling out fixes/changes for the well known boards. Unless customization is necessary, I think users should still just reference a board by its name.
A possible approach would maybe be one where a board may be referenced by name, but whether it is or isn't, custom settings can be used to override or define parameters.

To put it into concrete examples:

# Ravedude.toml for using well known board
[general]
console = true
baudrate = 57600

board = "uno"

# Ravedude.toml for using a completely custom board
[general]
console = true
baudrate = 57600

[board]
name = "ATtiny85 through AVRISP"

[board.reset]
automatic = true

[board.avrdude]
programmer = "avrisp"
partno = "t85"
baudrate = 19200
do_chip_erase = true

# Ravedude.toml for customizing well known board
[general]
console = true
baudrate = 57600

[board]
name = "Uno without automatic reset"
inherit = "uno"

[board.reset]
automatic = false

What do you think?

Creative0708 · 2024-03-19T16:42:00Z

Feel free to drop the entire Board trait abstraction. I was assuming that you were going to do that anyway :D

Whatever we end up doing with the "builtin boards", they should also simply get encoded in the new TOML format somewhere, as you suggested.

Sounds good!

First of all, I would then suggest to drop a lot of the commandline arguments and push them into Ravedude.toml instead. Something like -P should stay, but also get a corresponding setting in the file.

I assume in this case command-line options would override the TOML file. Would we want a warning to be printed in that case?

I don't see a big reason why not to go down this route,

Does this signify approval? Because I'd rather not change things just for the sake of changing things. If mandatory TOML configs don't benefit the project in any way, they shouldn't be implemented.

I'm not a fan of the suggestion that people will always encode all board settings in their own Ravedude.toml rather than referencing a "well known board". The reason is that this prevents us from rolling out fixes/changes for the well known boards. Unless customization is necessary, I think users should still just reference a board by its name.

This is a very good point, and I hadn't considered that. Yeah, I agree that this would be a better solution.

The examples look great! However, I would like a consistent case across keys. (Either snake_case or kebab-case is fine, although I'm leaning towards kebab case to mirror Cargo.toml.)

Also, annoyingly, serde doesn't have "inheritance" behavior built-in, and from some very basic searching around the web I've gathered that the best way to do this is just to make everything an Option<whatever> and merge the configs after deserialization. Not such a big deal though.

A final note/question: Would this be considered a breaking change...? If not, we'd need to find a way to keep this backwards-compatible. Maybe a "default" config that's used if the file doesn't exist? It depends on how much of a priority backwards compatibility is.

Rahix · 2024-03-30T23:37:56Z

Hi, sorry for only getting back to you now...

A final note/question: Would this be considered a breaking change...? If not, we'd need to find a way to keep this backwards-compatible. Maybe a "default" config that's used if the file doesn't exist? It depends on how much of a priority backwards compatibility is.

To me, it's really important not to completely break existing users. At least, there should be a trivial migration path that doesn't require deeper understanding of the inner workings of ravedude and avrdude...

For simplicity, my suggestion would be to keep the existing invocations working as is, as long as no Ravedude.toml is present. In such a case, I'd like a warning to inform users that they should migrate.

If all else fails, a hacky solution to do this might be to throw the current "legacy" argument parser into a separate module and invoke it when no Ravedude.toml is found. Then the given options are used to construct an in-memory "compatibility" Ravedude.toml and use that with the new code.

I assume in this case command-line options would override the TOML file. Would we want a warning to be printed in that case?

Hm, so let's look at all the CLI args we have right now and I'll share my thoughts for each one:

-c, --open-console: Field in Ravedude.toml, we might want true/false overwrite from CLI to be possible.
--debug-avrdude: Keep as is, no field in Ravedude.toml
-b, --baudrate <baudrate>: Field in Ravedude.toml, can be overwritten from CLI
-P, --port <port>: Optional field in Ravedude.toml, can be overwritten from CLI or env ($RAVEDUDE_PORT)
-d, --reset-delay <reset-delay>: Field in Ravedude.toml, can be overwritten from CLI
<BOARD>: The chosen board should only be passed on CLI in legacy mode. When Ravedude.toml is present, passing a board should result in an error.

Generally, I wouldn't display warnings for the overrides. I'd only show warnings for deprecated things or errors when someone combines legacy invocations with a new Ravedude.toml.

Does this signify approval? Because I'd rather not change things just for the sake of changing things. If mandatory TOML configs don't benefit the project in any way, they shouldn't be implemented.

Yeah, let's do it. For the purpose that ravedude wants to fulfil, this seems the most appropriate path.

although I'm leaning towards kebab case to mirror Cargo.toml

Same here 👍

Thanks again for working on this! I'm excited to see the result :) This will make ravedude so much more usable and finally elevate it beyond the ugly hack that it currently is...

Creative0708 · 2024-04-08T02:23:19Z

Whew! Yeah, no worries, I've been busy as well...

Anyways, a ton of stuff is complete! Additionally, it's now in a state where this could be merged, although a few (minor) things differ from your suggestions. Listing them off:

Board inheritance now works! Something like

[general]
open-console = true
serial-baudrate = -1
port = "/dev/ttyUSB0"

[board]
name = "Arduino uno without automatic reset, for some reason"
inherit = "uno"

[board.reset]
automatic = false
message = "Take a minute to admire the coolness of the RAVEDUDE!"

is valid. Everything (well, everything that can be overriden) in the RavedudeConfig struct is now an Option<T>, and ravedude "merges" the user-provided configuration to one of the hardcoded boards in this case.

As in the TOML above, entries for the command-line options are now present.

Just using a named board works too:

[general]
open-console = true
port = "/dev/ttyUSB0"

board = "uno"

All the hardcoded boards have been translated it to src/boards.toml. Currently the entire file is just include_str!'d into the binary because I can't seem to find a way to embed the actual configs into the program instead of parsing the toml at runtime (static-toml looks promising, but it only returns a variable TOML mapping, not a specific, validated type). There's a test added to make sure the boards are valid, though. Maybe add that to CI if we can't find a better solution?

A warning is displayed for people still using the old format:

$ # cargo runner is `ravedude uno -P /dev/ttyUSB0`
$ cargo run
Warning: Passing the board as command-line argument is deprecated, use Ravedude.toml instead:

# Ravedude.toml
[general]
port = "/dev/ttyUSB0"

[board]
inherit = "uno"

# ravedude continues like normal...

Warning text/format is up for discussion, obviously.

Various other errors are displayed if the user doesn't include a port, avrdude programmer, avrdude part number, passes a board name as a command-line argument while using Ravedude.toml, doesn't provide a board config, etc.

Additionally, using Ravedude.toml I have successfully uploaded a rust program to the ATtiny85 through the Arduino Nano!

Anyways, yeah, unless we want to make changes to any of this (or if there are bugs), this PR is ready to merge! 🎉 (Not saying that it should be merged right now, but that it can be... there are also merge conflicts that need clearing up)

Thanks again for working on this! I'm excited to see the result :)

No problem! ravedude (and the entirety of avr-hal, really) makes my life a lot easier when using Rust for AVR-related projects! Happy to help :D

Creative0708 · 2024-04-08T02:36:46Z

Oh yeah -- We should also probably modify stuff like the avr-hal template to use Ravedude.toml, along with adding options for completely custom setups.

Rahix

Finally found time to take a closer look. Thanks a lot for working on this, it's coming together quite nicely :)

Please check my comments below!

ravedude/src/main.rs

Rahix · 2024-04-20T17:16:36Z

ravedude/src/main.rs

+ return Ok(());
+ }
+
+ let ravedude_config_exists = std::path::Path::new("Ravedude.toml").try_exists()?;


Right now you are searching for Ravedude.toml in the current working directory. This breaks when you e.g. cargo run inside the src/ directory. Maybe you can check how other tools like cargo-embed find their config file and implement something similar?

As a hint, it seems cargo sets some environment variables when running a runner process. One of them is CARGO_MANIFEST_DIR which seems to indicate the directory where cargo found its own Cargo.toml...

cargo-embed uses figment, which looks interesting. I'm most likely gonna switch ravedude over to it to mirror its behavior (and cargo's behavior) with environment variables, although I've held back on it for now due to it possibly being overkill (?) although ravedude itself isn't going to run on a microcontroller so that probably wasn't a good reason.

CARGO_MANIFEST_DIR seems to be only available when using ravedude as a runner. ravedude should be able to access Ravedude.toml no matter how it's being run, in my opinion.

Currently, I just have ravedude scan all parent directories until it finds a Ravedude.toml somewhere, which works fine for now... this behavior will probably be replaced by figment, though.

ravedude/src/config.rs

ravedude/src/main.rs

ravedude/src/config.rs

ravedude/src/board.rs

Creative0708 · 2024-05-18T16:02:12Z

Hi, again, sorry for the delay. I've made modifications to the code and left comments based on your feedback. Thanks!

…ything take the new structs

…gs descriptions

…ey are not, in fact, bugs

…oard as argument

…ted error messages

…n warning showing up when it's not supposed to

… in board.rs, add test to ci

… not an Option<T>

Rahix

Heads up: I rebased your changes ontop of the latest main to resolve some conflicts. Please make sure to continue working ontop of the rebased version.

I have a few more comments, check below. I'll hopefully get to testing the new ravedude in more depth soon, then I'll report back if I find anything else. It's looking very very promising already, though! :)

Rahix · 2024-06-08T11:25:36Z

ravedude/src/main.rs

+ args
+ .board
+ .as_deref()
+ .ok_or_else(|| anyhow::anyhow!("no board given and couldn't find Ravedude.toml in project, either pass a board as an argument or make a Ravedude.toml."))?


Actually, don't reference the deprecated board name method in this error message. We don't want to advertise the legacy mechanism in any way. Simply state that we couln't find a Ravedude.toml and give a pointer on how to create one.

Rahix · 2024-06-08T11:44:11Z

ravedude/src/main.rs

+ let manifest_path = 'manifest_path: {
+ // By default Cargo scans the current dir and all of its parents for Cargo.toml,
+ // so we mirror its behavior here.
+ let current_dir = std::env::current_dir()?;
+
+ for dir_to_test in current_dir.ancestors() {
+ let path_to_test = dir_to_test.join(Path::new("Ravedude.toml"));
+ if path_to_test.exists() {
+ break 'manifest_path Some(path_to_test);
+ }
+ }
+
+ None
+ };


I wonder if we should still honor CARGO_MANIFEST_DIR when it is present in the environment. The reason I have in mind is usecases when calling cargo run with --manifest-path while the working directory is completely different.

Rahix · 2024-06-08T11:50:56Z

ravedude/src/main.rs

- board: String,
+ // When Ravedude.toml exists, the binary is placed where the board should be. This is an OsString to not lose
+ // informaton when we have to take the board as the binary.
+ board: Option<OsString>,


Got a bit of a tricky problem here right now: The ravedude --help output is still completely wrong. Ideally, we can get it to just show the new way of doing things and secretly handle the legacy method in the background.

To do this, I think we need to take the following steps:

Rename the current bin to something like oldbin with the help message stating that this is an argument used for backwards compatibility.

Rename the current board to bin. Adjust the docstring to the current one from bin and drop the verbatim_doc_comment setting.

Creative0708 changed the title ~~Add temporary file config support for ravedude~~ Add file config support for ravedude Mar 18, 2024

Rahix added the ravedude label Mar 18, 2024

Rahix linked an issue Apr 1, 2024 that may be closed by this pull request

Add configurable board options to ravedude #521

Open

Creative0708 marked this pull request as ready for review April 8, 2024 02:23

Rahix reviewed Apr 20, 2024

View reviewed changes

Creative0708 added 18 commits June 8, 2024 13:39

add temporary file config support for ravedude

f67806f

fix incorrect substitution in get_board

87cd575

move board configs to a toml file, implement --dump-config, make ever…

bcc91d5

…ything take the new structs

add options for command-line args in Ravedude.toml

83cbe3b

that should be kebab case

b30e58a

add board inheritance, change baudrate syntax, add test for boards.toml

d2b9e94

remove warnings for command-line overrides + fix some command-line ar…

4dac9a6

…gs descriptions

make BoardConfig not say that nonexistent options are bugs because th…

9af2384

…ey are not, in fact, bugs

change config format, tweak error messages, add warning for passing b…

45c70bb

…oard as argument

add direct board name option in [general] for Ravedude.toml + associa…

d508d3d

…ted error messages

fix chip erase error message

f3bd4e1

fix serial-baudrate not working in Ravedude.toml

30b5488

make ravedude search in parent dirs for manifest, also fix deprecatio…

eb59786

…n warning showing up when it's not supposed to

ravedude: change board arg to OsString, add error messages to asserts…

8e5acc0

… in board.rs, add test to ci

remove error message field from ResetOptions & change open_console to…

6d0f0e0

… not an Option<T>

ravedude: fix open_console serialization & tweak error messages

0ee4493

ravedude: make board reset prompt only display when a binary is given

0825c28

ravedude: add comments for possibly confusing code

07e201e

Rahix force-pushed the main branch from 0a07f64 to 07e201e Compare June 8, 2024 11:41

Rahix self-requested a review June 8, 2024 11:42

Rahix mentioned this pull request Jun 8, 2024

[ravedude] add output modes, newline after n bytes or after char #549

Open

Rahix reviewed Jun 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add file config support for ravedude #522

Add file config support for ravedude #522

Creative0708 commented Mar 18, 2024

Creative0708 commented Mar 18, 2024

Rahix commented Mar 19, 2024

Creative0708 commented Mar 19, 2024

Rahix commented Mar 30, 2024

Creative0708 commented Apr 8, 2024

Creative0708 commented Apr 8, 2024

Rahix left a comment

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Creative0708 commented May 18, 2024

Rahix left a comment

Rahix Jun 8, 2024

Rahix Jun 8, 2024

Rahix Jun 8, 2024

Add file config support for ravedude #522

Are you sure you want to change the base?

Add file config support for ravedude #522

Conversation

Creative0708 commented Mar 18, 2024

Creative0708 commented Mar 18, 2024

Rahix commented Mar 19, 2024

Creative0708 commented Mar 19, 2024

Rahix commented Mar 30, 2024

Creative0708 commented Apr 8, 2024

Creative0708 commented Apr 8, 2024

Rahix left a comment

Choose a reason for hiding this comment

Rahix Apr 20, 2024

Choose a reason for hiding this comment

Creative0708 May 18, 2024

Choose a reason for hiding this comment

Creative0708 commented May 18, 2024

Rahix left a comment

Choose a reason for hiding this comment

Rahix Jun 8, 2024

Choose a reason for hiding this comment

Rahix Jun 8, 2024

Choose a reason for hiding this comment

Rahix Jun 8, 2024

Choose a reason for hiding this comment