-
-
Notifications
You must be signed in to change notification settings - Fork 76
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 support for decoding wire format version 3 #750
Conversation
Thank you for fixing this! The new wire format was a huge pain for our logging backend that now needed to support both versions 😅 |
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.
defmt-print
prints the versions it supports, but this is currently only defmt_decoder::DEFMT_VERSION
. Would be good to add DEFMT_VERSION_3
as well
I thought about it but decided against it to keep the fix minimal. Perhaps the message should just be changed from |
That is fair and makes sense. Just some indication that it supports more than just the latest version would be nice, but if you consider it out of scope for this PR that works too. |
LGTM, but I propose we wait for @japaric or @Urhengulas to return for this one. |
parser/src/display_hint.rs
Outdated
crate_name: crate_name.into(), | ||
crate_name: Some(crate_name.into()), | ||
}); | ||
} | ||
[bitflags_name, package, disambiguator] => { | ||
return Some(DisplayHint::Bitflags { | ||
name: bitflags_name.into(), | ||
package: package.into(), | ||
disambiguator: disambiguator.into(), | ||
crate_name: None, |
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.
Can you please switch to an if-else because I think the match is getting confusing? And also add a comment to each case telling which wire format version it refers to?
Something like:
let [bitflags_name, package, disambiguator, crate_name] = if parts.len() == 4 {
// wire format version 4
[parts[0], parts[1], parts[2], parts[3]]
} else if parts.len() == 3 {
// wire format version 3
[parts[0], parts[1], parts[2], None]
} else {
return Some(DisplayHint::Unknown(s.into())):
}
return Some(DisplayHint::Bitflags {
// ...
}
lol, the length of parts
is the same as the wire format version; note to future: this is just by coincidence
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 variant became a little more complicated due to the fact that crate_name
needs to be an Optional
.
I chose to structure it a little bit different to simplify the code. Do you like this variant?
if parts.len() < 3 || parts.len() > 4 {
return Some(DisplayHint::Unknown(s.into()));
}
return Some(DisplayHint::Bitflags {
name: parts[0].into(),
package: parts[1].into(),
disambiguator: parts[2].into(),
crate_name: parts.get(3).map(|&s| s.to_string()),
});
The part || parts.len() > 4
could be omitted, so the code would ignore additional components. That way, it would be possible to add further information in the future, without a breaking change.
However I'm not sure if such an extension is likely, or if it's better to have a stricter parser.
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.
Looks good to me. And let's err for being more strict.
@@ -218,10 +218,10 @@ pub fn parse_impl(elf: &[u8], check_version: bool) -> Result<Option<Table>, anyh | |||
|
|||
/// Checks if the version encoded in the symbol table is compatible with this version of the `decoder` crate | |||
fn check_version(version: &str) -> Result<(), String> { | |||
if version != DEFMT_VERSION { | |||
if !DEFMT_VERSIONS.contains(&version) { | |||
let msg = format!( | |||
"defmt wire format version mismatch: firmware is using {}, `probe-run` supports {}\nsuggestion: use a newer version of `defmt` or `cargo install` a different version of `probe-run` that supports defmt {}", |
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.
Just a side note: This function is not only used by probe-run
but also by other binaries. That can become a little bit confusing if cargo embed
calls itself probe-run
: rp-rs/rp2040-project-template#57 (comment)
Any idea for a better wording?
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.
yeah, this is a bit annoying. we are tracking it here: #412
While it is ugly, we could just add a second parameter which is the application name.
fn check_version(version: &str, app_name: &str) -> Result<(), String>
"defmt wire format version mismatch: firmware is using {}, `probe-run` supports {}\nsuggestion: use a newer version of `defmt` or `cargo install` a different version of `probe-run` that supports defmt {}", | |
"defmt wire format version mismatch: firmware is using {}, `{app_name}` supports {}\nsuggestion: use a newer version of `defmt` or use a different version of `{app_name}` that supports defmt {}", |
pub const DEFMT_VERSION: &str = "4"; | ||
pub const DEFMT_VERSIONS: &[&str] = &["3", "4"]; | ||
// To avoid a breaking change, still provide `DEFMT_VERSION`. | ||
#[deprecated = "Please use DEFMT_VERSIONS instead"] |
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 wasn't really aware of the deprecated attribute. That's neat!
@@ -111,7 +110,7 @@ pub enum TimePrecision { | |||
/// | |||
/// Returns the integer and remaining text, if `s` started with an integer. Any errors parsing the | |||
/// number (which we already know only contains digits) are silently ignored. | |||
fn parse_integer<T: FromStr>(s: &str) -> Option<(&str, usize)> { | |||
fn parse_integer<T: FromStr>(s: &str) -> Option<(&str, T)> { |
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.
Good drive-by catch
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.
Thank you!
bors r+
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
As suggested in #749 (comment), it would be nice if defmt-decoder could handle older wire format versions transparently.
This patch implements support for version 3. A version of
probe-run
compiled against this was verified to work with defmt versions 0.3.2, 0.3.3 and 0.3.4.