Skip to content
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 pub struct Formatter to defmt_decoder::log. #781

Merged
merged 15 commits into from
Oct 4, 2023
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

- [#781]: `defmt-decoder`: Add `pub struct Formatter` to `defmt_decoder::log`
- [#778]: `defmt-decoder`: Add support for nested log formatting
- [#777]: `defmt-decoder`: Simplify StdoutLogger
- [#775]: `defmt-decoder`: Ignore AArch64 mapping symbols
- [#771]: `defmt-macros`: Ignore empty items in DEFMT_LOG
- [#769]: `defmt-decoder`: Add support for color, style, width and alignment to format

[#781]: https://github.com/knurling-rs/defmt/pull/781
[#778]: https://github.com/knurling-rs/defmt/pull/778
[#777]: https://github.com/knurling-rs/defmt/pull/777
[#775]: https://github.com/knurling-rs/defmt/pull/775
Expand Down
12 changes: 7 additions & 5 deletions decoder/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,15 @@ use std::{
};

use byteorder::{ReadBytesExt, LE};
use decoder::Decoder;
use defmt_parser::Level;
use elf2table::parse_impl;

pub use elf2table::{Location, Locations};
pub use frame::Frame;
pub use stream::StreamDecoder;
use crate::{decoder::Decoder, elf2table::parse_impl};

pub use crate::{
elf2table::{Location, Locations},
frame::Frame,
stream::StreamDecoder,
};

/// Specifies the origin of a format string
#[derive(PartialEq, Eq, Debug)]
Expand Down
98 changes: 83 additions & 15 deletions decoder/src/log/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,16 @@ mod format;
mod json_logger;
mod stdout_logger;

use std::fmt;

use log::{Level, LevelFilter, Log, Metadata, Record};
use serde::{Deserialize, Serialize};

use std::fmt;

use self::{json_logger::JsonLogger, stdout_logger::StdoutLogger};
use self::{
format::{LogMetadata, LogSegment},
json_logger::JsonLogger,
stdout_logger::{Printer, StdoutLogger},
};
use crate::Frame;

const DEFMT_TARGET_MARKER: &str = "defmt@";
Expand All @@ -27,18 +31,7 @@ pub fn log_defmt(
line: Option<u32>,
module_path: Option<&str>,
) {
let timestamp = frame
.display_timestamp()
.map(|ts| ts.to_string())
.unwrap_or_default();

let level = frame.level().map(|level| match level {
crate::Level::Trace => Level::Trace,
crate::Level::Debug => Level::Debug,
crate::Level::Info => Level::Info,
crate::Level::Warn => Level::Warn,
crate::Level::Error => Level::Error,
});
let (timestamp, level) = timestamp_and_level_from_frame(frame);

let target = format!(
"{}{}",
Expand Down Expand Up @@ -170,3 +163,78 @@ impl DefmtLoggerInfo {
self.has_timestamp
}
}

/// Format [`Frame`]s according to a `log_format`.
///
/// The `log_format` makes it possible to customize the defmt output.
///
/// The `log_format` is specified here: TODO
// TODO:
// - use two Formatter in StdoutLogger instead of the log format
// - add fn format_to_sink
// - specify log format
#[derive(Debug)]
pub struct Formatter {
format: Vec<LogSegment>,
}

impl Formatter {
pub fn new(log_format: &str) -> Self {
let format = format::parse(log_format)
.unwrap_or_else(|_| panic!("log format is invalid '{log_format}'"));
Urhengulas marked this conversation as resolved.
Show resolved Hide resolved
Self { format }
}

pub fn format_to_string(
&self,
frame: Frame<'_>,
file: Option<&str>,
line: Option<u32>,
module_path: Option<&str>,
) -> String {
let (timestamp, level) = timestamp_and_level_from_frame(&frame);

#[allow(clippy::match_single_binding)]
match format_args!("{}", frame.display_message()) {
args => {
let log_record = &Record::builder()
.args(args)
.module_path(module_path)
.file(file)
.line(line)
.build();

let record = DefmtRecord {
log_record,
payload: Payload { level, timestamp },
};

match level {
Some(_) => Printer::new_defmt(&record, &self.format),
Urhengulas marked this conversation as resolved.
Show resolved Hide resolved
None => {
// handle defmt::println separately
const RAW_FORMAT: &[LogSegment] = &[LogSegment::new(LogMetadata::Log)];
Printer::new_defmt(&record, RAW_FORMAT)
}
}
.format_frame()
.unwrap()
}
}
}
}

fn timestamp_and_level_from_frame(frame: &Frame<'_>) -> (String, Option<Level>) {
let timestamp = frame
.display_timestamp()
.map(|ts| ts.to_string())
.unwrap_or_default();
let level = frame.level().map(|level| match level {
crate::Level::Trace => Level::Trace,
crate::Level::Debug => Level::Debug,
crate::Level::Info => Level::Info,
crate::Level::Warn => Level::Warn,
crate::Level::Error => Level::Error,
});
(timestamp, level)
}
54 changes: 37 additions & 17 deletions decoder/src/log/stdout_logger.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,50 +131,70 @@ impl StdoutLogger {
}

/// Printer for `DefmtRecord`s.
struct Printer<'a> {
pub(super) struct Printer<'a> {
record: Record<'a>,
format: &'a [LogSegment],
min_timestamp_width: usize,
}

impl<'a> Printer<'a> {
pub fn new(record: Record<'a>, format: &'a [LogSegment]) -> Self {
fn new(record: Record<'a>, format: &'a [LogSegment]) -> Self {
Self {
record,
format,
min_timestamp_width: 0,
}
}

pub fn new_defmt(record: &'a DefmtRecord<'a>, format: &'a [LogSegment]) -> Self {
Urhengulas marked this conversation as resolved.
Show resolved Hide resolved
Self {
record: Record::Defmt(record),
format,
min_timestamp_width: 0,
}
}

/// Pads the defmt timestamp to take up at least the given number of characters.
/// TODO: Remove this, shouldn't be needed now that we have width field support
pub fn min_timestamp_width(&mut self, min_timestamp_width: usize) -> &mut Self {
fn min_timestamp_width(&mut self, min_timestamp_width: usize) -> &mut Self {
self.min_timestamp_width = min_timestamp_width;
self
}

/// Prints the formatted log frame to `sink`.
pub fn print_frame<W: io::Write>(&self, sink: &mut W) -> io::Result<()> {
for segment in self.format {
let s = match &segment.metadata {
LogMetadata::String(s) => s.to_string(),
LogMetadata::Timestamp => self.build_timestamp(&segment.format),
LogMetadata::FileName => self.build_file_name(&segment.format),
LogMetadata::FilePath => self.build_file_path(&segment.format),
LogMetadata::ModulePath => self.build_module_path(&segment.format),
LogMetadata::LineNumber => self.build_line_number(&segment.format),
LogMetadata::LogLevel => self.build_log_level(&segment.format),
LogMetadata::Log => self.build_log(&segment.format),
LogMetadata::NestedLogSegments(segments) => {
self.build_nested(segments, &segment.format)
}
};

let s = self.build_segment(segment);
write!(sink, "{s}")?;
}
writeln!(sink)
}

pub(super) fn format_frame(&self) -> Result<String, std::fmt::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to move all of the formatting logic to Formatter and pass a Formatter instead to Printer::new(). What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about that. But the Printer holds a reference to the record aka. the log frame. So you basically have one Printer per log frame, which makes sense because it uses this reference in almost all methods.

The Formatter should not keep a reference to the record, but just receive it for the method call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name Printer does not really make sense anymore. I think SingleRecordFormatter would be the most correct, but maybe a bit bulky. But I'd like to pull the naming discussion out of this PR.

Copy link
Member Author

@Urhengulas Urhengulas Oct 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I have written as a TODO in code, I think the StdoutLogger should use two Formatters instead of just storing two log_formats. So the hierachy would be:

graph TD
    StdoutLogger --> Formatter
    Formatter --> Printer
Loading

But I'd also like to do that in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name Printer does not really make sense anymore. I think SingleRecordFormatter would be the most correct, but maybe a bit bulky. But I'd like to pull the naming discussion out of this PR.

I disagree, the purpose of Printer after this PR should not be to format, but to print. The only reason why SingleRecordFormatter would make sense after this PR, is because you added format_frame to Printer. I think format_frame should be the responsibility of Formatter.

The Formatter should not keep a reference to the record, but just receive it for the method call.

I agree, then in print_frame the Printer would just need to call

write!(sink, "{}", self.formatter.format_frame(&self.record)):

I think the StdoutLogger should use two Formatters instead of just storing two log_formats.

Yeah, I think that makes sense.

let mut sink = String::new();
for segment in self.format {
let s = self.build_segment(segment);
write!(sink, "{s}")?;
}
Ok(sink)
}

fn build_segment(&self, segment: &LogSegment) -> String {
match &segment.metadata {
LogMetadata::String(s) => s.to_string(),
LogMetadata::Timestamp => self.build_timestamp(&segment.format),
LogMetadata::FileName => self.build_file_name(&segment.format),
LogMetadata::FilePath => self.build_file_path(&segment.format),
LogMetadata::ModulePath => self.build_module_path(&segment.format),
LogMetadata::LineNumber => self.build_line_number(&segment.format),
LogMetadata::LogLevel => self.build_log_level(&segment.format),
LogMetadata::Log => self.build_log(&segment.format),
LogMetadata::NestedLogSegments(segments) => {
self.build_nested(segments, &segment.format)
}
}
}

fn build_nested(&self, segments: &[LogSegment], format: &LogFormat) -> String {
let mut result = String::new();
for segment in segments {
Expand Down