Add read/subscribe event function for darwin #24057
Merged
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
Add functions for darwin-framework-tool -discover commissionables -pairing ethernet -read-event-by-id Added Matter.Framework APIs MTRDeviceController -discovercommissionableNodes -setDeviceDiscoveryDelegate MTRBaseDevice -readEventsWithEndpointID
|
|
|
PR #24057: Size comparison from 9e371e6 to 4c46a1a Increases (5 builds for bl602, psoc6, telink)
Decreases (5 builds for bl602, esp32, psoc6, telink)
Full report (53 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bzbarsky-apple
requested changes
Dec 14, 2022
There was a problem hiding this comment.
I'd like to understand why we are adding these things to darwin-framework-tool and the framework.
In particular, the sequence here should be figuring out what we want in the framework and why, then adding it to the framework for those reasons, then adding a way for darwin-framework-tool to exercise it. We should not be adding things just so they can be used from darwin-framework-tool.
|
Also, note the memory leaks this is introducing (hence some of the CI failures). |
Discovery and ethernet pairing features are useful for test but not mendatory for Matter certification. So the removed codes will be committed after more verification.
|
I removed all discovery and ethernet pairing changes because they are useful for test but not a mandatory feature for Matter certification. I updated the commit and description to focus on "read event" function needed for Matter Certification of controller app. |
seokhee-lee
changed the title
Removed because I think that this file seems not in pull request scope and it can be generated by using commands.zapt in this pull request.
Rollback Commands.h to open source version. I think that this file seems not in pull request scope and it can be generated by using commands.zapt in this pull request.
Support any cluster for read-event-by-id of darwin-framework-tool
examples/darwin-framework-tool/commands/clusters/ReportCommandBridge.h
Outdated
Show resolved
Hide resolved
examples/darwin-framework-tool/commands/clusters/ReportCommandBridge.h
Outdated
Show resolved
Hide resolved
|
@seokhee-lee You should probably also fix the restyle issues, so this will request reviews more widely... |
|
@seokhee-lee If you check restyle, you can use below script. |
zzz_generated/darwin-framework-tool/zap-generated/cluster/Commands.h
|
PR #24057: Size comparison from 74296b8 to 1e8ee98 Increases (3 builds for cc13x2_26x2, nrfconnect, telink)
Decreases (11 builds for bl702, cc13x2_26x2, esp32, nrfconnect, psoc6, telink)
Full report (53 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #24057: Size comparison from 74296b8 to 8b80dd2 Increases (5 builds for bl602, cc13x2_26x2, psoc6, telink)
Decreases (7 builds for bl702, cc13x2_26x2, psoc6, telink)
Full report (46 builds for bl602, bl702, cc13x2_26x2, cyw30739, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bzbarsky-apple
added
acl
|
@ready2die4u @joonhaengHeo I feel bad asking you to hurry after it took me so long to get to the review, but if we can iron out the final few bits and get this merged in the next few days that would make it much more likely that it appears in a system Matter.framework in the near future. One thing that I suspect we do need here that I still don't see, by the way, is tests for the new code... |
@bzbarsky-apple , I thank you for the reviews and sorry for the late reply because there was big holiday (New Year's Day in lunar calendar) in Korea. We will update the patches by tomorrow according to your reviews. We are testing the codes with darwin-framework-tool described in the commit message and our controller iOS app. In addition, all commands of darwin-framework-tool are being tested to guarantee that our codes do not cause any side effects to the existing codes. If we need more works to verify our code changes, please let me know. |
|
@seokhee-lee I totally understand about the holiday! In addition to the manual tests, these APIs need automated tests (see existing tests in MTRDeviceTests.m for examples). That can be a follow-up PR as long as it happens. |
- Checked the invlalid id values are treated as wildcards in both subscribeToEventsWithEndpointID and readEventsWithEndpointID. - Added suggested changes.
|
I'm sorry for delay. I have changed it as shown below:
I will prepare a follow-up PR for automated tests. |
|
PR #24057: Size comparison from 6e12e92 to b66a9e1 Increases (4 builds for nrfconnect, psoc6, telink)
Decreases (5 builds for bl602, bl702, esp32, psoc6, telink)
Full report (43 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
There was a problem hiding this comment.
The following issues from the previous review are not addressed:
- Add read/subscribe event function for darwin #24057 (comment)
- The part about
subscribeWithQueuein Add read/subscribe event function for darwin #24057 (comment) - Filing followups for the wildcard handling as described in Add read/subscribe event function for darwin #24057 (comment) and Add read/subscribe event function for darwin #24057 (comment). This does not block landing, but please make sure the issues are filed.
- Removed mEventNumber.SetValue on MTRSubscribeParams - Added the wildcard handling as nil for Darwin API - Removed the casts on descriptions
|
PR #24057: Size comparison from 2ce5f2d to 4957f01 Increases (5 builds for cc13x2_26x2, cyw30739, nrfconnect, psoc6, telink)
Decreases (11 builds for bl602, bl702, cc13x2_26x2, nrfconnect, psoc6, qpg, telink)
Full report (43 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #24057: Size comparison from 2ce5f2d to 345b0d9 Increases (7 builds for bl602, bl702, cyw30739, nrfconnect, psoc6, telink)
Decreases (6 builds for esp32, nrfconnect, psoc6, telink)
Full report (43 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
Fast-tracking platform-specific change with platform owner review. |
|
PR #24057: Size comparison from 2ce5f2d to 4857114 Increases (12 builds for bl602, bl702, cyw30739, nrfconnect, psoc6, telink)
Decreases (6 builds for bl602, bl702, esp32, psoc6, telink)
Full report (43 builds for bl602, bl702, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
@joonhaengHeo @ready2die4u Thank you again for your patience, and sorry this took so long! I filed #24666 to track adding automated tests for these APIs. |
@bzbarsky-apple I thank you for your effort to review and merge the changes. I will commit the automated test patches soon. |
This was referenced Jan 27, 2023
lecndav
pushed a commit
to lecndav/connectedhomeip
that referenced
this pull request
Mar 22, 2023
* Add functions for darwin-framework-tool Add functions for darwin-framework-tool -discover commissionables -pairing ethernet -read-event-by-id Added Matter.Framework APIs MTRDeviceController -discovercommissionableNodes -setDeviceDiscoveryDelegate MTRBaseDevice -readEventsWithEndpointID * Remove discovery and ethernet pairing changes Discovery and ethernet pairing features are useful for test but not mendatory for Matter certification. So the removed codes will be committed after more verification. * Update PairingCommandBridge.mm * Update ReportCommandBridge.h * Delete Commands.h Removed because I think that this file seems not in pull request scope and it can be generated by using commands.zapt in this pull request. * Rollback Commands.h Rollback Commands.h to open source version. I think that this file seems not in pull request scope and it can be generated by using commands.zapt in this pull request. * Update MTRDeviceController.h * Update commands.zapt Support any cluster for read-event-by-id of darwin-framework-tool * Add Commands.h to fix zap build error * Update Commands.h based on the latest version in upstream * Remove fabric-filtered option in read-event-by-id * Revert "Update Commands.h based on the latest version in upstream" This reverts commit 8b80dd2. * Restyled by whitespace * Restyled by clang-format * Restyle zzz_generated/darwin-framework-tool/zap-generated/cluster/Commands.h * Update Commands.h to fix zap-related error * Added subscribeToEventsWithEndpointID - Added event-min to ReadEvent * Restyled by clang-format * Added Read/SubscribeEvent to PowerSourceCluster * Added BufferedReadClientCallback for both attribte and event - Used ConcreteClusterPath and ValueId instead of ConcreteAttribute/EventPath - Removed BufferedReadEventCallback - Added eventMin to MTRReadParams for EventFilter - Added isUrgentEvent to MTRSubscribeParams for EventRequest - Restyled by clang-format * Modified to support 'any subscribe-event-by-id' - Checked the invlalid id values are treated as wildcards in both subscribeToEventsWithEndpointID and readEventsWithEndpointID. - Added suggested changes. * Restyled by clang-format * Removed the hardcoded true on subscribeWithQeuue - Removed mEventNumber.SetValue on MTRSubscribeParams - Added the wildcard handling as nil for Darwin API - Removed the casts on descriptions * Revert "Removed the casts on descriptions" * Restore the mIsUrgentEvent in subscribeWithQueue. Co-authored-by: Restyled.io <commits@restyled.io> Co-authored-by: HyunKoo Ryu <hyunkoo.ryu@lge.com> Co-authored-by: ready2die4u <combygod@gmail.com> Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
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.
This change adds "read/subscribe event" function for darwin.
Problem
darwin matter framework is supporting API to subscribe all events but there is no way to read/subscribe only some selected events. Controller needs read/subscribe methods for the selected events to avoid performance issues happening when it subscribe all events.
Changes
(void)readEventsWithEndpointID:(NSNumber * _Nullable)endpointID clusterID:(NSNumber * _Nullable)clusterID eventID:(NSNumber * _Nullable)eventID params:(MTRReadParams * _Nullable)params queue:(dispatch_queue_t)queue completion:(MTRDeviceResponseHandler)completion (void)subscribeToEventsWithEndpointID:(NSNumber * _Nullable)endpointID clusterID:(NSNumber * _Nullable)clusterID eventID:(NSNumber * _Nullable)eventID params:(MTRSubscribeParams * _Nullable)params queue:(dispatch_queue_t)queue reportHandler:(MTRDeviceResponseHandler)reportHandler subscriptionEstablished:(MTRSubscriptionEstablishedHandler _Nullable)subscriptionEstablished