-
Notifications
You must be signed in to change notification settings - Fork 123
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
Support loading flash settings from the config file #627
Conversation
Allow loading the flash frequency, size, and mode from the config file.
Rename the FlashFrequency, FlashMode, and FlashSize enum variants to more user-friendly names. Note that will break attempting to deserialize these values that were serialized by old versions of the code. However, I did not see anywhere currently using serialization for this field. The serialization support was added in esp-rs#528, for probe-rs/probe-rs#1952, but as best I can tell it doesn't look like this functionality ended up being used in the probe-rs code.
/// Boolean flag to merge binaries into single binary | ||
#[arg(long)] | ||
pub merge: bool, | ||
/// Custom partition table for merging | ||
#[arg(long, short = 'T', requires = "merge", value_name = "FILE")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change does remove the requires = "merge"
setting from the partition_table
argument for save-image
, since the ImageArgs
can't see the merge
flag. This attribute was previously different between the save-image
and flash
commands.
We could add back our own manual check to reject the --partition-table
flag if --merge
is not specified. However, it does look like the partition_table
argument is in fact used to print the partition size even when the --merge
flag is not specified.
@@ -60,27 +60,38 @@ pub(crate) mod stubs; | |||
#[repr(u8)] | |||
pub enum FlashFrequency { | |||
/// 12 MHz | |||
#[serde(rename = "12MHz")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this in the commit comments, but just to explicitly call it out: these rename
attributes will break anything that tries to deserialize FlashFrequency/FlashMode/FlashSize values that were serialized by old versions of the code.
However, I didn't see anything that appeared to be using this serialization. The serialization support was added in #528, for probe-rs/probe-rs#1952, but it seems like that functionality wasn't ultimately used in probe-rs as best as I can tell.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bugadani Will this cause issues in probe-rs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. I think what happened here is that #528 was merged, then I relaised I can simplify something and didn't end up needing serde.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this feature! Changes LGTM! Could you to please document it on both readmes under the config section (eg: https://github.com/esp-rs/espflash/tree/main/espflash#configuration-file)???
Also, have you conducted any testing?
Did some testing and seems to be working as expected, the following config file:
baudrate = 460800
[flash]
mode = "qio"
size = "2MB"
frequency = "80MHz"
[connection]
[[usb_device]]
vid = "303a"
pid = "1001"
Produced the following log:
....
[2024-04-30T08:58:06Z DEBUG] Config: Config {
baudrate: Some(
460800,
),
bootloader: None,
connection: Connection {
serial: None,
},
partition_table: None,
usb_device: [
UsbDevice {
vid: 12346,
pid: 4097,
},
],
flash: FlashSettings {
mode: Some(
Qio,
),
size: Some(
_2Mb,
),
freq: Some(
_80Mhz,
),
},
save_path: "/home/sergio/Documents/Espressif/third-parties/espflash/espflash/espflash.toml",
}
....
@@ -60,27 +60,38 @@ pub(crate) mod stubs; | |||
#[repr(u8)] | |||
pub enum FlashFrequency { | |||
/// 12 MHz | |||
#[serde(rename = "12MHz")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bugadani Will this cause issues in probe-rs?
I've updated espflash/README.md with some brief documentation of the
I've been using it for my own development, and I have confirmed that the image header that is written contains the correct flash mode, size, and speed settings taken from the config file. I've also confirmed that passing in flash mode CLI arguments takes precedence over values stored in the config file. |
I think the changes are not pushed, also, mind also updating the cargo-espflash readme? Thanks a lot! This is a very nice addition. |
Whoops you're right, my local repo had the wrong tracking branch so the push didn't update this branch. I've updated the cargo-espflash readme too, and pushed to the right branch this time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for the contribution!
This supports loading the flash size, frequency, and mode from the config file.