Repository: knurling-rs/defmt
Pull request #184 (2020-10-08 15:41:35)

enhance bitfield compression
resolves https://github.com/knurling-rs/defmt/issues/101 without the middle bytes compression part.

imo the code is pretty wonky in parts, so I'd be grateful for any feedback on how to make it more idiomatic (I've marked some of the weird parts with `TODO`). The bitfield deduplication in `prepare_params()` has been the biggest fight and I'm not really happy with the tangled index/tracking vars I ended up with.


Patch:
diff --git a/book/src/ser-bitfield.md b/book/src/ser-bitfield.md
index dec64e0..e421f08 100644
--- a/book/src/ser-bitfield.md
+++ b/book/src/ser-bitfield.md
@@ -9,3 +9,13 @@ defmt::error!("l: {0:0..8}, m: {0:8..12}, h: {0:12..16}", x /*: u16*/);
 // on the wire: [1, 1, 2]
 //  string index ^  ^^^^ `u16::to_le_bytes(x)`
 ```
+
+Leading or trailing bytes that are not needed to display a bitfield are removed during serialization:
+
+``` rust
+# extern crate defmt;
+defmt::error!("m: {0:8..12}", 0b0110_0011_0000_1111_u32);
+// on the wire: [1, 0b0110_0011]
+//  string index ^  ^^^^^^^^^^ argument truncated into u8:
+//                             leading and trailing byte are irrelevant
+```
diff --git a/decoder/src/lib.rs b/decoder/src/lib.rs
index b38cbbb..558f4e1 100644
--- a/decoder/src/lib.rs
+++ b/decoder/src/lib.rs
@@ -5,6 +5,7 @@
 
 use core::fmt::{self, Write as _};
 use std::cmp::Ordering;
+use core::ops::Range;
 use std::collections::BTreeMap;
 use std::{
     error::Error,
@@ -18,7 +19,7 @@ use std::{
 use byteorder::{ReadBytesExt, LE};
 use colored::Colorize;
 
-use defmt_parser::{Fragment, Level, Type};
+use defmt_parser::{Fragment, Level, Parameter, Type, get_max_bitfield_range};
 
 include!(concat!(env!("OUT_DIR"), "/version.rs"));
 
@@ -371,6 +372,66 @@ pub fn decode<'t>(
     Ok((frame, consumed))
 }
 
+/// Note that this will not change the Bitfield params in place, i.e. if `params` was sorted before
+/// a call to this function, it won't be afterwards.
+fn merge_bitfields(params: &mut Vec<Parameter>) {
+    if params.len() == 0 {
+        return;
+    }
+
+    let mut merged_bitfields = Vec::new();
+
+    let max_index: usize = *params
+        .iter()
+        .map(|param| &param.index)
+        .max()
+        .unwrap();
+
+    for index in 0..=max_index {
+        let mut bitfields_with_index = params
+            .iter()
+            .filter(|param| match (param.index, &param.ty) {
+                (i, Type::BitField(_)) if i == index => true,
+                _ => false,
+            })
+            .peekable();
+
+        if bitfields_with_index.peek().is_some() {
+            let (smallest, largest) = get_max_bitfield_range(bitfields_with_index).unwrap();
+
+            // create new merged bitfield for this index
+            merged_bitfields.push(Parameter {
+                index: index,
+                ty: Type::BitField(Range {
+                    start: smallest,
+                    end: largest,
+                }),
+            });
+
+            // remove old bitfields with this index
+            // TODO refactor when `drain_filter()` is stable
+            let mut i = 0;
+            while i != params.len() {
+                match &params[i].ty {
+                    Type::BitField(_) => {
+                        if params[i].index == index {
+                            params.remove(i);
+                        } else {
+                            i += 1; // we haven't removed a bitfield -> move i forward
+                        }
+                    }
+                    _ => {
+                        i += 1; // we haven't removed a bitfield -> move i forward
+                    }
+                }
+            }
+        }
+    }
+
+    // add merged bitfields to unsorted params
+    params.append(&mut merged_bitfields);
+}
+
 struct Decoder<'t, 'b> {
     table: &'t Table,
     bytes: &'b [u8],
@@ -403,6 +464,16 @@ impl<'t, 'b> Decoder<'t, 'b> {
         Ok(())
     }
 
+    /// Sort and deduplicate `params` so that they can be interpreted correctly during decoding
+    fn prepare_params(&self, params: &mut Vec<Parameter>) {
+        // deduplicate bitfields by merging them by index
+        merge_bitfields(params);
+
+        // sort & dedup to ensure that format string args can be addressed by index too
+        params.sort_by(|a, b| a.index.cmp(&b.index));
+        params.dedup_by(|a, b| a.index == b.index);
+    }
+
     /// Gets a format string from
     /// - the `FormatList`, if it's in `Use` mode, or
     /// - from `bytes` and `table` if the `FormatList` is in `Build` mode or was not provided
@@ -452,33 +523,7 @@ impl<'t, 'b> Decoder<'t, 'b> {
             })
             .collect::<Vec<_>>();
 
-        // sort & dedup to ensure that format string args can be addressed by index too
-        params.sort_by(|a, b| {
-            if a.index == b.index {
-                match (&a.ty, &b.ty) {
-                    (Type::BitField(a_range), Type::BitField(b_range)) => {
-                        b_range.end.cmp(&a_range.end)
-                    }
-                    _ => Ordering::Equal,
-                }
-            } else {
-                a.index.cmp(&b.index)
-            }
-        });
-
-        params.dedup_by(|a, b| {
-            if a.index == b.index {
-                match (&a.ty, &b.ty) {
-                    (Type::BitField(a_range), Type::BitField(b_range)) => a_range.end < b_range.end,
-                    /* reusing an arg for bitfield- and non bitfield params is not allowed */
-                    (Type::BitField(_), _) => unreachable!(),
-                    (_, Type::BitField(_)) => unreachable!(),
-                    _ => true,
-                }
-            } else {
-                false
-            }
-        });
+        self.prepare_params(&mut params);
 
         for param in &params {
             match &param.ty {
@@ -652,19 +697,23 @@ impl<'t, 'b> Decoder<'t, 'b> {
                     args.push(Arg::F32(f32::from_bits(data)));
                 }
                 Type::BitField(range) => {
-                    let data: u64;
-
-                    match range.end {
-                        0..=8 => {
+                    let mut data: u64;
+                    let lowest_byte = range.start / 8;
+                    // -1 because `range` is range-exclusive
+                    let highest_byte = (range.end - 1) / 8;
+                    let size_after_truncation = highest_byte - lowest_byte + 1; // in octets
+
+                    match size_after_truncation {
+                        1 => {
                             data = self.bytes.read_u8()? as u64;
                         }
-                        0..=16 => {
+                        2 => {
                             data = self.bytes.read_u16::<LE>()? as u64;
                         }
-                        0..=24 => {
+                        3 => {
                             data = self.bytes.read_u24::<LE>()? as u64;
                         }
-                        0..=32 => {
+                        4 => {
                             data = self.bytes.read_u32::<LE>()? as u64;
                         }
                         _ => {
@@ -672,6 +721,8 @@ impl<'t, 'b> Decoder<'t, 'b> {
                         }
                     }
 
+                    data <<= lowest_byte * 8;
+
                     args.push(Arg::Uxx(data));
                 }
                 Type::Str => {
@@ -802,7 +853,7 @@ mod tests {
     use std::collections::BTreeMap;
     use super::{Frame, Level, Table};
     use super::*;
-    use crate::Arg;
+    use crate::{Arg, merge_bitfields};
 
     // helper function to initiate decoding and assert that the result is as expected.
     //
@@ -1266,6 +1317,23 @@ mod tests {
         );
     }
 
+    #[test]
+    fn bitfields_mixed() {
+        let bytes = [
+            0, // index
+            2, // timestamp
+            0b1111_0000,
+            0b1110_0101, // u16 bitfields
+            42,          // u8
+            0b1111_0001, // u8 bitfields
+        ];
+        decode_and_expect(
+            "#0: {0:7..12}, #1: {1:u8}, #2: {2:0..5}",
+            &bytes,
+            "0.000002 INFO #0: 0b1011, #1: 42, #2: 0b10001",
+        );
+    }
+
     #[test]
     fn bitfields_across_boundaries() {
         let bytes = [
@@ -1281,6 +1349,53 @@ mod tests {
         );
     }
 
+    #[test]
+    fn bitfields_across_boundaries_diff_indices() {
+        let bytes = [
+            0, // index
+            2, // timestamp
+            0b1101_0010,
+            0b0110_0011, // u16
+            0b1111_1111, // truncated u16
+        ];
+        decode_and_expect(
+            "bitfields {0:0..7} {0:9..14} {1:8..10}",
+            &bytes,
+            "0.000002 INFO bitfields 0b1010010 0b10001 0b11",
+        );
+    }
+
+    #[test]
+    fn bitfields_truncated_front() {
+        let bytes = [
+            0,           // index
+            2,           // timestamp
+            0b0110_0011, // truncated(!) u16
+        ];
+        decode_and_expect(
+            "bitfields {0:9..14}",
+            &bytes,
+            "0.000002 INFO bitfields 0b10001",
+        );
+    }
+
+    #[test]
+    fn bitfields_non_truncated_u32() {
+        let bytes = [
+            0,           // index
+            2,           // timestamp
+            0b0110_0011, // -
+            0b0000_1111, //  |
+            0b0101_1010, //  | u32
+            0b1100_0011, // -
+        ];
+        decode_and_expect(
+            "bitfields {0:0..2} {0:28..31}",
+            &bytes,
+            "0.000002 INFO bitfields 0b11 0b100",
+        );
+    }
+
     #[test]
     fn slice() {
         let bytes = [
@@ -1377,4 +1492,125 @@ mod tests {
         let frame = super::decode(&bytes, &table).unwrap().0;
         assert_eq!(frame.display(false).to_string(), "0.000001 INFO x=None");
     }
+
+    #[test]
+    fn merge_bitfields_simple() {
+        let mut params = vec![
+            Parameter {
+                index: 0,
+                ty: Type::BitField(0..3),
+            },
+            Parameter {
+                index: 0,
+                ty: Type::BitField(4..7),
+            },
+        ];
+
+        merge_bitfields(&mut params);
+        assert_eq!(
+            params,
+            vec![Parameter {
+                index: 0,
+                ty: Type::BitField(0..7)
+            }]
+        );
+    }
+
+    #[test]
+    fn merge_bitfields_overlap() {
+        let mut params = vec![
+            Parameter {
+                index: 0,
+                ty: Type::BitField(1..3),
+            },
+            Parameter {
+                index: 0,
+                ty: Type::BitField(2..5),
+            },
+        ];
+
+        merge_bitfields(&mut params);
+        assert_eq!(
+            params,
+            vec![Parameter {
+                index: 0,
+                ty: Type::BitField(1..5)
+            }]
+        );
+    }
+
+    #[test]
+    fn merge_bitfields_multiple_indices() {
+        let mut params = vec![
+            Parameter {
+                index: 0,
+                ty: Type::BitField(0..3),
+            },
+            Parameter {
+                index: 1,
+                ty: Type::BitField(1..3),
+            },
+            Parameter {
+                index: 1,
+                ty: Type::BitField(4..5),
+            },
+        ];
+
+        merge_bitfields(&mut params);
+        assert_eq!(
+            params,
+            vec![
+                Parameter {
+                    index: 0,
+                    ty: Type::BitField(0..3)
+                },
+                Parameter {
+                    index: 1,
+                    ty: Type::BitField(1..5)
+                }
+            ]
+        );
+    }
+
+    #[test]
+    fn merge_bitfields_overlap_non_consecutive_indices() {
+        let mut params = vec![
+            Parameter {
+                index: 0,
+                ty: Type::BitField(0..3),
+            },
+            Parameter {
+                index: 1,
+                ty: Type::U8,
+            },
+            Parameter {
+                index: 2,
+                ty: Type::BitField(1..4),
+            },
+            Parameter {
+                index: 2,
+                ty: Type::BitField(4..5),
+            },
+        ];
+
+        merge_bitfields(&mut params);
+        // note: current implementation appends merged bitfields to the end. this is not a must
+        assert_eq!(
+            params,
+            vec![
+                Parameter {
+                    index: 1,
+                    ty: Type::U8
+                },
+                Parameter {
+                    index: 0,
+                    ty: Type::BitField(0..3)
+                },
+                Parameter {
+                    index: 2,
+                    ty: Type::BitField(1..5)
+                }
+            ]
+        );
+    }
 }
diff --git a/firmware/nrf52/src/bin/exception.rs b/firmware/nrf52/src/bin/exception.rs
index 04729ea..47c8597 100644
--- a/firmware/nrf52/src/bin/exception.rs
+++ b/firmware/nrf52/src/bin/exception.rs
@@ -3,9 +3,9 @@
 
 use core::sync::atomic::{AtomicUsize, Ordering};
 
-use defmt_rtt as _; // <- global logger
 use cortex_m::{asm, peripheral::SCB};
 use cortex_m_rt::{entry, exception};
+use defmt_rtt as _; // <- global logger
 use nrf52840_hal as _; // <- memory layout
 use panic_probe as _; // <- panicking behavior
 
diff --git a/firmware/nrf52/src/bin/hello.rs b/firmware/nrf52/src/bin/hello.rs
index 4e95e58..feb31e7 100644
--- a/firmware/nrf52/src/bin/hello.rs
+++ b/firmware/nrf52/src/bin/hello.rs
@@ -3,9 +3,9 @@
 
 use core::sync::atomic::{AtomicUsize, Ordering};
 
-use defmt_rtt as _; // <- global logger
 use cortex_m::asm;
 use cortex_m_rt::entry;
+use defmt_rtt as _; // <- global logger
 use nrf52840_hal as _; // <- memory layout
 use panic_probe as _; // <- panicking behavior
 
diff --git a/firmware/nrf52/src/bin/log.rs b/firmware/nrf52/src/bin/log.rs
index 34a5f18..813d86c 100644
--- a/firmware/nrf52/src/bin/log.rs
+++ b/firmware/nrf52/src/bin/log.rs
@@ -1,10 +1,10 @@
 #![no_std]
 #![no_main]
 
-use defmt::Format;
-use defmt_rtt as _; // <- global logger
 use cortex_m::asm;
 use cortex_m_rt::entry;
+use defmt::Format;
+use defmt_rtt as _; // <- global logger
 use embedded_hal::timer::CountDown as _;
 use nrf52840_hal::{
     target::{self, TIMER0},
diff --git a/macros/src/lib.rs b/macros/src/lib.rs
index 275ce0d..282ca4c 100644
--- a/macros/src/lib.rs
+++ b/macros/src/lib.rs
@@ -723,29 +723,28 @@ impl Codegen {
                     exprs.push(quote!(_fmt_.usize(#arg)));
                 }
                 defmt_parser::Type::BitField(_) => {
-                    // TODO reused in decoder::parse_args(), can we share this somehow without Cargo bug troubles?
-                    // TODO -> move this to parser!
                     let all_bitfields = parsed_params.iter().filter(|param| param.index == i);
-                    let largest_bit_index = all_bitfields
-                        .map(|param| match &param.ty {
-                            defmt_parser::Type::BitField(range) => range.end,
-                            _ => unreachable!(),
-                        })
-                        .max()
-                        .unwrap();
-
-                    match largest_bit_index {
-                        0..=8 => {
-                            exprs.push(quote!(_fmt_.u8(&defmt::export::truncate(*#arg))));
+                    let (smallest_bit_index, largest_bit_index) = defmt_parser::get_max_bitfield_range(all_bitfields).unwrap();
+
+                    // indices of the lowest and the highest octet which contains bitfield-relevant data
+                    let lowest_byte = smallest_bit_index / 8;
+                    let highest_byte = (largest_bit_index - 1) / 8;
+                    let truncated_sz = highest_byte - lowest_byte + 1; // in bytes
+
+                    match truncated_sz {
+                        1 => {
+                            // shift away unneeded lower octet
+                            // TODO: create helper for shifting because readability
+                            exprs.push(quote!(_fmt_.u8(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8)))));
                         }
-                        9..=16 => {
-                            exprs.push(quote!(_fmt_.u16(&defmt::export::truncate(*#arg))));
+                        2 => {
+                            exprs.push(quote!(_fmt_.u16(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8)))));
                         }
-                        17..=24 => {
-                            exprs.push(quote!(_fmt_.u24(&defmt::export::truncate(*#arg))));
+                        3 => {
+                            exprs.push(quote!(_fmt_.u24(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8)))));
                         }
-                        25..=32 => {
-                            exprs.push(quote!(_fmt_.u32(&defmt::export::truncate(*#arg))));
+                        4 => {
+                            exprs.push(quote!(_fmt_.u32(&defmt::export::truncate((*#arg) >> (#lowest_byte * 8)))));
                         }
                         _ => unreachable!(),
                     }
diff --git a/parser/src/lib.rs b/parser/src/lib.rs
index 1fa966e..7c478a8 100644
--- a/parser/src/lib.rs
+++ b/parser/src/lib.rs
@@ -223,6 +223,35 @@ fn push_literal<'f>(
     Ok(())
 }
 
+/// returns Some(smallest_bit_index, largest_bit_index) contained in `params` if
+///         `params` contains any bitfields.
+///         None otherwise
+pub fn get_max_bitfield_range<'a, I>(params: I) -> Option<(u8, u8)>
+where
+    I: Iterator<Item = &'a Parameter> + Clone,
+{
+    let largest_bit_index = params
+        .clone()
+        .map(|param| match &param.ty {
+            Type::BitField(range) => range.end,
+            _ => unreachable!(),
+        })
+        .max();
+
+    let smallest_bit_index = params
+        .map(|param| match &param.ty {
+            Type::BitField(range) => range.start,
+            _ => unreachable!(),
+        })
+        .min();
+
+    match (smallest_bit_index, largest_bit_index) {
+        (Some(smallest), Some(largest)) => Some((smallest, largest)),
+        (None, None) => None,
+        _ => unreachable!(),
+    }
+}
+
 pub fn parse<'f>(format_string: &'f str) -> Result<Vec<Fragment<'f>>, Cow<'static, str>> {
     let mut fragments = Vec::new();
 


Test patch:
diff --git a/tests/encode.rs b/tests/encode.rs
index 674fbe1..4700088 100644
--- a/tests/encode.rs
+++ b/tests/encode.rs
@@ -231,7 +231,7 @@ fn bitfields_across_octets() {
     assert_eq!(
         f.bytes(),
         &[
-            index, // bitfields {0:7..12}, {1:0..5}",
+            index, // bitfields {0:0..7} {0:9..14}",
             timestamp,
             0b1101_0010,
             0b0110_0011, // u16
@@ -239,6 +239,44 @@ fn bitfields_across_octets() {
     );
 }
 
+#[test]
+fn bitfields_truncate_lower() {
+    let index = fetch_string_index();
+    let timestamp = fetch_timestamp();
+    let mut f = Formatter::new();
+
+    winfo!(
+        f,
+        "bitfields {0:9..14}",
+        0b0000_0000_0000_1111_0110_0011_1101_0010u32
+    );
+    assert_eq!(
+        f.bytes(),
+        &[
+            index, // bitfields {0:9..14}",
+            timestamp,
+            0b0110_0011, // the first octet should have been truncated away
+        ]
+    );
+}
+
+#[test]
+fn bitfields_assert_range_exclusive() {
+    let index = fetch_string_index();
+    let timestamp = fetch_timestamp();
+    let mut f = Formatter::new();
+
+    winfo!(f, "bitfields {0:6..8}", 0b1010_0101u8,);
+    assert_eq!(
+        f.bytes(),
+        &[
+            index, // "bitfields {0:6..8}"
+            timestamp,
+            0b1010_0101
+        ]
+    );
+}
+
 #[test]
 fn boolean_struct() {
     #[derive(Format)]