Define internal message flags in PacketHeader to contain the flags for internal use #4571

yufengwangca · 2021-01-29T19:36:10Z

Problem

Current packet header flags in transport are 16 bits and only contains the flags need to be encoded into the packet sent out on the wire.

We need to define more flags associated with a inbound or outbound CHIP message for internal use, such as:
The flag indicates if the peer's group key message counter is not synchronized, this is used by MCSP in secure channel.

The flag indicates if the message buffer should not be freed after sending, this is to tell LwIP stack not free the buffer after sending.

Those flags are used internally and not get encoded to the packet sent over the wire.

Summary of Changes

Define internal message flags in PacketHeader to contain the flags for internal use.

src/transport/raw/MessageHeader.h

bzbarsky-apple · 2021-01-29T19:49:14Z

src/transport/raw/MessageHeader.h

+    bool IsPeerGroupMsgIdNotSynchronized() const { return mFlags.Has(Header::FlagValues::kPeerGroupMsgIdNotSynchronized); }
+
+    /** Check if the message buffer should not be freed after sending. */
+    bool IsRetainedBuffer() const { return mFlags.Has(Header::FlagValues::kRetainedBuffer); }


Where and why would this get used? We used to have a thing for this, and we removed it, due to lack of use cases and the caller being able to just hold a handle if that's what they want.

We do. #4372

When retaining a buffer with CRMP, the caller expects the msg to be unmodified. LwIP stack will normally prepend the packet headers as the packet traverses the UDP/IP/netif layers, which normally modifies the packet.

We need to deep-copies msg into a fresh object, and queues that for transmission, leaving the original msg available after return.

I'll follow up in #4372, but I don't see why that needs this API on the packet header.

This flag finally needs to be passed to Inet layer
INET_ERROR UDPEndPoint::SendMsg(const IPPacketInfo * pktInfo, System::PacketBufferHandle msg, uint16_t sendFlags)

The buffer is retained in messaging layer, currently, we only have PacketHeader and PayloadHeader passed to transport layer. Passing this flag from Messaging layer to Transport layer via PacketHeader seems the only viable path.

I will post another PR to pass this flag to Inet::UDP for #4372

woody-apple

Couldn't agree more with @turon - if this is just API between layers, why not codify it as such - do we even need flags here? Seems like some simple parameters may even make this easier?

yufengwangca · 2021-01-29T21:13:13Z

Couldn't agree more with @turon - if this is just API between layers, why not codify it as such - do we even need flags here? Seems like some simple parameters may even make this easier?

We already have PacketHeader to pass all flags from Messaging layer to Transport layer.

woody-apple

We should not be using flags here. Can we either use a struct with 1 bit members that we need, and add to it, or just have 1 bit members that we add to over time?

yufengwangca · 2021-01-30T00:03:15Z

We should not be using flags here. Can we either use a struct with 1 bit members that we need, and add to it, or just have 1 bit members that we add to over time?

It is doable, but this struct will be similar to class BitFlags. The two internal flags defined in this PR are immediately needed now, more need to be added. Here is the full set of message flags defined in messaging layer: https://github.com/project-chip/connectedhomeip/blob/master/src/messaging/Flags.h

/**

@brief
Flags associated with a inbound or outbound CHIP message.
The values defined here are for use within the ChipMessageInfo.Flags field.
*/
enum class MessageFlagValues : uint32_t
{
/< Indicates that the existing source node identifier must be reused. */
kReuseSourceId = 0x00000020,
/< Indicates that the CHIP message is already encoded. */
kMessageEncoded = 0x00001000,
/< Indicates that default IPv6 source address selection should be used when sending IPv6 multicast messages. */
kDefaultMulticastSourceAddress = 0x00002000,
/< Indicates that the sender of the message requested an acknowledgment. */
kPeerRequestedAck = 0x00004000,
/< Indicates that the message is a duplicate of a previously received message. */
kDuplicateMessage = 0x00008000,
/< Indicates that the peer's group key message counter is not synchronized. */
kPeerGroupMsgIdNotSynchronized = 0x00010000,
/< Indicates that the source of the message is the initiator of the CHIP exchange. */
kFromInitiator = 0x00020000,
/< Indicates that message is being sent/received via the local ephemeral UDP port. */
kViaEphemeralUDPPort = 0x00040000,
};

enum class SendMessageFlags : uint16_t
{
kNone = 0x0000,
/< Used to indicate that automatic retransmission is enabled. */
kAutoRetrans = 0x0001,
/< Used to indicate that a response is expected within a specified timeout. */
kExpectResponse = 0x0002,
/< Used to indicate that the source node ID in the message header can be reused. */
kReuseSourceId = 0x0020,
/< Used to indicate that the message is already encoded. */
kAlreadyEncoded = 0x0080,
/< Used to indicate that default IPv6 source address selection should be used when sending IPv6 multicast messages. */
kDefaultMulticastSourceAddress = 0x0100,
/< Used to indicate that the current message is the initiator of the exchange. */
kFromInitiator = 0x0200,
/< Used to send a ReliableMessageProtocol message requesting an acknowledgment. */
kRequestAck = 0x0400,
/< Suppress the auto-request acknowledgment feature when sending a message. */
kNoAutoRequestAck = 0x0800,
};

Some of them are inherited from Weave and we might not need it eventually, but most of them will be passed to transport layer as corresponding feature comes in.

…r internal use

github-actions · 2021-02-01T07:05:14Z

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

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

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,9291
.debug_str,0,1349
.debug_loc,0,153
.debug_line,0,133
.debug_ranges,0,120
.flash.text,20,20
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,2

github-actions · 2021-02-01T07:05:40Z

Size increase report for "nrfconnect-example-build" from a83086a

File	Section	File	VM
chip-lock.elf	text	16	16
chip-lighting.elf	text	16	16

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,9497
.debug_str,0,1349
.debug_loc,0,111
.debug_line,0,51
.debug_ranges,0,32
text,16,16

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

sections,vmsize,filesize
.debug_info,0,9497
.debug_str,0,1349
.debug_loc,0,119
.debug_line,0,51
.debug_ranges,0,32
text,16,16

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

sections,vmsize,filesize

yufengwangca · 2021-02-01T17:26:42Z

We should not be using flags here. Can we either use a struct with 1 bit members that we need, and add to it, or just have 1 bit members that we add to over time?

It is doable, but this struct will be similar to class BitFlags. The two internal flags defined in this PR are immediately needed now, more need to be added. Here is the full set of message flags defined in messaging layer: https://github.com/project-chip/connectedhomeip/blob/master/src/messaging/Flags.h

/**

@brief

Flags associated with a inbound or outbound CHIP message.

The values defined here are for use within the ChipMessageInfo.Flags field.
*/
enum class MessageFlagValues : uint32_t
{
/< Indicates that the existing source node identifier must be reused. */
kReuseSourceId = 0x00000020,
/< Indicates that the CHIP message is already encoded. */
kMessageEncoded = 0x00001000,
/< Indicates that default IPv6 source address selection should be used when sending IPv6 multicast messages. */
kDefaultMulticastSourceAddress = 0x00002000,
/< Indicates that the sender of the message requested an acknowledgment. */
kPeerRequestedAck = 0x00004000,
/< Indicates that the message is a duplicate of a previously received message. */
kDuplicateMessage = 0x00008000,
/< Indicates that the peer's group key message counter is not synchronized. */
kPeerGroupMsgIdNotSynchronized = 0x00010000,
/< Indicates that the source of the message is the initiator of the CHIP exchange. */
kFromInitiator = 0x00020000,
/< Indicates that message is being sent/received via the local ephemeral UDP port. */
kViaEphemeralUDPPort = 0x00040000,
};

enum class SendMessageFlags : uint16_t
{
kNone = 0x0000,
/< Used to indicate that automatic retransmission is enabled. */ kAutoRetrans = 0x0001, /< Used to indicate that a response is expected within a specified timeout. */
kExpectResponse = 0x0002,
/< Used to indicate that the source node ID in the message header can be reused. */ kReuseSourceId = 0x0020, /< Used to indicate that the message is already encoded. */
kAlreadyEncoded = 0x0080,
/< Used to indicate that default IPv6 source address selection should be used when sending IPv6 multicast messages. */ kDefaultMulticastSourceAddress = 0x0100, /< Used to indicate that the current message is the initiator of the exchange. */
kFromInitiator = 0x0200,
/< Used to send a ReliableMessageProtocol message requesting an acknowledgment. */ kRequestAck = 0x0400, /< Suppress the auto-request acknowledgment feature when sending a message. */
kNoAutoRequestAck = 0x0800,
};

Some of them are inherited from Weave and we might not need it eventually, but most of them will be passed to transport layer as corresponding feature comes in.

@woody-apple
I was wondering if we need to invent another struct to represents internal message flags since we already have construct BitFlags.
Right now, we have three sets of flags, one for Exchange Flags, one for Internal Flags and one for Flags need to be encoded into the packet. If we should handle them consistently using the same construct?

woody-apple

Having internal flags that's uint16_t, plus another member that's uint16_t, plus another uint8_t seems super wasteful memory structure wise. I really suggest using bits for these, instead of a uint16_t, given it will save space, but also be more reasonable.

yufengwangca · 2021-02-02T20:10:58Z

Having internal flags that's uint16_t, plus another member that's uint16_t, plus another uint8_t seems super wasteful memory structure wise. I really suggest using bits for these, instead of a uint16_t, given it will save space, but also be more reasonable.

@woody-apple

We already used bits for those feature flags, BitFlags is manipulating on bit directly. Here is the current allocations:

PayloadHeader:

ExFlagValues : uint8_t, those bits are defined by the spec for Exchange flags, those bits are in Payload header instead of Packet Header.

PacketHeader:
2. FlagValues : uint16_t, those bits are also defined by the spec, 8 bits for Message flags and 8 bits for Security flags
3. InternalFlagValues : uint16_t those bits are reserved for internal feature flags, currently we only defined two flags in Transport layer, but we have more than 8 internal feature flags defined in messaging layer, we might need to add them to InternalFlagValues as the feature comes in, so I reserved 16 bits here for them.

stale · 2021-02-16T05:42:45Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

boring-cyborg bot added the transport label Jan 29, 2021

pullapprove bot requested review from andy31415, BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, saurabhst and woody-apple January 29, 2021 19:38

yufengwangca requested a review from pan-apple January 29, 2021 19:38

turon reviewed Jan 29, 2021

View reviewed changes

src/transport/raw/MessageHeader.h Outdated Show resolved Hide resolved

bzbarsky-apple reviewed Jan 29, 2021

View reviewed changes

woody-apple requested changes Jan 29, 2021

View reviewed changes

bzbarsky-apple mentioned this pull request Jan 29, 2021

Sending PacketBuffer with "ref count > 1" fails in LwIP #4372

Closed

yufengwangca requested a review from woody-apple January 29, 2021 21:13

yufengwangca changed the title ~~Extend message flags from 16 bits to 32 bits to contain the flags for…~~ Define internal message flags in PacketHeader to contain the flags for internal use Jan 29, 2021

woody-apple requested changes Jan 29, 2021

View reviewed changes

Define internal message flags in PacketHeader to contain the flags fo…

db894f2

…r internal use

yufengwangca requested a review from woody-apple February 1, 2021 17:26

woody-apple requested changes Feb 2, 2021

View reviewed changes

This was referenced Feb 8, 2021

Implement Message Counter Synchronization Protocol (MCSP). #4626

Closed

Implement Message Counter Synchronization Protocol part #4712

Merged

yufengwangca requested a review from woody-apple February 9, 2021 05:11

stale bot added the stale Stale issue or PR label Feb 16, 2021

pullapprove bot requested a review from msandstedt February 16, 2021 05:42

yufengwangca closed this Feb 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define internal message flags in PacketHeader to contain the flags for internal use #4571

Define internal message flags in PacketHeader to contain the flags for internal use #4571

yufengwangca commented Jan 29, 2021 •

edited

Loading

bzbarsky-apple Jan 29, 2021

yufengwangca Jan 29, 2021

bzbarsky-apple Jan 29, 2021

yufengwangca Jan 29, 2021

woody-apple left a comment

yufengwangca commented Jan 29, 2021

woody-apple left a comment

yufengwangca commented Jan 30, 2021

github-actions bot commented Feb 1, 2021

github-actions bot commented Feb 1, 2021

yufengwangca commented Feb 1, 2021

woody-apple left a comment

yufengwangca commented Feb 2, 2021 •

edited

Loading

stale bot commented Feb 16, 2021

Define internal message flags in PacketHeader to contain the flags for internal use #4571

Define internal message flags in PacketHeader to contain the flags for internal use #4571

Conversation

yufengwangca commented Jan 29, 2021 • edited Loading

Problem

Summary of Changes

bzbarsky-apple Jan 29, 2021

Choose a reason for hiding this comment

yufengwangca Jan 29, 2021

Choose a reason for hiding this comment

bzbarsky-apple Jan 29, 2021

Choose a reason for hiding this comment

yufengwangca Jan 29, 2021

Choose a reason for hiding this comment

woody-apple left a comment

Choose a reason for hiding this comment

yufengwangca commented Jan 29, 2021

woody-apple left a comment

Choose a reason for hiding this comment

yufengwangca commented Jan 30, 2021

github-actions bot commented Feb 1, 2021

github-actions bot commented Feb 1, 2021

yufengwangca commented Feb 1, 2021

woody-apple left a comment

Choose a reason for hiding this comment

yufengwangca commented Feb 2, 2021 • edited Loading

stale bot commented Feb 16, 2021

yufengwangca commented Jan 29, 2021 •

edited

Loading

yufengwangca commented Feb 2, 2021 •

edited

Loading