New API for reading DLT messages #44
Conversation
kruss
force-pushed
the
add_message_reader
branch
from
96f9631 to
5206e33
Compare
kruss
force-pushed
the
add_message_reader
branch
from
5206e33 to
606b1fc
Compare
kruss
force-pushed
the
add_message_reader
branch
4 times, most recently
from
5219ba4 to
070a869
Compare
This change includes: - new API to read DLT messages from a source - new API to read DLT messages from a stream (esrlabs#34) - new API to collect generic DLT statistics (esrlabs#31) - removing of any dependency to the buf_redux crate - consolidation of the crate's feature names - increase of the crate's version to 0.20.0
kruss
force-pushed
the
add_message_reader
branch
from
070a869 to
d1e5b12
Compare
| - Add feature "serde-support", which adds to crate's types Serialize/Deserialize | ||
| - Add feature "serialization", which adds to crate's types Serialize/Deserialize |
There was a problem hiding this comment.
I don't think we should change changelog entry here, because the feature name in version 0.18.0 is still serde-support and it changed from 0.20.0 only
| fibex = ["quick-xml"] | ||
| serialization = ["serde", "serde_json"] | ||
| statistics = ["rustc-hash"] | ||
| stream = [] |
There was a problem hiding this comment.
I think it's good practice add the suffix dep: to the optional dependencies to distinguish them from other features names. link
This is totally optional, since we don't have any hard rules about it
| loop { | ||
| let consumed: usize = match reader.fill_buf() { | ||
| Ok(content) => { | ||
| if content.is_empty() { | ||
| println!("empty content after {} parsed messages", parsed); | ||
| break; | ||
| } | ||
| let available = content.len(); | ||
|
|
||
| match dlt_message(content, None, true) { | ||
| Ok((rest, _maybe_msg)) => { | ||
| let consumed = available - rest.len(); | ||
| parsed += 1; | ||
| consumed | ||
| } | ||
| Err(DltParseError::IncompleteParse { needed }) => { | ||
| println!("parse incomplete, needed: {:?}", needed); | ||
| return; | ||
| } | ||
| Err(DltParseError::ParsingHickup(reason)) => { | ||
| println!("parse error: {}", reason); | ||
| 4 //skip 4 bytes | ||
| } | ||
| Err(DltParseError::Unrecoverable(cause)) => { | ||
| println!("unrecoverable parse failure: {}", cause); | ||
| return; | ||
| } | ||
| } | ||
| match read_message(&mut dlt_reader, None).expect("read dlt message") { | ||
| Some(_message) => { | ||
| message_count += 1; | ||
| } | ||
| Err(e) => { | ||
| println!("Error reading: {}", e); | ||
| return; | ||
| None => { | ||
| break; |
There was a problem hiding this comment.
I think this can be optionally replaced with while let Some(...) loop.
| loop { | ||
| match read_message(&mut dlt_reader, None) | ||
| .await | ||
| .expect("read dlt message") | ||
| { | ||
| Some(_message) => { | ||
| message_count += 1; | ||
| } | ||
| None => { | ||
| break; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Also this can optionally replaced with while let Some(...) loop
| *n = LevelDistribution { | ||
| log_fatal: n.log_fatal + 1, | ||
| ..*n | ||
| } |
There was a problem hiding this comment.
Wouldn't be easier to increase the field value instead of reassigning the struct?
n.log_fatal += 1;If so, this can be applied for the code below as well.
| if self | ||
| .source | ||
| .read_exact(&mut self.buffer[..header_len]) | ||
| .await | ||
| .is_err() | ||
| { | ||
| return Ok(&[]); | ||
| } | ||
|
|
||
| let (_, message_len) = parse_length(&self.buffer[storage_len..header_len])?; | ||
| let total_len = storage_len + message_len; | ||
|
|
||
| self.source | ||
| .read_exact(&mut self.buffer[header_len..total_len]) | ||
| .await?; |
There was a problem hiding this comment.
I think we need to consider cancel safety here...
If the first await call (line 90) success, then we drop the task on the next await call (line 101), then the loaded data in the buffer will be lost, since the next call of next_message_slice() will rewrite the buffer.
If we don't need this function to be cancel-safe, then it would be helpful to right in the function documentation that this function is not cancel safe
| let with_storage_header = reader.with_storage_header(); | ||
| let slice = reader.next_message_slice().await?; | ||
|
|
||
| if !slice.is_empty() { | ||
| Ok(Some( | ||
| dlt_message(slice, filter_config_opt, with_storage_header)?.1, | ||
| )) | ||
| } else { | ||
| Ok(None) | ||
| } | ||
| } |
There was a problem hiding this comment.
We need to consider Cancel-Safety here because next_message_slice() isn't cancel safe (Comment below provides more descriptions)
This change includes: