Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Initial support for adding codegen for interaction model #4298
Initial support for adding codegen for interaction model #4298
Changes from 3 commits
ce94756
985c180
2b6955c
9c96f22
a36e832
8025933
0495551
d68c569
d4c885f
8bd2e32
0fc9435
22d4b27
ad17020
3575e10
99f183c
b07106c
19a979e
2821af0
4793938
f908c8a
53b6c53
d943b36
464cdb3
375bd5e
6c9cd33
bfa1bec
8c55141
39a7ea5
b3ac301
0f77b89
f91173a
1d62044
bc8d316
2af3084
30ec612
f36d0c7
e51686b
9ee85c8
1d6d2ac
a44c5d2
cea3c04
4193657
8072f39
23de978
3a995e0
8a007c1
5d7370f
49ceaf9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said, the fact that
examples/lighting-app
are the direct consumer of the IM looks wrong.If that was me, an in order to move forward with the encoder/decoder part of the IM, I will get rid of those callbacks, and get
DataModelHandler
to directly consumechip::app::Command
so it can be dispatched properly to ZCL.As a result it means there should be no changes to any of the
examples/
since IM will be considered an encoding/decoding scheme.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this file is quite misleading, so I moved it to src/app/clusters/on-off-server. Sorry for misleading.
I agree that the app should not handle commands directly, and there are some issues if we re-encode the message for current DataModelHandler.
The first one is the content of the message, as you can see, DataModelHandler takes raw packet from SecureSessionManager, but the Commands takes cooked message from ExchangeManager. There are some gaps between the SecureSessionManager and ExchangeManager.
Another issue is how to deal with outgoing messages. The content of return message by current zcl library is not consistent from the messages required by IM model.
After reading the code, I guess one possible solution is to call callbacks from src/app/clusters, (the function body is not implemented as this is not the target of this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About differences between incoming/outgoing messages from
SecureSessionManager
/ExchangeManager
that's the part we need to figure out.In fact this is where the hard part of the patch lives :)
Especially for outgoing messages I would say since the IM design possibly required the results of various commands and I believe this is not how the ZCL code is designed
As a stopgap I would first focus on dispatching an IM packet with a single command, that expects a single command response, and see how to integrate it into the current ZCL code. Once we are here it will requires a few other patches to get to the point where we can aggregate the result of various commands to be sent back as a response
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can find that the interaction model engine is using std::map, I will submit another PR after this to remove the std::map from IME, and there might be a new API like
DispatchCommand(chip::ClusterId aClusterId, chip::CommandId aCommandId, chip::TLV::TLVReader & aReader, Command * apCommandObj, Command::CommandRoleId aCommandRoleId)
. Which should be useful for you to make tests.Large diffs are not rendered by default.
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse
src/app/zap-templates/templates/chip/CHIPClusters.zapt
instead of adding a new interface to use the IM code ?Basically I would like to get rid of
src/app/encoder.cpp
at some point, but in the current state some apps andchip-tool
still use it so I have kept it instead of directly generating the right code into theCHIPClusters-src.zapt
template.There are a few differences between the 2 templates but it can probably be unified.
The current namespace defined by
CHIPClusters.h
ischip::Controller::{{asCamelCased name false}Cluster
while this one is underchip::app::cluster::{{asCamelCased name false}}
. It makes more sense to usechip::app::cluster
imho. FWIW I have reuse the namespacechip::Controller
since the initial landing ofCHIPCluster
was using it and I was trying to minimise the changes.CHIPCluster
makes the cluster classes to inherit fromsrc/controller/CHIPCluster.h
which has theendpointId
and theclusterId
has protected member. This template also usesgroupId
. It can be added toClusterBase
.CHIPCluster
usesCallback::Callback<> * onCompletion
as the first command parameter. A new method can be generated that takes the same arguments list but replace the callback bychip::app::Command * ZCLcommand
As an example for the
GoToPercent
method ofBarrierControl
, 2 methods can be generated until everything is switched to the IM version:CHIPCluster
does not usesconst
for the parameters while this template does. Sounds good to me to use it.About
InitCluster
andShutdownCluster
, the way their work is still unclear to me.My current understanding of the code is that
InitCluster
is used to register callbacks methods when a method is received ? If so it means that it bypass all the ZCL imported code fromsrc/app
which looks wrong to me. But maybe I have misunderstood the code.I would like to discuss those points before going into a more granular review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a glance of the CHIPCluster.zapt here are some ideas:
EncodeXXXCommand
toXXXCluster
class?InitCluster
does registerss callbacks and current ZCL implementation does not works since they does not go through the MessageLayer, which is required by the InvokingCommands, since the imported ZCL does not support MessageLayer api, it does not work currently. So I adds a build flag so current code still works. This is a follow up and we can deal with it later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
I think it would be better if we all work on the same class instead of having some code that uses
chip::Controller::XXXCluster
class and some other that useschip::app:cluster::XXX
class.If we do that, my understanding is that the core difference between the current
XXXCluster
methods and what you did as specified by the IM, is that in the first case the command is sent directly along with a callback per command, while in your case achip::app:Command
parameter will be passed along to multiple clusters before a higher level APIs decides to send it.I guess my main concern is that this patch moves handling of packets received from
src/app
to the application side directly, effectively bypassing the ZCL internals. And the current patch does not makes it clear to me what is the plan for handling received packets, dispatching them to the ZCL internals, before dispatching them to the possible application handlers.Can we move this encoding/decoding scheme forward without the
ExchangeMgr
at the moment ?For example can we send the IM encoded packets over the wire using our network code, decode the IM packet inside
connectedhomeip/src/app/server/DataModelHandler.cpp
Line 46 in d05149c
That would be a first step towards replacing the current encoding code.
From here (maybe in a different PR) one may be able to pass the retrieved
chip::app:Command
packet toemberAfProcessMessage
instead of the rawuint8_t *
parameter.What do you think ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this PR, the Handlers for invoking commands is missing, and they should placed in
src/app/clusters
, the handlers inexamples/lighting/linux
is a temporary solution so the linux example app can use the IM for ZCL.Some follow up PRs are needed for the actual command handlers.