Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

samples: shell_module: add shell support over CDC ACM #32586

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

jfischer-no
Copy link
Collaborator

@jfischer-no jfischer-no commented Feb 23, 2021

Add shell support over CDC ACM

@jfischer-no jfischer-no added area: USB Universal Serial Bus area: Samples Samples RFC Request For Comments: want input from the community labels Feb 23, 2021
@jfischer-no jfischer-no force-pushed the pr-shell-over-cdcacm branch 3 times, most recently from 994ff54 to 236f787 Compare February 23, 2021 17:39
Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

This has been often requested by users

Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

"console" and "shell" are distinct functions. The role of this sample is to demonstrate a console on CDC-ACM.

Demonstration of a shell on CDC-ACM should be done as a sibling samples/subsys/usb/shell, or even better added as an option in samples/subsys/shell/shell_module so the shell that's being demonstrated is actually useful.

@jfischer-no
Copy link
Collaborator Author

"console" and "shell" are distinct functions. The role of this sample is to demonstrate a console on CDC-ACM.

Demonstration of a shell on CDC-ACM should be done as a sibling samples/subsys/usb/shell, or even better added as an option in samples/subsys/shell/shell_module so the shell that's being demonstrated is actually useful.

Understand, but do not fully agree, console sample is really simple, to have that again only with 2..3 different lines makes no sense in my opinion. Also, IMO samples/subsys/usb is the one where the users would look for an example. What do you think about renaming it samples/subsys/usb/shell?

@pabigot
Copy link
Collaborator

pabigot commented Feb 24, 2021

Understand, but do not fully agree, console sample is really simple, to have that again only with 2..3 different lines makes no sense in my opinion. Also, IMO samples/subsys/usb is the one where the users would look for an example. What do you think about renaming it samples/subsys/usb/shell?

I care far more about console than I do about shell, so I would prefer to keep the USB console example showing how to get a USB console. That sample is what I go to for basic USB console verification, and that it generates repeated text (as I'd get from log messages on a console) is exactly what I want for its behavior.

When I do want a shell, I want a shell that actually does something useful, which is samples/subsys/shell/shell_module. I think users are more likely to look there for a shell sample, regardless of whether the connection is by standard serial or USB serial.

Add shell support over CDC ACM.

Signed-off-by: Johann Fischer <[email protected]>
@jfischer-no jfischer-no changed the title samples: usb: add shell support sample to console sample samples: shell_module: add shell support over CDC ACM Feb 25, 2021
Copy link
Collaborator

@pabigot pabigot left a comment

Choose a reason for hiding this comment

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

Thanks!


dev = device_get_binding(CONFIG_UART_SHELL_ON_DEV_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you use const struct device *dev = DEVICE_DT_GET(DT_CHOSEN(zephyr_shell_uart));?

Copy link
Collaborator

@pabigot pabigot Feb 26, 2021

Choose a reason for hiding this comment

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

That could be misleading. https://docs.zephyrproject.org/latest/reference/devicetree/api.html specifies that zephyr,shell-uart is used only to supply the default value for CONFIG_UART_SHELL_ON_DEV_NAME. The user can override the actual device through Kconfig.

It could be switched for the sample on an argument that the sample's prj.conf doesn't do that override, but that could confuse a user who finds the Kconfig then doesn't understand why the device selection code copied from the sample doesn't work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

More relevantly: is it even possible to reference a CDC-ACM UART from devicetree? I think the answer is "no".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

More relevantly: is it even possible to reference a CDC-ACM UART from devicetree? I think the answer is "no".

It is not possible, but something to discuss, I have use case for that.

@nashif nashif merged commit 01a0a6a into zephyrproject-rtos:master Feb 26, 2021
@jfischer-no jfischer-no deleted the pr-shell-over-cdcacm branch February 26, 2021 13:12
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 RFC Request For Comments: want input from the community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants