Add a continuous browse for Matter operational advertisements on Darwin. #25317
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
|
PR #25317: Size comparison from 31da720 to 359f7aa Decreases (2 builds for bl702)
Full report (6 builds for bl602, bl702, qpg)
|
The information is propagated to MTRDevice, but we're not making use of it there yet. The fabricIndex changes were due to a pre-existing issue: we are in fact getting it from the wrong thread/queue in various places (MTRDevice init, MTRDevice command invocation), such that an assertChipStackLockedByCurrentThread() in the getter failed.
bzbarsky-apple
force-pushed
the
darwin-operational-browse
branch
from
359f7aa to
91874dd
Compare
pullapprove
bot
requested review from
amitnj,
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
CodeChronos928,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
franck-apple,
gjc13,
harimau-qirex,
harsha-rajendran,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
jtung-apple,
kcoppock and
kkasperczyk-no
pullapprove
bot
requested review from
mlepage-google,
msandstedt,
mspang,
nivi-apple,
pjzander-signify,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
woody-apple and
xylophone21
|
PR #25317: Size comparison from 31da720 to 91874dd Increases (3 builds for cc13x2_26x2, psoc6)
Decreases (7 builds for bl702, cc13x2_26x2, cc32xx, psoc6)
Full report (27 builds for bl602, bl702, cc13x2_26x2, cc32xx, cyw30739, linux, mbed, nrfconnect, psoc6, qpg)
|
andy31415
reviewed
Feb 28, 2023
| #import <Matter/MTRDeviceControllerFactory.h> | ||
| #import <dns_sd.h> | ||
|
|
||
| class MTROperationalBrowser |
There was a problem hiding this comment.
would it make sense to have a core object for these things? MinMDNS can have similar capabiltity as it receives every single mdns packet.
There was a problem hiding this comment.
would it make sense to have a core object for these things?
Perhaps, but the core "browse" API surface we have does not support the things that need to be done here. MinMDNS can do it, the Darwin backend can do it, dunno about the other backends, but the API on top of them cannot.
If we fix that, then we can change the bits here to use that core object instead. But it's going to take some API refactoring to make it happen.
There was a problem hiding this comment.
My concern with the platform-only approach is that it makes it more likely that functionality diverges across platforms, so developing apps on one platform vs another starts becoming different in terms of functionality.
I believe it is fine platforms to have code that is platform-specific, however I would also argue in that case it should be part of a separate repo not directly in the CHIP SDK (like we have embedded platform SDKs for various targets which provide different functionality)
There was a problem hiding this comment.
We have tons of differences across platforms right now. Darwin and Android have different API surfaces with different capabilities, for example. And neither one matches the API surface of the raw C++ SDK.
Note that in this case it's not even an API difference. It's just an implementation detail of the Darwin framework. Just like the fact that we auto-resubscribe subscriptions by default is an implementation detail, and the fact that we ensure that we don't blow out read handler limits is a detail, etc.
I agree that ideally things that can be done in the core should be done in the core. In this case, I tried to do this in the core, and it's not possible there without significantly more API work than I have time for right now. That's not sufficient reason to not ship the functionality on iOS, though... And if I find more time, or if someone else makes the core more reasonable, we can move the functionality into the core.
The argument about same vs separate repo is an interesting one, but unless you are arguing that Matter.framework should not be in-tree at all, I don't think it really applies in this case.
jtung-apple
approved these changes
Feb 28, 2023
|
Fast-tracking platform-specific change with platform owner review. |
shchen-Lab
added a commit
to bouffalolab/connectedhomeip-1
that referenced
this pull request
Mar 1, 2023
…p-1 into bl702l_matter * 'bl702l_matter' of github.com:bouffalolab/connectedhomeip-1: (446 commits) [Python] Add Python commissioning flow (project-chip#25119) Add to flake8 in workflow and fix python files (project-chip#25280) Align Time Format Localization cluster XML to spec changes. (project-chip#25289) Use the PathsFinder module in scripts/tests/run_test_suite.py instead of having duplicated code (project-chip#25368) Add a continuous browse for Matter operational advertisements on Darwin. (project-chip#25317) Chef doorlock sample update (project-chip#24118) Fix implementation of OnChipScanComplete and OnScanComplete - second PR (project-chip#24873) Add to flake8 in workflow and fix python files (project-chip#25279) Make PASE setup a bit more robust if multiple clients race. (project-chip#25352) Add dependent lib kotlin-stdlib for kotlin version of java-matter-controller (project-chip#25358) [python tests] Add to flake8 in workflow and fix python files (part project-chip#25193) (project-chip#25312) Add to flake8 in workflow and fix python files (project-chip#25283) Add a way to read a concrete attribute path from AttributePathIB::Parser. (project-chip#25293) Make sure various tests in TestReadInteraction are not no-ops. (project-chip#25298) [Android] Add isUrgent option in Android (project-chip#25301) [NXP] Add to flake8 in workflow and fix python files (part project-chip#25193) (project-chip#25305) [placeholder] Allow applications can specify which additional sources to build (project-chip#25346) Set thread sleep and yield backends for rpc (project-chip#25350) [config-data] Remove some enums that just don't generate anything (project-chip#25370) [Tizen] CI workflow for running QEMU-based tests (project-chip#24871) ...
shchen-Lab
added a commit
to bouffalolab/connectedhomeip-1
that referenced
this pull request
Mar 1, 2023
* official/master: (449 commits) tv-casting-app: Updating the context we pass to FindOrEstablishSession Changing caching logic to match video players using hostname before other attributes Enable -Wconversion tree-wide on darwin. (project-chip#25376) [Python] Add Python commissioning flow (project-chip#25119) Add to flake8 in workflow and fix python files (project-chip#25280) Align Time Format Localization cluster XML to spec changes. (project-chip#25289) Use the PathsFinder module in scripts/tests/run_test_suite.py instead of having duplicated code (project-chip#25368) Add a continuous browse for Matter operational advertisements on Darwin. (project-chip#25317) Chef doorlock sample update (project-chip#24118) Fix implementation of OnChipScanComplete and OnScanComplete - second PR (project-chip#24873) Add to flake8 in workflow and fix python files (project-chip#25279) Make PASE setup a bit more robust if multiple clients race. (project-chip#25352) Add dependent lib kotlin-stdlib for kotlin version of java-matter-controller (project-chip#25358) [python tests] Add to flake8 in workflow and fix python files (part project-chip#25193) (project-chip#25312) Add to flake8 in workflow and fix python files (project-chip#25283) Add a way to read a concrete attribute path from AttributePathIB::Parser. (project-chip#25293) Make sure various tests in TestReadInteraction are not no-ops. (project-chip#25298) [Android] Add isUrgent option in Android (project-chip#25301) [NXP] Add to flake8 in workflow and fix python files (part project-chip#25193) (project-chip#25305) [placeholder] Allow applications can specify which additional sources to build (project-chip#25346) ...
lecndav
pushed a commit
to lecndav/connectedhomeip
that referenced
this pull request
Mar 22, 2023
…in. (project-chip#25317) The information is propagated to MTRDevice, but we're not making use of it there yet. The fabricIndex changes were due to a pre-existing issue: we are in fact getting it from the wrong thread/queue in various places (MTRDevice init, MTRDevice command invocation), such that an assertChipStackLockedByCurrentThread() in the getter failed.
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.
The information is propagated to MTRDevice, but we're not making use of it there yet.
The fabricIndex changes were due to a pre-existing issue: we are in fact getting it from the wrong thread/queue in various places (MTRDevice init, MTRDevice command invocation), such that an assertChipStackLockedByCurrentThread() in the getter failed.
Lays the groundwork for fixing #25091: we now browse for operational advertisements, but do not act on them yet.