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

Jump to bottom

Link Layer Control Procedure overhaul #15256

Closed
26 of 35 tasks
carlescufi opened this issue Apr 8, 2019 · 12 comments
Closed
26 of 35 tasks

Link Layer Control Procedure overhaul #15256

carlescufi opened this issue Apr 8, 2019 · 12 comments
Assignees
@cvinayak @thoh-ot @erbr-ot @kruithofa
Labels
area: Bluetooth Controller area: Bluetooth LLCP area: Bluetooth Enhancement Changes/Updates/Additions to existing features
Projects
Release Plan

Comments

@carlescufi
Copy link
Member

carlescufi commented Apr 8, 2019 •
edited by kruithofa

There are three types of issues that can occur today when handling Control Procedures in the Link layer (both legacy and split):

  • Type 1: Peer initiated procedure first and then local initiated procedure will lead to HCI_CMD_DISALLOWED
  • Type 2: Local initiated procedure first and then peer initiated procedure will lead to disconnect
  • Type 3: Multiple local initiated procedures processed simultaneously, can cause the peer side to disconnect.

We need 2 tables, one per Type, listing the combinations of Control Procedures that can lead to the undesired outcome (HCI_CMD_DISALLOWED or disconnection).

Type 1

Peer (row) Local (col) chan_map_update conn_update phy_req phy_ind min_used_chan ll_ping
phy_req HCI_CMD_DISALLOWED HCI_CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED
feature_exchange HCI_CMD_DISALLOWED HCI_CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED
versiond_ind HCI_CMD_DISALLOWED HCI_CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED
chan_map_update CMD_DISALLOWED CMD_DISALLOWED
length_update HCI_CMD_DISALLOWED HCI_CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED CMD_DISALLOWED

Type 2

Peer (row) Local (col) conn_update chan_map phy_upd phy_req encryption
version_ind disconnect disconnect disconnect reject
feature_exchange disconnect disconnect disconnect reject
length_request reject
slave_feature_exch MIC failure

Type 3

Peer (row) Local (col) conn_param_req
feature_exchange disconnect

Following tasks to do have been defined July 2020

Phase 1:

  • Combine old and new LLCP implementation, using KConfig for selecting either one
  • Integrate new LLCP into LLL controller
  • Integrate new LLCP into HCI
  • Implement procedure payload for PHY update
  • Implement procedure payload for ENCryption start
  • Implement procedure payload for Connectup parameter request
  • Implement procedure payload for Minimum Number of Used Channels
  • Hook procedures into controller logic so that the payload is used
  • Implement ACL termination
  • Implement Data Length Update
  • Implement ENCryption pause
  • Implement Channel Map update
  • Handle termination of connections, both planned and unplanned

Phase 2:

  • Use KConfig for enabling/disabling features
  • Add rejection logic for non-supported features
  • Handle procedure timeouts
  • Handle limited resources: tx-buffers and procedure contexts
  • Validate input parameters for the control procedures

Phase 3:

Phase 4:

  • Fix all TODO's
  • Evaluate ASSERTs: either add proper error handling, or leave them

Phase 5: (added march 2021):

Following only if code-size is an issue:

  • Optimise code: remove duplicate functionality
  • Optimise code: improve state machines
  • Optimise code: memory management, reuse the RX node in the PDU it is in instead of copying to new memory

Design improvements (added august 2022):

See #39435 for the preparations that were done for the upmerge from topic-ble-llcp to main

Timeline in main:

  • 3.0.0 contains the feature, disabled by default
  • 3.2.0 contains the feature, enabled by default
  • 3.3.0 remove the old LLCP code
@carlescufi carlescufi added the Enhancement Changes/Updates/Additions to existing features label Apr 8, 2019
@joerchan
Copy link
Contributor

joerchan commented Apr 9, 2019

Sniffer trace showing disconnect (type 2) channel map update receveid while feature exchange has been queued in the controller. Link disconnected by controller with error code 0x2a

disconnect_transaction_collision_chan_map_phy_req_enqueued.zip

@cvinayak cvinayak mentioned this issue Apr 10, 2019
Merged
@cvinayak cvinayak mentioned this issue Apr 29, 2019
Closed
@joerchan
Copy link
Contributor

joerchan commented Jul 9, 2019

Attached sniffer log of type 3
type3.zip

@maz3max
Copy link

maz3max commented Jul 23, 2019 •
edited

A sniffer log where both peers try to set the security level to FIPS (and fails).
both_check.zip

@maz3max
Copy link

maz3max commented Jul 23, 2019

A sniffer log where only the central tries to set the security level to FIPS (and fails).
only-central-checks.zip

@maz3max maz3max mentioned this issue Jul 23, 2019
Closed
@maz3max
Copy link

maz3max commented Aug 25, 2019

@joerchan After the merge of #17836, the peripheral still cannot set the security level right after connecting. I suspect the problem to be on the central side this time.
Logs are here: https://pastebin.com/55kritYE
Sniffer Capture is here (it loops a few times):
only-periph-req-sec.zip

@carlescufi
Copy link
Member Author

@joerchan After the merge of #17836, the peripheral still cannot set the security level right after connecting. I suspect the problem to be on the central side this time.
Logs are here: https://pastebin.com/55kritYE
Sniffer Capture is here (it loops a few times):
only-periph-req-sec.zip

Thanks for the report @maz3max. @joerchan can you decide whether this requires a new GH issue to be open, or will it be fixed by an existing PR?

@joerchan
Copy link
Contributor

@carlescufi @maz3max :
Reproduced the issue and reported here: #18645

@carlescufi
Copy link
Member Author

carlescufi commented Aug 30, 2019 •
edited

@pskhansen @cvinayak please discuss the resolution of this issue. Note that #15320 is a workaround PR, and there are two options:

@ioannisg ioannisg added this to the v2.1.0 milestone Sep 2, 2019
@joerchan joerchan mentioned this issue Sep 3, 2019
Merged
@nashif nashif moved this from v2.4 - API Freeze to v2.5 - Feature Freeze in Release Plan Oct 20, 2020
@nashif nashif moved this from v2.5 - Feature Freeze to v2.6 - LTS2 in Release Plan Oct 20, 2020
@jhedberg jhedberg moved this from v2.6 - LTS2 to v2.5 - Feature Freeze in Release Plan Nov 3, 2020
@nashif nashif modified the milestones: v2.4.0, v2.6.0 Dec 1, 2020
@nashif nashif moved this from v2.5 - Feature Freeze to v2.6 - LTS2 in Release Plan Dec 1, 2020
@galak galak moved this from v2.6 - Feature Freeze to v2.7 - LTS2 in Release Plan Apr 20, 2021
@galak galak modified the milestones: v2.6.0, v2.7.0 Apr 20, 2021
@cfriedt cfriedt added priority: medium Medium impact/importance bug and removed priority: medium Medium impact/importance bug labels Jul 12, 2021
@dleach02 dleach02 moved this from v2.7 - LTS2 to Parking lot (1 year outlook) in Release Plan Jul 13, 2021
@dleach02 dleach02 removed this from the v2.7.0 milestone Jul 13, 2021
@nashif nashif moved this from Parking lot (1 year outlook) to v3.0 in Release Plan Sep 22, 2021
@kruithofa kruithofa mentioned this issue Oct 14, 2021
Closed
10 tasks
@kruithofa kruithofa mentioned this issue Nov 8, 2021
Merged
kruithofa added a commit to kruithofa/zephyr that referenced this issue Nov 16, 2021
@kruithofa
Bluetooth: controller: push topic branch to main
8a39814 
Pushes all work done in the topic-ble-llcp branch into main branch
This is a refactoring of the LL control procedures; the refactored
control procedures are hidden behind a KConfig option and
per default disabled

Goal of the refactoring:

close issue Link Layer Control Procedure overhaul zephyrproject-rtos#15256
make it easier to add/update control procedures
Refactoring consists in principal of writing explicit state machines
for the control procedures.
To reduce the risk of regression errors unit-tests have been added

Following control procedures are implemented:

Connection update procedure
Channel map update procedure
Encryption procedure
Feature exchange procedure
Version exchange procedure
ACL termination procedure
Connection parameters request procedure
LE Ping procedure
Data Length Update procedure
PHY update procedure
Min. nr. Of channels used procedure
Constant Tone extension request procedure

This is a joined work by the people listed in the signed-off-by
list (in alphabetical order)

Signed-off-by: Andries Kruithof Andries.Kruithof@nordicsemi.no
Signed-off-by: Erik Brockhoff erbr@oticon.com
Signed-off-by: Piotr Pryga piotr.pryga@nordicsemi.no
Signed-off-by: Szymon Janc szymon.janc@codecoup.pl
Signed-off-by: Thomas Ebert Hansen thoh@oticon.com
Signed-off-by: Tommie Skriver tosk@demant.com

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
nashif pushed a commit that referenced this issue Nov 17, 2021
@kruithofa @nashif
Bluetooth: controller: push topic branch to main
f023b5f 
Pushes all work done in the topic-ble-llcp branch into main branch
This is a refactoring of the LL control procedures; the refactored
control procedures are hidden behind a KConfig option and
per default disabled

Goal of the refactoring:

close issue Link Layer Control Procedure overhaul #15256
make it easier to add/update control procedures
Refactoring consists in principal of writing explicit state machines
for the control procedures.
To reduce the risk of regression errors unit-tests have been added

Following control procedures are implemented:

Connection update procedure
Channel map update procedure
Encryption procedure
Feature exchange procedure
Version exchange procedure
ACL termination procedure
Connection parameters request procedure
LE Ping procedure
Data Length Update procedure
PHY update procedure
Min. nr. Of channels used procedure
Constant Tone extension request procedure

This is a joined work by the people listed in the signed-off-by
list (in alphabetical order)

Signed-off-by: Andries Kruithof Andries.Kruithof@nordicsemi.no
Signed-off-by: Erik Brockhoff erbr@oticon.com
Signed-off-by: Piotr Pryga piotr.pryga@nordicsemi.no
Signed-off-by: Szymon Janc szymon.janc@codecoup.pl
Signed-off-by: Thomas Ebert Hansen thoh@oticon.com
Signed-off-by: Tommie Skriver tosk@demant.com

Signed-off-by: Andries Kruithof <Andries.Kruithof@nordicsemi.no>
@carlescufi carlescufi moved this from v3.0 to v3.1 in Release Plan Jan 12, 2022
@stephanosio stephanosio moved this from v3.1 to v3.2 in Release Plan Jun 6, 2022
@fabiobaltieri
Copy link
Member

@carlescufi can you update this one? Looks like all issues/PRs linked here have been merged/closed, is this done?

@carlescufi
Copy link
Member Author

@carlescufi can you update this one? Looks like all issues/PRs linked here have been merged/closed, is this done?

This can be considered as done for 3.2.0 @fabiobaltieri

@mmahadevan108 mmahadevan108 mentioned this issue Sep 9, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cvinayak cvinayak

@thoh-ot thoh-ot

@erbr-ot erbr-ot

@kruithofa kruithofa

Labels
area: Bluetooth Controller area: Bluetooth LLCP area: Bluetooth Enhancement Changes/Updates/Additions to existing features
Projects
Release Plan
Archived in project
Release Plan
  
v3.2
Milestone
No milestone
Development

No branches or pull requests