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.

Sign up for GitHub

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

Jump to bottom

Improve monitoring #737

Merged
merged 8 commits into from
Feb 12, 2025
Merged

Improve monitoring #737

merged 8 commits into from
Feb 12, 2025

Conversation

SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented Jan 31, 2025 •
edited
Loading

  • Adds non-interactive to flash
  • Creates a struct with monitoring config
  • Add the --no-reset flag
  • Use different baudrate for connecting and for monitoring

SergioGasquez
SergioGasquez commented Jan 31, 2025
View reviewed changes
Copy link
Member Author

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

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

A few open questions/notes:

espflash/src/cli/mod.rs Outdated
#[non_exhaustive]
pub struct MonitorConfigArgs {
/// Baud rate at which to communicate with target device
#[arg(short = 'r', long, env = "MONITOR_BAUD", default_value = "115_200", value_parser = parse_uint32)]
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about the name and short, we use baud in ConnectArgs to set the baudrate at which we connect to the target. We could modify any of the two

Copy link
Member

Choose a reason for hiding this comment

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

We have an issue (or maybe multiple) for this type of change, so we can just punt it off to another PR for now.

@SergioGasquez SergioGasquez force-pushed the feat/non-interactive branch 2 times, most recently from dd4bd30 to cd21e74 Compare January 31, 2025 12:14
SergioGasquez
SergioGasquez commented Jan 31, 2025
View reviewed changes
espflash/src/cli/mod.rs
Comment on lines +143 to +150
#[clap(flatten)]
pub monitor_args: MonitorConfigArgs,
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how to make this require monitor (I coulndt get requires to work as it flatten), but it doesnt make sense that an user could set the monitor_args and not set monitor. I can only think of a check in the flash method that checks this and throws an error.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm yeah not sure how much we can really do about this. There are unfortunately some issues with using flatten, but it's a bit of a necessity with how things are currently structured. For now it's probably fine, we can merge as-is and open an issue to investigate this further.

Copy link
Member Author

Choose a reason for hiding this comment

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

Issue created: #764

This was linked to issues Jan 31, 2025
Make --non-interactive usable everywhere #655
Closed
Flash and monitor commands do not use the same baudrate source #736
Closed
@SergioGasquez SergioGasquez marked this pull request as ready for review February 3, 2025 16:26
@SergioGasquez SergioGasquez force-pushed the feat/non-interactive branch 4 times, most recently from 70c7ea4 to a52fa28 Compare February 7, 2025 15:36
@SergioGasquez
Copy link
Member Author

If #754 gets merged before this, we should add a test for espflash flash --monitor

@jessebraham
Copy link
Member

If #754 gets merged before this, we should add a test for espflash flash --monitor

#754 has been merged, do you plan to add the test here, or would you like it to be done in a separate PR?

(Will try to get this reviewed ASAP, sorry for the delay!)

@SergioGasquez
Copy link
Member Author

SergioGasquez commented Feb 11, 2025 •
edited
Loading

#754 has been merged, do you plan to add the test here, or would you like it to be done in a separate PR?

Will quickly add it here!

Update: Added!

JurajSadel
JurajSadel reviewed Feb 11, 2025
View reviewed changes
Copy link
Contributor

@JurajSadel JurajSadel left a comment

Choose a reason for hiding this comment

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

LGTM, not sure what we want to do about open discussion threads.

jessebraham
jessebraham approved these changes Feb 12, 2025
View reviewed changes
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@SergioGasquez SergioGasquez added this pull request to the merge queue Feb 12, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 12, 2025
@SergioGasquez SergioGasquez added this pull request to the merge queue Feb 12, 2025
Merged via the queue into esp-rs:main with commit da99a6d Feb 12, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jessebraham jessebraham jessebraham approved these changes

@MabezDev MabezDev MabezDev left review comments

@JurajSadel JurajSadel JurajSadel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Flash and monitor commands do not use the same baudrate source Make --non-interactive usable everywhere
4 participants
@SergioGasquez @jessebraham @MabezDev @JurajSadel