Add basic support for reporting application installation status #33614
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
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bauerschwan,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
fessehaeve and
harimau-qirex
|
PR #33614: Size comparison from 294dc18 to 9def1a3 Full report (38 builds for bl602, bl702, bl702l, cc13x4_26x4, cyw30739, linux, mbed, nrfconnect, psoc6)
|
andy31415
reviewed
Jun 3, 2024
andy31415
reviewed
Jun 3, 2024
andy31415
reviewed
Jun 3, 2024
|
PR #33614: Size comparison from a42570b to f7f8d3d Full report (11 builds for cc13x4_26x4, linux, mbed, nrfconnect)
|
|
PR #33614: Size comparison from d8e7e43 to 6c2fa94 Full report (46 builds for cc13x4_26x4, cyw30739, esp32, linux, mbed, nrfconnect, psoc6, qpg, tizen)
|
|
PR #33614: Size comparison from ef28056 to bc2ec4a Full report (69 builds for bl602, bl702, bl702l, cc32xx, cyw30739, efr32, linux, mbed, nrfconnect, psoc6, telink)
|
|
PR #33614: Size comparison from 0050994 to ae6715d Full report (26 builds for cyw30739, efr32, mbed, nrfconnect, psoc6, qpg)
|
|
PR #33614: Size comparison from 4a8dc73 to c9e783f Full report (44 builds for bl602, bl702, bl702l, efr32, esp32, linux, mbed, nrfconnect, nxp, telink, tizen)
|
andy31415
reviewed
Jun 5, 2024
| @@ -681,12 +722,22 @@ std::list<ClusterId> ContentAppFactoryImpl::GetAllowedClusterListForStaticEndpoi | |||
| CHIP_ERROR AppTvInit() | |||
| { | |||
| #if CHIP_DEVICE_CONFIG_APP_PLATFORM_ENABLED | |||
| // test data for apps | |||
| const uint16_t APP1_VENDOR_ID = 1; | |||
There was a problem hiding this comment.
nit: naming should probably be kCamelCasename and using constexpr.
Suggested change
| const uint16_t APP1_VENDOR_ID = 1; | |
| constexpr uint16_t kApp1VendorId = 1; |
andy31415
reviewed
Jun 5, 2024
| * | ||
| * CdError status is designed for CommissionerDeclaration and should be set by using SetErrorCode() and | ||
| * sent back to the client as a CDC Message. It is expected that app during installation can return following errors: | ||
| * kNoError, kAppInstallConsentPending, kAppInstalling, kAppInstallFailed, kAppInstalledRetryNeeded |
There was a problem hiding this comment.
If this is a subset of errors, the comment should explain why only this subset will be returned here (and that it is a subset).
If this is an exhaustive set, then we should NOT have this list as this would mean we have to maintain the list in sync.
Overall listing anything except kNoError or special one-offs adds a maintenance burden to update the comment if the underlying code changes (and worse, you return a generic CdError yet your api claims to never return the entire set of errors and if clients rely on that, they may break on updates).
How about something like:
Returns a subset of CDError that are specific during application instalation lifecycle. In particular:
- kNoError means no error
- kAppInstallConsentPending is returned if no application is available
- other kApp* constants for lifecycle managemnt (kAppInstalling, kAppInstallFailed, kAppInstallRetryNeeded)
- other CdError constants are EXPLICITLY DISALLOWED
unsure if the above comment is true ... however please see how it can be phrased to make expectations clear. Is for example kUnexpectedCommissionerPasscodeReady (random pick) possible here? it is a valid CdError value. How about kConfigurationFailed?
Should we maybe consider having a separate enum that is more restricted and that can be directly converted to a CdError instead?
|
PR #33614: Size comparison from d15f6c1 to 1f7ba4e Full report (32 builds for bl602, bl702, bl702l, cc13x4_26x4, efr32, esp32, mbed, nrfconnect, nxp, qpg, tizen)
|
|
PR #33614: Size comparison from dc8187b to 7d35e6c Full report (35 builds for bl602, bl702, bl702l, cc32xx, efr32, esp32, linux, mbed, nrfconnect)
|
auto-merge was automatically disabled
June 14, 2024 10:26
Pull request was closed
|
I will follow up with new PR soon. |
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.
Adding basic support for reporting application's installation status
Problem
In case app is installed, there's no mechanism to report the current installation status.
Change overview
GetInstallationStatusOfAppwith ContentAppFactoryTests
Build the TV app using command:
scripts/examples/gn_build_example.sh examples/tv-app/linux out/debug/Build the TV casting app using command:
scripts/examples/gn_build_example.sh examples/tv-casting-app/linux out/debug/Start TV App
out/debug/chip-tv-appStart TV Casting App
out/debug/chip-tv-casting-appOn TV App: Uninstall the app
app uninstall 65521 32769On TV Casting App: Send casting request to the tv-app using
cast request 0On TV App: Accepted the Cast request from the tv-casting app
controller ux okNote that you'll see the error 13, which stands for
kAppInstallConsentPendingOn TV App: Once TV app confirmed there is no Content App, try and set other app installation statuses for example:
app setinstallerrorstatus 65521 32769 15Try to connect to the TV app again and after you enter
controller ux okyou'll see error :[1716811631946] [62904:4025232] [SVR] ---- Commissioner Declaration Start ----[1716811631946] [62904:4025232] [SVR] error code: 15[1716811631946] [62904:4025232] [SVR] ---- Commissioner Declaration End ----