From 03ba43799ea179834fc01ec8b7925511d318e685 Mon Sep 17 00:00:00 2001 From: Roman Sukhorukov Date: Sat, 10 Aug 2024 09:25:44 +0300 Subject: [PATCH] Reviewed code during gatt offset research, added REVIEW comments to discuss changes. --- nimble/host/src/ble_hs_hci_evt.c | 1 + nimble/host/src/ble_l2cap.c | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/nimble/host/src/ble_hs_hci_evt.c b/nimble/host/src/ble_hs_hci_evt.c index 48012f3b47..db1fcd6e8b 100644 --- a/nimble/host/src/ble_hs_hci_evt.c +++ b/nimble/host/src/ble_hs_hci_evt.c @@ -1094,6 +1094,7 @@ ble_hs_hci_evt_acl_process(struct os_mbuf *om) /* Final fragment received. */ BLE_HS_DBG_ASSERT(rx_cb != NULL); rc = rx_cb(conn->bhc_rx_chan); + // REVIEW: Seems like the function ble_l2cap_remove_rx could use only one argument conn, it could take chan from the conn. ble_l2cap_rx uses it. It's called if channel for recieved packet has rx_buf. Will conn->bhc_rx_chan be equal to chan->rx_buf in this case? ble_l2cap_remove_rx(conn, conn->bhc_rx_chan); break; diff --git a/nimble/host/src/ble_l2cap.c b/nimble/host/src/ble_l2cap.c index 50fe18c651..8c64139665 100644 --- a/nimble/host/src/ble_l2cap.c +++ b/nimble/host/src/ble_l2cap.c @@ -356,10 +356,12 @@ ble_l2cap_rx(struct ble_hs_conn *conn, *out_reject_cid = -1; + // REVIEW: The function is dependent only on PB, this could be passed instead of the whole struct. Could we change this? pb = BLE_HCI_DATA_PB(hci_hdr->hdh_handle_pb_bc); switch (pb) { case BLE_HCI_PB_FIRST_FLUSH: /* First fragment. */ + // REVIEW: Could change this to ble_l2cap_strip_hdr (parse + strip) in analogy to ble_hs_hci_util_data_hdr_strip. rc = ble_l2cap_parse_hdr(om, 0, &l2cap_hdr); if (rc != 0) { goto err; @@ -427,7 +429,10 @@ ble_l2cap_rx(struct ble_hs_conn *conn, goto err; } + // REVIEW: It would be nice not to pass conn to decrease number of arguments, timer reschedule could be processed here. Actually it could be hoisted to the caller as it already processes the return value and knows about l2cap fragments existence. So just return ble_l2cap_rx_payload(chan, om, out_rx_cb); rc = ble_l2cap_rx_payload(conn, chan, om, out_rx_cb); + // REVIEW: There is no need to nullify the pointer, it's nullified in the caller. + // Also I believe this function shouldn't own the buffer and try to free it, it's the caller duty, just return the error from this function, and caller decides what to do. It'll simplify the code in this function a bit. om = NULL; if (rc != 0) { goto err;