Server side event support #12059
Closed
Server side event support #12059
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not yet completely hooked up to the EventManagement but the key aspect of the API present.
The event is being logged from the TestEmitTestEventRequest command, which both provides a flavor the API and sets us up for a loopback/unit test
|
PR #12059: Size comparison from 5bad4ed to 6708064 Increases (1 build for linux)
Decreases (1 build for esp32)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
src/app/EventLogging.h
Outdated
|
|
||
| #include <app/ConcreteEventPath.h> | ||
| #include <app/EventLoggingDelegate.h> | ||
| #include <app/data-model/Decode.h> |
There was a problem hiding this comment.
Suggested change
| #include <app/data-model/Decode.h> | |
| #include <app/data-model/Encode.h> |
| }; | ||
|
|
||
| template <typename T> | ||
| CHIP_ERROR LogEvent(const T & aEventData, EndpointId aEndpoint, EventOptions aEventOptions, EventNumber & aEventNumber) |
There was a problem hiding this comment.
We need to decide whether we're doing the EventSchema thing (which would mean endpoint, etc is included in the EventOptions) or whether we are getting the cluster and event id from the data type and passing in the endpoint like this.
Seems to me that EventSchema should just go away.
That said event priority is not statically deducible from the data type, so we will need to pass that in, via either the EventOptions or some other mechanism.
There was a problem hiding this comment.
Agreed.
I'd rather we consolidate around EventOptions, remove EventSchema (schema is poor term for it anyways) and consequently, remove mpEventSchema from EventOptions.
| }; | ||
|
|
||
| template <typename T> | ||
| CHIP_ERROR LogEvent(const T & aEventData, EndpointId aEndpoint, EventOptions aEventOptions, EventNumber & aEventNumber) |
There was a problem hiding this comment.
aEventNumber needs documentation. Is this an outparam? And in/out param? What purpose does it serve?
When would it be used, outside of our test command?
There was a problem hiding this comment.
EventNumber is outParam,
Yes, we need to consolidate around EventOption, remove EventSchema, and put ConcreteEventPath as a member inside EventOption
There was a problem hiding this comment.
EventNumber is outParam,
My point is that this should be documented, not in a github comment.
Comment on lines
358
to
359
| chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath, | ||
| const chip::app::Clusters::TestCluster::Commands::TestEmitTestEventRequest::DecodableType & commandData) |
There was a problem hiding this comment.
Suggested change
| chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath, | |
| const chip::app::Clusters::TestCluster::Commands::TestEmitTestEventRequest::DecodableType & commandData) | |
| CommandHandler * commandObj, const ConcreteCommandPath & commandPath, | |
| const Commands::TestEmitTestEventRequest::DecodableType & commandData) |
Comment on lines
+362
to
+363
| DataModel::List<const Structs::SimpleStruct::Type> arg5; | ||
| DataModel::List<const SimpleEnum> arg6; |
There was a problem hiding this comment.
Why not pass these in the command payload?
There was a problem hiding this comment.
I'd recommend using NullablesAndOptionalsStruct since it covers lists as well as nullables/optionals, and embedding that in both the command and the event.
There was a problem hiding this comment.
That was the original pattern I was shooting for, but the interface to go from DecodableType to Type proved to be marginally more complicated, so I mocked up something temporary. Will fix.
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
yufengwangca
reviewed
Dec 2, 2021
| EventLogger<T> eventData(aEventData); | ||
| ConcreteEventPath path(aEndpoint, aEventData.GetClusterId(), aEventData.GetEventId()); | ||
| // log the actual event | ||
| aEventNumber = 0; |
There was a problem hiding this comment.
Shall the eventNumber be an monotonically increasing ephemeral unique number?
This was referenced Dec 2, 2021
yunhanw-google
added a commit
to yunhanw-google/connectedhomeip
that referenced
this pull request
Dec 6, 2021
yunhanw-google
added a commit
to yunhanw-google/connectedhomeip
that referenced
this pull request
Dec 7, 2021
yunhanw-google
added a commit
to yunhanw-google/connectedhomeip
that referenced
this pull request
Dec 7, 2021
yunhanw-google
added a commit
to yunhanw-google/connectedhomeip
that referenced
this pull request
Dec 7, 2021
yunhanw-google
added a commit
to yunhanw-google/connectedhomeip
that referenced
this pull request
Dec 7, 2021
yunhanw-google
added a commit
that referenced
this pull request
Dec 7, 2021
* Port change from #12059 * Hook server with IM EventManagment and adjust EventManagment * run codegen
|
Merged by Integrate IM event with server #12641 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
Initial cut at the server event logging API.
Change overview
In the current form,
Logging API. For any of the generated
Events::*::Typeobjects, the API looks as follows:EventOptions eventOptions;
Events::TestEvent::Type event;
EndpointId endpoint;
EventNumber eid;
LogEvent(event, endpoint, options, eid);
the API is not completely hooked up in this draft, this is will change before converting to a regular PR
test code to emit events as a result of a command.
Testing
How was this tested? (at least one bullet point required)