-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Introducing CLI tv-casting-app for Linux #8824
Conversation
6cc49f9
to
5c5b357
Compare
Could you separate out commits between generated files and hand-edited code? This is a 12K line added PR and it would help a lot to have the 'generated files' separated out. |
5c5b357
to
997ac1f
Compare
Sure, I just separated the changes into different commits (zap/gen code and handwritten code): https://github.com/project-chip/connectedhomeip/pull/8824/commits - is that better? |
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.
please see comments
6a8f6b1
to
bb7be07
Compare
examples/chip-tool/commands/discover/DiscoverCommissionersCommand.cpp
Outdated
Show resolved
Hide resolved
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.
While I can understand that the current state of some of the API surfaces make the desired change harder to do, the solution used here to add a lot of application-dependant #ifdef
will not scale. This is just one more application desiring to implement a given feature, and I can imagine N other such applications with slightly different requirements. Instead of adding more #ifdef
that enshrine more assumptions (some of which are temporary), there should be some effort to refactor or at least discuss the obstacles found in API usage that cause the need to include all this specific steering of code.
The pervasive new usage of #if CONFIG_DEVICE_LAYER
ifdefs really indicate to me that some APIs are contradict some the assumptions of the author in terms of what their application ought to have to provide or implement. Rather than #ifdef
, let's discuss how to improve the API for everyone.
Finally, this PR touches much code, including code not related to its own requirements, and does so without adding or modifying a single unit test or integration test. Manual testing of a feature of this size is insufficient to maintain it in the tree.
Flagging as SDK Discussion required concering the approach: sudo usage (is it BT, something else)?, usage of ifdefs, split between functionality and example app development. Generally looking for a path forward to have incremental development instead of large patches mixing examples and functionality code. |
bb7be07
to
92d0ec7
Compare
1bd961f
to
b3a5c31
Compare
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.
Minor nit inline, but this looks good to me.
ce10168
to
3e591f8
Compare
Size increase report for "gn_qpg-example-build" from deb227f
Full report output
|
Size increase report for "esp32-example-build" from deb227f
Full report output
|
Size increase report for "nrfconnect-example-build" from deb227f
Full report output
|
I think specific commands like sending a UDC request can be modeled as a chip-tool command as well. But this is an example of an end-to-end tv-casting-app which implements this whole commissioning flow: InitiatingSetup.adoc#24-commissioner-discovery-from-an-on-network-device and is then supposed to send TV specific casting commands (TBD). We will also port this over to separate Android/iOS apps later that can be used as reference by any content provider to implement commissioning of itself and casting in their respective apps. |
Problem
The CHIP SDK did not have an app that implemented the flow where a commissionable node discovers a commissioner and requests it for commissioning. See InitiatingSetup.adoc#24-commissioner-discovery-from-an-on-network-device
What is being fixed?
Added a CLI based CHIP TV Casting app for Linux with the following steps implemented (from the flow referenced above):
TBD: Implementation to actually send a casting command to a TV will be done in a separate PR.
Change overview
Testing
A. Test cases added for commissioner discovery
B. Tested the tv-casting-app on Linux (VSCode/docker) by using the tv-app, minimal-mdns-client and chipTool:
$
out/host/chip-tv-casting-app
out/host/minimal-mdns-client -q _matterc._udp.local
)out/host/chip-tool pairing onnetwork 0 34567890 2976 192.168.65.4 5540
) and gets commissioned successfully.