Skip to content

Conversation

@jfischer-no
Copy link
Contributor

Allow to use Kconfig files in snippets. Remove all USB and CDC ACM
configuration in favor of snippet cdc-acm-console (west build -S
cdc-acm-console ...).

This function can be useful outside of HWMv2, for example to add Kconfig
files to snippets.

Signed-off-by: Johann Fischer <[email protected]>
@jfischer-no jfischer-no added the area: USB Universal Serial Bus label Nov 11, 2024
@jfischer-no jfischer-no added this to the v4.1.0 milestone Nov 11, 2024
@carlescufi
Copy link
Member

Related issue: #58349

This is a draft and have some limitations, WiP.

Signed-off-by: Johann Fischer <[email protected]>
Allow snippet to be used with any board that wants to use the CDC ACM
UART as a logging or shell backend.

Signed-off-by: Johann Fischer <[email protected]>
Many boards have similar code to configure the USB and CDC ACM UART that
they want to use as a logging or shell backend. Also, many boards have
incorrect or incomplete configuration. Remove all USB and CDC ACM
configuration in favor of snippet cdc-acm-console (west build -S
cdc-acm-console ...).

Signed-off-by: Johann Fischer <[email protected]>
#
# SPDX-License-Identifier: Apache-2.0

config SHELL_BACKEND_SERIAL_CHECK_DTR
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not actually a Kconfig file, it's actually a Kconfig.defconfig file, Kconfig files add settings and Kconfig.defconfig files set defaults. I do not think snippets should be adding Kconfig because all that's going to do is cause confusion or require someone to do -DCONFIG_x=y on the command line when using snippets and the whole point in snippets is to not have to do -DCONFIG_x=y

Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Not convinced we should allow Kconfig files in snippets.

Some of the reasons why I see this as problematic are:

  • Snippets only does what users themselves can do using EXTRA_CONF_FILE and EXTRA_DTC_OVERLAY_FILE.
    Supporting Kconfig files in snippets breaks this principle because users cannot inject extra Kconfig files in the same way.
  • The Kconfig tree is checked by compliance to ensure that the Kconfig files are not refering to undefined symbol, such as select UNDEFINED_FOO or depends on UNDEFINED_BAR.
    Allowing snippets to inject extra Kconfig files provides a whole whereby one can suddenly introduce new symbols, dependencies, etc. without this being verified by the compliance check.
  • Sourcing Kconfig.defconfig from a snippet makes it harder to predict default values and adjust accordingly.
    Currently the shield, board, and SoC defconfigs are sourced first, and thus takes precedence in default values.
    Next Zephyr modules' Kconfig files are sourced, which allows Zephyr modules to further adjust defaults or provide new symbols.
    Boards and SoCs can be consider are quite static, meaning that if a given Zephyr module supports a given set of boards / SoCs, then it can be determine what default values can be, and from there the module can know what to further adjust..
    Adding snippets into this loop makes it much more unpredictable and harder to make proper adjustments as anyone can just create new Kconfig snippet part.
    Thus allowing this PR makes things harder for others, and things can already be hard enough in today's design.

Having the need for snippets to adjust defaults indicates to me that something is not well designed / written.
Why exactly is it that snippets needs to adjust the defaults and cannot explicitly set the configuration using CONFIG_FOO=y ?
If the snippet requires a value to be set, then it should just set it.
If the symbol is promptless, then it should be considered if the symbol instead should have a prompt, and if not, whether we should support assigning values to promptless symbols in certain cases.

Before being able to approve this approach I would like to see an issue / good description of what exactly the limitation with using conf files for snippet is, cause it's not clear to me exactly what this solution tries to solve, except just making a shortcut for moving existing Kconfig files out of the board and into the snippet.

@nordicjm
Copy link
Contributor

I agree that I don't think snippets should be using Kconfigs like that. The one thing I think is a possible future idea is for shields to use them, there are things like mikro bus connectors and there can be multiple on a board, as it stands right now a shield will only ever go into one of these (which is whatever one it has been created for) so it's not possible to add 2 which use the same socket if you wanted to have one in each socket. So what would be useful there is to have Kconfig (though this would also need some way to represent it in dts too, which is much harder) that have a replacement of the slot number, e.g.

config <shield_name>_<instance>_PORT
        int
        default <generated_from_@_or_other_in_-DSHIELD_argument>

@github-actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jan 19, 2025
@jfischer-no jfischer-no removed the Stale label Jan 21, 2025
@jfischer-no jfischer-no removed this from the v4.1.0 milestone Feb 5, 2025
@github-actions
Copy link

github-actions bot commented Apr 7, 2025

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Apr 7, 2025
@github-actions github-actions bot closed this Apr 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: USB Universal Serial Bus Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants