Revise PacketBufferWriter interface #4874

kpschoedel · 2021-02-16T15:20:30Z

Problem

The PacketBufferWriter constructor that takes a size and allocates the
PacketBuffer internally is awkward for some cases, and useless for the
case of an existing PacketBufferHandle.

Summary of Changes

PacketBufferWriter() takes ownership of an existing PacketBufferHandle
(same as PacketBufferTLVWriter).
Added a Finalize() overload that matches that of PacketBufferTLVWriter.

#### Problem The `PacketBufferWriter` constructor that takes a size and allocates the `PacketBuffer` internally is awkward for some cases, and useless for the case of an existing `PacketBufferHandle`. #### Summary of Changes - `PacketBufferWriter()` takes ownership of an existing `PacketBufferHandle` (same as `PacketBufferTLVWriter`). - Added a `Finalize()` overload that matches that of `PacketBufferTLVWriter`.

kpschoedel · 2021-02-16T15:54:33Z

src/system/SystemPacketBuffer.h

@@ -730,10 +730,9 @@ class PacketBufferWriterBase : public Writer
     *  @param[in]  aAvailableSize  Length bound of the BufferWriter.
     *  @param[in]  aReservedSize   Reserved packet buffer space for protocol headers; see \c PacketBufferHandle::New().
     */
-    PacketBufferWriterBase(size_t aAvailableSize, uint16_t aReservedSize = CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE) :
-        Writer(nullptr, 0)
+    PacketBufferWriterBase(System::PacketBufferHandle && aPacket) : Writer(aPacket->Start(), aPacket->AvailableDataLength())


Looks like this still needs a length (possibly optional; checking the cases), for the case that the pool-backed buffer is actually larger than requested.

bzbarsky-apple

I'm OK with a followup for the MessagePacketBuffer bits, so that we don't combine behavior changes with refactoring in a single PR. Up to you.

But we do need to sort out the way we get the pointer+length out of the buffer.

bzbarsky-apple · 2021-02-16T15:52:38Z

src/protocols/bdx/BdxTransferSession.cpp

@@ -25,19 +25,13 @@ constexpr size_t kStatusReportMinSize = 2 + 4 + 2; ///< 16 bits for GeneralCode,
 CHIP_ERROR WriteToPacketBuffer(const ::chip::bdx::BdxMessage & msgStruct, ::chip::System::PacketBufferHandle & msgBuf)
 {
    size_t msgDataSize = msgStruct.MessageSize();
-    ::chip::Encoding::LittleEndian::PacketBufferWriter bbuf(msgDataSize);
+    ::chip::Encoding::LittleEndian::PacketBufferWriter bbuf(::chip::System::PacketBufferHandle::New(msgDataSize));


I think this should be a MessagePacketBuffer eventually, but a followup for that is OK.

I'll add them here so they don't get overlooked.

bzbarsky-apple · 2021-02-16T15:52:54Z

src/protocols/bdx/BdxTransferSession.cpp

@@ -848,21 +842,21 @@ void TransferSession::PrepareStatusReport(StatusCode code)
 {
    mStatusReportData.StatusCode = code;

-    Encoding::LittleEndian::PacketBufferWriter bbuf(kStatusReportMinSize);
+    Encoding::LittleEndian::PacketBufferWriter bbuf(System::PacketBufferHandle::New(kStatusReportMinSize));


Again, I expect MessagePacketBuffer here.

bzbarsky-apple · 2021-02-16T15:58:22Z

src/system/SystemPacketBuffer.h

@@ -730,10 +730,9 @@ class PacketBufferWriterBase : public Writer
     *  @param[in]  aAvailableSize  Length bound of the BufferWriter.
     *  @param[in]  aReservedSize   Reserved packet buffer space for protocol headers; see \c PacketBufferHandle::New().
     */
-    PacketBufferWriterBase(size_t aAvailableSize, uint16_t aReservedSize = CHIP_SYSTEM_CONFIG_HEADER_RESERVE_SIZE) :
-        Writer(nullptr, 0)
+    PacketBufferWriterBase(System::PacketBufferHandle && aPacket) : Writer(aPacket->Start(), aPacket->AvailableDataLength())


That combination of args doesn't really make sense if there's data in the buffer already.

We want to either use aPacket->Start() with aPacket->MaxDataLength(), or aPacket->Start() + aPacket->DataLength() with aPacket->AvailableDataLength().

Correct, I started off intended the former, but I think the latter is more useful.

bzbarsky-apple · 2021-02-16T15:58:39Z

src/system/SystemPacketBuffer.h

@@ -757,6 +756,22 @@ class PacketBufferWriterBase : public Writer
     */
    System::PacketBufferHandle Finalize() { return PacketBufferWriterUtil::Finalize(*this, mPacket); }

+    /**
+     * Finish the writing to the buffer and release ownership of the underlying PacketBuffer.


Suggested change

* Finish the writing to the buffer and release ownership of the underlying PacketBuffer.

* Finish writing to the buffer and release ownership of the underlying PacketBuffer.

bzbarsky-apple · 2021-02-16T15:59:33Z

src/system/SystemPacketBuffer.h

+    /**
+     * Finish the writing to the buffer and release ownership of the underlying PacketBuffer.
+     *
+     * This signature matches `PacketBufferTLVWriter::Finalize()`.


It's really not clear how one decides between the two signatures in practice... It might be better to only have one signature; if we want to match PacketBufferTLVWriter, let's just do that.

The original signature is more useful in practice; almost all Writer users immediately pass the buffer somewhere else.

OK, but why add the new one then?

Consistency with PacketBufferTLVWriter seemed like a good idea at the time.

bzbarsky-apple · 2021-02-16T16:00:58Z

src/transport/NetworkProvisioning.cpp

@@ -264,7 +264,7 @@ CHIP_ERROR NetworkProvisioning::SendThreadCredentials(const DeviceLayer::Interna
        4;                  // threadData.ThreadChannel, threadData.FieldPresent.ThreadExtendedPANId,
                            // threadData.FieldPresent.ThreadMeshPrefix, threadData.FieldPresent.ThreadPSKc
    /* clang-format on */
-    Encoding::LittleEndian::PacketBufferWriter bbuf(credentialSize);
+    Encoding::LittleEndian::PacketBufferWriter bbuf(System::PacketBufferHandle::New(credentialSize));


This should be a MessagePacketBuffer too.

github-actions · 2021-02-16T17:47:43Z

Size increase report for "esp32-example-build" from 3c2238a

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	44	44

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,18102
.debug_loc,0,9364
.debug_line,0,8858
.shstrtab,0,685
.strtab,0,363
.debug_str,0,336
.symtab,0,160
.xt.prop._ZN4chip8Encoding22PacketBufferWriterBaseINS0_12BufferWriterEEC2EONS_6System18PacketBufferHandleE,0,116
.xt.prop._ZN4chip8Encoding22PacketBufferWriterBaseINS0_12LittleEndian12BufferWriterEEC2EONS_6System18PacketBufferHandleEj,0,76
.debug_frame,0,48
.flash.text,44,44
.xt.prop._ZN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplD2Ev,0,40
.xt.prop._ZN4chip11DeviceLayer8Internal44GenericDeviceNetworkProvisioningDelegateImplINS0_37DeviceNetworkProvisioningDelegateImplEE15ProvisionThreadERNS1_17DeviceNetworkInfoE,0,40
.xt.prop._ZNK4chip8OptionalIyE5ValueEv,0,40
.debug_abbrev,0,34
.debug_aranges,0,16
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,-2
.debug_ranges,0,-168

github-actions · 2021-02-16T17:48:26Z

Size increase report for "nrfconnect-example-build" from 3c2238a

File	Section	File	VM
chip-lock.elf	text	72	72
chip-lock.elf	shell_root_cmds_sections	-8	-8
chip-lighting.elf	text	72	72
chip-lighting.elf	shell_root_cmds_sections	-8	-8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,19277
.debug_loc,0,7968
.debug_line,0,5807
.debug_abbrev,0,380
.debug_str,0,336
.strtab,0,313
.symtab,0,96
text,72,72
.debug_frame,0,56
.debug_aranges,0,16
.shstrtab,0,-1
shell_root_cmds_sections,-8,-8
.debug_ranges,0,-136

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,527
.debug_abbrev,0,270
.debug_aranges,0,-8
.debug_frame,0,-32
.debug_ranges,0,-32
.debug_line,0,-117
.debug_str,0,-120
.debug_loc,0,-272

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,19277
.debug_loc,0,7968
.debug_line,0,5807
.debug_abbrev,0,380
.debug_str,0,336
.strtab,0,313
.symtab,0,96
text,72,72
.debug_frame,0,56
.debug_aranges,0,16
.shstrtab,0,-1
shell_root_cmds_sections,-8,-8
.debug_ranges,0,-136

bzbarsky-apple · 2021-02-16T18:14:49Z

src/protocols/bdx/BdxTransferSession.cpp

@@ -25,7 +26,7 @@ constexpr size_t kStatusReportMinSize = 2 + 4 + 2; ///< 16 bits for GeneralCode,
 CHIP_ERROR WriteToPacketBuffer(const ::chip::bdx::BdxMessage & msgStruct, ::chip::System::PacketBufferHandle & msgBuf)
 {
    size_t msgDataSize = msgStruct.MessageSize();
-    ::chip::Encoding::LittleEndian::PacketBufferWriter bbuf(msgDataSize);
+    ::chip::Encoding::LittleEndian::PacketBufferWriter bbuf(chip::MessagePacketBuffer::New(msgDataSize));


Should this also pass msgDataSize as second arg? Or document why we don't care if this overallocates?

boring-cyborg bot added app system transport labels Feb 16, 2021

kpschoedel requested a review from bzbarsky-apple February 16, 2021 15:20

pullapprove bot requested review from andy31415, chrisdecenzo, hawk248, jelderton, msandstedt, mspang, rwalker-apple, saurabhst, selissia, turon and woody-apple February 16, 2021 15:20

* fix doxygen

af6e552

kpschoedel mentioned this pull request Feb 16, 2021

Fix missing footer space during WiFi provisioning #4868

Merged

kpschoedel commented Feb 16, 2021

View reviewed changes

kpschoedel marked this pull request as draft February 16, 2021 15:56

Merge remote-tracking branch 'origin/master' into pbw3

a318fbf

mspang approved these changes Feb 16, 2021

View reviewed changes

bzbarsky-apple requested changes Feb 16, 2021

View reviewed changes

* review

607deb0

kpschoedel marked this pull request as ready for review February 16, 2021 16:56

bzbarsky-apple approved these changes Feb 16, 2021

View reviewed changes

* review 2

317fc80

msandstedt approved these changes Feb 16, 2021

View reviewed changes

woody-apple approved these changes Feb 17, 2021

View reviewed changes

woody-apple merged commit 0729778 into project-chip:master Feb 17, 2021

kpschoedel deleted the pbw3 branch February 17, 2021 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revise PacketBufferWriter interface #4874

Revise PacketBufferWriter interface #4874

kpschoedel commented Feb 16, 2021

kpschoedel Feb 16, 2021

bzbarsky-apple left a comment

bzbarsky-apple Feb 16, 2021

kpschoedel Feb 16, 2021

bzbarsky-apple Feb 16, 2021

bzbarsky-apple Feb 16, 2021

kpschoedel Feb 16, 2021

bzbarsky-apple Feb 16, 2021

bzbarsky-apple Feb 16, 2021

kpschoedel Feb 16, 2021

bzbarsky-apple Feb 16, 2021

kpschoedel Feb 16, 2021

bzbarsky-apple Feb 16, 2021

github-actions bot commented Feb 16, 2021

github-actions bot commented Feb 16, 2021

bzbarsky-apple Feb 16, 2021

	* Finish the writing to the buffer and release ownership of the underlying PacketBuffer.
	* Finish writing to the buffer and release ownership of the underlying PacketBuffer.

Revise PacketBufferWriter interface #4874

Revise PacketBufferWriter interface #4874

Conversation

kpschoedel commented Feb 16, 2021

Problem

Summary of Changes

Choose a reason for hiding this comment

bzbarsky-apple left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 16, 2021

github-actions bot commented Feb 16, 2021

Choose a reason for hiding this comment