pw_bluetooth: Fixes to IsoDataFramePacket

There were a number of problems in the definitions of the
IsoDataFramePacket, mostly related to conditional fields:
  * iso_sdu_fragment was improperly included in the SDU header
    conditional field due to indentation
  * offsets of fields were being incorrectly calculated
  * size of the iso_sdu_fragment was only correct if the frame was
    a complete SDU

Test: pw presubmit --step gn_chre_googletest_nanopb_sapphire_build
Change-Id: I71772bb7ee6c6544384011be0a2d0df1f316d4a1
Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/pigweed/+/236012
Reviewed-by: Alan Rosenthal <[email protected]>
Commit-Queue: Josh Conner <[email protected]>
Lint: Lint 🤖 <[email protected]>
Reviewed-by: Ben Lawson <[email protected]>
Presubmit-Verified: CQ Bot Account <[email protected]>
Docs-Not-Needed: Josh Conner <[email protected]>

pw_bluetooth/emboss_test.cc

@@ -51,13 +51,100 @@ TEST(EmbossTest, MakeView) {
   EXPECT_EQ(view.payload().Read(), 0x03);
 }
 
+static void InitializeIsoPacket(const emboss::IsoDataFramePacketWriter& view,
+                                emboss::TsFlag ts_flag,
+                                emboss::IsoDataPbFlag pb_flag,
+                                size_t sdu_fragment_size) {
+  view.header().connection_handle().Write(0x123);
+  view.header().ts_flag().Write(ts_flag);
+  view.header().pb_flag().Write(pb_flag);
+
+  size_t optional_fields_total_size = 0;
+  if (ts_flag == emboss::TsFlag::TIMESTAMP_PRESENT) {
+    optional_fields_total_size += 4;
+  }
+
+  if ((pb_flag == emboss::IsoDataPbFlag::FIRST_FRAGMENT) ||
+      (pb_flag == emboss::IsoDataPbFlag::COMPLETE_SDU)) {
+    optional_fields_total_size += 4;
+  }
+
+  view.header().data_total_length().Write(sdu_fragment_size +
+                                          optional_fields_total_size);
+}
+
 // This definition has a mix of full-width values and bitfields and includes
 // conditional bitfields. Let's add this to verify that the structure itself
 // doesn't get changed incorrectly and that emboss' size calculation matches
 // ours.
 TEST(EmbossTest, CheckIsoPacketSize) {
-  int max_size = 12 /* max header */ + 4095 /* max payload */;
-  EXPECT_EQ(emboss::IsoDataFramePacket::MaxSizeInBytes(), max_size);
+  std::array<uint8_t, 2048> buffer;
+  const size_t kSduFragmentSize = 100;
+  auto view = emboss::MakeIsoDataFramePacketView(&buffer);
+
+  InitializeIsoPacket(view,
+                      emboss::TsFlag::TIMESTAMP_NOT_PRESENT,
+                      emboss::IsoDataPbFlag::FIRST_FRAGMENT,
+                      kSduFragmentSize);
+  ASSERT_TRUE(view.IntrinsicSizeInBytes().Ok());
+  EXPECT_EQ(static_cast<size_t>(view.IntrinsicSizeInBytes().Read()),
+            view.hdr_size().Read() + kSduFragmentSize + 4);
+
+  InitializeIsoPacket(view,
+                      emboss::TsFlag::TIMESTAMP_NOT_PRESENT,
+                      emboss::IsoDataPbFlag::INTERMEDIATE_FRAGMENT,
+                      kSduFragmentSize);
+  ASSERT_TRUE(view.IntrinsicSizeInBytes().Ok());
+  EXPECT_EQ(static_cast<size_t>(view.IntrinsicSizeInBytes().Read()),
+            view.hdr_size().Read() + kSduFragmentSize);
+
+  InitializeIsoPacket(view,
+                      emboss::TsFlag::TIMESTAMP_NOT_PRESENT,
+                      emboss::IsoDataPbFlag::COMPLETE_SDU,
+                      kSduFragmentSize);
+  ASSERT_TRUE(view.IntrinsicSizeInBytes().Ok());
+  EXPECT_EQ(static_cast<size_t>(view.IntrinsicSizeInBytes().Read()),
+            view.hdr_size().Read() + kSduFragmentSize + 4);
+
+  InitializeIsoPacket(view,
+                      emboss::TsFlag::TIMESTAMP_NOT_PRESENT,
+                      emboss::IsoDataPbFlag::LAST_FRAGMENT,
+                      kSduFragmentSize);
+  ASSERT_TRUE(view.IntrinsicSizeInBytes().Ok());
+  EXPECT_EQ(static_cast<size_t>(view.IntrinsicSizeInBytes().Read()),
+            view.hdr_size().Read() + kSduFragmentSize);
+
+  InitializeIsoPacket(view,
+                      emboss::TsFlag::TIMESTAMP_PRESENT,
+                      emboss::IsoDataPbFlag::FIRST_FRAGMENT,
+                      kSduFragmentSize);
+  ASSERT_TRUE(view.IntrinsicSizeInBytes().Ok());
+  EXPECT_EQ(static_cast<size_t>(view.IntrinsicSizeInBytes().Read()),
+            view.hdr_size().Read() + kSduFragmentSize + 8);
+
+  InitializeIsoPacket(view,
+                      emboss::TsFlag::TIMESTAMP_PRESENT,
+                      emboss::IsoDataPbFlag::INTERMEDIATE_FRAGMENT,
+                      kSduFragmentSize);
+  ASSERT_TRUE(view.IntrinsicSizeInBytes().Ok());
+  EXPECT_EQ(static_cast<size_t>(view.IntrinsicSizeInBytes().Read()),
+            view.hdr_size().Read() + kSduFragmentSize + 4);
+
+  InitializeIsoPacket(view,
+                      emboss::TsFlag::TIMESTAMP_PRESENT,
+                      emboss::IsoDataPbFlag::COMPLETE_SDU,
+                      kSduFragmentSize);
+  ASSERT_TRUE(view.IntrinsicSizeInBytes().Ok());
+  EXPECT_EQ(static_cast<size_t>(view.IntrinsicSizeInBytes().Read()),
+            view.hdr_size().Read() + kSduFragmentSize + 8);
+
+  InitializeIsoPacket(view,
+                      emboss::TsFlag::TIMESTAMP_PRESENT,
+                      emboss::IsoDataPbFlag::LAST_FRAGMENT,
+                      kSduFragmentSize);
+  ASSERT_TRUE(view.IntrinsicSizeInBytes().Ok());
+  EXPECT_EQ(static_cast<size_t>(view.IntrinsicSizeInBytes().Read()),
+            view.hdr_size().Read() + kSduFragmentSize + 4);
 }
 
 // Test and demonstrate various ways of reading opcodes.

pw_bluetooth/public/pw_bluetooth/hci_data.emb

@@ -136,25 +136,30 @@ struct IsoDataFramePacket:
   -- Core Spec v5.4, Vol 4, Part E, Section 5.4.5
   -- Bits 28, and 29 are reserved for future use (RFU)
   let hdr_size = IsoDataFrameHeader.$size_in_bytes
-  0     [+hdr_size]          IsoDataFrameHeader      header
+  0                   [+hdr_size]           IsoDataFrameHeader         header
   if header.ts_flag == TsFlag.TIMESTAMP_PRESENT:
-    $next [+4]               UInt                    time_stamp
+    hdr_size            [+4]                UInt                       time_stamp
       -- A time in microseconds.
 
+  let ts_size = (header.ts_flag == TsFlag.TIMESTAMP_PRESENT) ? 4 : 0
+  let sdu_hdr_offset = hdr_size+ts_size
   if header.pb_flag == IsoDataPbFlag.FIRST_FRAGMENT || header.pb_flag == IsoDataPbFlag.COMPLETE_SDU:
-    $next [+4]  bits:
-      0     [+16]            UInt                    packet_sequence_number
+    sdu_hdr_offset      [+4]  bits:
+      0                   [+16]             UInt                       packet_sequence_number
         -- The sequence number of the SDU.
 
-      16    [+12]            UInt                    iso_sdu_length
+      16                  [+12]             UInt                       iso_sdu_length
         -- The total length of the SDU (and not of any individual fragments),
         -- in octets.
 
-      30    [+2]             IsoDataPacketStatus     packet_status_flag
+      30                  [+2]              IsoDataPacketStatus        packet_status_flag
         -- Packet status, only for frames sent by the controller.
 
-    $next [+iso_sdu_length]  UInt:8[iso_sdu_length]  iso_sdu_fragment
-      -- Isochronous data (the SDU or fragment of the SDU)
+  let sdu_hdr_size = (header.pb_flag == IsoDataPbFlag.FIRST_FRAGMENT || header.pb_flag == IsoDataPbFlag.COMPLETE_SDU) ? 4 : 0
+  let sdu_fragment_offset = hdr_size+ts_size+sdu_hdr_size
+  let sdu_fragment_size = header.data_total_length-(ts_size+sdu_hdr_size)
+  sdu_fragment_offset [+sdu_fragment_size]  UInt:8[sdu_fragment_size]  iso_sdu_fragment
+    -- Isochronous data (the SDU or fragment of the SDU)
 
 
 enum ScoDataPacketStatus: