Bluetooth: controller: Data Length Update port to ULL/LLL arch #13235
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13235 +/- ##
==========================================
+ Coverage 52.93% 52.95% +0.01%
==========================================
Files 309 309
Lines 45251 45269 +18
Branches 10447 10450 +3
==========================================
+ Hits 23953 23971 +18
- Misses 16533 16534 +1
+ Partials 4765 4764 -1
Continue to review full report at Codecov.
|
cvinayak
force-pushed
the
github_dle_port
branch
from
bdfdce3 to
7ea80ce
Compare
cvinayak
force-pushed
the
github_dle_port
branch
from
7ea80ce to
37d0aeb
Compare
mped-oticon
left a comment
mped-oticon
left a comment
There was a problem hiding this comment.
Looks good, but have a few comments.
Also not completed full review; will await response before continuing.
There was a problem hiding this comment.
Since CONFIG_BT_RX_BUF_LEN is now always required to be defined, I guess we can also remove the #if defined(CONFIG_BT_RX_BUF_LEN)?
There was a problem hiding this comment.
btw, could you please add a define for magic 11?
(I belive it covers aa+(mic)+crc? Or is preamble included? But does the node_rx actually need to store aa?)
There was a problem hiding this comment.
Readability comment.
2nd arg meaning BIT(0) meaning 1Mbps. It would be great to something like BIT(EXT_ADV_AUX_PHY_LE_1M) in these places instead of a 0. (Granted; we shouldn't couple phy with advertisement extensions)
There was a problem hiding this comment.
BIT(2) means coded, which of course is a safe, but not very tight, bound.
So this should be #ifdef'ed on whether coded phy is supported.
There was a problem hiding this comment.
This always assumes coded phy.
There was a problem hiding this comment.
doesn't BIT(2) imply coded phy? Yet it is outside #if defined(CONFIG_BT_CTLR_PHY_CODED)
There was a problem hiding this comment.
This makes good sense. req != ack, so we have an ongoing/pending operation, thus we shall deny this new request.
There was a problem hiding this comment.
Here req == ack, so we shall grant the new request. Fine.
There was a problem hiding this comment.
Given above control flow, req will always be one higher than ack (modulo 256 (which here is the same as modulo 4)), so this condition will always be false.
There was a problem hiding this comment.
This then-body is dead code?
There was a problem hiding this comment.
as ISR can request a control procedure, which then is doing llcp_ack-- between the lines 1265 and 1266, the then body is required to detect the race-condition.
cvinayak
force-pushed
the
github_dle_port
branch
from
37d0aeb to
d975ffe
Compare
|
@mped-oticon updated, please re-review. |
Ported implementation of Data Length Update Procedure to ULL/LLL architecture of the Link Layer. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Minor refactoring to move PKT_US into ULL internal header, and rename ull_conn_allowed_check to ull_conn_llcp_req. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
Added implementation to pause data PDU transmission during PHY update procedure in order to comply to BT Spec. v5.1 Vol.6, Part B, Section 5.1.10.1 Packet transmit time restrictions. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>
cvinayak
force-pushed
the
github_dle_port
branch
from
d975ffe to
21a2219
Compare
4 participants
Ported implementation of Data Length Update Procedure to
ULL/LLL architecture of the Link Layer.
Signed-off-by: Vinayak Kariappa Chettimada [email protected]