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.

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

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

Open
Apollon77 opened this issue Jun 9, 2024 · 12 comments
Open
Labels
documentation Improvements or additions to documentation needs triage

Comments

@Apollon77
Copy link
Contributor

Apollon77 commented Jun 9, 2024

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

@Apollon77 Apollon77 added documentation Improvements or additions to documentation needs triage labels Jun 9, 2024
@tehampson
Copy link
Contributor

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
Copy link
Contributor

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

@Apollon77
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

Apollon77 commented Jun 10, 2024

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
Copy link
Contributor Author

Apollon77 commented Jun 13, 2024

@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

* @def CHIP_SYSTEM_HEADER_RESERVE_SIZE
and - 16for MIC). So there should fit more in one UDP MTU. Or where I am wrong now?

@tehampson
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

Apollon77 commented Jun 15, 2024

For completeness: my calculations base on

* @def CHIP_SYSTEM_HEADER_RESERVE_SIZE
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
Copy link
Contributor Author

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:

  • MAX_UDP_MESSAGE_SIZE = 1280 - 48 (Min ipv6 MTU minus 48 bytes IP and UDP header size for IPv6) = 1232 bytes
  • minus 26 (Matter Message Header)
  • minus 12 (Matter Payload Header)
  • minus 16 bytes MIC (128 byte)

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
Copy link
Contributor

tehampson commented Jun 20, 2024

Summary of chat on Slack:

  • Section 8.8 as the entire interaction as a whole, and not the individual messages that make up the interaction. Technically the spec text right now is right as is.
  • It is hard to read this section of the spec text so it still might be worthwhile to add a note possible:
NOTE: Sending the |NO_COMMAND_RESPONSE| should only be done after we have received
all encoded/transported messages that indicates the interaction is completed and response is
missing for those requests.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation needs triage
Projects
None yet
Development

No branches or pull requests

2 participants