-
Notifications
You must be signed in to change notification settings - Fork 132
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
Normalize similar arguments #759
Conversation
Shall I also edit the other places of the codebase where we use |
667f557
to
8824e55
Compare
Id like to come up with better names for monitoring args:
Here is what esptool does: #736 |
8824e55
to
f9a34a2
Compare
For the moment, I'm using |
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
@@ -277,7 +280,7 @@ pub struct MonitorConfigArgs { | |||
#[arg(long, requires = "non_interactive")] | |||
no_reset: bool, | |||
/// Logging format. | |||
#[arg(long, short = 'L', default_value = "serial", requires = "elf")] | |||
#[arg(long, short = 'L', default_value = "serial")] |
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 removed the requires
because it was failing when using it as a custom runner, since you have the elf from the image, but dont have the elf
arg.
You still get a pretty decent error when you do espflash monitor --log-format defmt
and dont provide and elf:
❯ cargo r -r --bin espflash -- monitor --log-format defmt
...
Commands:
CTRL+R Reset chip
CTRL+C Exit
Error: espflash::monitor::defmt::no_elf
× No elf data available
help: Please provide an ELF file with the `--elf` argument
But you get that after selecting the port
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.
Maybe we could investigate improving this in #764, since is somewhat similar
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.
It's also an issue in that espflash flash <image> --elf <image>
is meaningless, but it's allowed.
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!
I also removed some short arguments that didnt make much sense to me (I can revert those)