nRF53: HCI driver & sample #20467

kapi-no · 2019-11-08T14:33:17Z

Bluetooth HCI over OpenAMP sample
Bluetooth HCI driver over OpenAMP

kapi-no

Highlighting a few unresolved issues

kapi-no · 2019-11-08T14:35:05Z

samples/bluetooth/hci_open_amp/src/main.c

Is there a DTS symbol for shared memory?

Yes, @kapi-no there is DTS information in the nrf5340 DTS structure that points to the shared memory.

Yes, I see - there is sram0_shared DT node, but I don't see any alias / define generated for it

kapi-no · 2019-11-08T14:38:18Z

drivers/bluetooth/hci/Kconfig

I would like to configure channel direction here. However, this syntax does not work

select IPM_MSG_CH_0_TX select IPM_MSG_CH_1_RX

since IPM_MSG_CH_0_TX is a choice item. How can I work around this?

Could you do what we did with choosing a libc implementation?

Add a layer of indirection by having the user choose something that affected the default choice.

https://github.com/zephyrproject-rtos/zephyr/blob/master/lib/libc/Kconfig#L17

Unfortunately, the choice option is defined within a Kconfig template file (the template file depends on channel number), so it would be messy to have it there (if possible at all)

https://github.com/zephyrproject-rtos/zephyr/blob/9523043d97ce6ed895a02185378971c67e342846/drivers/ipm/Kconfig.nrfx_ipc_channel

It is also impossible to give a parameterized name to the choice option:
choice IPM_MSG_CH_$(nrfx_ipc_num)_DIR
Kconfig python parser scripts cannot handle that.

With name, I could define the default in the Kconfig board file:

choice IPM_MSG_CH_0_DIR default IPM_MSG_CH_0_TX if BT_NRF_OPEN_AMP endchoice

carlescufi · 2019-11-08T14:41:08Z

CC @alexandru-porosanu-nxp

carlescufi

A quick naming change request.

Both the driver and the sample should be named hci_rpmsg. This is because RPMsg is the transport, and you don't need OpenAMP on the other side, you can use any other RPMsg implementation instead.

jhedberg · 2019-11-08T14:52:59Z

drivers/bluetooth/hci/open_amp_nrf.c

The whole idea of the bt_recv_prio() vs. bt_recv() separation is that they do not happen from the same context (thread). This is particularly important when you're implementing the RX thread on the HCI driver side rather than letting the host do it. In this case bt_recv_prio() should probably be called in whatever context is feeding rx_queue (this queue should only be given non-prio packets) and here in the rx_thread you'd only use bt_recv()

Actually, you're not selecting BT_RECV_IS_RX_THREAD in your Kconfig definition but still creating your own RX thread inside the driver? You need to decide whether you want to keep the thread inside the driver or the host (or perhaps both, which would have a substantial memory overhead). As long as you don't select BT_RECV_IS_RX_THREAD it is actually safe to call the recv() APIs like you're doing since the host-side will take care of splitting processing into independent threads, however if you did mean to select BT_RECV_IS_RX_THREAD then you need to separate them inside the driver.

Actually, you're not selecting BT_RECV_IS_RX_THREAD in your Kconfig definition but still creating your own RX thread inside the driver?

That BT_RECV_IS_RX_THREAD option is still poorly named, but the documentation is better.

# If the host has its own RX thread it is safe to call bt_recv and # bt_recv_prio from the same priority context.

Since we call it from the same priority context, we need to select the BT_RECV_IS_RX_THREAD option.

Isn't that the other way around? The host has its own RX thread when BT_RECV_IS_RX_THREAD is not enabled

Isn't that the other way around? The host has its own RX thread when BT_RECV_IS_RX_THREAD is not enabled

Yes. The intention of the current option name is to say "The bt_recv() call happens in the Bluetooth RX thread", which then implies that there's no need for the host to have such a thread. I'm open to better names if you think it's confusing.

Since we call it from the same priority context, we need to select the BT_RECV_IS_RX_THREAD option.

But do you need to call it from the same context? What's the reason there's something called the RX thread inside the driver, and that thread uses the Bluetooth RX thread Kconfig options like BT_RX_THREAD_SIZE. That makes it very much look like "the" Bluetooth RX thread 😄

So, do you need a separate thread like this in the driver? If not, just call bt_recv() and bt_recv_prio() directly from the context that currently feeds the rx_queue and let the host take care of the details. If you do need the separate thread, move the bt_recv_prio() like I suggested and select the BT_RECV_IS_RX_THREAD option.

Since we call it from the same priority context, we need to select the BT_RECV_IS_RX_THREAD option.

Like Carles said it's actually the opposite. My previous comment was based on imagining a "don't" in the above quote 😄

Isn't that the other way around? The host has its own RX thread when BT_RECV_IS_RX_THREAD is not enabled

Yes, sorry. Messed it up.

In h4.c bt_recv is called from thread. and bt_recv_prio is called from interrupt.
In the hci_driver.c bt_recv is called from a thread, and bt_recv_prio is called from a different thread with higher priority.
In open_amp_nrf.c bt_recv and bt_recv_prio is called from the same thread.

So h4 and hci_driver both fulfull the requirement of sending from different priority contexts, and can select BT_RECV_IS_RX_THREAD.
This one cannot select it because it it calls them from the same priority context.

So then this is correct.

So then this is correct.

It's not wrong from a HCI driver API contract perspective, but my question still remains: is this "RX thread" in the driver actually needed, and why should it have its stack-size determined by the Kconfig option when it never reaches the application level (that's why the normal RX thread size is user-configurable).

Thank you for clarifying. I completely misunderstood your comment. And I think you have a good point (assuming I have understood it correctly :P).

@kapi-no Have you considered getting rid of the TX thread here and calling bt_recv and bt_recv_prio directly from bt_open_amp_rx. In bt_recv we are basically doing the same thing as what is being done here (see here: https://github.com/zephyrproject-rtos/zephyr/blob/master/subsys/bluetooth/host/hci_core.c#L4866). That would mean that bt_recv_prio would be processed in the context of bt_open_amp_rx, which seem to be interrupt context.
So I guess the real question is if there is any downside to calling bt_recv_prio directly from interrupt context (there are other HCI drivers that do exactly that).

jhedberg · 2019-11-08T14:53:53Z

drivers/ipm/Kconfig

What's this about? I don't think we've had Quark SE support in the tree for quite awhile.

Yep, just get rid of all Quark-related changes @kapi-no

Something went really wrong in the rebase

Yes, this is leftover from rebasing

SebastianBoe · 2019-11-08T16:17:14Z

drivers/ipm/Kconfig.nrfx

SebastianBoe · 2019-11-08T16:20:40Z

drivers/ipm/ipm_nrfx_ipc.c

Nice, I haven't seen anyone else figure out how to do this until now.

SebastianBoe · 2019-11-08T16:24:03Z

drivers/ipm/ipm_nrfx_ipc.c

vipm_nrf_##_idx##_init

We should do as little code generation as possible. Co-incidentally,

vipm_nrf_##_idx##_init is the same for all _idx. So we can have vipm_nrf_n_init, and re-use it for all _idx.

SebastianBoe · 2019-11-08T16:31:34Z

drivers/ipm/ipm_nrfx_ipc.c

Isn't this code-path impossible due to https://github.com/zephyrproject-rtos/zephyr/pull/20467/files#diff-ec888dfc90d8c3eb9142711f8e6a3281R226 ?

Right, removed

SebastianBoe · 2019-11-08T16:35:39Z

drivers/ipm/Kconfig.template.nrf_ipc_channel

Would changing RX to a bool, and having

config IPM_MSG_CH_$(nrfx_ipc_num)_TX default ! IPM_MSG_CH_$(nrfx_ipc_num)_RX

help with your issues with choice?

Do you mean something along these lines?

menu "IPM Message Channel [$(nrfx_ipc_num)] configuration" config IPM_MSG_CH_$(nrfx_ipc_num)_ENABLE bool "Enable IPM Message Channel $(nrfx_ipc_num)" config IPM_MSG_CH_$(nrfx_ipc_num)_RX bool "IPM Message RX Channel" depends on IPM_MSG_CH_$(nrfx_ipc_num)_ENABLE config IPM_MSG_CH_$(nrfx_ipc_num)_TX bool "IPM Message TX Channel" depends on IPM_MSG_CH_$(nrfx_ipc_num)_ENABLE default ! IPM_MSG_CH_$(nrfx_ipc_num)_RX endmenu

Yes, getting rid of choice option solves issues with choice :) and I can simply select proper configuration.

I think RX and TX could both get enabled with that configuration. I was thinking of having one option promptless to prevent this. But that might not allow you to select from other options as you want.

So I don't really see a clean solution here.

Yes, I am going to use my suggestion in this PR. It could be improved in other PR if we find a cleaner way of doing it

SebastianBoe · 2019-11-08T16:41:03Z

soc/arm/nordic_nrf/nrf53/Kconfig.defconfig.series

zephyrbot · 2019-11-08T19:27:32Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:53: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#53: FILE: boards/arm/nrf5340_dk_nrf5340/nrf5340_dk_nrf5340_shared_sram_planning_conf.dts:15:
+    chosen {$

-:55: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#55: FILE: boards/arm/nrf5340_dk_nrf5340/nrf5340_dk_nrf5340_shared_sram_planning_conf.dts:17:
+      zephyr,ipc_shm = &sram0_shared;$

-:56: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#56: FILE: boards/arm/nrf5340_dk_nrf5340/nrf5340_dk_nrf5340_shared_sram_planning_conf.dts:18:
+    };$

- total: 0 errors, 3 warnings, 1678 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

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

carlescufi · 2019-11-15T09:56:57Z

@jhedberg @arnopo @dbkinder can you please take another look?

ioannisg

Looks pretty good now, approved.
@kapi-no can you take care of the minor issues i have spotted i the last pass?

Stale, please re-review

arnopo · 2019-11-15T16:13:11Z

drivers/bluetooth/hci/rpmsg_nrf53.c

+	/* Since we are using name service, we need to wait for a response
+	 * from NS setup and than we need to process it
+	 */
+	virtqueue_notification(vq[0]);


This seems useless. when called remote processor still not ready to send the NS message, so nothing to process in queue.
Application will be blocked on sync_sem waiting rpmsg from the remote side.
ipm_callback should call the virtqueue_notification that will process message and call the ns_bind_cb

You are right - this is redundant.

arnopo · 2019-11-15T16:15:51Z

drivers/bluetooth/hci/rpmsg_nrf53.c

+static void ipm_callback(void *context, u32_t id, volatile void *data)
+{
+	BT_DBG("Got callback of id %u", id);
+	virtqueue_notification(vq[0]);


Same issue, processing in interrupt context

I tried to switch context for virtqueue_notification from ISR to the workqueue context, but oddly enough this triggers MPU fault.

There is a flash write attempt that crashes the application. It occurs inside sys_dlist_remove in the k_sys_work_q context. Do you see any potential scenario in which the MPU fault could be caused by OpenAMP library (e.g. pointer corruption)?

However, using a dedicated thread to process virtqueue_notification seems to work.

if you are sending a message you can take rdev->lock (rpmsg_virtio_send_offchannel_raw). And ipm_callback is called you will treat received messages , try to take rdev->lock a second time(rpmsg_virtio_rx_callback) => deadlock
yes using a dedicated thread is a good solution

Added a dedicated thread for virtqueue_notification

drivers/bluetooth/hci/rpmsg_nrf53.c

kapi-no · 2019-11-19T12:49:05Z

@arnopo, please rereview

We are adding code owners for the Nordic nRFx IPM driver files. Signed-off-by: Ioannis Glaropoulos <[email protected]>

This commit contributes the nRFx IPM driver. Signed-off-by: Karol Lasończyk <[email protected]>

This commit adds ipc-0 aliases in the DTS framework for nRF5340 Application and Network MCU. Signed-off-by: Emil Obalski <[email protected]>

The interprocessor communication can be based on shared memory. Allow to declare this memory as with a generic name derived from chosen declaration. Signed-off-by: Arnaud Pouliquen <[email protected]>

Bound shared memory definition with IPC shim for nRF5340 DK. Signed-off-by: Kamil Piszczek <[email protected]>

The HCI transport implemented by an application using the HCI raw interface may have its own buffer headroom requirements. Currently the available headroom gets completely determined by the selected HCI driver. E.g. most of the time this is the native controller driver which doesn't reserve any headroom at all. To cover for the needs of HCI raw users, add a new Kconfig variable for the apps to set to whatever they need. Correspondingly, use the maximum of the HCI driver and HCI raw headroom requirements for the buffer pool definitions and the headroom initializations. Signed-off-by: Johan Hedberg <[email protected]>

This commit contributes a BLE HCI-over-RPMsg sample. Co-authored-by: Kamil Piszczek <[email protected]> Co-authored-by: Nikodem Kastelik <[email protected]> Signed-off-by: Kamil Piszczek <[email protected]>

This commit contributes an RPMsg-based transport for BLE HCI. Signed-off-by: Kamil Piszczek <[email protected]>

When building with support for BLE stack, enable the Vendor Specific commands for the nRF5340 APP CPU (Application MCU). Signed-off-by: Kamil Piszczek <[email protected]>

When building with support for BLE stack, enable the BT_ECC for the nRF5340 NET CPU (Network MCU). Signed-off-by: Kamil Piszczek <[email protected]>

When building with support for BLE stack on the nRF5340 APP CPU (Application MCU), use RPMsg HCI driver by default. Signed-off-by: Kamil Piszczek <[email protected]>

jhedberg self-requested a review November 8, 2019 14:37

kapi-no commented Nov 8, 2019

View reviewed changes

carlescufi requested review from alexeRadu, cvinayak and joerchan November 8, 2019 14:40

carlescufi requested review from anangl and carlescufi November 8, 2019 14:41

carlescufi requested changes Nov 8, 2019

View reviewed changes

jhedberg requested changes Nov 8, 2019

View reviewed changes

SebastianBoe reviewed Nov 8, 2019

View reviewed changes

drivers/ipm/ipm_nrfx_ipc.c Outdated

Copy link

Contributor

SebastianBoe Nov 8, 2019

Choose a reason for hiding this comment

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

Nice, I haven't seen anyone else figure out how to do this until now.

SebastianBoe reviewed Nov 8, 2019

View reviewed changes

soc/arm/nordic_nrf/nrf53/Kconfig.defconfig.series Outdated

Copy link

Contributor

SebastianBoe Nov 8, 2019

Choose a reason for hiding this comment

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

rsource

joerchan previously approved these changes Nov 8, 2019

View reviewed changes

ioannisg force-pushed the hci_nrf53 branch from da786cf to 9c629e3 Compare November 8, 2019 18:46

zephyrbot added area: Devicetree area: Boards area: Bluetooth area: Samples Samples labels Nov 8, 2019

ioannisg requested review from barsok, kl-cruz and masz-nordic November 8, 2019 23:00

ioannisg added the TSC Topics that need TSC discussion label Nov 9, 2019

ioannisg marked this pull request as ready for review November 9, 2019 13:33

ioannisg requested review from ioannisg and lemrey as code owners November 9, 2019 13:33

jhedberg approved these changes Nov 15, 2019

View reviewed changes

ioannisg approved these changes Nov 15, 2019

View reviewed changes

ioannisg requested a review from dbkinder November 15, 2019 10:11

kapi-no force-pushed the hci_nrf53 branch from cc544f9 to b0cabbe Compare November 15, 2019 10:42

joerchan requested a review from arnopo November 15, 2019 11:34

ioannisg approved these changes Nov 15, 2019

View reviewed changes

ioannisg requested a review from SebastianBoe November 15, 2019 12:00

SebastianBoe approved these changes Nov 15, 2019

View reviewed changes

arnopo requested changes Nov 15, 2019

View reviewed changes

ioannisg force-pushed the hci_nrf53 branch from b0cabbe to a81331b Compare November 18, 2019 20:29

ioannisg mentioned this pull request Nov 18, 2019

Ncs nrf53 nrfconnect/sdk-zephyr#239

Merged

kapi-no force-pushed the hci_nrf53 branch from a81331b to 752a5a5 Compare November 19, 2019 12:43

arnopo approved these changes Nov 19, 2019

View reviewed changes

ioannisg and others added 11 commits November 19, 2019 14:50

CODEOWNERS: add code owners for nRFx IPM driver

0e17225

We are adding code owners for the Nordic nRFx IPM driver files. Signed-off-by: Ioannis Glaropoulos <[email protected]>

drivers: ipm: add nRFx IPM driver

d788b90

This commit contributes the nRFx IPM driver. Signed-off-by: Karol Lasończyk <[email protected]>

soc: arm: nordic: adding ipc aliases for nRF53

3916949

This commit adds ipc-0 aliases in the DTS framework for nRF5340 Application and Network MCU. Signed-off-by: Emil Obalski <[email protected]>

scripts: dts: Add new chosen declaration for the IPC shared memory

bb94dbc

The interprocessor communication can be based on shared memory. Allow to declare this memory as with a generic name derived from chosen declaration. Signed-off-by: Arnaud Pouliquen <[email protected]>

boards: arm: nrf5340_dk_nrf5340: bind shared memory with ipc shim in dts

da75764

Bound shared memory definition with IPC shim for nRF5340 DK. Signed-off-by: Kamil Piszczek <[email protected]>

samples: bluetooth: adding hci_rpmsg sample

a9083b5

This commit contributes a BLE HCI-over-RPMsg sample. Co-authored-by: Kamil Piszczek <[email protected]> Co-authored-by: Nikodem Kastelik <[email protected]> Signed-off-by: Kamil Piszczek <[email protected]>

drivers: bluetooth: hci: driver based on RPMsg transport

df63dd8

This commit contributes an RPMsg-based transport for BLE HCI. Signed-off-by: Kamil Piszczek <[email protected]>

boards: nrf5340_dk_nrf5340: enable VS commands if building with BT

fcfabdb

When building with support for BLE stack, enable the Vendor Specific commands for the nRF5340 APP CPU (Application MCU). Signed-off-by: Kamil Piszczek <[email protected]>

boards: nrf5340_dk_nrf5340: enable ECC if building with BT

d9daa8c

When building with support for BLE stack, enable the BT_ECC for the nRF5340 NET CPU (Network MCU). Signed-off-by: Kamil Piszczek <[email protected]>

boards: nrf5340_dk_nrf5340: use RPMsg as default HCI driver for BLE

2553316

When building with support for BLE stack on the nRF5340 APP CPU (Application MCU), use RPMsg HCI driver by default. Signed-off-by: Kamil Piszczek <[email protected]>

ioannisg force-pushed the hci_nrf53 branch from 752a5a5 to 2553316 Compare November 19, 2019 13:50

carlescufi merged commit 2cceb62 into zephyrproject-rtos:master Nov 20, 2019

nRF53: HCI driver & sample #20467

nRF53: HCI driver & sample #20467

Uh oh!

Conversation

kapi-no commented Nov 8, 2019 • edited by ioannisg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kapi-no left a comment

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 commented Nov 8, 2019

Uh oh!

carlescufi left a comment

Choose a reason for hiding this comment

Uh oh!

jhedberg Nov 8, 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!

joerchan Nov 8, 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!

carlescufi Nov 8, 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!

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!

SebastianBoe Nov 8, 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

kapi-no commented Nov 8, 2019 •

edited by ioannisg

Loading

jhedberg Nov 8, 2019 •

edited

Loading

joerchan Nov 8, 2019 •

edited

Loading

carlescufi Nov 8, 2019 •

edited

Loading

SebastianBoe Nov 8, 2019 •

edited

Loading

SebastianBoe Nov 8, 2019 •

edited

Loading

zephyrbot commented Nov 8, 2019 •

edited

Loading