Repository: knurling-rs/defmt
Pull request #228 (2020-11-04 16:52:52)

Derive and Decoder fixes around bools
Fixes https://github.com/knurling-rs/defmt/issues/208

This makes our bool compression scheme match the documentation in the book – previously we've flushed the compression after every `Format` impl, which wastes bandwidth.


Patch:
diff --git a/decoder/src/lib.rs b/decoder/src/lib.rs
index 88c2324..df20291 100644
--- a/decoder/src/lib.rs
+++ b/decoder/src/lib.rs
@@ -1,5 +1,5 @@
 // NOTE: always runs on the host
-#![cfg(feature="unstable")]
+#![cfg(feature = "unstable")]
 #![cfg_attr(docsrs, feature(doc_cfg))]
 #![cfg_attr(docsrs, doc(cfg(unstable)))]
 
@@ -19,7 +19,7 @@ use byteorder::{ReadBytesExt, LE};
 use colored::Colorize;
 
 pub use defmt_parser::Level;
-use defmt_parser::{Fragment, Parameter, Type, get_max_bitfield_range};
+use defmt_parser::{get_max_bitfield_range, Fragment, Parameter, Type};
 
 include!(concat!(env!("OUT_DIR"), "/version.rs"));
 
@@ -359,6 +359,10 @@ pub fn decode<'t>(
         below_enum: false,
     };
     let args = decoder.decode_format(format)?;
+    if !decoder.bools_tbd.is_empty() {
+        // Flush end of compression block.
+        decoder.read_and_unpack_bools()?;
+    }
 
     let frame = Frame {
         level,
@@ -381,11 +385,7 @@ fn merge_bitfields(params: &mut Vec<Parameter>) {
 
     let mut merged_bitfields = Vec::new();
 
-    let max_index: usize = *params
-        .iter()
-        .map(|param| &param.index)
-        .max()
-        .unwrap();
+    let max_index: usize = *params.iter().map(|param| &param.index).max().unwrap();
 
     for index in 0..=max_index {
         let mut bitfields_with_index = params
@@ -778,11 +778,6 @@ impl<'t, 'b> Decoder<'t, 'b> {
             }
         }
 
-        if self.bools_tbd.len() > 0 {
-            // flush end of compression block
-            self.read_and_unpack_bools()?;
-        }
-
         Ok(args)
     }
 }
@@ -857,10 +852,10 @@ fn zigzag_decode(unsigned: u64) -> i64 {
 
 #[cfg(test)]
 mod tests {
-    use std::collections::BTreeMap;
-    use super::{Frame, Level, Table};
     use super::*;
-    use crate::{Arg, merge_bitfields};
+    use super::{Frame, Level, Table};
+    use crate::{merge_bitfields, Arg};
+    use std::collections::BTreeMap;
 
     // helper function to initiate decoding and assert that the result is as expected.
     //
diff --git a/firmware/qemu/src/bin/log.out b/firmware/qemu/src/bin/log.out
index 56cd299..06f276d 100644
--- a/firmware/qemu/src/bin/log.out
+++ b/firmware/qemu/src/bin/log.out
@@ -87,3 +87,5 @@
 0.000086 INFO [(u32,u32);4]: [(1, 2), (3, 4), (5, 6), (7, 8)]
 0.000087 INFO 1-variant enum: A { fld: 123 }
 0.000088 INFO wrapped: A(A { fld: 200 })
+0.000089 INFO (A(true), B(true)), (A(false), B(true)), (A(true), B(false))
+0.000090 INFO true, [1, 2]: DhcpReprMin { broadcast: true, a: [1, 2] }
diff --git a/firmware/qemu/src/bin/log.release.out b/firmware/qemu/src/bin/log.release.out
index 9949e32..ae1b56b 100644
--- a/firmware/qemu/src/bin/log.release.out
+++ b/firmware/qemu/src/bin/log.release.out
@@ -85,3 +85,5 @@
 0.000084 INFO [(u32,u32);4]: [(1, 2), (3, 4), (5, 6), (7, 8)]
 0.000085 INFO 1-variant enum: A { fld: 123 }
 0.000086 INFO wrapped: A(A { fld: 200 })
+0.000087 INFO (A(true), B(true)), (A(false), B(true)), (A(true), B(false))
+0.000088 INFO true, [1, 2]: DhcpReprMin { broadcast: true, a: [1, 2] }
diff --git a/firmware/qemu/src/bin/log.rs b/firmware/qemu/src/bin/log.rs
index 8ff6cb2..f05413f 100644
--- a/firmware/qemu/src/bin/log.rs
+++ b/firmware/qemu/src/bin/log.rs
@@ -386,6 +386,40 @@ fn main() -> ! {
         defmt::info!("wrapped: {:?}", Wrap::A(Single::A { fld: 200 }));
     }
 
+    {
+        // check that bools are compressed per *log frame*, not per `Format` impl
+
+        #[derive(Format)]
+        struct A(bool);
+
+        #[derive(Format)]
+        struct B(bool);
+
+        defmt::info!(
+            "{:?}, {:?}, {:?}",
+            (A(true), B(true)),
+            (A(false), B(true)),
+            (A(true), B(false))
+        );
+    }
+
+    {
+        // issue #208
+
+        #[derive(Format)]
+        pub struct DhcpReprMin {
+            pub broadcast: bool,
+            pub a: [u8; 2],
+        }
+
+        let dhcp_repr = DhcpReprMin {
+            broadcast: true,
+            a: [1, 2],
+        };
+
+        defmt::info!("true, [1, 2]: {:?}", dhcp_repr);
+    }
+
     loop {
         debug::exit(debug::EXIT_SUCCESS)
     }
diff --git a/macros/src/lib.rs b/macros/src/lib.rs
index 1d2d97b..8b0b9a4 100644
--- a/macros/src/lib.rs
+++ b/macros/src/lib.rs
@@ -14,8 +14,8 @@ use syn::{
     parse_macro_input,
     punctuated::Punctuated,
     spanned::Spanned as _,
-    Data, DeriveInput, Expr, Fields, FieldsNamed, FieldsUnnamed, ItemFn, ItemStruct, LitInt,
-    LitStr, ReturnType, Token, Type, WherePredicate,
+    Data, DeriveInput, Expr, Fields, FieldsNamed, FieldsUnnamed, ItemFn, ItemStruct, LitStr,
+    ReturnType, Token, Type, WherePredicate,
 };
 
 #[proc_macro_attribute]
@@ -193,15 +193,9 @@ pub fn format(ts: TokenStream) -> TokenStream {
                     }
                     fs.push_str(&vident.to_string());
 
-                    let mut pats = quote!();
-                    let exprs = fields(
-                        &var.fields,
-                        &mut fs,
-                        &mut field_types,
-                        Kind::Enum {
-                            patterns: &mut pats,
-                        },
-                    );
+                    let mut pats = vec![];
+                    let exprs = fields(&var.fields, &mut fs, &mut field_types, &mut pats);
+                    let pats = quote!( { #(#pats),* } );
 
                     let encode_discriminant = if de.variants.len() == 1 {
                         // For single-variant enums, there is no need to encode the discriminant.
@@ -215,6 +209,11 @@ pub fn format(ts: TokenStream) -> TokenStream {
                     arms.push(quote!(
                         #ident::#vident #pats => {
                             #encode_discriminant
+
+                            // When descending into an enum variant, force all discriminants to be
+                            // encoded. This is required when encoding arrays like `[None, Some(x)]`
+                            // with `{:?}`, since the format string of `x` won't appear for the
+                            // first element.
                             f.with_tag(|f| {
                                 #(#exprs;)*
                             });
@@ -236,9 +235,20 @@ pub fn format(ts: TokenStream) -> TokenStream {
 
         Data::Struct(ds) => {
             fs = ident.to_string();
-            let args = fields(&ds.fields, &mut fs, &mut field_types, Kind::Struct);
-            // FIXME expand this `write!` and conditionally omit the tag (string index)
-            exprs.push(quote!(defmt::export::write!(f, #fs #(,#args)*);))
+            let mut pats = vec![];
+            let args = fields(&ds.fields, &mut fs, &mut field_types, &mut pats);
+
+            let sym = mksym(&fs, "fmt", false);
+            exprs.push(quote!(
+                if f.needs_tag() {
+                    f.istr(&defmt::export::istr(#sym));
+                }
+            ));
+            exprs.push(quote!(match self {
+                Self { #(#pats),* } => {
+                    #(#args;)*
+                }
+            }));
         }
 
         Data::Union(..) => {
@@ -272,17 +282,12 @@ pub fn format(ts: TokenStream) -> TokenStream {
     .into()
 }
 
-enum Kind<'p> {
-    Struct,
-    Enum { patterns: &'p mut TokenStream2 },
-}
-
 fn fields(
     fields: &Fields,
     format: &mut String,
     // collect all *non-native* types that appear as fields
     field_types: &mut Vec<Type>,
-    mut kind: Kind,
+    pats: &mut Vec<TokenStream2>,
 ) -> Vec<TokenStream2> {
     let mut list = vec![];
     match fields {
@@ -301,7 +306,6 @@ fn fields(
                     format.push_str("(");
                 }
                 let mut first = true;
-                let mut pats = vec![];
                 for (i, f) in fs.iter().enumerate() {
                     if first {
                         first = false;
@@ -315,40 +319,28 @@ fn fields(
                     if let Some(ident) = f.ident.as_ref() {
                         core::write!(format, "{}: {{:{}}}", ident, ty).ok();
 
-                        match &kind {
-                            Kind::Struct => {
-                                list.push(quote!(self.#ident));
-                            }
-                            Kind::Enum { .. } => {
-                                if ty == "?" {
-                                    list.push(quote!(f.fmt(#ident, false)));
-                                } else {
-                                    let method = format_ident!("{}", ty);
-                                    list.push(quote!(f.#method(#ident)));
-                                }
-                                pats.push(ident.clone());
-                            }
+                        if ty == "?" {
+                            list.push(quote!(f.fmt(#ident, false)));
+                        } else {
+                            let method = format_ident!("{}", ty);
+                            list.push(quote!(f.#method(#ident)));
                         }
+                        pats.push(quote!( #ident ));
                     } else {
+                        // Unnamed (tuple) field.
+
                         core::write!(format, "{{:{}}}", ty).ok();
 
-                        match &kind {
-                            Kind::Struct => {
-                                let ident = LitInt::new(&i.to_string(), Span2::call_site());
-                                list.push(quote!(self.#ident));
-                            }
-                            Kind::Enum { .. } => {
-                                let ident = format_ident!("arg{}", i);
-                                if ty == "?" {
-                                    list.push(quote!(f.fmt(#ident, false)));
-                                } else {
-                                    let method = format_ident!("{}", ty);
-                                    list.push(quote!(f.#method(#ident)));
-                                }
-
-                                pats.push(ident);
-                            }
+                        let ident = format_ident!("arg{}", i);
+                        if ty == "?" {
+                            list.push(quote!(f.fmt(#ident, false)));
+                        } else {
+                            let method = format_ident!("{}", ty);
+                            list.push(quote!(f.#method(#ident)));
                         }
+
+                        let i = syn::Index::from(i);
+                        pats.push(quote!( #i: #ident ));
                     }
                 }
                 if named {
@@ -356,14 +348,6 @@ fn fields(
                 } else {
                     format.push_str(")");
                 }
-
-                if let Kind::Enum { patterns } = &mut kind {
-                    if named {
-                        **patterns = quote!({ #(#pats),* })
-                    } else {
-                        **patterns = quote!((#(#pats),*))
-                    }
-                }
             }
         }
 
diff --git a/src/impls.rs b/src/impls.rs
index e3562b6..4ad4803 100644
--- a/src/impls.rs
+++ b/src/impls.rs
@@ -171,7 +171,7 @@ impl Format for bool {
             let t = internp!("{:bool}");
             fmt.u8(&t);
         }
-        fmt.u8(&(*self as u8));
+        fmt.bool(self);
     }
 }
 


Test patch:
diff --git a/tests/encode.rs b/tests/encode.rs
index 84bbe1a..9378162 100644
--- a/tests/encode.rs
+++ b/tests/encode.rs
@@ -54,6 +54,7 @@ fn inc(index: u8, n: u8) -> u8 {
 fn check_format_implementation(val: &(impl Format + ?Sized), expected_encoding: &[u8]) {
     let mut f = Formatter::new();
     val.format(&mut f);
+    f.finalize();
     assert_eq!(f.bytes(), expected_encoding);
 }
 
@@ -550,6 +551,21 @@ fn slice_of_usize() {
     )
 }
 
+#[test]
+fn slice_of_bools() {
+    let index = fetch_string_index();
+    let val: &[bool] = &[true, true, false];
+    check_format_implementation(
+        val,
+        &[
+            index,           // "{:[?]}"
+            val.len() as u8, // length
+            inc(index, 1),   // "{:bool}"
+            0b110,           // compressed bools: true, true, false
+        ],
+    )
+}
+
 #[test]
 fn format_primitives() {
     let index = fetch_string_index();
@@ -892,3 +908,50 @@ fn derive_with_bounds() {
         ],
     );
 }
+
+#[test]
+fn format_bools() {
+    #[derive(Format)]
+    struct A(bool);
+
+    #[derive(Format)]
+    struct B(bool);
+
+    let index = fetch_string_index();
+    check_format_implementation(
+        &(A(true), B(true)),
+        &[
+            index,         // "({:?}, {:?})"
+            inc(index, 1), // "A({:bool})"
+            inc(index, 2), // "B({:bool})"
+            0b11,          // compressed bools
+        ],
+    );
+}
+
+#[test]
+fn issue_208() {
+    #[derive(Format)]
+    struct DhcpReprMin {
+        pub broadcast: bool,
+        pub a: [u8; 2],
+    }
+
+    let dhcp_repr = DhcpReprMin {
+        broadcast: true,
+        a: [10, 10],
+    };
+
+    let index = fetch_string_index();
+    check_format_implementation(
+        &dhcp_repr,
+        &[
+            index,         // "DhcpReprMin {{ broadcast: {:bool}, a: {:?} }}"
+            inc(index, 1), // "{:[?;2]}"
+            inc(index, 2), // "{:u8}"
+            10,            // a[0]
+            10,            // a[1]
+            1,             // compressed bools
+        ],
+    );
+}