[IM] Decouple CommandSender with CHIPDevice #6802
Conversation
pullapprove
bot
requested review from
chrisdecenzo,
jelderton,
jmartinez-silabs,
LuDuda,
mspang,
saurabhst,
vivien-apple,
wehale and
woody-apple
erjiaqing
force-pushed
the
im-multiple-commands
branch
from
5db90a0 to
bdfe43f
Compare
|
@cjandhyala Please verify if this PR fixes #6797, when testing against all-cluster-apps, thanks. |
|
Confirmed that this works with Pankaj's Op-cert change using chip-device-tool and BLE connection. Also works for the upcoming IP commissioning stuff. Thanks - much cleaner than my previous hacks. |
yunhanw-google
force-pushed
the
im-multiple-commands
branch
from
bdfe43f to
573a5ad
Compare
|
|
||
| return mDevice->SendCommands(); | ||
| return mDevice->SendCommands(mpCommandSender); |
There was a problem hiding this comment.
So this isn't actually batching up multiple commands into a single InvokeInteraction right? This is still very much, 1 command PER invoke interaction?
There was a problem hiding this comment.
Yes, when talking about "sending multiple commands", we need to
- Make it possible to have multiple transactions between controller and device.
- The transactions can carry more than one commands.
This PR is focus on the first one, and the second will be implemented in another PR.
There was a problem hiding this comment.
I'd suggest holding off on 2, since as part of the proposal/prototype I'm writing up, I'm combining CommandSender/CommandHandler/Command into a single 'InvokeInteraction' class that will handle:
- Permitting multiple commands to be bundled in a single invoke
- Allow for async responses to be sent
- Permit multiple invoke interactions over a single exchange AND InvokeInteraction object instance, to reduce allocation/de-allocation transients.
That being said, I'm also not certain what the real use-case for 2) is right now, since everything I've seen so far would only necessitate single-command-in-invoke interactions. Is there an example of it that is needed in the upcoming TEs?
There was a problem hiding this comment.
Block transfer for OTA comes to mind.
| { | ||
| chip::app::CommandSender * commandSenderObj = nullptr; | ||
| VerifyOrReturnError(commandSender != nullptr, CHIP_ERROR_INVALID_ARGUMENT); | ||
| ReturnErrorOnFailure(chip::app::InteractionModelEngine::GetInstance()->NewCommandSender(&commandSenderObj)); |
There was a problem hiding this comment.
Why does it even have to be allocated out of a pool? Why can't the allocation of the CommandSender be managed by the application here?
There was a problem hiding this comment.
I would still prefer to make im engine manage the command sender.
According to the encoding spec, command is a one time transaction, so the application can dispose the command sender once they finished the sending routine.
Another concern is that we should avoid the use of heap if possible and we still need to pass the reference (and maybe ownership) of command sender to im to avoid use after free issue.
There was a problem hiding this comment.
I would still prefer to make im engine manage the command sender.
Haven't provided a reason.
According to the encoding spec, command is a one time transaction, so the application can dispose the command sender once they finished the sending routine.
Doesn't mean it has to be allocated by the IM...application can still dispose of the command if they own the allocation for it (in fact, it's even easier...).
Another concern is that we should avoid the use of heap if possible and we still need to pass the reference (and maybe ownership) of command sender to im to avoid use after free issue.
It doesn't have to allocate out of heap. It however, has more flexibility to decide between heap OR pool-based.
There was a problem hiding this comment.
So for now, I think we can hold-off on conversion to any kind of 'application-allocated' model for CommandSender, till we review the upcoming proposal I'll put out on InvokeInteraction as a concept. If we think that makes sense, we can then re-assess how exactly the allocation and ownership model should be for that.
|
@erjiaqing all-clusters-app on your branch also failed with 'Error occurred calling SyncSetKeyValue.' when connecting to the device Device log$ sudo ./out/host/chip-all-clusters-app --wifi |
erjiaqing
force-pushed
the
im-multiple-commands
branch
from
573a5ad to
2905d94
Compare
Make number of command handler and command sender configurableconnectedhomeip/src/app/InteractionModelEngine.h Lines 50 to 55 in 2905d94 This comment was generated by todo based on a
|
Can you try cherry-pick #6840 on server side to see if this issue persists? |
|
Note: Test log on chip-device-ctrl / linux https://gist.github.com/erjiaqing/e7debaa4cc760f2e0506e71b0cbf952d And pairing over BLE https://gist.github.com/erjiaqing/c0df573aacdc85c8f449e63e665be966 |
yunhanw-google
force-pushed
the
im-multiple-commands
branch
from
5617981 to
39f3346
Compare
There was a problem hiding this comment.
Holding based on comments from @mrjerryjohns and review from @pan-apple @vivien-apple
erjiaqing
force-pushed
the
im-multiple-commands
branch
from
39f3346 to
16de55b
Compare
erjiaqing
force-pushed
the
im-multiple-commands
branch
from
16de55b to
3980654
Compare
|
@vivien-apple By decouple CommandSender with CHIPDevice, the CHIPDevice no longer holds the CommandSender object, which allows sending multiple commands (having multiple command transactions) to the same device in parallel, the IMEngine will hold the ownership of CommandSedner and maintains its lifespan. @pan-apple This PR also updated some API used in controller to send commands during pairing. Tested locally and @cecille reports this PR works. |
|
Size increase report for "nrfconnect-example-build" from 8a51de2
Full report output |
|
Size increase report for "esp32-example-build" from 8a51de2
Full report output |
There was a problem hiding this comment.
The changes looks fine to me. I would still like @pan-apple to look at the changes to fix the operational credentials things.
Also, in my understanding this only partially fix #6560 right ? One can now have one in-flight commands per cluster, but can not send multiple commands for the same cluster right ? This is still much better than the previous situation but I just want to make sure I did understand correctly what's going on.
|
@woody-apple - your "change requested" was on pending from Jerry, Vivien and Pankaj, all of which gave checkmarks here. Assuming your change requested is now "ok". |
Problem
Current IM command interaction can send multiple command in one interaction, but device controller has limited this capabilit with one command, actually the ember needs to send multiple commands in one invoke command interaction in some user scenario,
Summary of Changes
After this PR, the CHIPDevice nolonger holds the commandSender object, for backward compatibility, the ClusterObjects will accept an optional CommandSender object, if not provided, it will request one from IMDelegate.
This also uses a pool of CommandSender with two objects (previous one), this will allow sending multiple commands at the same time.
This also fixes the broken chip-device-ctrl after Integrate commissioner and device clusters with operational credentials #6705 introduced OpCred provisioning.
Note: The API for sending multiple commands at the same time should be ready after this PR, it is used by OpCred provisioning routine now, will add Python api support for more commands later.
Fixes #6560
Also Fixes #6797, fixes #6803.