From f8994ac937ee1003c3701822339e9c06bff0e651 Mon Sep 17 00:00:00 2001 From: Josh Conner Date: Wed, 18 Sep 2024 15:26:33 +0000 Subject: [PATCH] pw_bluetooth: Fixes to IsoDataFramePacket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Commit-Queue: Josh Conner Lint: Lint 🤖 Reviewed-by: Ben Lawson Presubmit-Verified: CQ Bot Account Docs-Not-Needed: Josh Conner --- pw_bluetooth/emboss_test.cc | 91 ++++++++++++++++++- pw_bluetooth/public/pw_bluetooth/hci_data.emb | 21 +++-- 2 files changed, 102 insertions(+), 10 deletions(-) diff --git a/pw_bluetooth/emboss_test.cc b/pw_bluetooth/emboss_test.cc index baf8b81224..6c2c9160ab 100644 --- a/pw_bluetooth/emboss_test.cc +++ b/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 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(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(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(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(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(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(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(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(view.IntrinsicSizeInBytes().Read()), + view.hdr_size().Read() + kSduFragmentSize + 4); } // Test and demonstrate various ways of reading opcodes. diff --git a/pw_bluetooth/public/pw_bluetooth/hci_data.emb b/pw_bluetooth/public/pw_bluetooth/hci_data.emb index 24a6bb5640..bb745a9ed5 100644 --- a/pw_bluetooth/public/pw_bluetooth/hci_data.emb +++ b/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: