Bluetooth: controller: Introduce ULL LLL architecture #12404

cvinayak · 2019-01-09T20:00:43Z

This Pull Request introduces a new, improved Link Layer based on the concept of split responsibilities:

The Upper Link Layer (ULL) is in charge of control procedures, inter-event scheduling and overal role management. The code for the ULL is shared among all hardware implementations.
The Lower Link Layer (LLL) is responsible for the intra-event scheduling and radio access.

The communication between ULL and LLL is achieved through a set of FIFOs that contain both control and data packets.

zephyrbot · 2019-01-09T20:06:45Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

codecov-io · 2019-01-09T20:36:37Z

Codecov Report

Merging #12404 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #12404   +/-   ##
=======================================
  Coverage   53.94%   53.94%           
=======================================
  Files         242      242           
  Lines       27654    27654           
  Branches     6717     6717           
=======================================
  Hits        14917    14917           
  Misses       9932     9932           
  Partials     2805     2805

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99aadec...e5d23a2. Read the comment docs.

carlescufi · 2019-01-10T11:44:49Z

subsys/bluetooth/controller/ll_sw/nordic/lll/lll_tmp.c

Are those printk staatements intentional?

tmp was a template role, this I will remove as part of the rework, this should not be part of the merge into master.

carlescufi · 2019-01-10T11:55:01Z

subsys/bluetooth/controller/util/mfifo.h

If you are using this only to store ring buffers of pointers, then you could simplify by:
void* m[(cnt) + 1]; and get rid of s and sz altogether.

This would also get rid of all the multiplications in the getters/setters

I will take relook at this implementation once I resolve all the checkpatch issues.

carlescufi · 2019-01-10T11:56:06Z

subsys/bluetooth/controller/util/mfifo.h

If you need to support non-pointer mfifos then this should be a memcpy

carlescufi · 2019-01-10T11:58:03Z

subsys/bluetooth/controller/util/mfifo.h

This file needs comments on each function explaining what they do.

I've documented this in #12167

Note that #12167 was spun out of https://github.com/NordicPlayground/zephyr-bt/pull/26 as that was the intersection between non-split and split stack.

Thank you @mped-oticon, that is a great effort. A while back I added the memq_link_t in order to make the code more readable, and with your comments this closes the circle.

I've documented this in #12167

That PR doesn't include mfifo but you have another branch for that.

Remind me why we need a different fifo/list implementation?

For 3 reasons:

Memory efficiency and code size

Latency: this is lockless unlike the kernel's FIFOs, never disables interrupts

Independence from the kernel. This is only used on interrupt context, should never block or use kernel primitives

In essence this is a custom-made, highly efficient, lockless implementation for the specific purpose of exchanging data among different interrupt contexts.

carlescufi · 2019-01-10T12:01:21Z

subsys/bluetooth/controller/util/mfifo.h

This will only work with void* MFIFOs

Correct. I've written about this in the documentation. See prev comment.

carlescufi · 2019-01-10T12:04:22Z

subsys/bluetooth/controller/util/mfifo.h

This will only work with void* MFIFOs

See header of https://github.com/mped-oticon/zephyr-bt/blob/783481527a1c435251484783bd4ef01bd237654b/subsys/bluetooth/controller/util/mfifo.h

Oh nice! Will you submit this after we merge this PR?

carlescufi · 2019-01-10T12:33:27Z

subsys/bluetooth/controller/ll_sw/ull_conn.c

The problem I see with these is that the registers are typically loaded first, and then the comparison is executed:

if (conn->llcp_req != conn->llcp_ack) { 13916: f890 30d0 ldrb.w r3, [r0, #208] ; 0xd0 1391a: f890 20d1 ldrb.w r2, [r0, #209] ; 0xd1 1391e: 429a cmp r2, r3

Which means that if you get interrupted between 1391a and 1391e you will be comparing a stale value.

I will add a commit to this PR to handle context-safety of the control procedures.

carlescufi · 2019-01-14T11:09:44Z

subsys/bluetooth/controller/ll_sw/ull_conn.c

On Armv7-M this would be better served by using atomic_cas() is my opinion. I am not sure about other architectures (ping @mped-oticon and @aescolar) but on Armv7-M atomic_cas() uses CONFIG_ATOMIC_OPERATIONS_BUILTIN which then uses the builtin __atomic_compare_exchange_n. This translates into the assembly below, which in my opinion is going to be smaller in size than your code once assembled.
Note that on Armv6-M atomic_cas() disables interrupts and so there you will need the software implementation you have here.

return __atomic_compare_exchange_n(target, &old_value, new_value, 4d4: 4b17 ldr r3, [pc, #92] ; (534 <bt_ready+0x74>) 4d6: 9403 str r4, [sp, #12] 4d8: 2104 movs r1, #4 4da: f3bf 8f5b dmb ish 4de: e853 2f00 ldrex r2, [r3] 4e2: 42a2 cmp r2, r4 4e4: d103 bne.n 4ee <bt_ready+0x2e> 4e6: e843 1000 strex r0, r1, [r3] 4ea: 2800 cmp r0, #0 4ec: d1f7 bne.n 4de <bt_ready+0x1e>

Note: Oticon's mcu does not have atomic_cas.

Since the value is incremented, should we use atomic_inc(&conn->llcp_req) ?

mped-oticon

Remove DNM?

thoh-ot

QACK

cvinayak · 2019-01-17T09:29:18Z

@mped-oticon focusing on the CI failures, once its resolved I will remove the DNM

jhedberg · 2019-01-22T12:09:11Z

subsys/bluetooth/shell/hci.h

All header guards in Zephyr were recently changed to be constructed out of the full path. This file's should then be ZEPHYR_SUBSYS_BLUETOOTH_SHELL_HCI_H_

Actually, does this file even need a guard? Isn't it completely internal?

Agree will remove it.

jhedberg · 2019-01-22T12:10:04Z

subsys/bluetooth/shell/ll.c

Is it appropriate for a shell module to use printk? Isn't the recommendation to use the shell's own output APIs?

This is an oversight, will fix it.

Vudentz · 2019-01-22T12:48:18Z

subsys/bluetooth/controller/ll_sw/ull_slave.c

There is some many duplicates of this logic bellow, can't we make a helper function to check llcp_req against llcp_ack?

Agree. Can we agree to create an issue and assign it to me? This is mentioned by @carlescufi to evaluate the use of atomic_CAS instead.

No problem, lets have it separately with use of atomic as well.

Agree. Can we agree to create an issue and assign it to me? This is mentioned by @carlescufi to evaluate the use of atomic_CAS instead.

Note: Oticon's mcu does not have atomic_cas.
Otherwise, fully agree about helper function or macro.

jhedberg · 2019-01-22T13:08:43Z

subsys/bluetooth/shell/ll.c

shell_print() should not have a line terminator, so the \n needs to be removed

jhedberg · 2019-01-22T13:08:51Z

subsys/bluetooth/shell/ll.c

Refactored the internal LL interfaces to have return value to match the HCI error code u8_t data type. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Missing updates to old architecture implementation towards introduction of new ULL LLL architecture. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Preliminary work done towards Mesh extensions on the old LL architecture implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Updates related to new ULL LLL controller architecture. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

This is a squash merge of commits introducing the new split Upper Link Layer and Lower Link Layer architecture of the Bluetooth Low Energy controller. This introduces a new, improved Link Layer based on the concept of split responsibilities; The Upper Link Layer (ULL) is in charge of control procedures, inter-event scheduling and overall role management. The code for the ULL is shared among all hardware implementations. The Lower Link Layer (LLL) is responsible for the intra-event scheduling and vendor specific radio hardware access. The communication between ULL and LLL is achieved through a set of FIFOs that contain both control and data packets. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]> Signed-off-by: Alberto Escolar Piedras <[email protected]> Signed-off-by: Wolfgang Puffitsch <[email protected]> Signed-off-by: Morten Priess <[email protected]>

Fix the control procedure context safety by adding checks in thread mode control path to detect pre-emption by interrupt. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak requested review from Vudentz, carlescufi, jhedberg and sjanc as code owners January 9, 2019 20:00

carlescufi requested review from aescolar and mped-oticon January 10, 2019 11:29

carlescufi reviewed Jan 10, 2019

View reviewed changes

aescolar requested review from mtpr-ot and thoh-ot January 10, 2019 11:46

carlescufi reviewed Jan 10, 2019

View reviewed changes

cvinayak changed the title ~~Bluetooth: controller: Introduce ULL LLL architecture~~ DNM: Bluetooth: controller: Introduce ULL LLL architecture Jan 10, 2019

cvinayak added the DNM This PR should not be merged (Do Not Merge) label Jan 10, 2019

cvinayak force-pushed the github_ull_lll branch 3 times, most recently from 444603c to 8dac0b5 Compare January 14, 2019 10:26

carlescufi reviewed Jan 14, 2019

View reviewed changes

cvinayak force-pushed the github_ull_lll branch from 8dac0b5 to 8fc1cbf Compare January 14, 2019 13:00

mped-oticon approved these changes Jan 17, 2019

View reviewed changes

thoh-ot approved these changes Jan 17, 2019

View reviewed changes

mtpr-ot approved these changes Jan 17, 2019

View reviewed changes

cvinayak force-pushed the github_ull_lll branch 2 times, most recently from 207156d to 5a24a68 Compare January 21, 2019 11:01

cvinayak force-pushed the github_ull_lll branch from 5a24a68 to 0bdd7c2 Compare January 21, 2019 12:27

carlescufi approved these changes Jan 22, 2019

View reviewed changes

cvinayak force-pushed the github_ull_lll branch from 0bdd7c2 to 59e5cfd Compare January 22, 2019 11:56

jhedberg reviewed Jan 22, 2019

View reviewed changes

Vudentz reviewed Jan 22, 2019

View reviewed changes

cvinayak force-pushed the github_ull_lll branch from 59e5cfd to f3baa14 Compare January 22, 2019 12:55

jhedberg reviewed Jan 22, 2019

View reviewed changes

subsys/bluetooth/shell/ll.c Outdated

Copy link

Member

jhedberg Jan 22, 2019

Choose a reason for hiding this comment

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

shell_print() should not have a line terminator, so the \n needs to be removed

jhedberg reviewed Jan 22, 2019

View reviewed changes

subsys/bluetooth/shell/ll.c Outdated

Copy link

Member

jhedberg Jan 22, 2019

Choose a reason for hiding this comment

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

same here

cvinayak force-pushed the github_ull_lll branch from f3baa14 to 986d606 Compare January 22, 2019 13:13

aescolar approved these changes Jan 22, 2019

View reviewed changes

cvinayak changed the title ~~DNM: Bluetooth: controller: Introduce ULL LLL architecture~~ Bluetooth: controller: Introduce ULL LLL architecture Jan 22, 2019

cvinayak removed the DNM This PR should not be merged (Do Not Merge) label Jan 22, 2019

jhedberg approved these changes Jan 22, 2019

View reviewed changes

cvinayak added 6 commits January 23, 2019 01:45

Bluetooth: controller: Refactor the internal LL interfaces

543399e

Refactored the internal LL interfaces to have return value to match the HCI error code u8_t data type. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: controller: Updates to prep to introduce new arch

3c443ae

Missing updates to old architecture implementation towards introduction of new ULL LLL architecture. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: controller: Preliminary work in progress mesh ext.

d0a2254

Preliminary work done towards Mesh extensions on the old LL architecture implementation. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: shell: Updates related to new ULL LLL architecture

e5d23a2

Updates related to new ULL LLL controller architecture. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

Bluetooth: controller: Fix control procedure context safety

1965a97

Fix the control procedure context safety by adding checks in thread mode control path to detect pre-emption by interrupt. Signed-off-by: Vinayak Kariappa Chettimada <[email protected]>

cvinayak force-pushed the github_ull_lll branch from 986d606 to e5d23a2 Compare January 23, 2019 00:51

carlescufi merged commit 5478e82 into zephyrproject-rtos:master Jan 23, 2019

carlescufi mentioned this pull request Jan 24, 2019

BLE Split Link Layer #12681

Closed

6 tasks

carlescufi mentioned this pull request Mar 17, 2019

BLE Split Link Layer Improvements #13519

Closed

10 tasks

cvinayak deleted the github_ull_lll branch March 1, 2021 00:43

Bluetooth: controller: Introduce ULL LLL architecture #12404

Bluetooth: controller: Introduce ULL LLL architecture #12404

Uh oh!

Conversation

cvinayak commented Jan 9, 2019 • edited by carlescufi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zephyrbot commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jan 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi Jan 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi Jan 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mped-oticon left a comment

Choose a reason for hiding this comment

Uh oh!

thoh-ot left a comment

Choose a reason for hiding this comment

Uh oh!

cvinayak commented Jan 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvinayak commented Jan 9, 2019 •

edited by carlescufi

Loading

zephyrbot commented Jan 9, 2019 •

edited

Loading

codecov-io commented Jan 9, 2019 •

edited

Loading

carlescufi Jan 22, 2019 •

edited

Loading

carlescufi Jan 10, 2019 •

edited

Loading

carlescufi Jan 14, 2019 •

edited

Loading