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

Bluetooth: Controller: Fix instant based procedure complete event #44791

Merged

Conversation

cvinayak
Copy link
Contributor

@cvinayak cvinayak commented Apr 12, 2022

Fix instant based procedure complete event generation to be
held until after the on-air instant has elapsed.

Fixes #21994.

Signed-off-by: Vinayak Kariappa Chettimada [email protected]

@cvinayak cvinayak force-pushed the github_llcp_rx_hold_fix branch 2 times, most recently from 5b59ba6 to a3ad601 Compare April 12, 2022 12:24
@cvinayak cvinayak marked this pull request as ready for review April 12, 2022 12:59
@cvinayak cvinayak self-assigned this Apr 12, 2022
@cvinayak cvinayak requested a review from ppryga-nordic April 12, 2022 13:00
@cvinayak cvinayak requested review from wopu-ot and erbr-ot April 22, 2022 03:43
@@ -1953,6 +1979,14 @@ void ull_conn_tx_ack(uint16_t handle, memq_link_t *link, struct node_tx *tx)
pdu_tx->ll_id = PDU_DATA_LLID_RESV;
} else {
LL_ASSERT(handle != LLL_HANDLE_INVALID);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing "if defined( CONFIG_BT_LL_SW_LLCP_LEGACY)" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erbr-ot Do I need the conditional compilation as non of the CI tests are complaining?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, the rx_hold-functionality is under CONFIG_BT_LL_SW_LLCP_LEGACY is it not? So I'd expect the usage to be as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess no builds are made with refactored LLCP AND CONFIG_BT_CTLR_RX_ENQUEUE_HOLD?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kruithofa can confirm, the bsim LL conformance tests run the refactored LLCP, but I do not think CONFIG_BT_CTLR_RX_ENQUEUE_HOLD would be enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@erbr-ot I have added the depends on CONFIG_BT_LL_SW_LLCP_LEGACY

@cvinayak cvinayak force-pushed the github_llcp_rx_hold_fix branch from a3ad601 to 52e0e32 Compare May 5, 2022 13:19
kruithofa
kruithofa previously approved these changes May 18, 2022
@erbr-ot erbr-ot self-requested a review May 18, 2022 12:59
erbr-ot
erbr-ot previously approved these changes May 18, 2022
Copy link
Collaborator

@erbr-ot erbr-ot left a comment

Choose a reason for hiding this comment

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

LGTM

@cvinayak cvinayak requested a review from Thalley May 19, 2022 06:04
subsys/bluetooth/controller/ll_sw/ull_conn.c Outdated Show resolved Hide resolved
subsys/bluetooth/controller/ll_sw/ull_conn.c Outdated Show resolved Hide resolved
subsys/bluetooth/controller/ll_sw/ull_conn.c Outdated Show resolved Hide resolved
@cvinayak cvinayak dismissed stale reviews from erbr-ot and kruithofa via 17a1dff May 19, 2022 13:36
@cvinayak cvinayak force-pushed the github_llcp_rx_hold_fix branch from 52e0e32 to 17a1dff Compare May 19, 2022 13:36
@cvinayak cvinayak requested a review from ppryga-nordic May 19, 2022 13:39
@cvinayak cvinayak added this to the v3.1.0 milestone May 19, 2022
@cvinayak cvinayak added the bug The issue is a bug, or the PR is fixing a bug label May 19, 2022
Thalley
Thalley previously approved these changes May 19, 2022
subsys/bluetooth/controller/ll_sw/lll_conn.h Show resolved Hide resolved
cvinayak added 2 commits May 19, 2022 20:44
Fix instant based procedure complete event generation to be
held until after the on-air instant has elapsed.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Rework instant based procedure complete event generation to be
held until after the on-air instant has elapsed, to have
conditional compilation around the code where the event
generation be held or immediately dispatched so that it
improves readability.

Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
@cvinayak cvinayak force-pushed the github_llcp_rx_hold_fix branch from 17a1dff to d8a45f8 Compare May 19, 2022 15:15
@cvinayak cvinayak requested a review from Thalley May 19, 2022 15:35
Copy link
Collaborator

@Thalley Thalley left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@ppryga-nordic ppryga-nordic left a comment

Choose a reason for hiding this comment

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

LGTM

@mbolivar-nordic mbolivar-nordic merged commit ca2701d into zephyrproject-rtos:main May 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Controller area: Bluetooth bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bluetooth: controller: split: Fix procedure complete event generation
6 participants