Skip to content
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

Tv casting app stop connect when user navigates back before confirming passcode in commission #37278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yuenton-amazon
Copy link

@yuenton-amazon yuenton-amazon commented Jan 28, 2025

Updated Android tv-casting-app to send stopConnect command when user
exits UDC before confirming passcode

Changes

  1. MCConnectionExampleViewModel.swift and ConnectionExampleFragment.java track if user confirmed passcode or
    not
  2. ConnectionExampleFragment.java onDestroy() send stopConnect() if user
    has not confirmed passcode
  3. MCConnectionExampleView.swift trigger stopConnect when Connecting view
    dissappear

Test

  1. Manually Verfied stopConnect works for normal UDC and commissioner generated
    passcode UDC for both iOS and Android tv-casting-app
  2. Manually Verified alternating attempt of normal UDC and commissioner generated
    passcode UDC works both iOS and Android tv-casting-app
  3. Manually Verified stopConnect is not called in other states other than
    pending passcode confirmation both iOS and Android tv-casting-app

@yuenton-amazon yuenton-amazon requested review from a team as code owners January 28, 2025 23:14
@CLAassistant
Copy link

CLAassistant commented Jan 28, 2025

CLA assistant check
All committers have signed the CLA.

@yuenton-amazon yuenton-amazon marked this pull request as draft January 29, 2025 00:49
@yuenton-amazon yuenton-amazon marked this pull request as ready for review January 29, 2025 01:31
Updated Android & iOS tv-casting-app to send stopConnect command when user
exits UDC before confirming passcode

Changes
1. ConnectionExampleFragment.java track if user confirmed passcode or
   not
2. ConnectionExampleFragment.java onDestroy() send stopConnect() if user
   has not confirmed passcode
3. MCConnectionExampleViewModel track if user confirmed passcode or not
4. MCConnectionExampleView trigger stopConnect when Connecting view
   dissapear

Test
1. Manually verfied stopConnect works for normal UDC and commissioner generated
   passcode UDC
2. Manually verified alternating attempt of normal UDC and commissioner generated
   passcode UDC works
3. Manually verified stopConnect is not called in other states other than
   pending passcode confirmation
@yuenton-amazon yuenton-amazon force-pushed the tv-casting-app-stop-connect-when-user-navigates-back-before-confirming-passcode-in-commission branch from 039b564 to 479a03b Compare January 29, 2025 17:29
Copy link
Contributor

@sharadb-amazon sharadb-amazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines -231 to -235
VerifyOrReturnValue(
mIdOptions.mCommissionerPasscode, CHIP_ERROR_INCORRECT_STATE,
ChipLogError(AppServer,
"CastingPlayer::StopConnecting() mIdOptions.mCommissionerPasscode == false, ContinueConnecting() should only "
"be called when the CastingPlayer/Commissioner-Generated passcode commissioning flow is in progress."););
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pgregorr-amazon - Correct me if I am wrong, but I remember you added this check to guard against some error. Is it safe to remove this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharadb-amazon – This check wasn’t guarding against a specific error but was added as a defensive measure to restrict CastingPlayer::StopConnecting() API to the CommissioneR-Generated passcode flow. The CommissioneE-Generated passcode flow wasn’t tested at the time, and there was no use case for it. Before removing this check, we should verify that the API functions correctly with the CommissioneE-Generated passcode flow.
cc @yuenton-amazon

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yuenton-amazon can you please verify the above?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Talked to Philip. He mentioned he checks that the tv-app's log to check if its received the udc cancel message. I've already verified that in commissionee generated passcode, stop connect results in tv-app receiving the message and removes the udc session with controller udc-print CLI command

Comment on lines +45 to +51
enum ConnectionState {
PENDING_PASSCODE_CONFIRMATION,
CONNECTING,
CONNECTED,
FAILED,
CANCELLED,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The important state to trigger stopConnecting() is actually only PENDING_PASSCODE_CONFIRMATION. Since after after continueConnect() is called in CONNECTING state, stopConnecting() cannot be called.

The condition to call stopConnecting() is PENDING_PASSCODE_CONFIRMATION. I can covert this to a bool to simplify the solution, like the approach taken on iOS side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants