Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't prepend header in BDX messages #8234

Merged
merged 5 commits into from
Jul 16, 2021
Merged

Don't prepend header in BDX messages #8234

merged 5 commits into from
Jul 16, 2021

Conversation

holbrookt
Copy link
Contributor

Problem

What is being fixed? Examples:

Change overview

  • remove code in BdxTransferSession that prepended PayloadHeaders to
    outgoing messages
  • add MessageTypeData to convey message type and protocol
  • add header prepending in TestBdxTransferSession.cpp
  • fix some comments

Testing

Fixed TestBdxTransferSession to not expect messages to already have a PayloadHeader. Tests pass.

@holbrookt
Don't prepend header in BDX messages
58c22c2 
- remove code in BdxTransferSession that prepended PayloadHeaders to
outgoing messages
- add MessageTypeData to convey message type and protocol
@holbrookt holbrookt added this to the Test Event 5 milestone Jul 8, 2021
@holbrookt holbrookt marked this pull request as ready for review July 9, 2021 04:54
@holbrookt holbrookt requested a review from tcarmelveilleux July 9, 2021 04:56
bzbarsky-apple
bzbarsky-apple reviewed Jul 9, 2021
View reviewed changes
src/protocols/bdx/BdxTransferSession.h Outdated
@@ -80,12 +80,22 @@ class DLL_EXPORT TransferSession
bool IsEof = false;
};

struct MessageTypeData
{
uint32_t ProtocolId = 0; // Should only be SecureChannel or BDX
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not store this as an actual protocol id, as opposed to uint32_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I originally wrote this header, I ran into compilation errors when I tried using non-trivial types in the structs included in the anonymous union.

That being said, I tried doing this change today and got it to compile locally, so I'm not sure what I did wrong last time. I agree with you that it's much simpler and cleaner to use Protocols::Id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New commit has these changes

src/protocols/bdx/tests/TestBdxTransferSession.cpp Outdated
{
NL_TEST_ASSERT(inSuite, false);
return;
payloadHeader.SetMessageType(Protocols::BDX::Id, typeData.MessageType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the typedata stored a Protocols::Id, could just have a single SetMessageType call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, will implement

src/protocols/bdx/BdxTransferSession.h
@@ -97,6 +107,7 @@ class DLL_EXPORT TransferSession
TransferAcceptData transferAcceptData;
BlockData blockdata;
StatusReportData statusData;
MessageTypeData msgTypeData;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this used in practice anywhere outside the unit test? I'm not obviously seeing it....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is - see PrepareOutgoingMessageEvent() in BdxTransferSession.cpp

Also this struct will be crucial to any application that uses TransferSession because it will need to pass the ProtocolId and MessageType to ExchangeContext::SendMessage()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, it's actually used as an arg to AttachHeaderAndSend looks like. OK, that's the part I was missing.

@github-actions
Copy link

github-actions bot commented Jul 9, 2021

Size increase report for "esp32-example-build" from e55ad30

File Section File VM
chip-all-clusters-app.elf .flash.text -24 -24
Full report output 
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize
.xt.prop._ZN4chip6System5Mutex6UnlockEv,0,108
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE14_LockChipStackEv,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,48
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE16_UnlockChipStackEv,0,48
[Unmapped],0,24
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE14_LockChipStackEv,0,12
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE15_StartChipTimerEj,0,12
.xt.prop._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE16_UnlockChipStackEv,0,12
.flash.text,-24,-24
.xt.prop._ZN4chip11DeviceLayer8Internal26GenericPlatformManagerImplINS0_19PlatformManagerImplEE14_InitChipStackEv,0,-40
.xt.lit._ZN4chip11DeviceLayer8Internal35GenericPlatformManagerImpl_FreeRTOSINS0_19PlatformManagerImplEE10_PostEventEPKNS0_15ChipDeviceEventE,0,-80
.xt.lit._ZN4chip6System5Mutex6UnlockEv,0,-128

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize

@holbrookt
add PayloadHeader argument to message handler methods
f0b3899 
- ExchangeDelegate already consumes PayloadHeader from the message, so
this needs to be removed from HandleMessageReceived()
@holbrookt
Copy link
Contributor Author

I added one more commit to fix HandleMessageReceived(). It was expecting the PayloadHeader to have not been consumed yet, but ExchangeDelegate does this already. I changed it to take the decoded PayloadHeader as an argument instead.

@andy31415 andy31415 merged commit ac52aa6 into project-chip:master Jul 16, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
@holbrookt
Don't prepend header in BDX messages (project-chip#8234)
26f95a9 
* Don't prepend header in BDX messages

- remove code in BdxTransferSession that prepended PayloadHeaders to
outgoing messages
- add MessageTypeData to convey message type and protocol

* use Protocols::Id instead of uint32_t

* add PayloadHeader argument to message handler methods

- ExchangeDelegate already consumes PayloadHeader from the message, so
this needs to be removed from HandleMessageReceived()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmartinez-silabs jmartinez-silabs jmartinez-silabs approved these changes

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@andy31415 andy31415 andy31415 approved these changes

@tcarmelveilleux tcarmelveilleux tcarmelveilleux approved these changes

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@jelderton jelderton Awaiting requested review from jelderton

@LuDuda LuDuda Awaiting requested review from LuDuda

@mspang mspang Awaiting requested review from mspang

@saurabhst saurabhst Awaiting requested review from saurabhst

@woody-apple woody-apple Awaiting requested review from woody-apple

Assignees
No one assigned
Labels
review - approved
Projects
None yet
Milestone
Test Event 5
Development

Successfully merging this pull request may close these issues.

Rework BDX TransferSession to not encode PayloadHeader
5 participants
@holbrookt @tcarmelveilleux @andy31415 @bzbarsky-apple @jmartinez-silabs