usb: device_next: add BT HCI USB transport layer #53870

jfischer-no · 2023-01-17T17:11:33Z

Add Bluetooth HCI USB transport layer implementation for the new USB device support,
porting of subsys/usb/device/class/bluetooth.c without H4 bloat.

jhedberg

Looks pretty good

jhedberg · 2023-01-17T17:37:09Z

subsys/usb/device_next/class/bt_hci.c

+/*
+ * Bluetooth subsystem does not support multiple HCI instances,
+ * but we are almost ready for it.
+ */
+BT_HCI_DESCRIPTOR_DEFINE(0)
+BT_HCI_CLASS_DATA_DEFINE(0)


I think this relates to #51503 too. Along with making the HCI driver DTS-based, it becomes more natural to declare multiple instances. That said, the host stack treats the controller as singleton. There's a single bt_dev struct that's accessed as a global variable in several places of the host. To make the host truly multi-HCI aware there would need to be this bt_dev context per controller instance that's then passed around as a parameter in most existing Bluetooth APIs.

@jhedberg @jfischer-no does anything need to be done here?

As I understood @carlescufi, not in the near future.

Thalley

Mostly code style comments, and some questions

samples/bluetooth/hci_usb/src/main.c

subsys/usb/device_next/class/bt_hci.c

Thalley · 2023-01-17T18:20:52Z

subsys/usb/device_next/class/bt_hci.c

+ * RAM usage efficient.
+ */
+NET_BUF_POOL_FIXED_DEFINE(bt_hci_ep_pool,
+			  3, 512,


Why these specific numbers?

Three endpoints, maximum three transfers used synchronously, 512 is the maximum packet size of a high speed bulk endpoint. I will add a comment.

If 512 is a maximum defined by the USB specification, then I'd suggest to #define that value and use that

subsys/usb/device_next/class/bt_hci.c

desowin · 2023-01-18T06:33:21Z

subsys/usb/device_next/class/bt_hci.c

+#define USB_BT_HCI_PROTOCOL		0x01
+
+#define BT_HCI_DEFAULT_BULK_EP_MPS	0	/* Get maximum supported */
+#define BT_HCI_DEFAULT_INT_EP_MPS	64


BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part B page 1667 suggests 16 bytes Max Packet Size as the default.

subsys/usb/device_next/class/bt_hci.c

Thalley · 2023-01-24T13:58:32Z

subsys/usb/device_next/class/bt_hci.c

+	}
+
+	bi = udc_get_buf_info(buf);
+	memset(bi, 0, sizeof(struct udc_buf_info));


When the intention is to clear a struct like here, it may be more readable to use the pointer to the struct for the sizeof

Suggested change

memset(bi, 0, sizeof(struct udc_buf_info));

memset(bi, 0, sizeof(*bi));

No it is not more readable.

subsys/usb/device_next/class/bt_hci.c

Thalley · 2023-01-24T14:00:18Z

subsys/usb/device_next/class/bt_hci.c

+	case BT_BUF_ISO_OUT: {
+		struct bt_hci_iso_hdr *iso_hdr;
+
+		hdr_len = sizeof(*iso_hdr);
+		iso_hdr = (struct bt_hci_iso_hdr *)data;
+		len = bt_iso_hdr_len(sys_le16_to_cpu(iso_hdr->len)) + hdr_len;
+		break;
+	}


Given that this driver otherwise doesn't support ISO, should this be removed?

Maybe, but then it must later be added for H4 support.

Seems odd to only partially add ISO/H4 support

subsys/usb/device_next/class/bt_hci.c

Zero data payload size for isochronous endpoints is a is a valid setting for default interface. Also do not update MPS of control endpoint since it is set by the driver. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

jfischer-no · 2023-01-24T21:46:35Z

Cherry-picked first commit from #53998 and added interface descriptor for ISO endpoints (still unused).

subsys/usb/device_next/class/bt_hci.c

Add Bluetooth HCI USB transport layer implementation for the new experimental USB device support based on the existing implementation subsys/usb/device/class/bluetooth.c. This implementation, unlike existing one, contains the interface descriptors for isochronous endpoints. Since we do not support voice channels, these are just there to avoid issues with Linux kernel btusb driver when this implementation is part of a composite configuration. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

Enable Bluetooth HCI USB tranport layer from new experimental USB device support. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

Thalley

Some minor suggestions left, otherwise LGTM

Thalley · 2023-01-25T10:53:52Z

subsys/usb/device_next/class/bt_hci.c

+
+static uint8_t bt_hci_get_int_in(struct usbd_class_node *const c_nd)
+{
+	struct usbd_bt_hci_desc *desc = c_nd->data->desc;


Minor:

Suggested change

struct usbd_bt_hci_desc *desc = c_nd->data->desc;

const struct usbd_bt_hci_desc *desc = c_nd->data->desc;

Thalley · 2023-01-25T10:54:03Z

subsys/usb/device_next/class/bt_hci.c

+
+static uint8_t bt_hci_get_bulk_in(struct usbd_class_node *const c_nd)
+{
+	struct usbd_bt_hci_desc *desc = c_nd->data->desc;


Minor:

Suggested change

struct usbd_bt_hci_desc *desc = c_nd->data->desc;

const struct usbd_bt_hci_desc *desc = c_nd->data->desc;

Thalley · 2023-01-25T10:54:12Z

subsys/usb/device_next/class/bt_hci.c

+
+static uint8_t bt_hci_get_bulk_out(struct usbd_class_node *const c_nd)
+{
+	struct usbd_bt_hci_desc *desc = c_nd->data->desc;


Minor:

Suggested change

struct usbd_bt_hci_desc *desc = c_nd->data->desc;

const struct usbd_bt_hci_desc *desc = c_nd->data->desc;

subsys/usb/device_next/class/bt_hci.c

Thalley · 2023-01-25T10:56:11Z

subsys/usb/device_next/class/bt_hci.c

+/*
+ * Bluetooth subsystem does not support multiple HCI instances,
+ * but we are almost ready for it.
+ */
+BT_HCI_DESCRIPTOR_DEFINE(0)
+BT_HCI_CLASS_DATA_DEFINE(0)


@jhedberg @jfischer-no does anything need to be done here?

jfischer-no added area: USB Universal Serial Bus Experimental Experimental features not enabled by default labels Jan 17, 2023

jfischer-no requested review from desowin, cvinayak and carlescufi January 17, 2023 17:11

jfischer-no requested review from jhedberg, Vudentz, alwa-nordic and sjanc as code owners January 17, 2023 17:11

zephyrbot added the area: Bluetooth label Jan 17, 2023

zephyrbot requested review from asbjornsabo, hermabe, jori-nordic and Thalley January 17, 2023 17:12

zephyrbot assigned jfischer-no Jan 17, 2023

jfischer-no mentioned this pull request Jan 17, 2023

Planned USB device/host support rework/enhancements #42066

Open

52 tasks

jhedberg previously approved these changes Jan 17, 2023

View reviewed changes

Thalley requested changes Jan 17, 2023

View reviewed changes

desowin requested changes Jan 18, 2023

View reviewed changes

desowin reviewed Jan 19, 2023

View reviewed changes

subsys/usb/device_next/class/bt_hci.c Show resolved Hide resolved

jfischer-no dismissed jhedberg’s stale review via 1839950 January 24, 2023 13:21

jfischer-no force-pushed the pr-device-next-bt branch from 45b80bc to 1839950 Compare January 24, 2023 13:21

Thalley requested changes Jan 24, 2023

View reviewed changes

jfischer-no force-pushed the pr-device-next-bt branch from 1839950 to 731bac6 Compare January 24, 2023 21:41

jfischer-no force-pushed the pr-device-next-bt branch from 731bac6 to 2bd4e0e Compare January 24, 2023 23:14

desowin reviewed Jan 25, 2023

View reviewed changes

subsys/usb/device_next/class/bt_hci.c Show resolved Hide resolved

desowin previously approved these changes Jan 25, 2023

View reviewed changes

samples: hci_usb: enable new experimental USB device support

cf07ae8

Enable Bluetooth HCI USB tranport layer from new experimental USB device support. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

jfischer-no dismissed desowin’s stale review via cf07ae8 January 25, 2023 10:54

jfischer-no force-pushed the pr-device-next-bt branch from 2bd4e0e to cf07ae8 Compare January 25, 2023 10:54

Thalley approved these changes Jan 25, 2023

View reviewed changes

jfischer-no added this to the v3.3.0 milestone Jan 25, 2023

tmon-nordic approved these changes Jan 26, 2023

View reviewed changes

desowin approved these changes Jan 26, 2023

View reviewed changes

stephanosio merged commit cad8ae7 into zephyrproject-rtos:main Jan 26, 2023

jfischer-no deleted the pr-device-next-bt branch January 26, 2023 17:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usb: device_next: add BT HCI USB transport layer #53870

usb: device_next: add BT HCI USB transport layer #53870

jfischer-no commented Jan 17, 2023

jhedberg left a comment

jhedberg Jan 17, 2023

Thalley Jan 25, 2023

jfischer-no Jan 25, 2023

Thalley left a comment

Thalley Jan 17, 2023

jfischer-no Jan 19, 2023

Thalley Jan 24, 2023

desowin Jan 18, 2023

Thalley Jan 24, 2023

jfischer-no Jan 24, 2023

Thalley Jan 24, 2023

jfischer-no Jan 24, 2023

Thalley Jan 24, 2023

jfischer-no commented Jan 24, 2023

Thalley left a comment

Thalley Jan 25, 2023

Thalley Jan 25, 2023

Thalley Jan 25, 2023

Thalley Jan 25, 2023

	memset(bi, 0, sizeof(struct udc_buf_info));
	memset(bi, 0, sizeof(*bi));

	struct usbd_bt_hci_desc *desc = c_nd->data->desc;
	const struct usbd_bt_hci_desc *desc = c_nd->data->desc;

usb: device_next: add BT HCI USB transport layer #53870

usb: device_next: add BT HCI USB transport layer #53870

Conversation

jfischer-no commented Jan 17, 2023

jhedberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfischer-no commented Jan 24, 2023

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment