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.

Sign up for GitHub

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: split: Fix DLE duplicate requests #23707

Merged
merged 2 commits into from
Apr 1, 2020
Merged

Bluetooth: controller: split: Fix DLE duplicate requests #23707

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e70a1dc
Bluetooth: controller: split: Simplify DLE state checks
cvinayak Mar 24, 2020
bdb53f2
Bluetooth: controller: split: Fix DLE duplicate requests
cvinayak Mar 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 17 additions & 12 deletions subsys/bluetooth/controller/ll_sw/ull_conn.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4501,26 +4501,20 @@ static inline int length_req_rsp_recv(struct ll_conn *conn, memq_link_t *link,
if (/* Local idle, and Peer request then complete the Peer procedure
* with response.
*/
((conn->llcp_length.req == conn->llcp_length.ack) &&
(pdu_rx->llctrl.opcode == PDU_DATA_LLCTRL_TYPE_LENGTH_REQ)) ||
((conn->llcp_length.req == conn->llcp_length.ack) && tx) ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

The criteria is that opcode equals PDU_DATA_LLCTRL_TYPE_LENGTH_REQ, not that tx!=NULL
Even though tx!=NULL when the opcode is correct, this is not immediately obvious, you need to scroll back in the code.
IMHO the original was easier to follow.
Are there significant reductions in code-size and/or execution time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Compare and branch on zero take less instructions and CPU cycles, in compare to load two values compare it and branch set of instructions.

Before simplification:

Memory region         Used Size  Region Size  %age Used
           FLASH:      251093 B         1 MB     23.95%
            SRAM:       33652 B       256 KB     12.84%
        IDT_LIST:         136 B         2 KB      6.64%

After simplification:

Memory region         Used Size  Region Size  %age Used
           FLASH:      251029 B         1 MB     23.94%
            SRAM:       33652 B       256 KB     12.84%
        IDT_LIST:         136 B         2 KB      6.64%

/* or Local has active... */
((conn->llcp_length.req != conn->llcp_length.ack) &&
/* with Local requested and Peer request then complete the
* Peer procedure with response.
*/
((((conn->llcp_length.state == LLCP_LENGTH_STATE_REQ) ||
(conn->llcp_length.state == LLCP_LENGTH_STATE_REQ_ACK_WAIT)) &&
(pdu_rx->llctrl.opcode ==
PDU_DATA_LLCTRL_TYPE_LENGTH_REQ)) ||
tx) ||
/* with Local waiting for response, and Peer response then
* complete the Local procedure or Peer request then complete the
* Peer procedure with response.
*/
((conn->llcp_length.state == LLCP_LENGTH_STATE_RSP_WAIT) &&
((pdu_rx->llctrl.opcode ==
PDU_DATA_LLCTRL_TYPE_LENGTH_RSP) ||
(pdu_rx->llctrl.opcode ==
PDU_DATA_LLCTRL_TYPE_LENGTH_REQ)))))) {
(conn->llcp_length.state == LLCP_LENGTH_STATE_RSP_WAIT)))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why the test for PDU_DATA_LLCTRL_TYPE_LENGTH_RSP not is required?
I think the comment must be updated as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The function enters either for PDU_DATA_LLCTRL_TYPE_LENGTH_RSP or PDU_DATA_LLCTRL_TYPE_LENGTH_REQ, and it is redundant to check for the same (||) here.

struct pdu_data_llctrl_length_req *lr;
u16_t max_rx_octets;
u16_t max_tx_octets;
Expand Down Expand Up @@ -4684,9 +4678,20 @@ static inline int length_req_rsp_recv(struct ll_conn *conn, memq_link_t *link,
#endif /* CONFIG_BT_CTLR_PHY */
}
} else {
/* Drop response with no Local initiated request. */
LL_ASSERT(pdu_rx->llctrl.opcode ==
PDU_DATA_LLCTRL_TYPE_LENGTH_RSP);
/* Drop response with no Local initiated request and duplicate
* requests.
*/
if (pdu_rx->llctrl.opcode != PDU_DATA_LLCTRL_TYPE_LENGTH_RSP) {
mem_release(tx, &mem_conn_tx_ctrl.free);

/* Defer new request if previous in resize state */
if (conn->llcp_length.state ==
LLCP_LENGTH_STATE_RESIZE) {
return -EBUSY;
}
}

return 0;
}

send_length_resp:
Expand Down