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

MRP intervals and timeout computations aren't spec compliant #23328

Closed
mrjerryjohns opened this issue Oct 24, 2022 · 3 comments
Closed

MRP intervals and timeout computations aren't spec compliant #23328

mrjerryjohns opened this issue Oct 24, 2022 · 3 comments
Labels
spec Mismatch between spec and implementation stale Stale issue or PR

Comments

@mrjerryjohns
Copy link
Contributor

mrjerryjohns commented Oct 24, 2022

There are two distinct but related issues with our MRP timeout and retry interval computation logic:

  1. When we compute the logical timeout at which we think the exchange should timeout, it is computed from Session::GetAckTimeout. The two implementations present today for those do not take exponential back-off into account:
        switch (mPeerAddress.GetTransportType())
        {
        case Transport::Type::kUdp:
            return GetRemoteMRPConfig().mIdleRetransTimeout * (CHIP_CONFIG_RMP_DEFAULT_MAX_RETRANS + 1);

This results in the exchange timing out way too early (in most cases, before the last re-transmission can be sent out).

  1. GetMRPBaseTimeout doesn't correctly line up with the spec in terms of how it deduces if the other side is 'active'. The spec denotes that the presence of non-zero exchanges to the peer:

A Node determines whether it is in Active or Idle mode based on whether it has any outstanding Exchanges in the Message Layer.

In the impl however, it just assumes its active based on received traffic from the peer in the last 4s. This creates situations where a client that talks to a sleepy device can create an exchange that selects the active interval re-transmission even though the target is still sleeping and needs to be communicated with the IDLE interval.

@mrjerryjohns mrjerryjohns added the spec Mismatch between spec and implementation label Oct 24, 2022
@mrjerryjohns
Copy link
Contributor Author

#23307 solves issue #1 here.

@stale
Copy link

stale bot commented Apr 25, 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 25, 2023
@stale
Copy link

stale bot commented May 8, 2023

This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Mismatch between spec and implementation stale Stale issue or PR
Projects
None yet
Development

No branches or pull requests

1 participant