feat: Added initial support for inspecting and converting a trace#39
feat: Added initial support for inspecting and converting a trace#39
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial CLI support for inspecting Z-Wave ZLF traces and converting them to JSON, alongside expanding ZLF record classification to recognize attachment records.
Changes:
- Add an
InspectCLI subcommand to summarize ZLF contents (attachments/data/other). - Add a
ConvertCLI path for converting.zlftraces to pretty-printed JSON (viaserde_json), includingserdederives on core frame types. - Extend ZLF parsing with an
AttachmentAPI type / record variant.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/core/src/zlf/types.rs | Adds Attachment API type; changes API type decoding behavior. |
| crates/core/src/zlf/reader.rs | Introduces ZlfRecord::Attachment and routes attachment records during parsing. |
| crates/core/src/types.rs | Adds Serialize/Deserialize derives to enable JSON export of Frame and Region. |
| crates/cli/src/main.rs | Adds inspect command and implements .zlf → .json conversion. |
| crates/cli/src/generator/generator.rs | Skips non-frame records (e.g., attachments). |
| crates/cli/Cargo.toml | Adds serde_json dependency for JSON output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let input_extension = std::path::Path::new(input) | ||
| .extension() | ||
| .and_then(std::ffi::OsStr::to_str) | ||
| .unwrap_or("") | ||
| .to_lowercase(); |
There was a problem hiding this comment.
Path::new takes a reference; input here is a String, so Path::new(input) won’t compile. Use Path::new(&input) (or Path::new(input.as_str())) when extracting the extension.
| let output_extension = std::path::Path::new(output) | ||
| .extension() | ||
| .and_then(std::ffi::OsStr::to_str) | ||
| .unwrap_or("") | ||
| .to_lowercase(); |
There was a problem hiding this comment.
Same issue as above for output: Path::new(output) expects a reference, so this won’t compile as written. Use Path::new(&output) (or output.as_str()) before calling .extension().
| zlf::ZlfRecord::Data(data_frame) => { | ||
| data_frame_counter += 1; | ||
| }, | ||
| zlf::ZlfRecord::Other(raw_frame) => { |
There was a problem hiding this comment.
data_frame and raw_frame are bound but never used. If they’re intentionally unused, rename them to _data_frame/_raw_frame (or use _) to avoid unused-variable warnings.
| zlf::ZlfRecord::Data(data_frame) => { | |
| data_frame_counter += 1; | |
| }, | |
| zlf::ZlfRecord::Other(raw_frame) => { | |
| zlf::ZlfRecord::Data(_data_frame) => { | |
| data_frame_counter += 1; | |
| }, | |
| zlf::ZlfRecord::Other(_raw_frame) => { |
| match result { | ||
| zniffer_parser::ParserResult::ValidFrame { frame } => { | ||
|
|
||
| raw_frames.push(frame.clone()); |
There was a problem hiding this comment.
ParserResult::ValidFrame { frame } already yields an owned Frame (the parser clones internally before returning), so raw_frames.push(frame.clone()) does an extra clone. Push frame directly to avoid unnecessary allocations/copies when converting large traces.
| raw_frames.push(frame.clone()); | |
| raw_frames.push(frame); |
| impl From<u8> for ApiType { | ||
| fn from(value: u8) -> Self { | ||
| match value { | ||
| 0xF5 => ApiType::Pti, | ||
| 0xF8 => ApiType::Attachment, | ||
| 0xFE => ApiType::Zniffer, | ||
| _ => ApiType::Unknown(value), | ||
| _ => panic!("Unknown API type: 0x{:02X}", value), | ||
| } | ||
| } |
There was a problem hiding this comment.
ApiType::from(u8) panics on unknown values. Since this parser reads potentially malformed/untrusted trace files, panicking will crash callers and also prevents returning ZlfError::InvalidApiTypeField from the reader. Consider making this a fallible conversion (TryFrom<u8> returning a custom error) or reintroducing an Unknown(u8) variant so unknown values can be handled without aborting the process.
| let mut api_type = [0u8; 1]; | ||
| self.r.read_exact(&mut api_type)?; | ||
| match ApiType::from(api_type[0]) { | ||
| ApiType::Unknown(_) => { return Err(ZlfError::InvalidApiTypeField(api_type[0])); }, | ||
| _ => { }, | ||
| ApiType::Attachment => { return Ok(Some(ZlfRecord::Attachment)); }, | ||
| ApiType::Pti | ApiType::Zniffer => { | ||
| // TODO: Do we need the frame type? | ||
| let frame_type = FrameType::Data; | ||
| Ok(Some(ZlfRecord::Other(RawFrame { timestamp: 0, sof: 0, frame_type, payload }))) | ||
| }, | ||
| _ => { return Err(ZlfError::InvalidApiTypeField(api_type[0])); }, | ||
| } |
There was a problem hiding this comment.
This match includes an _ arm returning InvalidApiTypeField, but ApiType::from(api_type[0]) currently panics for unknown values, so this error path is unreachable (and the _ arm is also unreachable because all enum variants are already covered). Use a fallible API type decode (e.g., TryFrom<u8>) and map failures to ZlfError::InvalidApiTypeField so invalid inputs don’t crash the process.
| self.frame_counter += 1; | ||
|
|
||
| let mut api_type = [0u8; 1]; | ||
| self.r.read_exact(&mut api_type)?; | ||
| match ApiType::from(api_type[0]) { |
There was a problem hiding this comment.
frame_counter is incremented before reading/validating the api_type byte. If read_exact(&mut api_type) fails (e.g., EOF/corruption), the counter will still advance even though no record was successfully parsed. Increment the counter only after the full record (including api_type) has been read and classified.
No description provided.