-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
defmt-print
: Recover from decoding-errors
#598
Conversation
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.
changes look good. left some suggestions
print/src/main.rs
Outdated
Encoding::Raw => return Err(DecodeError::Malformed.into()), | ||
// rzcobs encoding allows recovery from decoding-errors. therefore we stop | ||
// decoding the current, corrupted, data and continue with new one | ||
Encoding::Rzcobs => break, |
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 think this should be continue
because may be more data cached in the stream_decoder
. if there isn't an entire frame then the next iteration will hit the Eof
case (which will make the program fetch more data from stdin)
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.
👍🏾
print/src/main.rs
Outdated
match stream_decoder.decode() { | ||
Ok(frame) => forward_to_logger(&frame, location_info(&locs, &frame, ¤t_dir)), | ||
Err(DecodeError::UnexpectedEof) => break, | ||
Err(DecodeError::Malformed) => match table.encoding() { |
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.
could you move the 'can this encoding recover?' logic into a method on the Encoding
enum itself? I think consumers should not be deciding on the Encoding
variant but rather the defmt-decoder
library should provide them that information.
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.
👍🏾
… `recoverable() -> bool`
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.
👍 Thanks! Feel free to send to bors with nits addressed or unaddressed
print/src/main.rs
Outdated
match stream_decoder.decode() { | ||
Ok(frame) => forward_to_logger(&frame, location_info(&locs, &frame, ¤t_dir)), | ||
Err(DecodeError::UnexpectedEof) => break, | ||
Err(DecodeError::Malformed) => match table.encoding().recoverable() { |
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.
(probably down to personal preference but if boolean
is more common than match boolean
IMO)
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 think it is more common to use if. But match saves one line 😋
bors r+ |
Build failed: |
bors retry |
Build succeeded: |
This PR uses the new
StreamDecoder
-API indefmt-print
and utilizes it to recover from decoding errors.Fixes #559