usb: device_next: Document if callbacks are mandatory #84584

Thalley · 2025-01-27T09:08:27Z

Adds additional documentation to the callbacks of uac2_ops to describe if/when they are mandatory to register.

Additionally add asserts before calling them to help debugging if the user did not register the required ones.

tmon-nordic

I would prefer to have the checks as early as possible, that is in usbd_uac2_set_ops().

tmon-nordic · 2025-01-28T14:27:53Z

subsys/usb/device_next/class/usbd_uac2.c

@@ -262,6 +264,7 @@ int usbd_uac2_send(const struct device *dev, uint8_t terminal,
 		LOG_ERR("No endpoint for terminal %d", terminal);
 		return -ENOENT;
 	}
+	__ASSERT_NO_MSG(ctx->ops != NULL && ctx->ops->buf_release_cb != NULL);


Checking for ctx->ops doesn't really make sense because uac2_init() will fail if ctx->ops is NULL which fails udc_init() which prevents the host from ever seeing the device.

This assumes that usbd_uac2_set_ops has always been called before usbd_enable. This isn't documented, nor does usbd_enable check for ctx->ops == NULL.

Should it be added as a requirement that usbd_uac2_set_ops is always called before usbd_enable, and usbd_enable verifies this? If so, then I agree that the above can be removed, but if not then ctx->ops can definitely be NULL.

Hmm, since the callbacks are specific to usbd_uac2, then such a check cannot happen in usbd_enable.

Do you have any suggestion on how to approach this? Just checking in usbd_uac2_set_ops isn't enough if we cannot ensure that usbd_uac2_set_ops has been called first

Why usbd_enable would have to check for ctx->ops == NULL if the udc_init() will fail the whole stack if the ops are not registered? If the whole stack fails on UDC, then the class won't ever be enabled because for that you need working control transfers (which do require working UDC).

Why usbd_enable would have to check for ctx->ops == NULL if the udc_init() will fail the whole stack if the ops are not registered?

udc_init does not check if dev->ctx->ops is set. As far as I can tell, ctx->ops is only a thing in usbd_uac2.c, since the ctx struct, struct uac2_ctx is only defined in that file.

udc_init does not check it directly, but usbd_init() does call uac2_init() in the process which does check ctx->ops. Returning -EINVAL in uac2_init() prevents the device from even presenting itself on the bus because usbd_device_init_core() will return an error.

tmon-nordic · 2025-01-28T14:28:30Z

subsys/usb/device_next/class/usbd_uac2.c

@@ -436,6 +441,7 @@ void uac2_update(struct usbd_class_data *const c_data,
 	as_idx = iface - iad->bFirstInterface - 1;

 	/* Notify application about terminal state change */
+	__ASSERT_NO_MSG(ctx->ops != NULL && ctx->ops->terminal_update_cb != NULL);


Seems redundant. Check in usbd_uac2_set_ops() should be enough.

tmon-nordic · 2025-01-28T14:28:57Z

subsys/usb/device_next/class/usbd_uac2.c

@@ -800,6 +808,7 @@ static void uac2_sof(struct usbd_class_data *const c_data)
 	struct uac2_ctx *ctx = dev->data;
 	int as_idx;

+	__ASSERT_NO_MSG(ctx->ops != NULL && ctx->ops->sof_cb != NULL);


Seems redundant. Check in usbd_uac2_set_ops() should be enough.

tmon-nordic · 2025-01-28T14:44:11Z

subsys/usb/device_next/class/usbd_uac2.c

@@ -379,6 +383,7 @@ static void write_explicit_feedback(struct usbd_class_data *const c_data,
 	bi = udc_get_buf_info(buf);
 	bi->ep = ep;

+	__ASSERT_NO_MSG(ctx->ops != NULL && ctx->ops->feedback_cb != NULL);


I would prefer to have the check in usbd_uac2_set_ops. The feedback_cb is necessary whether uac2_cfg (const struct uac2_cfg *cfg = dev->config;) array fb_indexes (range 0 to num_ifaces) has any non-zero value.

Isn't feedback_cb optional if using implicit-feedback? It's in a function called write_explicit_feedback, which I assume is not called when using implicit feedback

If the sample uses implicit feedback, then there won't be any non-zero value in fb_indexes array.

tmon-nordic · 2025-01-28T14:55:56Z

subsys/usb/device_next/class/usbd_uac2.c

@@ -328,6 +331,7 @@ static void schedule_iso_out_read(struct usbd_class_data *const c_data,
 	}

 	/* Prepare transfer to read audio OUT data from host */
+	__ASSERT_NO_MSG(ctx->ops != NULL && ctx->ops->get_recv_buf != NULL);


I would prefer to have the check in usbd_uac2_set_ops.

It is possible to determine whether there are any ISO IN endpoints (and therefore get_recv_buf is necessary) by looking at all non-zero ep_indexes (range 0 to num_ifaces) entries in uac2_cfg (const struct uac2_cfg *cfg = dev->config;). For each non-zero ep_indexes value ('idx') check whether the corresponding struct usb_ep_descriptor bEndpointAddress is IN with USB_EP_DIR_IS_IN().

Check get_as_data_ep() for reference. The check wouldn't have to worry about as_idx, just check all ep_indexes from 0 to num_ifaces. If there are both fs_descriptors and hs_descriptors then it is sufficient to check only one of the descriptor sets.

The check can also be used for data_recv_cb - if the USB_EP_DIR_IS_IN() was false, then it means it was OUT and therefore there is at least one ISO OUT endpoint and thus data_recv_cb is mandatory.

Hmm, I see that data_recv_cb is called only if if (USB_EP_DIR_IS_OUT(ep)) { - Did you mean USB_EP_DIR_IS_OUT in your comment? As far as I can tell data_recv_cb and get_recv_buf are used together, so shouldn't they check the same endpoint type?

Double negation in "if the USB_EP_DIR_IS_IN() was false" essentially means that USB_EP_DIR_IS_OUT() is true.

Thalley · 2025-02-03T09:38:50Z

@tmon-nordic @jfischer-no I've updated the PR to check only in usbd_uac2_set_ops. I've followed the existing design of using __ASSERT, but would it make sense to change usbd_uac2_set_ops to be able to return an error value instead?

tmon-nordic · 2025-02-03T09:43:41Z

would it make sense to change usbd_uac2_set_ops to be able to return an error value instead?

I don't think so. What would the application do with the error code? Not providing appropriate callabacks is a programmer error and asserts are the right tool to catch programmer errors.

Thalley · 2025-02-03T09:46:35Z

Not providing appropriate callabacks is a programmer error and asserts are the right tool to catch programmer errors.

I think that's debatable :D But will leave that design up to the USB subsystem people (i.e. not me) :D

tmon-nordic · 2025-02-03T09:56:05Z

subsys/usb/device_next/class/usbd_uac2.c

+		}
+
+		if (cfg->ep_indexes[i] != 0U) {
+			const struct usb_ep_descriptor *desc = get_as_data_ep(cfg->c_data, i);


This code must not call get_as_data_ep() because it does depend on actual bus speed (which is not available before the stack is initialized).

if (cfg->ep_indexes[i] != 0U) { if (cfg->fs_descriptors && cfg->fs_descriptors[cfg->ep_indexes[i]]) { desc = (const struct usb_ep_descriptor *)cfg->fs_descriptors[cfg->ep_indexes[i]]; } else { desc = (const struct usb_ep_descriptor *)cfg->hs_descriptors[cfg->ep_indexes[i]]; } }

UAC2 currently supports only Full-Speed and/or High-Speed and hence the above snippet will set desc to valid pointer if cfg->ep_indexes[i] is not zero.

Turns out, I slipped here. The && cfg->fs_descriptors[cfg->ep_indexes[i]] check is unnecessary. If cfg->ep_indexes[i] is non-zero and if fs_descriptors is not NULL, then fs_descriptors[cfg->ep_indexes[i]] is non-NULL.

jfischer-no · 2025-02-03T12:57:23Z

subsys/usb/device_next/class/usbd_uac2.c

+				if (USB_EP_DIR_IS_OUT(desc->bEndpointAddress)) {
+					__ASSERT(ops->get_recv_buf, "get_recv_buf is mandatory");
+					__ASSERT(ops->data_recv_cb, "data_recv_cb is mandatory");
+				} else if (USB_EP_DIR_IS_IN(desc->bEndpointAddress)) {
+					__ASSERT(ops->buf_release_cb,
+						 "buf_release_cb is mandatory");
+				}


Please change it to either

if (USB_EP_DIR_IS_OUT(desc->bEndpointAddress)) { __ASSERT(ops->get_recv_buf, "get_recv_buf is mandatory"); __ASSERT(ops->data_recv_cb, "data_recv_cb is mandatory"); } if (USB_EP_DIR_IS_IN(desc->bEndpointAddress)) { __ASSERT(ops->buf_release_cb, "buf_release_cb is mandatory"); }

or

if (USB_EP_DIR_IS_OUT(desc->bEndpointAddress)) { __ASSERT(ops->get_recv_buf, "get_recv_buf is mandatory"); __ASSERT(ops->data_recv_cb, "data_recv_cb is mandatory"); } else { __ASSERT(ops->buf_release_cb, "buf_release_cb is mandatory"); }

or

if (desc != NULL && USB_EP_DIR_IS_OUT(desc->bEndpointAddress)) { __ASSERT(ops->get_recv_buf, "get_recv_buf is mandatory"); __ASSERT(ops->data_recv_cb, "data_recv_cb is mandatory"); } if (desc != NULL && USB_EP_DIR_IS_IN(desc->bEndpointAddress)) { __ASSERT(ops->buf_release_cb, "buf_release_cb is mandatory"); }

I am confident that the compiler will produce equivalent results.

Applied the first suggestion

Adds additional documentation to the callbacks of uac2_ops to describe if/when they are mandatory to register. Additionally add asserts before calling them to help debugging if the user did not register the required ones. Signed-off-by: Emil Gydesen <[email protected]>

Thalley marked this pull request as ready for review January 28, 2025 07:19

zephyrbot added the area: USB Universal Serial Bus label Jan 28, 2025

zephyrbot requested review from jfischer-no and tmon-nordic January 28, 2025 07:20

zephyrbot assigned jfischer-no Jan 28, 2025

tmon-nordic requested changes Jan 28, 2025

View reviewed changes

Thalley requested a review from tmon-nordic January 29, 2025 07:11

Thalley force-pushed the usb_cbs_updates branch from f54d25b to 034125c Compare February 3, 2025 09:37

tmon-nordic reviewed Feb 3, 2025

View reviewed changes

Thalley force-pushed the usb_cbs_updates branch from 034125c to fd62329 Compare February 3, 2025 12:48

Thalley requested a review from tmon-nordic February 3, 2025 12:48

jfischer-no requested changes Feb 3, 2025

View reviewed changes

Thalley force-pushed the usb_cbs_updates branch from fd62329 to 4fbc348 Compare February 3, 2025 13:11

Thalley requested a review from jfischer-no February 3, 2025 13:11

Thalley force-pushed the usb_cbs_updates branch from 4fbc348 to 0ea5660 Compare February 3, 2025 13:16

Thalley force-pushed the usb_cbs_updates branch from 0ea5660 to 57b1fa9 Compare February 3, 2025 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

usb: device_next: Document if callbacks are mandatory #84584

usb: device_next: Document if callbacks are mandatory #84584

Thalley commented Jan 27, 2025

tmon-nordic left a comment

tmon-nordic Jan 28, 2025

Thalley Jan 29, 2025

Thalley Jan 29, 2025

tmon-nordic Jan 29, 2025

Thalley Jan 29, 2025

tmon-nordic Jan 31, 2025 •

edited

Loading

tmon-nordic Jan 28, 2025

tmon-nordic Jan 28, 2025

tmon-nordic Jan 28, 2025

Thalley Jan 29, 2025

tmon-nordic Jan 29, 2025

tmon-nordic Jan 28, 2025

Thalley Feb 3, 2025

tmon-nordic Feb 3, 2025

Thalley commented Feb 3, 2025

tmon-nordic commented Feb 3, 2025

Thalley commented Feb 3, 2025

tmon-nordic Feb 3, 2025

tmon-nordic Feb 3, 2025

jfischer-no Feb 3, 2025

Thalley Feb 3, 2025

usb: device_next: Document if callbacks are mandatory #84584

Are you sure you want to change the base?

usb: device_next: Document if callbacks are mandatory #84584

Conversation

Thalley commented Jan 27, 2025

tmon-nordic 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

tmon-nordic Jan 31, 2025 • edited Loading

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

Thalley commented Feb 3, 2025

tmon-nordic commented Feb 3, 2025

Thalley commented Feb 3, 2025

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

tmon-nordic Jan 31, 2025 •

edited

Loading