project-chip / connectedhomeip

Matter (formerly Project CHIP) creates more connections between more objects, simplifying development for manufacturers and increasing compatibility for consumers, guided by the Connectivity Standards Alliance.
https://buildwithmatter.com
Apache License 2.0
7.18k stars 1.9k forks source link

[Documentation] Partial Invoke Responses are very unclear in Specification #33815

Open Apollon77 opened 4 weeks ago

Apollon77 commented 4 weeks ago

Documentation issues

On one hand the specification define in .8.2.3. Incoming Invoke Request Action - Invoke Execution

An Invoke Response Generation SHALL be performed on completion of all valid commands. A partial Invoke Response Generation MAY be performed at any time when processing multiple commands, provided it carries at least a single response.

On the other hand 8.8.3.3. Incoming Invoke Response Action is written that only one of such response should be received, especially also because

For each entry in the initial Invoke Request action that triggered this incoming Invoke Response action, if there isn’t a corresponding entry in InvokeResponses, a CommandStatusIB with the NO_COMMAND_RESPONSE status SHALL be submitted to the layer above.

In fact that would mean that as soon as the first partial Respone is received the "above layer" already got NO_COMMAND_RESPONSE for any data not in that first response.

With the information written in "10.7.10.2. MoreChunkedMessages" one can assume that the above logic should happen as soon as the "last message" 8the one with moreChunkedMessages=false" was arrived. Is this the right assumption and interpretation?

Platform

No response

Anything else?

No response

tehampson commented 3 weeks ago

With the information written in "10.7.10.2. MoreChunkedMessages" one can assume that the above logic should happen as soon as the "last message" 8the one with moreChunkedMessages=false" was arrived. Is this the right assumption and interpretation?

Yes that is correct. It is something provided by Interaction Model layer to let the Application layer know that the server has not sent a response to that invoke interaction at the time that the server has indicated that the Invoke interaction is completed.

tehampson commented 3 weeks ago

@tehampson to get emails about this thread with how my email filters are set up

Apollon77 commented 3 weeks ago

Thank you for verifying my assumptions. I think it could help to redce the confusion a bit whe the spec contains an additional sentence about it.

One side question for "multiple invokes": Whats a good "max number" if there is not natural limit because of RAM or such? Ok there is a natural maximum for UDP based messages, like 57 invokes as max ... but with TCP it could be more, right? In fact it is a uint16, but I think it makes no sense to allow 65k invokes ;-)

tehampson commented 3 weeks ago

Thank you for verifying my assumptions. I think it could help to redce the confusion a bit whe the spec contains an additional sentence about it.

Do you have a recommendation on what to change/add? If you don't that is fine, it's just that I helped fix some part of the spec here so sometime I may be to close and may think that certain aspects are already.

One side question for "multiple invokes": Whats a good "max number" if there is not natural limit because of RAM or such? Ok there is a natural maximum for UDP based messages, like 57 invokes as max ... but with TCP it could be more, right? In fact it is a uint16, but I think it makes no sense to allow 65k invokes ;-)

yeah with UDP I found the max to tap out at around 57 command in the packet. But as you said TCP support is coming along.

Without knowing what exactly you are trying to optimize for it is hard to provide a recommendation. Also depends on server or client?

Server - whatever the constraint server device want to support up to the max of uint16 (IIRC). This is essentially an optional feature if the device doesn't want to support it at all they can just say MAX_PATHS_PER_INVOKE and nothing should ever send a batch command to it.

Client - depends on the constrains of the client device. If it is a fairly sizable device why not support whatever a theoretical max server code support 65k? Who know what the future hold and if there is a very large bridge device that may be on an industrial scale. That said a client that supports sending out batch commands needs to be dynamic to a degree. If your API accepts batch of n, but the server only supports receiving 1, the client code already need to handle splitting that work up to n individual commands, so you could use that same logic to break it down to whatever you think that clients max should be. So the API could accept 65k, but your implementation breaks it up to chunks of whatever you think a reasonable.

Apollon77 commented 3 weeks ago

Will think about textual ideas and provide here tomorrow.

For the Max-Invoke-Paths: I completely agree with you, because in fact I miss a bit the real world use cases for it. In fact it is about the "default max invoke paths" for matter.js as a framework (that can be overridden by the developer if needed) ... so my feeling is to go with something like 10 untell someine has good reasonsn for more - or stay at 1 :-)

And sure the controller/Client need to check how to send stuff and depending what the server allows it need to do multiple messages or send one.

Thank you for your feedback"

Apollon77 commented 3 weeks ago

@tehampson one question because of the 57 invokes max per UDP message. I yesterday tried to check that and with 57 simple toggle invokes with commandRef I end up with encoded invoke request message of 981 byte. That’s far away from 1280-48-26-12-4=1190. (1280 - headers see https://github.com/project-chip/connectedhomeip/blob/2d97cda23024e72f36216900ca667bf1a0d9499f/src/system/SystemConfig.h#L327 and - 16for MIC). So there should fit more in one UDP MTU. Or where I am wrong now?

tehampson commented 3 weeks ago

Interesting observation, I haven't looked at a packet to see how it got filled up, maybe we are leaving some space unused. I have only used SDK to fill up a packet as much as a I could before failure.

Something that might be worth while to investigate. I wonder if we are similarly under filling other packets like a Read

Apollon77 commented 3 weeks ago

I also just discovered that matter.js only fills till 1024 byte payload (so except headers and such) ... whyever ;-) and while fixing this I came around that. So question is now if I am missing anything regarding max size ;-) I am off next days. But that's my next topic to check when back Monday.

tehampson commented 3 weeks ago

I know that there is supposed to be some space reserved in the various headers essentially for future use, but your measurement is close to 200 bytes so it is worth looking into. I am also off tomorrow so let's pick this up next week.

Apollon77 commented 3 weeks ago

For completeness: my calculations base on https://github.com/project-chip/connectedhomeip/blob/2d97cda23024e72f36216900ca667bf1a0d9499f/src/system/SystemConfig.h#L327 and the infos I found there and where the variables are used. I might have missed a places, but I did not saw such reserves (except for e.g. mt793x platform where additional bytes gets added as reserve).

More interesting I more miss the 16 byte MIC to be added as bytes on the above link. So yes seems I miss some code places that does that chunking size calculation.

Apollon77 commented 2 weeks ago

For ideas on multi invioke descriptions lets chat hopefully in slack somewhen next.

For the size:

So here my calculation for max size that in theory should be like in chip, but I do not really 100% overlook the code tbh:

And I end up with 1178 bytes then for max message "payload" - sure all calculated without any "message extensions" or "secured extensions".

With this I get 69 "invokes without any parameter" into one Message ... ending at 1168 bytes :-)

In matter.js we also had a max message size defined as of 1024 bytes till now and this value was from our "early times" before Matter 1.0 got published ... so was reverse engineered.

I did one test now with matter.js: I just increased the limit to the said above instead of 1024 bytes, added some logging and started a device commissioned with Apple Home. With this the initial subscription priming was also chunked at a higher level and was working ... so at least apple was ok with it

Sending DataReport chunk with 33 attributes and 0 events: 1162 bytes Sending UDP message 238687343 to udp://fe80::10ce:9f8c:72de:3cca%en0:60030 on session secure/48451 with 1196 bytes. Sending DataReport chunk with 31 attributes and 0 events: 952 bytes Sending UDP message 238687344 to udp://fe80::10ce:9f8c:72de:3cca%en0:60030 on session secure/48451 with 986 bytes. Sending DataReport chunk with 7 attributes and 0 events: 1153 bytes Sending UDP message 238687345 to udp://fe80::10ce:9f8c:72de:3cca%en0:60030 on session secure/48451 with 1187 bytes. Sending DataReport chunk with 17 attributes and 0 events: 1177 bytes Sending UDP message 238687346 to udp://fe80::10ce:9f8c:72de:3cca%en0:60030 on session secure/48451 with 1211 bytes. Sending DataReport chunk with 38 attributes and 0 events: 1159 bytes Sending UDP message 238687347 to udp://fe80::10ce:9f8c:72de:3cca%en0:60030 on session secure/48451 with 1193 bytes. Sending DataReport chunk with 19 attributes and 2 events: 697 bytes Sending UDP message 238687348 to udp://fe80::10ce:9f8c:72de:3cca%en0:60030 on session secure/48451 with 731 bytes.

So it all stayed below the max 1232 bytes UDP max and actual matter overhead was 34 bytes in reality ... so 16 byte MIC and 18 byte headers ... seems ok ...

No idea which reserves should be planned in given the fact that we have no extensions in use

tehampson commented 2 weeks ago

Summary of chat on Slack:

I will do a spec text writeup after I am back from vacation on June 2nd