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

2M PHY + DLE and timing calculations on an encrypted link are wrong #23482

Closed
thoh-ot opened this issue Mar 16, 2020 · 2 comments · Fixed by #23557 or #23558
Closed

2M PHY + DLE and timing calculations on an encrypted link are wrong #23482

thoh-ot opened this issue Mar 16, 2020 · 2 comments · Fixed by #23557 or #23558
Assignees
Labels
area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug

Comments

@thoh-ot
Copy link
Collaborator

thoh-ot commented Mar 16, 2020

Describe the bug
We are seeing some issue regarding 2M PHY + DLE and timing calculations on an encrypted link.

We use DLE with 251 as maximum and have some internal test that tries to send 244 bytes via GATT with 7 bytes protocol overhead, so that becomes 251 bytes LL PDU Payload. When adding the 4 byte MIC, the LL PDU length becomes 255.

On the AIR this should become one LL packet, instead we are seeing two.

We see that when running 2M that struct lll_conn's max_tx_time is 1060 and when ull_conn_lll_max_tx_octets_get(struct lll_conn *lll) [subsys/bluetooth/controller/ll_sw/ull_conn.c] is called max_tx_octets becomes 1060/8 - 11 = 254 where it should be 255.

The correct max_tx_time should be 1064, with 4us extra for the missing byte.

We believe we have tracked the issue to PKT_US(octets, phy) [subsys/bluetooth/controller/ll_sw/ull_conn_internal.h] where the phy parameter is fixed to 1, when given to PREAMBLE_SIZE(phy) [subsys/bluetooth/controller/ll_sw/pdu.h].

Also we have noticed that in some calls to PKT_US a 0 is given as phy which doesn't make sense, so we guess it should be BIT(0).

Expected behavior
On the AIR the 244 bytes GATT operation should be one LL packet.

Impact
Throughput degradation.

Environment
Zephyr 2.2

@thoh-ot thoh-ot added bug The issue is a bug, or the PR is fixing a bug area: Bluetooth labels Mar 16, 2020
@aescolar
Copy link
Member

CC @cvinayak @kruithofa

@kruithofa kruithofa self-assigned this Mar 16, 2020
@kruithofa
Copy link
Collaborator

Same bug is present in the legacy controller

kruithofa added a commit to kruithofa/zephyr that referenced this issue Mar 18, 2020
A bug in the PKT_US resulted in wrong calculations for the 2M phy.
Fixes the bug, verified on EBQ.
Also adds some defines for improved readability.

Fixes zephyrproject-rtos#23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
@aescolar aescolar added has-pr priority: medium Medium impact/importance bug labels Mar 18, 2020
aescolar pushed a commit that referenced this issue Mar 18, 2020
A bug in the PKT_US resulted in wrong calculations for the 2M phy.
Fixes the bug, verified on EBQ.
Also adds some defines for improved readability.

Fixes #23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
kruithofa added a commit to kruithofa/zephyr that referenced this issue Mar 18, 2020
A bug in the PKT_US resulted in wrong calculations for the 2M phy.
Fixes the bug, verified on EBQ.
Also adds some defines for improved readability.

Fixes zephyrproject-rtos#23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
hakehuang pushed a commit to hakehuang/zephyr that referenced this issue Mar 18, 2020
A bug in the PKT_US resulted in wrong calculations for the 2M phy.
Fixes the bug, verified on EBQ.
Also adds some defines for improved readability.

Fixes zephyrproject-rtos#23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
kruithofa added a commit to kruithofa/zephyr that referenced this issue Mar 19, 2020
Fixes zephyrproject-rtos#23482.

There is a bug in calculation of time for the DLE procedure:
the preamble size is incorrect for the 2M phy

This is for the legacy code, see PR zephyrproject-rtos#23570 for the split controller

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
kruithofa added a commit to kruithofa/zephyr that referenced this issue Mar 26, 2020
This PR fixes zephyrproject-rtos#23482. The preamble size for a 2M phy was incorrect.

There is a bug in calculation of time for the DLE procedure:
the preamble size is incorrect for the 2M phy

Fixes zephyrproject-rtos#23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
carlescufi pushed a commit that referenced this issue Apr 9, 2020
This PR fixes #23482. The preamble size for a 2M phy was incorrect.

There is a bug in calculation of time for the DLE procedure:
the preamble size is incorrect for the 2M phy

Fixes #23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
jhedberg pushed a commit that referenced this issue Apr 12, 2020
A bug in the PKT_US resulted in wrong calculations for the 2M phy.
Fixes the bug, verified on EBQ.
Also adds some defines for improved readability.

Fixes #23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
avisconti pushed a commit to avisconti/zephyr that referenced this issue Apr 15, 2020
This PR fixes zephyrproject-rtos#23482. The preamble size for a 2M phy was incorrect.

There is a bug in calculation of time for the DLE procedure:
the preamble size is incorrect for the 2M phy

Fixes zephyrproject-rtos#23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
jhedberg pushed a commit that referenced this issue Apr 22, 2020
Fixes #23482.

There is a bug in calculation of time for the DLE procedure:
the preamble size is incorrect for the 2M phy

This is for the legacy code, see PR #23570 for the split controller

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
hakehuang pushed a commit to hakehuang/zephyr that referenced this issue Jun 20, 2020
This PR fixes zephyrproject-rtos#23482. The preamble size for a 2M phy was incorrect.

There is a bug in calculation of time for the DLE procedure:
the preamble size is incorrect for the 2M phy

Fixes zephyrproject-rtos#23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
pkral78 pushed a commit to cloudfieldcz/zephyr that referenced this issue Aug 4, 2020
A bug in the PKT_US resulted in wrong calculations for the 2M phy.
Fixes the bug, verified on EBQ.
Also adds some defines for improved readability.

Fixes zephyrproject-rtos#23482

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
pkral78 pushed a commit to cloudfieldcz/zephyr that referenced this issue Aug 4, 2020
Fixes zephyrproject-rtos#23482.

There is a bug in calculation of time for the DLE procedure:
the preamble size is incorrect for the 2M phy

This is for the legacy code, see PR zephyrproject-rtos#23570 for the split controller

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
3 participants