Skip to content
This repository has been archived by the owner on Jan 30, 2024. It is now read-only.

reject use of defmt and --no-flash (again) #129

Merged
merged 2 commits into from
Jan 7, 2021
Merged

reject use of defmt and --no-flash (again) #129

merged 2 commits into from
Jan 7, 2021

Conversation

japaric
Copy link
Member

@japaric japaric commented Jan 6, 2021

before PR #105 using defmt required the --defmt flag. At that time it was not possible to use the --defmt and --no-flash flags together. The rationale for that restriction is that without it cargo run can invoke probe-run with a newer version of the ELF different from the ELF that was last flashed on the device and that can result in log messages that make no sense and/or decoder errors.

After PR 105 landed it unintentionally became possible to use defmt with --no-flash due to the auto-detection (auto-enable) mechanism. This PR corrects that and makes the use of --no-flash flag and defmt an error again.

@japaric japaric requested a review from Lotterleben January 7, 2021 11:17
src/main.rs Outdated
@@ -399,6 +396,10 @@ fn notmain() -> Result<i32, anyhow::Error> {
.as_ref()
.map_or(false, |ch| ch.name() == Some("defmt"));

if use_defmt && opts.no_flash {
bail!("\"defmt\" RTT channel is in use, but `--no-flash` was specified -- this combination is not allowed");

Choose a reason for hiding this comment

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

nit: "defmt" RTT channel is in use requires system knowledge in order to grasp what the problem is– could this be rephrased to something like "attempting to specify --no-flashwhile using defmt -- this combination is not allowed"?

@japaric
Copy link
Member Author

japaric commented Jan 7, 2021

bors r+

@bors bors bot merged commit f59c74a into main Jan 7, 2021
@bors bors bot deleted the no-flash-no-defmt branch January 7, 2021 14:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants