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.

Sign up for GitHub

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

Jump to bottom

Remove BDX timeout tracking from TransferSession #12520

Open
holbrookt opened this issue Dec 2, 2021 · 5 comments · Fixed by #16743
Open

Remove BDX timeout tracking from TransferSession #12520

holbrookt opened this issue Dec 2, 2021 · 5 comments · Fixed by #16743
Labels
V1.X

Comments

@holbrookt
Copy link
Contributor

holbrookt commented Dec 2, 2021 •
edited
Loading

Problem

The TransferSession class currently forces the caller to provide timestamps in PollOutput and HandleMessageReceived in order to determine if an application-determined timeout has occurred. However, the BDX spec does not say that this is required:

For synchronous transfers, if the Driver fails to receive the response to any given request that is not received within a particular application-determined time, it SHOULD abort the session.

Tracking timeouts in the TransferSession class is unnecessary and makes the API more complicated and cumbersome.

Proposed Solution

  • put the responsibility on the application to determine if a timeout has occurred
  • add a public method OnTimeout for the application to call to abort the session (without sending a StatusReport like in Abort())
  • remove timestamp arguments from aforementioned methods
This was referenced Dec 3, 2021
Merged
Closed
Closed
@isiu-apple isiu-apple self-assigned this Mar 3, 2022
isiu-apple added a commit to isiu-apple/connectedhomeip that referenced this issue Mar 29, 2022
@isiu-apple
Issue project-chip#12520 Remove BDX timeout tracking from TransferSes…
b3528c5 
…sion
@isiu-apple isiu-apple mentioned this issue Mar 29, 2022
Merged
@carol-apple
Copy link
Contributor

This actually helps the provider catch the 5 minute idle timeout. If this is removed, there needs to be explicit implementation on the provider side to do the same idle check.

woody-apple pushed a commit that referenced this issue Apr 1, 2022
@isiu-apple
Issue 12520 - Remove BDX timeout tracking from TransferSession (#16743)
f126057 
* Issue #12520 Remove BDX timeout tracking from TransferSession

* - Removed timeout from TransferSession::StartTransfer() and TransferSession::WaitForTransfer()
- Remove kNoAdvanceTime and its usage
- Removed TestTimeout() test suite
chencheung pushed a commit to chencheung/connectedhomeip that referenced this issue Apr 6, 2022
@isiu-apple @chencheung
Issue 12520 - Remove BDX timeout tracking from TransferSession (proje…
ab55cb3 
…ct-chip#16743)

* Issue project-chip#12520 Remove BDX timeout tracking from TransferSession

* - Removed timeout from TransferSession::StartTransfer() and TransferSession::WaitForTransfer()
- Remove kNoAdvanceTime and its usage
- Removed TestTimeout() test suite
chencheung pushed a commit to chencheung/connectedhomeip that referenced this issue Apr 6, 2022
@isiu-apple @chencheung
Issue 12520 - Remove BDX timeout tracking from TransferSession (proje…
7f137fa 
…ct-chip#16743)

* Issue project-chip#12520 Remove BDX timeout tracking from TransferSession

* - Removed timeout from TransferSession::StartTransfer() and TransferSession::WaitForTransfer()
- Remove kNoAdvanceTime and its usage
- Removed TestTimeout() test suite
andrei-menzopol pushed a commit to andrei-menzopol/connectedhomeip that referenced this issue Apr 14, 2022
@isiu-apple @andrei-menzopol
Issue 12520 - Remove BDX timeout tracking from TransferSession (proje…
35612f0 
…ct-chip#16743)

* Issue project-chip#12520 Remove BDX timeout tracking from TransferSession

* - Removed timeout from TransferSession::StartTransfer() and TransferSession::WaitForTransfer()
- Remove kNoAdvanceTime and its usage
- Removed TestTimeout() test suite
isiu-apple added a commit to isiu-apple/connectedhomeip that referenced this issue Apr 15, 2022
@isiu-apple
Revert "Issue project-chip#12520 Remove BDX timeout tracking from Tra…
3541d4a 
…nsferSession"

This reverts commit b3528c5.
@isiu-apple isiu-apple mentioned this issue Apr 15, 2022
Merged
@isiu-apple
Copy link
Contributor

This issue is being reverted via PR #17427, so re-opening this issue.

@isiu-apple isiu-apple reopened this Apr 15, 2022
carol-apple pushed a commit that referenced this issue Apr 18, 2022
@isiu-apple
Revert PR 16743 (#17427)
4fc01e1 
* Revert "Issue #12520 Remove BDX timeout tracking from TransferSession"

This reverts commit b3528c5.

* Revert "- Removed timeout from TransferSession::StartTransfer() and TransferSession::WaitForTransfer()"

This reverts commit 7a4061a.
@isiu-apple isiu-apple removed their assignment Sep 27, 2022
@stale
Copy link

stale bot commented Apr 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Apr 2, 2023
@nivi-apple
Copy link
Contributor

work in progress

@stale stale bot removed the stale Stale issue or PR label Apr 6, 2023
@ghost
Copy link

ghost commented Oct 12, 2023

Hello, any chance for some sort of fix/final solution here?

We are currently trying to use the provided chip-ota-provider-app to validate a new device side implementation and we are running into a scenario where after the header is sent and received properly, the process just stops as it seems to be waiting for some time to pass on either side.

The code in #16743 seemed promising but then it was reverted?

This code has been officially released as part of the v1.1.0.1 tag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
V1.X
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Issue 12520 - Remove BDX timeout tracking from TransferSession isiu-apple/connectedhomeip
5 participants
@holbrookt @nivi-apple @carol-apple @franck-apple @isiu-apple