You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2021/05/21 15:26:51 UTC
[GitHub] [incubator-nuttx] v01d commented on a change in pull request #3754: wireless/bluetooth: decoupling bt_driver_s and bt_buf_s
v01d commented on a change in pull request #3754:
URL: https://github.com/apache/incubator-nuttx/pull/3754#discussion_r636995026
##########
File path: wireless/bluetooth/bt_hcicore.c
##########
@@ -1622,24 +1616,35 @@ void bt_driver_unregister(FAR const struct bt_driver_s *btdev)
/* TODO: rename to bt_receive? */
-void bt_hci_receive(FAR struct bt_buf_s *buf)
+int bt_hci_receive(FAR struct bt_driver_s *btdev,
+ enum bt_buf_type_e type,
+ FAR void *data, size_t len)
{
FAR struct bt_hci_evt_hdr_s *hdr;
+ struct bt_buf_s *buf;
int ret;
- wlinfo("buf %p len %u\n", buf, buf->len);
+ wlinfo("data %p len %zu\n", data, len);
/* Critical command complete/status events use the high priority work
* queue.
*/
- if (buf->type != BT_ACL_IN)
+ buf = bt_buf_alloc(type, NULL, 0);
Review comment:
I see a potential issue here. The buffer is typically allocated with required H4 header overhead (see code being removed in https://github.com/apache/incubator-nuttx/pull/3754/files#diff-5e9ec1dda4ad80ba37d99bf264be24a9eb50516a76bb2b0f029e59e637f2e0f4L305
This overhead is considered when consuming the bt_buf_s in netdev. Is this being accounted elsewhere?
##########
File path: drivers/serial/Kconfig
##########
@@ -617,4 +617,27 @@ config PSEUDOTERM_TXBUFSIZE
endif # PSEUDOTERM
+menuconfig UART_BTH4
+ bool "BT H4 uart pseudo device"
+ default n
+ select MM_CIRCBUF
+ ---help---
+ Enable Bluetooth H4 UART Pseudo Device(eg. /dev/ttyHCI)
Review comment:
This description is a bit obscure. Until we get proper documentation for BLE we should make it clear what this really does:
```suggestion
Enable support for Bluetooth H4 UART Pseudo Device(eg. /dev/ttyHCI). This instantiates a serial-like
interface over an existing bluetooth controller via HCI interface. Useful for external Bluetooth
stacks working this way instead of the socket based interface.
```
##########
File path: boards/sim/sim/sim/configs/btuart/defconfig
##########
@@ -1,73 +0,0 @@
-#
Review comment:
Please provide a config which excercises the new interface instead of just removing the previous one.
##########
File path: drivers/serial/uart_bth4.c
##########
@@ -25,25 +25,21 @@
#include <nuttx/fs/fs.h>
#include <nuttx/kmalloc.h>
#include <nuttx/semaphore.h>
-#include <nuttx/nuttx.h>
+#include <nuttx/mm/circbuf.h>
+#include <fcntl.h>
#include <string.h>
#include <poll.h>
-#include <queue.h>
-#include <nuttx/wireless/bluetooth/bt_uart.h>
#include <nuttx/wireless/bluetooth/bt_hci.h>
-
-#include "up_internal.h"
-#include "up_hcisocket_host.h"
+#include <nuttx/wireless/bluetooth/bt_uart.h>
+#include <nuttx/wireless/bluetooth/bt_driver.h>
/****************************************************************************
* Pre-processor Definitions
****************************************************************************/
-#define CONFIG_HCI_RECVBUF_SIZE 1024
-#define CONFIG_HCI_SENDBUF_SIZE 1024
-#define CONFIG_HCI_NPOLLWAITERS 2
+#define CONFIG_UART_BTH4_NPOLLWAITERS 2
Review comment:
Can you make this a Kconfig symbol?
##########
File path: arch/arm/src/nrf52/nrf52_sdc.c
##########
@@ -321,21 +279,14 @@ static void on_hci(void)
struct bt_hci_acl_hdr_s *hdr =
(struct bt_hci_acl_hdr_s *)g_sdc_dev.msg_buffer;
- wlinfo("received HCI ACL from softdevice (handle: %d)\n",
- hdr->handle);
-
Review comment:
why remove this? it is useful for debugging
##########
File path: arch/arm/src/nrf52/nrf52_sdc.c
##########
@@ -147,61 +147,44 @@ static int bt_open(FAR const struct bt_driver_s *btdev)
* Name: bt_open
****************************************************************************/
-static int bt_hci_send(FAR const struct bt_driver_s *btdev,
- FAR struct bt_buf_s *buf)
+static int bt_hci_send(FAR struct bt_driver_s *btdev,
+ enum bt_buf_type_e type,
+ FAR void *data, size_t len)
{
- int ret = OK;
+ int ret = -EIO;
/* Pass HCI CMD/DATA to SDC */
- if (buf->type == BT_CMD)
+ if (type == BT_CMD || type == BT_ACL_OUT)
{
- struct bt_hci_cmd_hdr_s *cmd = (struct bt_hci_cmd_hdr_s *)buf->data;
-
- wlinfo("passing CMD %d to softdevice\n", cmd->opcode);
-
/* Ensure non-concurrent access to SDC operations */
nxsem_wait_uninterruptible(&g_sdc_dev.exclsem);
- if (sdc_hci_cmd_put(buf->data) < 0)
+ if (type == BT_CMD)
{
- wlerr("sdc_hci_cmd_put() failed\n");
- ret = -EIO;
- }
-
- nxsem_post(&g_sdc_dev.exclsem);
+ wlinfo("passing CMD to softdevice\n");
Review comment:
this should not be done while semaphore is held, the print should be outside.
##########
File path: arch/arm/src/nrf52/nrf52_sdc.c
##########
@@ -283,32 +265,8 @@ static void on_hci(void)
len = sizeof(*hdr) + hdr->len;
-#ifdef CONFIG_DEBUG_WIRELESS_INFO
- if (hdr->evt == BT_HCI_EVT_CMD_COMPLETE)
- {
- struct hci_evt_cmd_complete_s *cmd_complete =
- (struct hci_evt_cmd_complete_s *)
- (g_sdc_dev.msg_buffer + sizeof(*hdr));
- uint8_t *status = (uint8_t *)cmd_complete + 1;
-
- wlinfo("received CMD_COMPLETE from softdevice "
- "(opcode: %d, status: 0x%x)\n",
- cmd_complete->opcode, *status);
- }
- else
- {
- wlinfo("received HCI EVT from softdevice "
- "(evt: %d, len: %zu)\n", hdr->evt, len);
- }
-#endif
Review comment:
Please do not remove debugging helpers
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org