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.

…ything take the new structs

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...

…gs descriptions

…ey are not, in fact, bugs

…oard as argument

…ted error messages

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!

Rahix · 2024-04-20T17:06:42Z

ravedude/src/main.rs

+        warning!(
+            "Passing the board as command-line argument is deprecated, use Ravedude.toml instead:\n\n# Ravedude.toml\n{}",
+            toml::to_string(&config::RavedudeConfig::from_args(&args)?)?
+        );


This warning is also shown when no commandline arguments are passed at all (and no Ravedude.toml is present. This is a bit confusing, instead we should show an error about Ravedude.toml being missing.

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.

Rahix · 2024-04-20T17:21:28Z

ravedude/src/config.rs

+    pub open_console: Option<bool>,
+    pub serial_baudrate: Option<NonZeroU32>,
+    pub port: Option<std::path::PathBuf>,
+    pub reset_delay: Option<u64>,
+    pub board: Option<String>,


Instead of making all fields Option<>s, you can also leverage serde's default attribute. Where it makes sense, of course.

Here, I think open_console would make much more sense that way:

#[serde(default)] pub open_console: bool,

Done! The only other place where I think this would be sensible is do_chip_erase in BoardAvrdudeOptions, but I don't want to use #[serde(default)] on it in case it's missing from the included boards.toml and goes unnoticed.

Rahix · 2024-04-20T17:26:27Z

ravedude/src/config.rs

+    #[serde(rename = "error")]
+    Error(String),


Should we really let the config specify the error message here? I know the old ravedude had each board config manually error out, but we don't need to keep that without good reason.

I think it'd be better for the config to simply state that port guessing is not supported and ravedude then provides a standardized error message.

Should we really let the config specify the error message here?
Honestly, probably not. I just kept it in because that's how it was before and I didn't want to make modifications to the preexisting behavior.

I've removed it. Now the message is just

Warning: this board cannot reset itself. Once reset, press ENTER here:

for all the boards without automatic reset. If the behavior should be changed, let me know!

Rahix · 2024-04-20T17:28:07Z

ravedude/src/main.rs

-    /// * duemilanove
+    /// * duemilanovepriori


What's this change about?

Oops, that's embarrassing. I must've accidentally pasted or typed something and somehow missed it. My bad, it's been removed.

Rahix · 2024-04-20T17:32:54Z

ravedude/src/main.rs

+                // The board arg is taken before the binary, so rearrange the args when Ravedude.toml exists
+                args.bin = Some(std::path::PathBuf::from(board));


Parsing the binary path into a String and then turning it back into a PathBuf is not a good idea: PathBuf is based on OsString which can potentially represent things a String can't. So doing this may error on certain paths because we try parsing them as utf-8. We should really handle the path opaquely as a PathBuf all the way.

I think the easiest solution for the time being is to make board a PathBuf, too, and only attempt converting it to a String in the legacy usecase. Or making board an OsString and then converting either direction.

Done 👍. board is now an OsString which gets converted into a String when used without Ravedude.toml.

Rahix · 2024-04-20T17:34:17Z

ravedude/src/config.rs

+        serialize_with = "serialize_baudrate",
+        deserialize_with = "deserialize_baudrate"
+    )]
+    pub baudrate: Option<Option<NonZeroU32>>,


Can you explain the double Option here?

The "inner" Option<NonZeroU32> signifies the avrdude baudrate, or None if it shouldn't be passed (i.e. use the default baudrate). The outer Option is there to allow overriding the inherited board's baudrate:

[board] inherit = "nano" [board.avrdude] # The nano has `baudrate = 57600` but it can be overriden. # Some(None) baudrate = -1 # Some(Some(NonZeroU32(8000)) baudrate = 8000 # None # < no baudrate field set >

You can think of it as an Option<i32>, but it's an Option<NonZeroU32> instead of an i32 where -1 is mapped to None. So, Option<Option<NonZeroU32>>. If this is too convoluted we can change it back into an i32.

Rahix · 2024-04-20T17:38:09Z

ravedude/src/board.rs

+use crate::config;
+
+fn get_all_boards() -> anyhow::Result<HashMap<String, config::BoardConfig>> {
+    toml::from_str(include_str!("boards.toml")).map_err(|err| {


To me this include_str!() is fine, I wouldn't waste time trying to parse the file at compile-time.

We should just make sure your testcase actually gets run in CI to catch errors in contributed pull-requests early. Can you adjust .github/workflows/ci.yml to also run cargo test for ravedude, in addition to cargo check please?

Rahix · 2024-04-20T17:40:51Z

ravedude/src/board.rs

-struct ArduinoNanoNew;
+pub fn get_board(board_name: Option<&str>) -> anyhow::Result<config::RavedudeConfig> {
+    Ok(match board_name {
+        Some(board_name) => {


This match arm is just for the legacy usecase, right?

Wouldn't it make more sense to split this into separate functions? The newer code from None => also feels to me like it should be a part of config.rs instead.

Done 👍. Yes, the match arm was for passing a named board. Now there's two functions, get_board_from_name and get_board_from_manifest. They're still both in board.rs because I feel like config.rs should be relatively separate from how it's actually read from the filesystem (get_all_boards which parses the included boards.toml is also in board.rs).

Rahix · 2024-04-20T17:44:19Z

ravedude/src/board.rs

+            assert!(board.name.is_some());
+            assert!(board.inherit.is_none());
+            assert!(board.reset.is_some());
+            assert!(board.avrdude.is_some());
+            let avrdude = board.avrdude.as_ref().unwrap();
+            assert!(avrdude.programmer.is_some());
+            assert!(avrdude.partno.is_some());
+            assert!(avrdude.baudrate.is_some());
+            assert!(avrdude.do_chip_erase.is_some());


Bonus points for making these asserts show what board failed the test :) You can do it like this:

for (name, board) in all_boards.items() { assert!(board.name.is_some(), "Board {name:?} is missing a `name`"); assert!(board.inherit.is_none(), "Board {name:?} has illegal `inherit` key");

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

… in board.rs, add test to ci

… not an Option<T>

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!

Creative0708 added 2 commits March 18, 2024 09:32

add temporary file config support for ravedude

7194571

fix incorrect substitution in get_board

8a45421

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

Creative0708 added 3 commits March 25, 2024 10:52

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

87ea6f6

…ything take the new structs

add options for command-line args in Ravedude.toml

c7cd830

that should be kebab case

1315fc3

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

Add configurable board options to ravedude #521

Open

Creative0708 added 6 commits April 6, 2024 23:02

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

c7207ee

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

83c147a

…gs descriptions

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

13b6b4a

…ey are not, in fact, bugs

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

d3fa042

…oard as argument

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

1f05afa

…ted error messages

fix chip erase error message

f0aa1bf

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

fix serial-baudrate not working in Ravedude.toml

92e6558

Rahix reviewed Apr 20, 2024

View reviewed changes

Creative0708 added 5 commits May 10, 2024 11:28

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

26057ca

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

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

4eb7681

… in board.rs, add test to ci

remove error message field from ResetOptions & change open_console to…

d9d7b43

… not an Option<T>

ravedude: fix open_console serialization & tweak error messages

b625dbb

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

e6d6987

ravedude: add comments for possibly confusing code

0a07f64

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

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Rahix Apr 20, 2024

Creative0708 May 18, 2024

Creative0708 commented May 18, 2024

		// The board arg is taken before the binary, so rearrange the args when Ravedude.toml exists
		args.bin = Some(std::path::PathBuf::from(board));

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Creative0708 commented May 18, 2024