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

Bluetooth: host: Initial code for 5.2 features #23199

Merged
merged 10 commits into from
May 2, 2020

Conversation

Vudentz
Copy link
Contributor

@Vudentz Vudentz commented Mar 2, 2020

This add support for for the following features:

  • L2CAP: Add support for Enhanced Credit Based Flow Control
  • ATT/GATT: Add Enhanced ATT support
  • ATT/GATT: Add Notify Multiple and Read Multiple Variable Length procedures

@Vudentz Vudentz added Feature A planned feature with a milestone area: Bluetooth RFC Request For Comments: want input from the community labels Mar 2, 2020
@zephyrbot zephyrbot added the area: API Changes to public APIs label Mar 2, 2020
@zephyrbot
Copy link
Collaborator

zephyrbot commented Mar 2, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

subsys/bluetooth/host/l2cap.c Outdated Show resolved Hide resolved
@Vudentz Vudentz force-pushed the bt5_2 branch 5 times, most recently from 944c5a4 to a73645d Compare March 3, 2020 01:03
subsys/bluetooth/host/Kconfig.gatt Outdated Show resolved Hide resolved
@Vudentz Vudentz force-pushed the bt5_2 branch 3 times, most recently from 7b58c1a to 889a429 Compare March 3, 2020 22:44
@Vudentz Vudentz requested a review from aescolar as a code owner March 3, 2020 22:44
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Mar 3, 2020
Copy link
Contributor

@joerchan joerchan left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to post feedback.
I haven't looked too closely at all of the implementation yet, so posting the more shallow comments. Will continue to look.

subsys/bluetooth/host/l2cap.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/l2cap_internal.h Outdated Show resolved Hide resolved
subsys/bluetooth/host/l2cap_internal.h Outdated Show resolved Hide resolved
include/bluetooth/gatt.h Outdated Show resolved Hide resolved
include/bluetooth/gatt.h Show resolved Hide resolved
subsys/bluetooth/host/att.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/att.c Show resolved Hide resolved
subsys/bluetooth/host/l2cap.c Outdated Show resolved Hide resolved
subsys/bluetooth/host/gatt.c Outdated Show resolved Hide resolved
include/bluetooth/gatt.h Show resolved Hide resolved
@joerchan
Copy link
Contributor

joerchan commented Apr 7, 2020

Regarding the RFC tag. Is that for the API part? If so I think that part looks good. But it would be nice to get feedback from users who are expected to use this API as well.

@Vudentz
Copy link
Contributor Author

Vudentz commented Apr 7, 2020

Regarding the RFC tag. Is that for the API part? If so I think that part looks good. But it would be nice to get feedback from users who are expected to use this API as well.

I can drop the RFC tag then, I was hoping to get more feedback but it looks like either there were no comments or people were busy with something else.

@Vudentz Vudentz removed the RFC Request For Comments: want input from the community label Apr 7, 2020
@joerchan
Copy link
Contributor

joerchan commented Apr 8, 2020

I can drop the RFC tag then, I was hoping to get more feedback but it looks like either there were no comments or people were busy with something else.

@aescolar @Rasmus-Abildgren-Bose @carlescufi Any comments on the proposed API changes?

@Rasmus-Abildgren-Bose
Copy link

I'm currently too busy to do a full review but have had a look at the API.

I see no problem with these changes.

@jhedberg
Copy link
Member

@Vudentz there seems to be several unanswered comments from @joerchan

@Vudentz
Copy link
Contributor Author

Vudentz commented Apr 28, 2020

@joerchan patches have been updated to address your comments.

include/bluetooth/l2cap.h Outdated Show resolved Hide resolved
@Vudentz Vudentz force-pushed the bt5_2 branch 2 times, most recently from ecb6cf0 to 100be26 Compare April 29, 2020 17:00
@Vudentz
Copy link
Contributor Author

Vudentz commented Apr 29, 2020

@joerchan Patches have been updated, EATT is now marked as experimental.

@Vudentz Vudentz dismissed jhedberg’s stale review April 29, 2020 18:39

Comments have been addressed.

@carlescufi
Copy link
Member

@joerchan can you please test this on a complex application?

@Vudentz
Copy link
Contributor Author

Vudentz commented Apr 30, 2020

@joerchan can you please test this on a complex application?

Preferable against something that implements EATT already like BlueZ, but you will need to update the enable the kernel parts if you want to test ecred procedures as otherwise it connects L2CAP channel by channel, that said the features is marked as experimental exactly to give people the opportunity to test this themselves.

@joerchan
Copy link
Contributor

@Vudentz My main concern was regression into ATT since some of the refactoring touches on common code. We have tested the nrf_desktop application in NCS with these changes to check for regressions. We haven't found any issues.

I don't have anything to test with for the new features, so I will just go over the latest changes.

@Vudentz
Copy link
Contributor Author

Vudentz commented Apr 30, 2020

@Vudentz My main concern was regression into ATT since some of the refactoring touches on common code. We have tested the nrf_desktop application in NCS with these changes to check for regressions. We haven't found any issues.

I don't have anything to test with for the new features, so I will just go over the latest changes.

I though the reason to add EDTT as part of CI was to extensively test ATT already, anyway now I have to resolve the conflicts you introduced with changes to the documentation which could have been avoided by just waiting this set to go in.

Vudentz added 10 commits April 30, 2020 10:01
This introduces new Enhanced Credit Based Flow Control PDUs and related
definitions which were introduced in 5.2.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This replaces the use of tabs with spaces to align defines.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This adds the initial implementation of ECRED mode which can connect up
to 5 channels simultaneously and is required by EATT.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This leaves only channels on the dynamic range to be offloaded to the
system queue so ATT and EATT handling are handling in the same context.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This adds a callback to indicate when the stack has released all
references to a given channel so the owner free up any resources
associated with that.

This is requires since EATT channels cannot rely on the destroy callback
as it does not use a fixed channel like ATT.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This adds the definitions for Enhanced ATT along with new PDUs and UUIDs
introduced in 5.2.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This adds support for EATT bearer which was introduced in 5.2, they work
as extra channels to have GATT traffic, at the moment it is completely
transparent to application when they are in use since the allocation
happens automatically.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This adds support for ATT_MULTIPLE_HANDLE_VALUE_NTF,
ATT_READ_MULTIPLE_VARIABLE_REQ and ATT_READ_MULTIPLE_VARIABLE_RSP.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This enables BT_EATT for shell samples so it can be build test by CI and
gives user the ability of test it using shell commands.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
This adds a new flag to track if the L2CAP channel is pending waiting
for encryption to be changed to resume connecting.

Signed-off-by: Luiz Augusto von Dentz <[email protected]>
@joerchan
Copy link
Contributor

@Vudentz nrf_desktop is a HID mouse application. It has caught many regressions earlier which is why I wanted to test it now as well. EDTT test cases are good, but they test one thing at a time and are mostly functional test-cases. Some stress-testing is good as well.

This is looking good to me. I only have the one question.

@joerchan joerchan self-requested a review April 30, 2020 17:56
@jhedberg jhedberg merged commit ec5603d into zephyrproject-rtos:master May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Bluetooth area: Tests Issues related to a particular existing or missing test Feature A planned feature with a milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants