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

L2CAP functions simplification [WIP] #1849

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vandy
Copy link

@vandy vandy commented Aug 12, 2024

This PR is related to #1846. These were the thoughts during gatt offset research.

One of the concerns is unnecessary (as it seems to me) arguments passed down the line to functions. It confuses a little about real dependencies.

The idea behind joining ble_l2cap_parse_hdr and os_mbuf_adj is to make it similar to ble_hs_hci_util_data_hdr_strip, as ble_l2cap_parse_hdr is not used anywhere outside the function.

The main concern is that ble_l2cap_rx and ble_l2cap_rx_payload besides business logic are messing with mbuf processing and other "infrastructure" stuff. Mixing logic from different layers of abstraction. This functions would be simpler if they just did l2cap processing and returning error codes immediately and the caller made cleaning and processing. So ble_hs_hci_evt_acl_process could clean the buffer as it already does, also schedule a timer on l2cap fragments as it already knows about fragments in the switch block.

@@ -427,7 +429,10 @@
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);

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host size/XS Extra small PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant