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.

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

Add log-format argument #493

Merged
merged 6 commits into from
Nov 3, 2023
Merged

Conversation

SergioGasquez
Copy link
Member

@SergioGasquez SergioGasquez commented Oct 18, 2023

Add -d/--defmt flag for monitor and flash subcommands.
Add -f/--log-format argument for monitor and flash subcommands.

cc @bugadani since you added this feature

Draft until properly tested. Did some testing and seems to be working fine!

We still need to figure out if we want opt-in or opt-out of defmt

@bugadani
Copy link
Contributor

bugadani commented Oct 18, 2023

Since the framing should mostly be compatible with the classic string streams, I'd probably suggest this to be opt out instead of opt in. It's probably much more annoying to enable defmt constantly for those who want it, than to disable for the few who have some custom binary in their logs. Transmission errors might shade this a bit for those who use noisy UART still, so I'm not sure.

If you end up going down the opt-in road, maybe it wouldn't be the worst idea to future proof it a bit with --log-format=defmt, so that if new formats (or new defmt versions!) appear, this doesn't have to be broken.

@SergioGasquez
Copy link
Member Author

To be honest, I don't have a preference if it should be opt-in or opt-out, any suggestion @esp-rs/espressif?

@SergioGasquez SergioGasquez marked this pull request as ready for review October 24, 2023 13:13
@bugadani
Copy link
Contributor

As an idea, maybe we could enable printing location info with --log-format=defmt-verbose or similar option, so I think I shall update my opinion and vote for that format :)

@SergioGasquez SergioGasquez changed the title Add defmt flag Add log-format argument Oct 26, 2023
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

I think with the --log-format change we're ready to go. I much prefer this to have to build a whole different espflash, and we can support other formats in the future.

Just one comment about making serial the default :)

espflash/src/cli/monitor/mod.rs Show resolved Hide resolved
espflash/Cargo.toml Outdated Show resolved Hide resolved
espflash/Cargo.toml Outdated Show resolved Hide resolved
Co-authored-by: Dániel Buga <[email protected]>
@jessebraham jessebraham added this to the v3 milestone Oct 26, 2023
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

@SergioGasquez SergioGasquez merged commit eaf14ae into esp-rs:main Nov 3, 2023
19 checks passed
@SergioGasquez SergioGasquez deleted the feat/defmt-arg branch November 3, 2023 16:34
@@ -102,7 +108,16 @@ pub fn monitor_with<L: InputParser>(
err => err,
}?;

parser.feed(&buff[0..read_count], &mut stdout);
match log_format {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why I didn't notice this but this isn't correct. The defmt parser is stateful, we can't just create a new one every time we have read some bytes.

@bugadani bugadani mentioned this pull request Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants