Missing #if NIMBLE_CFG_CONTROLLER guard for esp_bt_controller_disable in nimble_port_freertos_deinit

[AxelLin]

In porting/npl/freertos/src/nimble_port_freertos.c: The esp_bt_controller_enable() is guarded by #if NIMBLE_CFG_CONTROLLER, so the corresponding #if NIMBLE_CFG_CONTROLLER guard is required for esp_bt_controller_disable() in nimble_port_freertos_deinit().

Notice this issue when I try to build with BT_CONTROLLER_DISABLED and use uart hci with external bt controller.

BTW, I'm wondering if below hack should use

if !NIMBLE_CFG_CONTROLLER to replace #if !(SOC_ESP_NIMBLE_CONTROLLER) in below lines:

https://github.com/espressif/esp-nimble/blob/nimble-1.4.0-idf/nimble/host/src/ble_hs_hci_cmd.c#L75 https://github.com/espressif/esp-nimble/blob/nimble-1.4.0-idf/nimble/host/src/ble_hs_hci_cmd.c#L92 When CONFIG_BT_CONTROLLER_DISABLED=y, it does not need vhci hack, right?

[rahult-github]

Hi @AxelLin

corresponding #if NIMBLE_CFG_CONTROLLER guard is required for esp_bt_controller_disable() in nimble_port_freertos_deinit()

Thanks for pointing this out. Will get it checked and have correct flag added

BTW, I'm wondering if below hack should use

if !NIMBLE_CFG_CONTROLLER to replace #if !(SOC_ESP_NIMBLE_CONTROLLER) in below lines:

No, actually, these flags are meant for different purposes. There is a new internal implementation in progress and one part of it that VHCI layer will not be used when SOC_ESP_NIMBLE_CONTROLLER flag is enabled.

When CONFIG_BT_CONTROLLER_DISABLED=y, it does not need vhci hack, right?

True.

[AxelLin]

@rahult-github Thanks for your prompt reply.

I need below change to pass compilation when CONFIG_BT_CONTROLLER_DISABLED=y and use uart hci with external BT controller. The reason is "bsp/bsp.h" and SYSINIT_PANIC_ASSERT_MSG do not exist in freertos port. Maybe this is something you can improve as well. FYI.

diff --git a/nimble/transport/uart/src/ble_hci_uart.c b/nimble/transport/uart/src/ble_hci_uart.c
index cbb6dd42..ca0593b9 100644
--- a/nimble/transport/uart/src/ble_hci_uart.c
+++ b/nimble/transport/uart/src/ble_hci_uart.c
@@ -25,7 +25,7 @@
 #include "sysinit/sysinit.h"
 #include "syscfg/syscfg.h"
 #include "os/os_cputime.h"
-#include "bsp/bsp.h"
+//#include "bsp/bsp.h"
 #include "os/os.h"
 #include "mem/mem.h"
 #include "hal/hal_gpio.h"
@@ -1178,7 +1178,8 @@ ble_hci_uart_init(void)
     SYSINIT_PANIC_ASSERT(rc == 0);

     rc = ble_hci_uart_config();
-    SYSINIT_PANIC_ASSERT_MSG(rc == 0, "Failure configuring UART HCI");
+    SYSINIT_PANIC_ASSERT(rc == 0);
+    //SYSINIT_PANIC_ASSERT_MSG(rc == 0, "Failure configuring UART HCI");

     memset(&ble_hci_uart_state, 0, sizeof ble_hci_uart_state);
     STAILQ_INIT(&ble_hci_uart_state.tx_pkts);

[AxelLin]

When CONFIG_BT_CONTROLLER_DISABLED=y, it does not need vhci hack, right?

True.

Hi @rahult-github Can you fix this? I use below fix, but maybe you have better solution.

diff --git a/nimble/host/src/ble_hs_hci_cmd.c b/nimble/host/src/ble_hs_hci_cmd.c
index fec1f762..7ce491fc 100644
--- a/nimble/host/src/ble_hs_hci_cmd.c
+++ b/nimble/host/src/ble_hs_hci_cmd.c
@@ -72,7 +72,7 @@ ble_hs_hci_cmd_send(uint16_t opcode, uint8_t len, const void *cmddata)
     buf = ble_hci_trans_buf_alloc(BLE_HCI_TRANS_BUF_CMD);
     BLE_HS_DBG_ASSERT(buf != NULL);

-#if !(SOC_ESP_NIMBLE_CONTROLLER)
+#if !(SOC_ESP_NIMBLE_CONTROLLER) && !CONFIG_BT_CONTROLLER_DISABLED
     /* Hack for avoiding memcpy while handling tx pkt to VHCI,
      * keep one byte for type field*/
     buf++;
@@ -89,7 +89,7 @@ ble_hs_hci_cmd_send(uint16_t opcode, uint8_t len, const void *cmddata)
     BLE_HS_LOG(DEBUG, "\n");
 #endif

-#if !(SOC_ESP_NIMBLE_CONTROLLER)
+#if !(SOC_ESP_NIMBLE_CONTROLLER) && !CONFIG_BT_CONTROLLER_DISABLED
     buf--;
 #endif

[rahult-github]

Hi @AxelLin ,

Actually, ESP- IDF is designed to support only ESP chipsets out of box. My concern is that if CONFIG_BT_CONTROLLER_DISABLED is set to y , there may be more changes needed too ( disable VHCI support , callbacks and API registration between controller / host for data Rx/ Tx etc. ) I believe, this supporting external controllers with host from esp-idf may need more testing if needs to be supported. Also its use case need to be studied before it gets absorbed as a part of IDF .

May i request you to please explain in detail :

1. What is the hardware setup being tried ?
2. What is software changes being done ?
3. What is the intended end usage ?
4. Any other information you deem important that helps us to understand and make concise decision.
5. If we want to test the scenario, what modifications in existing IDF examples need to be done to test .

This may be taken up as a feature request, based on analysis of the above inputs .

Thanks, Rahul

[AxelLin]

Hi @AxelLin ,

Actually, ESP- IDF is designed to support only ESP chipsets out of box. My concern is that if CONFIG_BT_CONTROLLER_DISABLED is set to y , there may be more changes needed too ( disable VHCI support , callbacks and API registration between controller / host for data Rx/ Tx etc. ) I believe, this supporting external controllers with

You need to fix it anyway because the user can set CONFIG_BT_CONTROLLER_DISABLED=y. If there are unexpected side-effects, let's fix it as early as possible.

host from esp-idf may need more testing if needs to be supported. Also its use case need to be studied before it gets absorbed as a part of IDF .

May i request you to please explain in detail :

1. What is the hardware setup being tried ?
2. What is software changes being done ?
3. What is the intended end usage ?
4. Any other information you deem important that helps us to understand and make concise decision.
5. If we want to test the scenario, what modifications in existing IDF examples need to be done to test .

This may be taken up as a feature request, based on analysis of the above inputs .

There is only one antenna on ESP32 which means Wi-Fi and Bluetooth can not receive at the same time. I know the coexistence firmware does something to make BT an wifi work at the same time (with some drawbacks). Using external BT with dedicated antenna has better performance. The esp-idf can run as Bluetooth Host only (by setting BT_CONTROLLER_DISABLED), and the nimble supports UART HCI (H4). So it's actually not complex to use external BT. It would be great if esp-idf can support such use case.

[AxelLin]

@rahult-github

The fix in https://github.com/espressif/esp-nimble/issues/45#issuecomment-1223475704 is trivial. You agree that it does not need vhci hack when CONFIG_BT_CONTROLLER_DISABLED=y. The vhci hack logic is either correct or incorrect when CONFIG_BT_CONTROLLER_DISABLED=y. So you think it's incorrect when CONFIG_BT_CONTROLLER_DISABLED=y but you don't want to fix it because it does not impact your use case.

Supporting external BT controller is different things. I do have other fixes related to use external BT controller. However, IMHO above fix is a simple logic fix and should be fixed even without external BT support. I do hope this get fix.

[rahult-github]

Hi @AxelLin ,

Apologies for delay. The change you mentioned above is already created internally and queued up for commit purpose on master / release branches. However, since patches go through internal CI/CD checks and are sequentially added , hence expect some delay , as there are some more critical changes already in pipeline to be merged.

Thanks, Rahul