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

shell/usb cdc_acm: shell does not work on CDC_ACM #32581

Closed
alexanderwachter opened this issue Feb 23, 2021 · 6 comments
Closed

shell/usb cdc_acm: shell does not work on CDC_ACM #32581

alexanderwachter opened this issue Feb 23, 2021 · 6 comments
Assignees
Labels
area: Shell Shell subsystem area: USB Universal Serial Bus

Comments

@alexanderwachter
Copy link
Member

The shell is not responding on USB CDC_ACM after #31880 got merged.
However, the USB serial device is still working.

I'm on a custom board with an stm32g4.
Reverting the commit from 31880 brings the shell back.

@alexanderwachter alexanderwachter added bug The issue is a bug, or the PR is fixing a bug area: USB Universal Serial Bus area: Shell Shell subsystem labels Feb 23, 2021
@carlescufi carlescufi added the Regression Something, which was working, does not anymore label Feb 23, 2021
@jfischer-no
Copy link
Collaborator

CONFIG_SHELL_BACKEND_SERIAL_INIT_PRIORITY=51 is a possible workaround.

@pabigot
Copy link
Collaborator

pabigot commented Feb 23, 2021

Yes, this is at least partly due to device_get_binding() now returning null on devices that have not been initialized. We're going to need runtime equivalent to DEVICE_DT_GET() that looks up things by name regardless of whether they're ready to be used.

@galak again we should perhaps slow down on removing label properties.

@pabigot
Copy link
Collaborator

pabigot commented Feb 23, 2021

FWIW, I can't manage to get samples/subsys/shell/shell_module to run on a CDC ACM before or after #31880 was merged, even with updating the init priority. If anybody can provide clear instructions I'd appreciate it.

@jfischer-no
Copy link
Collaborator

FWIW, I can't manage to get samples/subsys/shell/shell_module to run on a CDC ACM before or after #31880 was merged, even with updating the init priority. If anybody can provide clear instructions I'd appreciate it.

I found an old patch that adds shell support to console sample, #32586

@pabigot
Copy link
Collaborator

pabigot commented Feb 23, 2021

I'm going to call this a configuration error, and so the issue is a question, not a bug or a regression.

The shell UART back end initializes in POST_KERNEL at CONFIG_SHELL_BACKEND_SERIAL_INIT_PRIORITY which defaults to 0.

Most UART devices initialize in PRE_KERNEL_1 at CONFIG_SHELL_BACKEND_SERIAL_INIT_PRIORITY which defaults to 50.

So everything just works as long as serial devices are initialized in an earlier init level.

But CDC ACM device initializes at POST_KERNEL at CONFIG_SHELL_BACKEND_SERIAL_INIT_PRIORITY which defaults to 50.

So the correct fix, if using CDC ACM for shell, is to increase CONFIG_SHELL_BACKEND_SERIAL_INIT_PRIORITY to at least 51 as @jfischer-no identified.

I don't see any obvious way to detect the situation and change the default.

FWIW, the solution prototyped in #32587 also makes it work, but for the same reason that it worked until device_is_ready() got fixed: the device is successfully retrieved before initialization but the operations performed on it appear to either be delayed until the device is ready, or are robust if invoked before the device has been initialized.

Making shell_uart robust in the face of UART devices that can be started late or turned off is a much larger task.

@pabigot pabigot added question and removed Regression Something, which was working, does not anymore bug The issue is a bug, or the PR is fixing a bug labels Feb 23, 2021
@alexanderwachter
Copy link
Member Author

I can confirm that setting CONFIG_SHELL_BACKEND_SERIAL_INIT_PRIORITY=51 solves the problem. Thanks @pabigot @jfischer-no

@pabigot pabigot closed this as completed Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Shell Shell subsystem area: USB Universal Serial Bus
Projects
None yet
Development

No branches or pull requests

4 participants