Skip to content

Conversation

@jfischer-no
Copy link
Contributor

Notify only if the device configuration has changed. Pass only the configuration value as the message status, the actual device speed can be obtained with usbd_bus_speed().

@jfischer-no jfischer-no added area: USB Universal Serial Bus Experimental Experimental features not enabled by default labels Jul 24, 2024
@jfischer-no jfischer-no added this to the v4.0.0 milestone Jul 24, 2024
@jfischer-no jfischer-no requested a review from tmon-nordic July 24, 2024 11:59
@zephyrbot zephyrbot added the area: Samples Samples label Jul 24, 2024
@zephyrbot zephyrbot requested review from kartben and nashif July 24, 2024 12:00
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that notifications are asynchronous make this somewhat fragile, because the connection speed can change between the publish and handling. If the device disconnects though (which is mandatory for the speed to change) any stale configuration notifications are no longer relevant anyway.

I consider this as a example to where the notifications should be synchronous.

Notify only if the device configuration has changed. Pass only the
configuration value as the message status, the actual device speed can
be obtained with usbd_bus_speed().

Signed-off-by: Johann Fischer <[email protected]>
@jfischer-no jfischer-no force-pushed the pr-device_next-msg-configuration-state branch from 8914fcc to 0497865 Compare July 24, 2024 13:10
@carlescufi carlescufi requested a review from tmon-nordic August 14, 2024 11:17
@carlescufi carlescufi assigned jfischer-no and unassigned kartben Aug 14, 2024
Copy link
Contributor

@tmon-nordic tmon-nordic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approve, but I think we should revisit having synchronous notifications later. The more I think about potential issues we might face later due to asynchronous notification, the more I think zbus would be appropriate (as it leaves the choice between synchronous/asynchronous to application developer).

@nashif nashif merged commit f851cad into zephyrproject-rtos:main Aug 16, 2024
@jfischer-no
Copy link
Contributor Author

Approve, but I think we should revisit having synchronous notifications later. The more I think about potential issues we might face later due to asynchronous notification, the more I think zbus would be appropriate (as it leaves the choice between synchronous/asynchronous to application developer).

You have not expressed any specific problems with what you call "asynchronous" behavior. The existing implementation has no known drawbacks, is simple and efficient. There is no reason to add something bloated, complicated, and with unpredictable behavior, and therefore there will never be notification with ZBUS in USB.

@jfischer-no jfischer-no deleted the pr-device_next-msg-configuration-state branch August 19, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Samples Samples area: USB Universal Serial Bus Experimental Experimental features not enabled by default

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants