usb: device_next: implementation of USB device support notification #68256

jfischer-no · 2024-01-29T23:27:36Z

usb: device_next: implementation of USB device support notification

The implementation uses the system workqueue and a callback provided
and registered by the application. The application callback is called in
the context of the workqueue. Notification messages are stored in a
queue and delivered to the callback in sequence. The callback may return
error code -EBUSY or -EAGAIN, in which case the implementation retries
to deliver the message again.

We cannot call application callback directly from the USB device stack
thread because the behavior of arbitrary code provided by the
application is unpredictable, and we do not want it to be executed in
the same context where all events from the device controller are
handled.

Nor can we use the ZBUS subsystem directly. ZBUS offloads responsibility
for defined behavior to the observers and application, and does not
provide any way for the publisher to enforce defined behavior and
execution context.
ZBUS listener would be called from the USB thread context and is not
acceptable. ZBUS subscriber does not provide delivery guarantee and
cached message can be overwritten. ZBUS message subscriber has
cumbersome global configuration and buffers that are too complicated to
handle from USB configuration, and even if we would use it, defined
behavior is not possible because of how listener and subscriber are
handled.

Resolves: #51034
Resolves: #50759

tmon-nordic · 2024-01-30T07:28:27Z

include/zephyr/usb/usbd_msg.h

+	/** Reserved */
+	USBD_MSG_RESERVED0,
+	/** Reserved */
+	USBD_MSG_RESERVED1,


What's the point of having reserved values here? Isn't just keeping API compatibility (i.e. not renaming the entries, but reordering is fine) enough here?

tmon-nordic · 2024-01-30T07:36:37Z

subsys/usb/device_next/usbd_msg.c

+	if (err == -EBUSY || err == -EAGAIN) {
+		LOG_DBG("Message callback is busy (%d)", err);
+		(void)k_work_submit(work);
+		return;
+	}


What's the point? This is likely to result in endless loops taking up 100% CPU time. System workqueue priority defaults to lowest cooperative priority so it is likely that whatever kept the callback busy won't have finished when system workqueue starts processing this work item again. If coop is not enabled, then it defaults to priority 0 which would be the highest priority in the system.

Fixed it with delayed work, but finaly removed, I have backup for the case there will be a need for this function.

tmon-nordic · 2024-01-30T07:40:31Z

include/zephyr/usb/usbd_msg.h

+/**
+ * @brief USB device support message types
+ *
+ * The first set of message types map to event types from the UDC driver API,


Silently casting the enum types and hoping that numerical value will match is bad design in my opinion. Things will break (because one enum will be updated without updating the other) during development and it'll be hard to track.

Right, and it does not make the code smaller.

You can update the comment and remove "reserved types are not used"

tmon-nordic · 2024-01-30T07:42:14Z

subsys/usb/device_next/usbd_core.c

 		break;
 	case UDC_EVT_VBUS_READY:
-		LOG_WRN("VBUS detected event");
+		LOG_DBG("VBUS detected event");
+		usbd_msg_pub_simple(uds_ctx, event->type, 0);


Don't silently cast one enum to another! Use proper enum value here (USBD_MSG_VBUS_READY).

#68256 (comment)

tmon-nordic · 2024-01-30T07:44:26Z

subsys/usb/device_next/usbd_msg.c

+	int err;
+
+	if (ctx->msg_cb == NULL) {
+		return -ENOTSUP;


Propably a personal preference, but I wouldn't consider this to be an error condition. I would just return 0; here.

reworked, error cannot be forwarded anyway.

tmon-nordic · 2024-01-30T08:41:57Z

@rodrigopex I would appreciate your review and/or opinion on this.

rodrigopex · 2024-01-30T11:49:07Z

@tmon-nordic, that is a clear reimplementation of part of what Zbus is made for. Regarding to:

ZBUS message subscriber has
cumbersome global configuration and buffers that are too complicated to
handle from USB configuration, and even if we would use it, defined
behavior is not possible because of how listener and subscriber are
handled.

@jfischer-no, I am intrigued about the cumbersome configuration you mentioned. There are only five configurations:

CONFIG_ZBUS_MSG_SUBSCRIBER to enable the feature;
CONFIG_ZBUS_MSG_SUBSCRIBER_BUF_ALLOC_DYNAMIC and CONFIG_ZBUS_MSG_SUBSCRIBER_BUF_ALLOC_STATIC telling zbus the allocation of the messages will be static or dynamic;
CONFIG_ZBUS_MSG_SUBSCRIBER_NET_BUF_STATIC_DATA_SIZE as the message is static, we need to tell zbus what that size is;
At last, the CONFIG_ZBUS_MSG_SUBSCRIBER_NET_BUF_POOL_SIZE will set the size of the net_buf_pool as you can find in any other network or Bluetooth code.

If you are using dynamic allocation for those messages, as you are doing in your code, you will only use: CONFIG_ZBUS_MSG_SUBSCRIBER=y and CONFIG_ZBUS_MSG_SUBSCRIBER_NET_BUF_POOL_SIZE=X. Set the proper pool size, and that is it.

Maybe you see limitations I am not able to see. Please let me know if you have an idea of how to make zbus useful for your case. You may have a special channel that only enables message subscribers to observe it. Or create a way of hiding some system channels. Or a way of separating message subscriber pools: one for the user and the other for the system. It would solve your problems.

If everyone who doesn't find a feature in zbus implements its own event manager, we will not converge in a great bus. I am here to help improve that by listening to the community's needs.

jfischer-no · 2024-01-30T23:51:00Z

@tmon-nordic, that is a clear reimplementation of part of what Zbus is made for. Regarding to:

It is not a re-implementation. Zbus (which is not a bus) does not provide this functionality and defined behavior for the publisher context (explained in the commit message).

ZBUS message subscriber has
cumbersome global configuration and buffers that are too complicated to
handle from USB configuration, and even if we would use it, defined
behavior is not possible because of how listener and subscriber are
handled.

@jfischer-no, I am intrigued about the cumbersome configuration you mentioned. There are only five configurations:
* `CONFIG_ZBUS_MSG_SUBSCRIBER` to enable the feature;

* `CONFIG_ZBUS_MSG_SUBSCRIBER_BUF_ALLOC_DYNAMIC` and `CONFIG_ZBUS_MSG_SUBSCRIBER_BUF_ALLOC_STATIC` telling zbus the allocation of the messages will be static or dynamic;

* `CONFIG_ZBUS_MSG_SUBSCRIBER_NET_BUF_STATIC_DATA_SIZE` as the message is static, we need to tell zbus what that size is;

* At last, the `CONFIG_ZBUS_MSG_SUBSCRIBER_NET_BUF_POOL_SIZE` will set the size of the `net_buf_pool` as you can find in any other network or Bluetooth code.
If you are using dynamic allocation for those messages, as you are doing in your code, you will only use: CONFIG_ZBUS_MSG_SUBSCRIBER=y and CONFIG_ZBUS_MSG_SUBSCRIBER_NET_BUF_POOL_SIZE=X. Set the proper pool size, and that is it.

Yes, and these are some of the problems I absolutely do not want to deal with, nor do I want to force USB users to do so. These configurations are global, instantiated in zbus.c. I do not want USB to share a pool with others. I do not want to force users to enable and configure HEAP. This could be fixed by instantiating memory (net_bufs, just blocks, ...) in the specific ZBUS_CHAN_DEFINE_FOOBAR macro, but other problems remains.

Maybe you see limitations I am not able to see. Please let me know if you have an idea of how to make zbus useful for your case. You may have a special channel that only enables message subscribers to observe it. Or create a way of hiding some system channels. Or a way of separating message subscriber pools: one for the user and the other for the system. It would solve your problems.

I described "limitations" in the commit message. It cannot be fixed because of the way ZBUS is designed. The behavior of the channel depends on the type of observers and can be changed by the application, regardless of whether the channel is "hidden" or not.

If everyone who doesn't find a feature in zbus implements its own event manager, we will not converge in a great bus. I am here to help improve that by listening to the community's needs.

It is not just a missing feature. I really see no reason to use ZBUS in Zephyr subsystems, 0️⃣. All the provided Zbus abstractions of kernel "data passing" can be implemented in a much cleaner and more efficient way without Zbus, but additionally with defined behavior.

Before where and what can be improved in Zbus goes further. In terms of configuration and what it is supposed to do, Zbus cannot beat solutions like this. We have zero dependencies here.

subsys/usb/device_next/usbd_msg.c

rodrigopex · 2024-01-31T14:03:01Z

Zbus (which is not a bus) does not provide this functionality and defined behavior for the publisher context (explained in the commit message)

@jfischer-no, please specify a formal bus definition that tells us zbus is not a bus. Is that your own definition of a bus?

It is not just a missing feature. I really see no reason to use ZBUS in Zephyr subsystems, 0️⃣. All the provided Zbus abstractions of kernel "data passing" can be implemented in a much cleaner and more efficient way without Zbus, but additionally with defined behavior.

I am curious to see the best way to do that because Zbus uses a semaphore and a memory copy to store data in the channel and deliver the messages using fifos (like you here), message queues, and callbacks.

It cannot be fixed because of the way ZBUS is designed.

Why did you not help us during the zbus submission, since you have a better way to do things? I am trying to recall the RFC and the PR adding Zbus; your name is not there.

I am confident we can solve this pool-sharing issue and other zbus gaps.

subsys/usb/device_next/usbd_msg.c

jfischer-no · 2024-02-02T13:42:25Z

Zbus (which is not a bus) does not provide this functionality and defined behavior for the publisher context (explained in the commit message)

@jfischer-no, please specify a formal bus definition that tells us zbus is not a bus. Is that your own definition of a bus?

Something that transfers data between components. In ZBUS message subscriber is closer to that, but from the publisher perspective you never know what happens, there could be listeners or subscribers, some global configurations enabled or disabled, global setting for all channels.... Btw, ZBUS is many channels with 1 -> n communication, not 1 <-> n, not m <-> n.

It is not just a missing feature. I really see no reason to use ZBUS in Zephyr subsystems, 0️⃣. All the provided Zbus abstractions of kernel "data passing" can be implemented in a much cleaner and more efficient way without Zbus, but additionally with defined behavior.

I am curious to see the best way to do that because Zbus uses a semaphore and a memory copy to store data in the channel and deliver the messages using fifos (like you here), message queues, and callbacks.

It cannot be fixed because of the way ZBUS is designed.

Why did you not help us during the zbus submission, since you have a better way to do things? I am trying to recall the RFC and the PR adding Zbus; your name is not there.

I have no better way "to do things". I have different subjective opinions on how and for what a message bus should be used in Zephyr.

I am confident we can solve this pool-sharing issue and other zbus gaps.

Well, that would be a breaking change and there are no ZBUS users in Zephyr subsystems, maybe it is okay for those who already use it. For USB, I am satisfied with effectively ~50 lines of code to do exactly what is needed, with kernel primitives already in place/enabled and zero dependencies. If someone wants to redistribute it to 1->n, they can feed it into a ZBUS channel and be responsible for defined behavior and correct usage, which is then entirely up to the user.

tmon-nordic · 2024-02-02T13:53:11Z

subsys/usb/device_next/usbd_msg.c

+
+	m_pkt = usbd_alloc_msg(ctx);
+	if (m_pkt == NULL) {
+		__ASSERT(false, "Failed to allocate message memory");


This doesn't quality for an ASSERT, because this is somewhat expected that the slab can run empty. Asserts are for things that indicate software bug.

There is no other channel to report this, the alternative is LOG_ERR or k_panic() or to block in alloc(). @tmon-nordic You preferences or combination?

LOG_ERR doesn't make sense here because it will flood the log (if we had equivalent to Linux log once methods then maybe...). k_panic is similar to assert here and therefore not proper. Blocking in alloc is better than other alternatives. There's also possibility to simply not log it at all and not block.

There's also possibility to simply not log it at all and not block.

Would it not be equivalent to assert abuse? assert is also used in the tree for troubleshooting.

Here only tracing is appropriate. Assert is used for troubleshooting because it is designed to expose incorrect code, e.g. calling function with invalid parameters and ao on. Abusing asserts for normal runtime behavior is wrong.

@tmon-nordic Please recheck.

tmon-nordic · 2024-02-02T13:53:49Z

subsys/usb/device_next/usbd_msg.c

+
+	m_pkt = usbd_alloc_msg(ctx);
+	if (m_pkt == NULL) {
+		__ASSERT(false, "Failed to allocate message memory");


This doesn't quality for an ASSERT, because this is somewhat expected that the slab can run empty. Asserts are for things that indicate software bug.

rodrigopex · 2024-02-02T15:20:57Z

@jfischer-no, I prefer to comment only on the facts.

but from the publisher perspective you never know what happens, there could be listeners or subscribers, some global configurations enabled or disabled, global setting for all channels....

Listeners and subscribers consume the data differently but receive the message when it is well implemented. We added global configurations and variations to solve community needs.

Btw, ZBUS is many channels with 1 -> n communication, not 1 <-> n, not m <-> n.

One-to-one, one-to-many, and many-to-many are possible with ZBus. When we say many-to-many, we are not implying many-toAndFrom-many. A channel-based bus forces us to think in different channels for different contexts. Suppose that worked in many-toAndFrom-many (m <-> n). You could use only one channel for everything, which is the idea of sockets but not a bus.

jfischer-no · 2024-02-02T15:44:08Z

@jfischer-no, I prefer to comment only on the facts.

but from the publisher perspective you never know what happens, there could be listeners or subscribers, some global configurations enabled or disabled, global setting for all channels....

Listeners and subscribers consume the data differently but receive the message when it is well implemented. We added global configurations and variations to solve community needs.

Btw, ZBUS is many channels with 1 -> n communication, not 1 <-> n, not m <-> n.

One-to-one, one-to-many, and many-to-many are possible with ZBus. When we say many-to-many, we are not implying many-toAndFrom-many. A channel-based bus forces us to think in different channels for different contexts. Suppose that worked in many-toAndFrom-many (m <-> n). You could use only one channel for everything, which is the idea of sockets but not a bus.

That is your opinion.

major concerns addressed, only the assert abuse remains

jfischer-no · 2024-02-02T18:52:08Z

CI fails because of #68479

tmon-nordic

Just one nit, but that can be revised later. Not sure if system workqueue is better than synchronous notifications, but we'll have to test it in the wild.

tmon-nordic · 2024-02-02T19:18:41Z

include/zephyr/usb/usbd_msg.h

+	"New CDC ACM line coding",
+	"New CDC ACM control line state",


Nit: this is not necessarily "New" as the line coding or control line state might be exactly the same as before (the notifications are for every SET_LINE_CODING and SET_CONTROL_LINE_STATE).

Yes, I am bad at producing human-readable text.

tmon-nordic

Approve, but my gut feeling is that the asynchronous notifications will cause troubles and there'll be push towards synchronous callbacks. (in USB thread context).

The implementation uses the system workqueue and a callback provided and registered by the application. The application callback is called in the context of the workqueue. Notification messages are stored in a queue and delivered to the callback in sequence. We cannot call application callback directly from the USB device stack thread because the behavior of arbitrary code provided by the application is unpredictable, and we do not want it to be executed in the same context where all events from the device controller are handled. Nor can we use the ZBUS subsystem directly. ZBUS offloads responsibility for defined behavior to the observers and application, and does not provide any way for the publisher to enforce defined behavior and execution context. ZBUS listener would be called from the USB thread context and is not acceptable. ZBUS subscriber does not provide delivery guarantee and cached message can be overwritten. ZBUS message subscriber has cumbersome global configuration and buffers that are too complicated to handle from USB configuration, and even if we would use it, defined behavior is not possible because of how listener and subscriber are handled. Signed-off-by: Johann Fischer <[email protected]>

Add message types for line coding and contol line state updates. Add a publish message function that takes a pointer to a device structure as payload, and use USB notification in the CDC ACM implementation. Signed-off-by: Johann Fischer <[email protected]>

This allows samples to receive earlier notification immediately after initialization. Signed-off-by: Johann Fischer <[email protected]>

Use USB notification messages to detect DTR and baudrate updates. Signed-off-by: Johann Fischer <[email protected]>

lopsided98 · 2024-03-23T22:06:23Z

#70417 added a new call to sample_usbd_init_device() right before this PR was merged and needs to be updated.

tmon-nordic · 2024-04-08T09:14:21Z

#70417 added a new call to sample_usbd_init_device() right before this PR was merged and needs to be updated.

@lopsided98 Could you please specify what exactly needs to be updated and why? Can you open PR?

Did #70672 solve the issue you mentioned?

lopsided98 · 2024-04-08T23:24:10Z

Yes, that was the fix.

jfischer-no added the area: USB Universal Serial Bus label Jan 29, 2024

jfischer-no added this to the v3.6.0 milestone Jan 29, 2024

jfischer-no requested review from carlescufi and tmon-nordic January 29, 2024 23:27

jfischer-no self-assigned this Jan 29, 2024

zephyrbot added the area: Samples Samples label Jan 29, 2024

zephyrbot requested review from kartben and nashif January 29, 2024 23:28

tmon-nordic previously requested changes Jan 30, 2024

View reviewed changes

jfischer-no force-pushed the pr-usbd-msg branch from 89c73bc to b65ae36 Compare January 30, 2024 13:57

carlescufi reviewed Jan 31, 2024

View reviewed changes

subsys/usb/device_next/usbd_msg.c Show resolved Hide resolved

jfischer-no force-pushed the pr-usbd-msg branch 2 times, most recently from 61f8310 to bdaf7a7 Compare February 1, 2024 15:07

jfischer-no added the Experimental Experimental features not enabled by default label Feb 1, 2024

jfischer-no mentioned this pull request Feb 1, 2024

Planned USB device/host support rework/enhancements #42066

Open

57 tasks

carlescufi reviewed Feb 2, 2024

View reviewed changes

subsys/usb/device_next/usbd_msg.c Outdated Show resolved Hide resolved

jfischer-no requested review from tmon-nordic and carlescufi February 2, 2024 13:43

tmon-nordic reviewed Feb 2, 2024

View reviewed changes

jfischer-no force-pushed the pr-usbd-msg branch from bdaf7a7 to b68d380 Compare February 2, 2024 17:25

jfischer-no requested a review from tmon-nordic February 2, 2024 17:39

carlescufi previously approved these changes Feb 2, 2024

View reviewed changes

tmon-nordic previously approved these changes Feb 2, 2024

View reviewed changes

henrikbrixandersen removed this from the v3.6.0 milestone Feb 6, 2024

jfischer-no force-pushed the pr-usbd-msg branch from b68d380 to 1688cb8 Compare February 7, 2024 15:39

jfischer-no dismissed stale reviews from tmon-nordic and carlescufi via 2e4bb12 February 7, 2024 15:49

jfischer-no force-pushed the pr-usbd-msg branch from 1688cb8 to 2e4bb12 Compare February 7, 2024 15:49

tmon-nordic approved these changes Feb 8, 2024

View reviewed changes

jfischer-no added 4 commits March 22, 2024 08:40

samples: usb: add message callback registration to the common code

db753b6

This allows samples to receive earlier notification immediately after initialization. Signed-off-by: Johann Fischer <[email protected]>

samples: cdc_acm: use USB notification messages to detect DTR

2ec3d4e

Use USB notification messages to detect DTR and baudrate updates. Signed-off-by: Johann Fischer <[email protected]>

jfischer-no force-pushed the pr-usbd-msg branch from 2e4bb12 to 2ec3d4e Compare March 22, 2024 07:41

carlescufi approved these changes Mar 22, 2024

View reviewed changes

carlescufi merged commit ae08fe9 into zephyrproject-rtos:main Mar 22, 2024

usb: device_next: implementation of USB device support notification #68256

usb: device_next: implementation of USB device support notification #68256

Uh oh!

Conversation

jfischer-no commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jfischer-no Feb 1, 2024 • 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!

jfischer-no Feb 1, 2024 • 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!

tmon-nordic commented Jan 30, 2024

Uh oh!

rodrigopex commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jfischer-no commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

rodrigopex commented Jan 31, 2024

Uh oh!

Uh oh!

jfischer-no commented Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmon-nordic Feb 2, 2024 • 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!

jfischer-no Feb 2, 2024 • 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!

rodrigopex commented Feb 2, 2024

Uh oh!

jfischer-no commented Feb 2, 2024

Uh oh!

jfischer-no commented Feb 2, 2024

Uh oh!

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

tmon-nordic left a comment

Choose a reason for hiding this comment

Uh oh!

jfischer-no commented Jan 29, 2024 •

edited

Loading

jfischer-no Feb 1, 2024 •

edited

Loading

jfischer-no Feb 1, 2024 •

edited

Loading

rodrigopex commented Jan 30, 2024 •

edited

Loading

jfischer-no commented Jan 30, 2024 •

edited

Loading

jfischer-no commented Feb 2, 2024 •

edited

Loading

tmon-nordic Feb 2, 2024 •

edited

Loading

jfischer-no Feb 2, 2024 •

edited

Loading

tmon-nordic commented Apr 8, 2024 •

edited

Loading