Bluetooth: L2CAP_BR: Enable Retransmission and Flow control #78879
Conversation
zephyrbot
added
area: Bluetooth
area: Bluetooth Host
area: Bluetooth Classic
zephyrbot
requested review from
alwa-nordic,
hermabe,
jhedberg,
jori-nordic,
sjanc,
Thalley and
theob-pro
include/zephyr/bluetooth/l2cap.h
Outdated
| #define BT_L2CAP_BR_FCS_NO 0x00 | ||
| /** Frame Check Sequence type. 16-bit FCS. */ | ||
| #define BT_L2CAP_BR_FCS_16BIT 0x01 |
There was a problem hiding this comment.
No tabs for alignment, please (only for indentation)
include/zephyr/bluetooth/l2cap.h
Outdated
| /** Endpoint Link Mode. | ||
| * The value is defined as BT_L2CAP_BR_LINK_MODE_* | ||
| */ | ||
| uint8_t mode; |
There was a problem hiding this comment.
Same here and anywhere else if you did the same. Use just spaces for alignment and tabs for indentation (or to put it in another way: the only place for tabs is in the very beginning of a line - as long as you have some non-tab character on a line there should not be any more tabs anywhere later in the line)
There was a problem hiding this comment.
Note: I know there are inconsistencies wrt this in the Bluetooth code base, but we should try to move toward the convention that I just described.
| config BT_L2CAP_RET | ||
| bool "Bluetooth L2CAP restransmission mode [EXPERIMENTAL]" | ||
| select EXPERIMENTAL | ||
| help | ||
| This option enables Bluetooth L2CAP restransmission mode | ||
|
|
There was a problem hiding this comment.
Please add some CI test cases for this. Right now it seems like the new c-code could be invalid garbage, and yet CI would show all green for this.
There was a problem hiding this comment.
OK. How about to add test code to shell?
There was a problem hiding this comment.
OK. How about to add test code to shell?
If there's some convenient and intuitive way to test this through the shell, then sure, why not.
lylezhu2012
force-pushed
the
br_l2cap_implement_ret_fc
branch
from
ee55bf4
to
aef11fb
Compare
lylezhu2012
force-pushed
the
br_l2cap_implement_ret_fc
branch
3 times, most recently
from
f3ce7b5
to
d884e3d
Compare
| config BT_L2CAP_RET | ||
| bool "Bluetooth L2CAP retransmission mode [EXPERIMENTAL]" | ||
| select EXPERIMENTAL | ||
| help | ||
| This option enables Bluetooth L2CAP retransmission mode | ||
|
|
||
| config BT_L2CAP_FC | ||
| bool "Bluetooth L2CAP flow control mode [EXPERIMENTAL]" | ||
| select EXPERIMENTAL | ||
| help | ||
| This option enables Bluetooth L2CAP flow control mode | ||
|
|
||
| config BT_L2CAP_ENH_RET | ||
| bool "Bluetooth L2CAP enhance retransmission [EXPERIMENTAL]" | ||
| select EXPERIMENTAL | ||
| help | ||
| This option enables Bluetooth L2CAP enhance retransmission | ||
|
|
||
| config BT_L2CAP_STREAM | ||
| bool "Bluetooth L2CAP streaming mode [EXPERIMENTAL]" | ||
| select EXPERIMENTAL | ||
| help | ||
| This option enables Bluetooth L2CAP streaming mode |
There was a problem hiding this comment.
Are there really no interdependencies with these, i.e. all four of them can be enabled independently without depending on any other option?
There was a problem hiding this comment.
Yes. I think they can be enabled independently.
| config BT_L2CAP_MPS | ||
| int "Bluetooth L2CAP MPS for retransmission and Flow control" | ||
| default 48 | ||
| range 48 BT_L2CAP_TX_MTU | ||
| help | ||
| Bluetooth L2CAP MPS for retransmission and Flow control | ||
|
|
||
| config BT_L2CAP_MAX_WINDOW_SIZE | ||
| int "Maximum Windows Size of Retransmission and Flow Control" | ||
| default 1 | ||
| range 1 32 | ||
| help | ||
| Maximum Windows Size of Retransmission and Flow Control. | ||
| The minimum (and default) number is 1. | ||
| The range is 1 to 32 for Flow Control mode and Retransmission mode. | ||
| The range is 1 to 63 for Enhanced Retransmission mode. | ||
| The range is 1 to 0x3FFF for Enhanced Retransmission mode with | ||
| externed control. | ||
|
|
||
| config BT_L2CAP_BR_RET_TIMEOUT | ||
| int "Retransmission timeout" | ||
| default 2000 | ||
| range 1000 2000 | ||
| help | ||
| the Retransmission timeout shall be three times the value of flush | ||
| timeout, subject to a minimum of 1 second and maximum of 2 seconds. | ||
|
|
||
| config BT_L2CAP_BR_MONITOR_TIMEOUT | ||
| int "Monitor timeout" | ||
| default 12000 if BT_L2CAP_ENH_RET | ||
| default BT_L2CAP_BR_RET_TIMEOUT | ||
| range BT_L2CAP_BR_RET_TIMEOUT 12000 |
There was a problem hiding this comment.
What about these? They really don't depend on any of the other newly added options in this PR?
There was a problem hiding this comment.
They are depended on BT_L2CAP_RET, BT_L2CAP_FC, BT_L2CAP_ENH_RET, or BT_L2CAP_STREAM.
Let me update the if condition for them.
lylezhu2012
force-pushed
the
br_l2cap_implement_ret_fc
branch
2 times, most recently
from
281c020
to
bde6262
Compare
| tx_win = (struct bt_l2cap_br_window *)sys_slist_peek_head( | ||
| &br_chan->_pdu_outstanding); | ||
| while (tx_win) { |
There was a problem hiding this comment.
Seems like this should fit a single line? You can make it shorter by doing a `(void *) typecast, since that's compatible with any target pointer type.
| tx_win = (struct bt_l2cap_br_window *)sys_slist_peek_head( | ||
| &br_chan->_pdu_outstanding); |
| struct bt_l2cap_br_window *tx_win, *next; | ||
|
|
||
| SYS_SLIST_FOR_EACH_CONTAINER_SAFE(&br_chan->_pdu_outstanding, tx_win, next, node) { | ||
| if (tx_win->srej) { | ||
| return tx_win; | ||
| } | ||
| } | ||
|
|
||
| return tx_win; |
There was a problem hiding this comment.
Should't the final return statement be return NULL;? Otherwise it looks like this might return an uninitialized pointer (if the for-loop didn't iterate a single time).
| tx_win = (struct bt_l2cap_br_window *)sys_slist_peek_head( | ||
| &br_chan->_pdu_outstanding); |
There was a problem hiding this comment.
Same comment here regarding unnecessary line splitting and potential to use a (void *) cast.
| connect_fixed_channel(BR_CHAN(chan)); | ||
| LOG_DBG("chan %p retransmission timeout", br_chan); | ||
|
|
||
| if (sys_slist_peek_head(&br_chan->_pdu_outstanding)) { |
There was a problem hiding this comment.
I'd propose to invert this condition and do an early return; from the function. That way you save one level of indentation from everything that's now inside the branch and thereby make the code more readable (there's a bunch of unfortunate line breaks right now which hurt readability).
| if (br_chan->tx.mode == BT_L2CAP_BR_LINK_MODE_RET) { | ||
| LOG_WRN("Retrans on chan %p", br_chan); | ||
| atomic_set_bit(br_chan->flags, L2CAP_FLAG_PDU_RETRANS); | ||
| /* Append channel to list if it still has data */ | ||
| if (chan_has_data(br_chan)) { | ||
| LOG_DBG("chan %p ready", br_chan); | ||
| raise_data_ready(br_chan); | ||
| } | ||
| } else if (br_chan->tx.mode == BT_L2CAP_BR_LINK_MODE_FC) { | ||
| uint16_t expected_ack_seq = | ||
| bt_l2cap_br_update_seq(br_chan, br_chan->expected_ack_seq + 1); | ||
|
|
||
| LOG_WRN("unacknowledged I-frame with sequence number %d", | ||
| br_chan->expected_ack_seq); | ||
| bt_l2cap_br_update_req_seq(br_chan, expected_ack_seq, false); | ||
| } else if (br_chan->tx.mode == BT_L2CAP_BR_LINK_MODE_ERET) { |
There was a problem hiding this comment.
These look like they should be a switch statement, since all of the branches test for the same br_chan->tx.mode value.
lylezhu2012
force-pushed
the
br_l2cap_implement_ret_fc
branch
from
bde6262
to
5f125ca
Compare
Enable signaling channel configuration for retransmission and Flow control feature. Send I-frame and S-frame. Support retransmission and Flow control for sending. Receive and handle I-frame and S-frame. Signed-off-by: Lyle Zhu <[email protected]>
Add mode optional support for BR l2cap connect initiator role. If `chan->rx.optional` is true, set the mode to basic mode instead of return error code `-ENOTSUP`. Signed-off-by: Lyle Zhu <[email protected]>
Improve the retransmission and flow control to support sending multiple SDU at the same time if the TX windows is not full. Signed-off-by: Lyle Zhu <[email protected]>
Add a sub-command set `l2cap` for command set `br`. Move command `l2cap-register` to sub-command set. And rename it to `register`. Add command `connect`, `disconnect`, and `send` for command set `l2cap`. Remove original net buffer pool from `data_pool` to `data_rx_pool`. Add a net buffer pool `data_tx_pool` for command `send`. Do not wait anymore if no net buffer can be allocated from `data_rx_pool`. Dump all received data in L2CAP data received callback. Signed-off-by: Lyle Zhu <[email protected]>
Add more parameters to command `connect` and `register`, including mode, mode_optional, extended_control, and hold_credit. Add command `credits` to give the rx credit. Signed-off-by: Lyle Zhu <[email protected]>
There is a corner case that the channel is in disconnecting status, due to there is a disconnect request sent from peer. At this time, the channel is status is `BT_L2CAP_CONNECTED`. But it has been removed from `conn->channels` by calling the function `l2cap_br_remove_tx_cid`. And the disconnect event is notified upper layer via the callback `ops->disconnected`. The thread of the callback context is blocked due to the the calling of `printk` in the callback function. Then the pending lower priority thread, sending the data in this l2cap channel, is activated. Then in the function `bt_l2cap_br_send_cb`, a NULL pointer will be got according to the given CID. And unexpected behavior happens when accessing a NULL pointer, since the invalid channel pointer is not checked in function `bt_l2cap_br_send_cb`. Check the channel pointer after function `bt_l2cap_br_lookup_tx_cid` called. If the channel pointer is invalid, return error code `-ESHUTDOWN` directly. Signed-off-by: Lyle Zhu <[email protected]>
lylezhu2012
force-pushed
the
br_l2cap_implement_ret_fc
branch
from
5f125ca
to
ad41023
Compare
Enable the retransmission, flow control, enhance retransmission, and streaming feature for BR/EDR L2cap.