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

Failed phy request not retried and may prevent DLE procedure during auto-initiation #31473

Closed
mtpr-ot opened this issue Jan 21, 2021 · 2 comments
Assignees

Comments

@mtpr-ot
Copy link
Collaborator

mtpr-ot commented Jan 21, 2021

When an auto-initiated phy request collides with other procedure initiated by the peer, the "Different Transaction Collision" error code is ignored, and auto-initiation continued, effectively skipping phy update.

To Reproduce
After connection is established, the central/peripheral LLCP timing must be such that the auto-initiated phy update request from peripheral is sent just before e.g. a encryption request is received by the peripheral. This will get the request in the air, and central responds with a rejection: "Different Transaction Collision".
This behavior can be observed in the simulation test basic_conn_split.sh, which is only "saved" by the central doing a phy update later.

Expected behavior
The host app expects that at least a reasonable effort has gone into executing the auto-initiated LLCPs, doing simple retries where applicable.

Impact
Link is left in 1 Mbps phy when 2 is expected.

Environment
Issue is seen on vendor implementation based on Zephyr 2.2.1, and on latest master with CI test basic_conn_split.sh.

@mtpr-ot mtpr-ot added the bug The issue is a bug, or the PR is fixing a bug label Jan 21, 2021
@mtpr-ot mtpr-ot self-assigned this Jan 21, 2021
@mtpr-ot mtpr-ot linked a pull request Jan 21, 2021 that will close this issue
cvinayak added a commit to cvinayak/zephyr that referenced this issue Jan 24, 2021
Fix missing PHY update procedure cachability omitted in
commit 16dbb9a ("Bluetooth: controller: split: Fix cmd
disallowed and collision disconnects").

Relates to zephyrproject-rtos#31473.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
carlescufi pushed a commit that referenced this issue Jan 25, 2021
Fix missing PHY update procedure cachability omitted in
commit 16dbb9a ("Bluetooth: controller: split: Fix cmd
disallowed and collision disconnects").

Relates to #31473.

Signed-off-by: Vinayak Kariappa Chettimada <vich@nordicsemi.no>
@mtpr-ot
Copy link
Collaborator Author

mtpr-ot commented Jan 26, 2021

Updated description to only consider "Different Transaction Collison" issue, as the "DISALLOWED" issue was fixed.

@nashif nashif added the priority: medium Medium impact/importance bug label Jan 26, 2021
@carlescufi carlescufi added priority: low Low impact/importance bug and removed priority: medium Medium impact/importance bug labels Jan 28, 2021
@carlescufi
Copy link
Member

carlescufi commented Jan 28, 2021

Downgrading to low because this has a very simple workaround, which is to retrigger the procedure from the application, and there are no explicit guarantees for retrial in the auto-initiated API.
I think we should create another enhancement issue covering the behavior of auto-initiated APIs when the procedure fails.

@carlescufi carlescufi added this to the v2.6.0 milestone Feb 4, 2021
@carlescufi carlescufi added Enhancement Changes/Updates/Additions to existing features and removed bug The issue is a bug, or the PR is fixing a bug priority: low Low impact/importance bug labels Feb 11, 2021
@galak galak removed this from the v2.6.0 milestone Jun 2, 2021
@mtpr-ot mtpr-ot closed this as completed Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants