-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
usb: patches to resolve issues around Kconfig option USB_UART_CONSOLE #40220
usb: patches to resolve issues around Kconfig option USB_UART_CONSOLE #40220
Conversation
@jfischer-no : May you please let me know if you have tested this fix with nrf52840 dongle? |
tests/bluetooth/shell/src/main.c
Outdated
if (IS_ENABLED(CONFIG_USB_UART_CONSOLE)) { | ||
usb_enable(NULL); | ||
k_sleep(K_SECONDS(2)); | ||
#if DT_NODE_HAS_COMPAT(DT_CHOSEN(zephyr_shell_uart), zephyr_cdc_acm_uart) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe you could extract it into a function, something like:
if (DT_NODE_HAS_COMPAT(DT_CHOSEN(zephyr_shell_uart), zephyr_cdc_acm_uart)) {
wait_usb_ready();
}
That would make it obvious why it is here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is removed now.
@@ -53,6 +53,12 @@ config CDC_ACM_DTE_RATE_CALLBACK_SUPPORT | |||
by Arduino style programmers to reset the device into the | |||
bootloader. | |||
|
|||
config USB_UART_CONSOLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get rid of this at all? Maybe something with dt_compat_enabled
could be used instead, i.e. here
default y if USB_UART_CONSOLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is there for reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
deprecated now
@@ -34,7 +34,7 @@ config CONSOLE_HANDLER | |||
|
|||
config CONSOLE_INIT_PRIORITY | |||
int "Console init priority" | |||
default 95 if USB_UART_CONSOLE || UART_MUX | |||
default 95 if UART_MUX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First observation, the default priority of 95
was chosen because of the USB stack initialization, see:
df2bb46
usb: console: Initialize USB console after USB Device stack
It does make sense to initialize USB console after USB Device stack.
Note that the value is selected only if we specify USB_UART_CONSOLE
in prj.conf, not in menuconfig afterwards.Fixes #16518
So what has changed, since console can now be initialized at 60
instead ?
This commit only changed the symbol used: d8bc37b
but the default value is still 50
SERIAL_INIT_PRIORITY
defaults to KERNEL_INIT_PRIORITY_DEVICE
default == 50
Second observation:
The CONSOLE_INIT_PRIORITY
defaults to KERNEL_INIT_PRIORITY_DEFAULT
== 40
when UART_CONSOLE is not enabled, and the setting is having a prompt.
This means this setting suffers from the stuck symbols syndrome:
https://docs.zephyrproject.org/latest/guides/build/kconfig/tips.html#stuck-symbols-in-menuconfig-and-guiconfig
meaning that if anyone later enables the USB_UART_CONSOLE
or the UART_CONSOLE
, then the priority will not change, in which case the 40
value would risk still being active and not the preferred
default of 60
.
I believe majority of boards are having CONFIG_UART_CONSOLE=y
in their _defconfig
so it might not be a real problem, but it still looks as we might have a risk of strangely behaving system.
And it sounds to me as there is also some upper bound dependency since 95
is no longer a good default.
So should this priority in reality be a range ?
Where the upper and lower bounds might be defined by other settings.
Or should the setting be promptless in certain cases ?
Note, a promptless setting can still be controlled through a Kconfig file, for example Kconfig.board
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what has changed, since console can now be initialized at
60
instead ?
This commit only changed the symbol used: d8bc37b
but the default value is still50
SERIAL_INIT_PRIORITY
defaults toKERNEL_INIT_PRIORITY_DEVICE
default ==50
CDC ACM UART driver has been reworked a bit and does not block when USB device support is not initialized, (behaves like regular UART).
Or should the setting be promptless in certain cases ?
I am in favor of this option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC ACM UART driver has been reworked a bit and does not block when USB device support is not initialized, (behaves like regular UART).
I would like to see this sentence in the commit message, as that is an important piece of information in order to understand why changing the default from 95
to 60
is safe.
This option is only used to enable relevant options in backend | ||
configuration (if any), and for backward compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sound like a setting we don't really assume is in use, but we don't know.
I think it should be investigated and marked deprecated so that it can be completely removed in two releases from now.
Else I fear this setting will just stay and never get cleaned up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is in use and is investigated, see commit message:
"Kconfig option USB_UART_CONSOLE does not have any dependencies
on console drivers, but is used to select backend specific
Kconfig option in subsys/shell/backends/Kconfig.backends.
Keep the name of the option USB_UART_CONSOLE but move it
to CDC ACM configuration and update description."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then I suggest to rework the comment, cause that is not clear to me.
And as this setting is deprecated, are there any plans / tasks to follow up on removing the dependency in subsys/shell/backends/Kconfig.backends
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commit was reworked. I will remove Kconfig option USB_UART_CONSOLE and dependency in subsys/shell/backends/Kconfig.backends in subsequent PR because MCUboot needs to be adapted as well.
yes I am testing it with nrf52840dongle_nrf52840 |
8ba5c06
to
58060d9
Compare
Use same init level and priority as serial drivers. Align priority to the changes in commit ad14505 ("drivers: serial: Refactor drivers to use shared init priority Kconfig") Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Do not change init level and priority of console driver if Kconfig option CONFIG_USB_UART_CONSOLE is enabled because CDC ACM UART driver now uses the same init level and priority as regular serial driver. Replace CONFIG_USB_UART_CONSOLE with DT_NODE_HAS_COMPAT(DT_CHOSEN(zephyr_...), zephyr_cdc_acm_uart) in the source code. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Kconfig option USB_UART_CONSOLE does not have any dependencies on console drivers, but is used to select backend specific Kconfig option in subsys/shell/backends/Kconfig.backends. Move Kconfig option USB_UART_CONSOLE to CDC ACM configuration, deprecate it and update description. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Possibility to initialize USB device support is useful when only CDC ACM class is enabled and CDC ACM UART is used as backend for console, shell, or logging. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Add generic shields that provide devicetree and configuration overlays, and configure USB device stack so that CDC ACM UART can be used as backend for console, logging, or shell. This approach allows us to avoid many identical overlays in samples and tests directories, and cumbersome board configurations. These shields use Kconfig option CONFIG_USB_DEVICE_INITIALIZE_AT_BOOT and is mainly intended to be used with boards like nrf52840dongle_nrf52840 or bl654_usb. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree and config overlay, and explicit USB support initialization in favor of generic CDC ACM shield. For shell support via USB CDC ACM, this example is to be built as follows: west build -b nrf52840dongle_nrf52840 samples/subsys/shell/shell_module -- -DSHIELD=cdc_acm_shell Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree and config overlay, and explicit USB support initialization in favor of generic CDC ACM shield. For shell support via USB CDC ACM, this example is to be built as follows: west build -b nrf52840dongle_nrf52840 tests/bluetooth/shell -- -DSHIELD=cdc_acm_shell Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree and config overlay, and explicit USB support initialization in favor of generic CDC ACM shield. For console support via USB CDC ACM, this example is to be built as follows: west build -b reel_board tests/drivers/uart/uart_basic_api -- -DSHIELD=cdc_acm_console Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree overlay, and explicit USB support initialization in favor of generic CDC ACM shield. For console support via USB CDC ACM, this example is to be built as follows: west build -b sensortile_box samples/boards/sensortile_box -- -DSHIELD=cdc_acm_console Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
This sample uses console driver and therefore Kconfig option CONFIG_CONSOLE and CONFIG_UART_CONSOLE are required. Kconfig option CONFIG_USB_UART_CONSOLE no longer has any influence here and should be removed. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Remove CDC ACM UART devicetree nodes, chosen properties and explicit USB support configuration in favor of generic CDC ACM shield. For console support via USB CDC ACM, in-tree samples for this boards is to be built as follows: west build -b nrf52840dongle_nrf52840 samples/basic/threads -- -DSHIELD=cdc_acm_console For shell support via USB CDC ACM, in-tree samples for this boards is to be built as follows: west build -b bl654_usb samples/subsys/shell/shell_module -- -DSHIELD=cdc_acm_shell Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Since there are no more users and dependencies of Kconfig option USB_UART_CONSOLE in the tree, remove the remains. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Add support for chosen property "zephyr,bt-c2h-uart". Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
fa8076d
to
6c32698
Compare
@@ -19,8 +19,7 @@ tests: | |||
filter: CONFIG_UART_CONSOLE | |||
harness: keyboard | |||
drivers.uart.cdc_acm: | |||
extra_args: OVERLAY_CONFIG="overlay-usb.conf" | |||
DTC_OVERLAY_FILE="usb.overlay" | |||
extra_args: SHIELD="cdc_acm_console" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A question here, I like the general direction, but I'd like to know how tests will work with boards that already declare usb for value of zephyr,console
chosen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK last property definition wins. With this PR, there are no more boards that define chosen property with reference to "zephyr,cdc-acm-uart" compatible node. But it works even if it is the case and shield is used, then there would be two CDC ACM devices, one of them would be used by the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not against this and I have tested it and works fine. but maybe we should rethink the word SHIELD
here. Perhaps we could come up with something that describes both meta-configurations and actual hardware shields. Some new name, like METACFG
?
Imagine we used it for other purposes too, like:
west build samples/hello_world -- -DMETACFG=usb_cdc_acm -DMETACFG=use_mcuboot -DMETACFG=my_hw_shield
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main issue I see is the SHIELD=cdc_acm_console
, please see detailed comments.
The reason for this PR is stated to be
Patches to resolve issues around Kconfig option USB_UART_CONSOLE.
#40220 (comment)
but it is not clear to me why this series of commits are needed to solve the issue.
Is is possible to split this PR into the part that is needed to solve the particular issue, and then a second part with the cdc acm console enhancement ?
If not, then please make a proper description why the big rewrite to the system is needed.
@@ -34,7 +34,7 @@ config CONSOLE_HANDLER | |||
|
|||
config CONSOLE_INIT_PRIORITY | |||
int "Console init priority" | |||
default 95 if USB_UART_CONSOLE || UART_MUX | |||
default 95 if UART_MUX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CDC ACM UART driver has been reworked a bit and does not block when USB device support is not initialized, (behaves like regular UART).
I would like to see this sentence in the commit message, as that is an important piece of information in order to understand why changing the default from 95
to 60
is safe.
This option is only used to enable relevant options in backend | ||
configuration (if any), and for backward compatibility. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then I suggest to rework the comment, cause that is not clear to me.
And as this setting is deprecated, are there any plans / tasks to follow up on removing the dependency in subsys/shell/backends/Kconfig.backends
?
@@ -0,0 +1,64 @@ | |||
.. _cdc_acm_shield: | |||
|
|||
Generic shields for CDC ACM UART |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shields today are a piece of hardware you may attach to another board.
file:///projects/github/ncs/zephyr/doc/_doc/html/boards/shields/index.html
This commit 4d53a79 uses the shield feature for something that is not a shield.
That is a direction I dislike.
It's another approach of #14740 and #33656, just through shield this time.
Using shield this way makes it unclear what a shield is, and it also suffers the same disadvantage / complexity as mentioned here: #14740 (comment)
If you would like to propose a solution like the shield then please make a dedicated PR with such proposal the can be discussed, also at dev-review. For example the METACFG
name as mentioned here: #40220 (review)
Not being against such idea, my concern is still related to:
- How does end-user know when this meta-config can be used.
- One more place (in an already complex system) from where files are picked up
- What would be the threshold when new meta-config should be introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am convinced that shield approach is a better fit for CDC ACM UART, similar to how one would connect a real UART controller through I2C/SPI using a shield.
@@ -1,18 +0,0 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this means that users are now forced to specify the -DSHIELD=cdc_acm_console
or they'll see those warnings:
Parsing /projects/github/ncs/zephyr/Kconfig
Loaded configuration '/projects/github/ncs/zephyr/boards/arm/nrf52840dk_nrf52840/nrf52840dk_nrf52840_defconfig'
Merged configuration '/projects/github/ncs/zephyr/samples/boards/sensortile_box/prj.conf'
Configuration saved to '/projects/github/ncs/zephyr/samples/boards/sensortile_box/build/zephyr/.config'
Kconfig header saved to '/projects/github/ncs/zephyr/samples/boards/sensortile_box/build/zephyr/include/generated/autoconf.h'
warning: USB_DEVICE_VID (defined at subsys/usb/Kconfig:21) was assigned the value '0x0483' but got
the value ''. Check these unsatisfied dependencies: USB_DEVICE_STACK (=n). See
http://docs.zephyrproject.org/latest/reference/kconfig/CONFIG_USB_DEVICE_VID.html and/or look up
USB_DEVICE_VID in the menuconfig/guiconfig interface. The Application Development Primer, Setting
Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful
too.
warning: USB_DEVICE_PID (defined at subsys/usb/Kconfig:27) was assigned the value '0x1234' but got
the value ''. Check these unsatisfied dependencies: USB_DEVICE_STACK (=n). See
http://docs.zephyrproject.org/latest/reference/kconfig/CONFIG_USB_DEVICE_PID.html and/or look up
USB_DEVICE_PID in the menuconfig/guiconfig interface. The Application Development Primer, Setting
Configuration Values, and Kconfig - Tips and Best Practices sections of the manual might be helpful
too.
warning: USB_DEVICE_PRODUCT (defined at subsys/usb/Kconfig:39) was assigned the value 'Zephyr CDC
SensorTile.box' but got the value ''. Check these unsatisfied dependencies: USB_DEVICE_STACK (=n).
See http://docs.zephyrproject.org/latest/reference/kconfig/CONFIG_USB_DEVICE_PRODUCT.html and/or
look up USB_DEVICE_PRODUCT in the menuconfig/guiconfig interface. The Application Development
Primer, Setting Configuration Values, and Kconfig - Tips and Best Practices sections of the manual
might be helpful too.
that's not a good user experience.
The user must be very familiar with Zephyr to spot that this line
extra_args: SHIELD="cdc_acm_console" |
indicates this sample must be build with:
-DSHIELD=cdc_acm_console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, they would look in samples/boards/sensortile_box/README.rst, preferably before, and see how to build it. 461b174#diff-9b13c471892c9de2d5bb33dce8bfaf55dd08edb938cb2ed9642f77bb32496000R40
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but having to apply an extra argument, in a place where app.overlay
used to work, seems to be a step backward.
A sample should preferable be able to built in normal mode without extra args.
@@ -5,16 +5,6 @@ config BOARD_ARDUINO_NANO_33_BLE | |||
bool "Arduino Nano 33 BLE board" | |||
depends on SOC_NRF52840_QIAA | |||
|
|||
config BOARD_ARDUINO_NANO_33_BLE_EN_USB_CONSOLE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so all boards cleaned up in this commit 9c03497 must now use -DSHIELD=cdc_acm_console
That sounds like a step backward in user-friendliness if those boards are expected to always use the usb as console.
For example, this board bl654_usb
doesn't seem to have any other option than using USB for console:
https://docs.zephyrproject.org/latest/boards/arm/bl654_usb/doc/bl654_usb.html
@kieran-mackey FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it is a step forward, only with these patches it becomes possible to use CDC ACM UART (not usb) as backend for console. The configuration of this board in the tree is not usable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configuration of this board in the tree is not usable.
Which board or all boards ?
Read my comment:
so all boards cleaned up in this commit 9c03497 must now use -DSHIELD=cdc_acm_console
i'm refering to the entire commit 9c03497, not a single board, but github requires me to put the comment on a single line.
That is, my comment covers all those boards you are modifying in that commit:
- arduino_nano_33_ble
- bl654_usb
- degu_evk
- nrf52840dongle_nrf52840
are you saying that the current configuration in Zephyr of those boards are not working for any of them ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each commit has a description, please read through it. I see no reason to copy it again in PR description. |
A pull request description in GitHub is like the cover letter for a patch series sent via email. It is an overall justification for the work, along with an overview of the individual commits. It clearly has a purpose. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this is probably the easiest way to accomplish the goal of adding some Kconfig and DT overlays into the build using a single variable setting (-DSHIELD=...
), but this is really a confusing abuse of the system.
I think what's going on here is that a "shield" is a special case of something more general that has what you want (@carlescufi 's "meta-config"), and you are using the existing mechanism because we haven't solved the real problem.
As discussed offline I think this is similar to the problem @JordanYates was trying to solve for mcuboot, which he discussed at the Zephyr Developers Summit (ZDS). We should try to solve the real problem, not just abuse shields, especially since there is also ongoing work to try to solve the multi-image build problem for the special case of mcuboot.
At ZDS, we discussed having an "include path" or a list of search directories for places that would contain both Kconfig and DTS files to apply to the build. Everyone agreed about the general idea, but nobody could agree what the call that thing, and we never agreed on anything. Let's try again; we have enough interested people that I hope we can do it this time.
I did, and for example I see this in one of the commit messages:
This talks about what you do, and a little why.
To me the particular commit message I just posted seems to solve a very different issue imho. |
I do not abuse anything, I am adding a shield for an emulated UART controller. It is also not an abuse to have emulated boards like qemu_cortex_m0.
@mbolivar-nordic, @tejlmand, @carlescufi I opened #40428 to remove USB_UART_CONSOLE option. |
Patches to resolve issues around Kconfig option USB_UART_CONSOLE.
Alternative to #40192
Alternative to #39987