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 😅 |
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.
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.
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.
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.
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.
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.
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)> { | |||
|
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-runcompiled against this was verified to work with defmt versions 0.3.2, 0.3.3 and 0.3.4.