Use the incoming exchange context to send responses in the data model #6438

bzbarsky-apple · 2021-05-04T01:15:35Z

Fixes #3584 and sets us up to more cleanly track what a send-destination is now that it's not just an integer.

Review note: It may well be simpler to review the individual changesets, not the whole diff.

vivien-apple · 2021-05-04T12:22:54Z

src/app/util/af.h

-                                                      EmberAfMessageSentFunction callback);
+// EmberStatus emberAfSendBroadcastWithAliasWithCallback(EmberNodeId destination, EmberApsFrame * apsFrame, uint16_t messageLength,
+//                                                      uint8_t * message, EmberNodeId alias, uint8_t sequence,
+//                                                      EmberAfMessageSentFunction callback);


Can we just completely delete those methods ?

I was trying to decide that.... I guess at this point we're not going to pull in any more silabs code that might use them, so you might be right that we can.

woody-apple · 2021-05-04T16:10:41Z

@mspang @saurabhst ?

mspang · 2021-05-04T10:43:18Z

src/app/util/chip-message-send.h

+ * @param[in] message The message to send after the APS frame.
+ * @param[in] sendFlags The SendFlags needed, if any.
+ */
+EmberStatus chipSendUnicast(chip::Messaging::ExchangeContext * exchange, EmberApsFrame * apsFrame, uint16_t messageLength,


nit: capitalize?

This is following the existing style (for both this file and this directory), for now. I am happy to change that style, but feel like we should do that in a coordinated way across the whole thing.

src/app/util/util.cpp

yufengwangca · 2021-05-04T17:44:07Z

src/app/util/af-types.h

    /**
     * APS frame for the incoming message
     */
    EmberApsFrame * apsFrame;
    EmberIncomingMessageType type;
-    chip::NodeId source;
+    chip::Messaging::ExchangeContext * source;


The exchange on the receiving side is closed immediately within unsolicited message handler.

void OnMessageReceived(Messaging::ExchangeContext * exchangeContext, const PacketHeader & packetHeader,
const PayloadHeader & payloadHeader, System::PacketBufferHandle buffer) override
{
.....
{
HandleDataModelMessage(exchangeContext, std::move(buffer));
}

exit: exchangeContext->Close();

}

The passed-in exchange in HandleDataModelMessage get populated to returnCmd struct and this struct is used in callback stubs of different bindings, how can we make sure the passed-in exchange is not dangling pointer before use?

It's used to synchronously, before HandleDataModelMessage returns, send a response.

So the API contract for now is that HandleDataModelMessage must respond sync. If that changes, we will need to condition the Close here on HandleDataModelMessage not being reached and hand off closing to HandleDataModelMessage.

Currently, all stub functions are not filled
bool attribute((weak)) emberAfPreCommandReceivedCallback(EmberAfClusterCommand * cmd)
{
return false;
}

If we implement those callbacks later, it can not be still synchronously if the response needs to be sent from different bindings, right?

The exchange needs to be closed at the end of the 'conversation', this syntax is defined within the protocol since it has the context to know which message mark the end of the conversation.

For TempZCL protocol without a protocol syntax defined, I feel it is more safe to use a dedicated exchange to send request and response message separately.

I'm not sure what you mean by "the response needs to be sent from different bindings".

Right now, the contract is that emberAfPreCommandReceivedCallback would need to send a response, if any, before returning.

Again, we could change this contract. I don't think we should do it in this PR.

For TempZCL protocol without a protocol syntax defined, I feel it is more safe to use a dedicated exchange to send request and response message separately.

There are a few problems with that:

It leads to problems correlating responses with requests.

It causes our existing tests to not exercise various exchange-related code (like CRMP). Note that in the process of working on this PR I ended up finding various controller and CRMP bugs that were due precisely due to that functionality not being exercised in existing tests.

In general, everything in TempZCL is currently of the form (1 request, 1 response). The one exception I can think of is whether we ever send Default Response from the initiator side (in response to a non-status response). But since the responder side ignores those responses anyway, it doesn't matter too much that it closes its exchange before it receives them.

Yes, I think we can still benefit more from this change if we can guarantee HandleDataModelMessage must handle the respond synchronously.

CRMP is disabled since separate exchange is used for request and response, should we enable CRMP now?

This PR enables CRMP on the DeviceController side and enables it for responses to an incoming message on the data model side. The one place we are still disabling it is for data-model-initiated sends to a specific node (e.g. attribute reporting).

yufengwangca · 2021-05-04T21:31:05Z

src/app/util/chip-message-send.cpp

@@ -106,18 +120,15 @@ EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16
    // receive the ack message. This logic needs to be deleted after we convert all legacy ZCL messages to IM messages.
    DeviceExchangeDelegate delegate;
    exchange->SetDelegate(&delegate);
+
    Messaging::SendFlags sendFlags;

    sendFlags.Set(Messaging::SendMessageFlags::kFromInitiator).Set(Messaging::SendMessageFlags::kNoAutoRequestAck);


Do we still need to disable CRMP after use same exchange to send response?

This is the codepath where we're sending to a specific node by node id, so still creating a temporary exchange. For attribute reporting, for example. To stop disabling CRMP here we would need to have a nontrivial exchange delegate that actually handles the messages from the other side properly and closes the exchange as needed. We can do that, but I was trying to not scope-creep this PR.

woody-apple · 2021-05-05T05:35:10Z

@bzbarsky-apple Darwin CI is failing :(

yunhanw-google · 2021-05-05T13:43:53Z

in addition, @woody-apple @bzbarsky-apple python device controller in cirque test is consistently failing with this change, something wrong inside exchange manager/crmp

2021-05-05 13:32:52,434 [CHIPMobileDevice] INFO Send EnableNetwork command to device 1
CHIP:ZCL: Successfully encoded 11 bytes
CHIP:IN: Secure message was encrypted: Msg ID 1
CHIP:IN: Sending msg from 0x000000000001b669 to 0x0000000000000001 at utc time: 917257 msec
CHIP:IN: Sending secure msg on generic transport
CHIP:IN: Secure msg send status No Error
CHIP:CTL: SendMessage returned No Error
CHIP:ZCL: Data model processing success!
CHIP:EM: Sending Standalone Ack for MsgId:00000000
CHIP:IN: Secure message was encrypted: Msg ID 2
CHIP:IN: Sending msg from 0x000000000001b669 to 0x0000000000000001 at utc time: 917258 msec
CHIP:IN: Sending secure msg on generic transport
CHIP:IN: Secure msg send status No Error
CHIP:EM: Flushed pending ack for MsgId:00000000
CHIP:IN: Sending msg from 0x000000000001b669 to 0x0000000000000001 at utc time: 922244 msec
CHIP:IN: Sending secure msg on generic transport
CHIP:IN: Secure msg send status No Error
CHIP:EM: Retransmit MsgId:00000001 Send Cnt 1
CHIP:IN: Sending msg from 0x000000000001b669 to 0x0000000000000001 at utc time: 922495 msec
CHIP:IN: Sending secure msg on generic transport
CHIP:IN: Secure msg send status No Error
CHIP:EM: Retransmit MsgId:00000001 Send Cnt 2
CHIP:IN: Sending msg from 0x000000000001b669 to 0x0000000000000001 at utc time: 922752 msec
CHIP:IN: Sending secure msg on generic transport
CHIP:IN: Secure msg send status No Error
CHIP:EM: Retransmit MsgId:00000001 Send Cnt 3
CHIP:EM: Failed to Send CHIP MsgId:00000001 sendCount: 3 max retries: 3
CHIP:EM: CHIP MsgId:00000001 not in RetransTable
CHIP:EM: CHIP MsgId:00000001 not in RetransTable
CHIP:EM: CHIP MsgId:00000001 not in RetransTable
CHIP:EM: CHIP MsgId:00000001 not in RetransTable

… functions.

Sending a reply needs a non-const ExchangeContext.

… for responses.

…reating a new one.

github-actions · 2021-05-05T19:36:55Z

Size increase report for "esp32-example-build" from 6b0d0d1

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	84	84
chip-all-clusters-app.elf	.dram0.bss	0	-16

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,909576
.debug_abbrev,0,40660
.debug_line,0,24079
.debug_str,0,1018
.strtab,0,627
.debug_loc,0,408
.flash.text,84,84
.debug_ranges,0,72
.xt.lit._ZN4chip9Messaging21MessageCounterSyncMgrD5Ev,0,40
.symtab,0,32
.xt.prop._ZTVN4chip24LifetimePersistedCounterE,0,3
.shstrtab,0,1
.debug_aranges,0,-8
.dram0.bss,-16,0
.debug_frame,0,-24
.xt.lit._ZNK4chip8OptionalIyE5ValueEv,0,-40

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

sections,vmsize,filesize

github-actions · 2021-05-05T19:53:22Z

Size increase report for "nrfconnect-example-build" from 6b0d0d1

File	Section	File	VM
chip-lock.elf	[LOAD #3 [RW]]	0	19
chip-lock.elf	text	12	12
chip-lock.elf	device_handles	4	4
chip-lock.elf	bss	0	-19
chip-lock.elf	rodata	-152	-148
chip-lighting.elf	text	12	12
chip-lighting.elf	device_handles	-12	-12
chip-lighting.elf	rodata	-144	-148

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,550982
.debug_abbrev,0,23205
.debug_line,0,17445
.debug_str,0,931
.strtab,0,366
.debug_ranges,0,144
[LOAD #3 [RW]],19,0
text,12,12
device_handles,4,4
.shstrtab,0,2
.debug_aranges,0,-16
bss,-19,0
.symtab,0,-32
.debug_frame,0,-56
rodata,-148,-152
.debug_loc,0,-547

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

sections,vmsize,filesize
.debug_info,0,589137
.debug_line,0,16171
.debug_abbrev,0,16140
.debug_str,0,931
.strtab,0,366
.debug_ranges,0,144
text,12,12
.shstrtab,0,2
device_handles,-12,-12
.debug_aranges,0,-16
.symtab,0,-32
.debug_frame,0,-56
rodata,-148,-144
.debug_loc,0,-499

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

sections,vmsize,filesize
.debug_info,0,349
.debug_loc,0,309
.debug_line,0,179
.debug_str,0,123
.debug_ranges,0,88
.debug_frame,0,32
.debug_aranges,0,8

bzbarsky-apple requested a review from vivien-apple May 4, 2021 01:15

boring-cyborg bot added app examples labels May 4, 2021

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, saurabhst, selissia and woody-apple May 4, 2021 01:17

bzbarsky-apple force-pushed the exchange-context-response branch from 31d2627 to fdaeada Compare May 4, 2021 02:24

restyled-io bot mentioned this pull request May 4, 2021

Restyle Use the incoming exchange context to send responses in the data model #6439

Closed

bzbarsky-apple force-pushed the exchange-context-response branch from fdaeada to 3c98260 Compare May 4, 2021 02:40

msandstedt approved these changes May 4, 2021

View reviewed changes

bzbarsky-apple force-pushed the exchange-context-response branch from 3c98260 to 00caede Compare May 4, 2021 04:51

mspang requested a review from yufengwangca May 4, 2021 10:48

vivien-apple approved these changes May 4, 2021

View reviewed changes

bzbarsky-apple force-pushed the exchange-context-response branch 4 times, most recently from f7ec910 to 47868f7 Compare May 4, 2021 15:00

woody-apple approved these changes May 4, 2021

View reviewed changes

mspang approved these changes May 4, 2021

View reviewed changes

yufengwangca requested changes May 4, 2021

View reviewed changes

bzbarsky-apple force-pushed the exchange-context-response branch 2 times, most recently from 15f50f7 to ba2fa24 Compare May 4, 2021 18:04

bzbarsky-apple requested a review from yufengwangca May 4, 2021 18:05

bzbarsky-apple force-pushed the exchange-context-response branch from ba2fa24 to f182a42 Compare May 4, 2021 20:22

yufengwangca reviewed May 4, 2021

View reviewed changes

yufengwangca approved these changes May 4, 2021

View reviewed changes

bzbarsky-apple force-pushed the exchange-context-response branch 2 times, most recently from ff268a0 to c7ad3a4 Compare May 5, 2021 03:38

bzbarsky-apple added 2 commits May 5, 2021 10:09

Switch to a union for the indexOrDestination argument to message send…

e8a8c25

… functions.

Return a non-const ExchangeContext from Command's getter.

cfc6eff

Sending a reply needs a non-const ExchangeContext.

bzbarsky-apple force-pushed the exchange-context-response branch 2 times, most recently from 6cf1e5a to c33f55b Compare May 5, 2021 15:36

restyled-io bot mentioned this pull request May 5, 2021

Restyle Use the incoming exchange context to send responses in the data model #6501

Closed

bzbarsky-apple added 4 commits May 5, 2021 12:13

Propagate the incoming-message exchange context down so we can use it…

f64dd1a

… for responses.

Add a way to Close() all the exchange contexts for a given delegate.

af0e577

Use the incoming exchange context to send reply messages instead of c…

81a7f24

…reating a new one.

Regenerate generated files

c0bb202

bzbarsky-apple force-pushed the exchange-context-response branch from c33f55b to c0bb202 Compare May 5, 2021 16:13

bzbarsky-apple merged commit 0fe16aa into project-chip:master May 5, 2021

bzbarsky-apple deleted the exchange-context-response branch May 5, 2021 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the incoming exchange context to send responses in the data model #6438

Use the incoming exchange context to send responses in the data model #6438

bzbarsky-apple commented May 4, 2021

vivien-apple May 4, 2021

bzbarsky-apple May 4, 2021

woody-apple commented May 4, 2021

mspang May 4, 2021

bzbarsky-apple May 4, 2021

yufengwangca May 4, 2021 •

edited

Loading

bzbarsky-apple May 4, 2021

bzbarsky-apple May 4, 2021

yufengwangca May 4, 2021 •

edited

Loading

yufengwangca May 4, 2021

bzbarsky-apple May 4, 2021

bzbarsky-apple May 4, 2021

yufengwangca May 4, 2021

bzbarsky-apple May 4, 2021

yufengwangca May 4, 2021

bzbarsky-apple May 4, 2021

woody-apple commented May 5, 2021

yunhanw-google commented May 5, 2021

github-actions bot commented May 5, 2021

github-actions bot commented May 5, 2021

Use the incoming exchange context to send responses in the data model #6438

Use the incoming exchange context to send responses in the data model #6438

Conversation

bzbarsky-apple commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woody-apple commented May 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yufengwangca May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yufengwangca May 4, 2021 • edited Loading

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

woody-apple commented May 5, 2021

yunhanw-google commented May 5, 2021

github-actions bot commented May 5, 2021

github-actions bot commented May 5, 2021

yufengwangca May 4, 2021 •

edited

Loading

yufengwangca May 4, 2021 •

edited

Loading