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

boards: shields: use Kconfig.defconfig system for shields #17310

Closed
yboutreux opened this issue Jul 3, 2019 · 12 comments · Fixed by #20934
Closed

boards: shields: use Kconfig.defconfig system for shields #17310

yboutreux opened this issue Jul 3, 2019 · 12 comments · Fixed by #20934
Labels
area: Shields Shields (add-on boards) Enhancement Changes/Updates/Additions to existing features

Comments

@yboutreux
Copy link
Contributor

yboutreux commented Jul 3, 2019

Is your enhancement proposal related to a problem? Please describe.
I am currently porting a BLE shield that use BlueNRG-MS chip (X-NUCLEO-IDB05A). The shield needs to set some specific CONFIG to work, such as CONFIG_SPI=y, that should in theory be enabled only if CONFIG_BT=y is set in a project.
The current way of doing this for shields is to add every config in configuration overlays, meaning that when a shield is added as a -DSHIELD during compilation, config value are automatically set.
For example, is this case, this mean as soon as we compile with specifying the BLE shield, CONFIG_SPI will automatically be set, even if CONFIG_BT isn't.

Describe the solution you'd like
Using Kconfig.defconfig system for shields would allow us to do things such as :

if BT
config SPI
	default y
choice BT_HCI_BUS_TYPE
	default BT_SPI
endchoice

config BT_SPI_BLUENRG
	default y
...
...
endif #BT

preventing the setting of configuration that might be unused.

Describe alternatives you've considered
The current way of doing it is to set configuration in overlays

CONFIG_SPI=y
CONFIG_BT_SPI=y
CONFIG_BT_SPI_BLUENRG_y
...
...

which will automatically set those configurations even if we don't use BT (when compiling with the shield specified)

Additional context
This might be useful for projects that interface multiple shields

@yboutreux yboutreux added the Enhancement Changes/Updates/Additions to existing features label Jul 3, 2019
@yboutreux yboutreux changed the title boards: shields: use Kconfig_defconfig system for shields boards: shields: use Kconfig.defconfig system for shields Jul 3, 2019
@erwango
Copy link
Member

erwango commented Jul 3, 2019

@avisconti , would it also help you in your microphone shield?

@avisconti
Copy link
Collaborator

@avisconti , would it also help you in your microphone shield?

Yes, definetely.
I need to define 3 things in the shield for microphones:

  1. I2S, timer and so on configuration. This seems to require a Kconfig.defconfig support in shields
  2. Extend the pinmuxing. Ideally would be nice to have a pinmux.c file in shields as well.
  3. extend the dts. It seems to me that maybe the overlay file is not enough. I would study more on this.

@erwango
Copy link
Member

erwango commented Jul 3, 2019

For reference, they were previously available but been removed in #12403 when we put dt parsing ahead of Kconfig.

This was done since we were also providing Kconfig.shield defining Shield Kconfig (CONFIG_SHIELD_X_NUCLEO_IKS01A2 for instance). These should be removed because had to be parsed before dts. Now, we're providing shield selection via command line (cmake stage) and we don't have these files.

Thought instead of previous:

if SHIELD_X_NUCLEO_IKS01A2

if HTS221

choice HTS221_TRIGGER_MODE
	default  HTS221_TRIGGER_NONE
endchoice

endif # HTS221
endif # SHIELD_X_NUCLEO_IKS01A2

We can do simply:

if HTS221

choice HTS221_TRIGGER_MODE
	default  HTS221_TRIGGER_NONE
endchoice

endif # HTS221

So it extends the board's Kconfig.defconfig, but w/o needing shield Kconfig symbol anymore.

@ulfalizer, do you think this would be a sound approach ?

@erwango
Copy link
Member

erwango commented Jul 4, 2019

Thinking about it more, I think we should allow Kconfig.defconfig and replace existing shields.conf by these files.
Similarly to the guideline for boards, it is up to the application to enable subsytems and this should not be done in board directly.
We should do the same in shields, so instead of having:

CONFIG_MODEM=y

We should have :

if NET
config MODEM
      def_bool y
[...]
endif # NET

This way we can keep application compatible with either a board with embedded modem or a board with a shield providing the same modem.

@mike-scott, I know we discussed this a bit earlier. How do you feel about this proposal.

@mike-scott
Copy link
Contributor

@erwango Yep, that makes sense. Perfect example is Particle Boron board (w/ embedded SARA R4 modem) vs. K64F + SparkFun modem shield.

@erwango
Copy link
Member

erwango commented Jul 25, 2019

I had a quick look on how to implement support of Kconfig.defconfig for shields.
Seems that, to support this, we're likely to revert part of #12403:

  • We'd still select shields using -DSHIELD
  • We'd have shields Kconfig symbols back (in order to be able to gate shields Kconfig.defconfig sections).

This is globally how boards are handled:

  • -DSHIELD="shields list" is provided (either by CLI, CMakelists.txt or env variable)
  • shields_dir/*/ is parsed looking for:
    • Kconfig.shield (defining shields Kconfig symbols):
      config SHIELD_MYSHIELD
    • Kconfig.defconfig (defining Kconfig symbols gated by shield Kconfig symbols:
      if SHIELD_MYSHIELD
      config OPTION
      def_bool y
      endif # SHIELD_MYSHIELD
  • Cmake parses matching shield paths looking for shields myshield_defconfig files in which shields Kconfig symbols are set:
    CONFIG_SHIELD_MYSHIELD=y
  • Kconfig does its job and we finally get the configuration specified

So, this would definitely work, but I wonder if this is the best option.
Problem is I don't see other way not hacking Kconfig or sacrificing multiple shields support.
@galak, @SebastianBoe, @ulfalizer maybe you have a better proposal?

@avisconti
Copy link
Collaborator

I had a quick look on how to implement support of Kconfig.defconfig for shields.
Seems that, to support this, we're likely to revert part of #12403:

  • We'd still select shields using -DSHIELD
  • We'd have shields Kconfig symbols back (in order to be able to gate shields Kconfig.defconfig sections).

This is globally how boards are handled:

  • -DSHIELD="shields list" is provided (either by CLI, CMakelists.txt or env variable)

  • shields_dir/*/ is parsed looking for:

    • Kconfig.shield (defining shields Kconfig symbols):
      config SHIELD_MYSHIELD
    • Kconfig.defconfig (defining Kconfig symbols gated by shield Kconfig symbols:
      if SHIELD_MYSHIELD
      config OPTION
      def_bool y
      endif # SHIELD_MYSHIELD
  • Cmake parses matching shield paths looking for shields myshield_defconfig files in which shields Kconfig symbols are set:
    CONFIG_SHIELD_MYSHIELD=y

  • Kconfig does its job and we finally get the configuration specified

So, this would definitely work, but I wonder if this is the best option.
Problem is I don't see other way not hacking Kconfig or sacrificing multiple shields support.
@galak, @SebastianBoe, @ulfalizer maybe you have a better proposal?

Let assume the case where the shield needs to specify in the dts other things other than the peripherals connected to the expansion connector. For example a microphone shield may need to enable a specific I2S controller on the mother board. Is the overlay file enough for that?

@erwango
Copy link
Member

erwango commented Jul 26, 2019

@avisconti

Let assume the case where the shield needs to specify in the dts other things other than the peripherals connected to the expansion connector. For example a microphone shield may need to enable a specific I2S controller on the mother board. Is the overlay file enough for that?

If this is dts then this is not related to current issue (which relates only to shields Kconfig). Can you raise a new issue to discuss this?

@galak
Copy link
Collaborator

galak commented Sep 12, 2019

Taked to @ulfalizer on this and think we could do something like:

config SHIELD_WNC_M14A2A
   def_bool "$(SHIELD)" = "wnc_m14a2a"

or

config SHIELD_WNC_M14A2A
   bool
   default "$(SHIELD)" = "wnc_m14a2a"

And have SHIELD pass into kconfig from the build system.

@galak
Copy link
Collaborator

galak commented Sep 12, 2019

Need to update https://github.com/zephyrproject-rtos/zephyr/blob/master/cmake/kconfig.cmake#L60 to pass SHIELD=... to kconfig like how ARCH, BOARD_DIR, SOC_DIR, etc are done.

@erwango
Copy link
Member

erwango commented Nov 19, 2019

@ulfalizer: if looking at previous example from @galak, anyway we can check if ""wnc_m14a2a" is part of a ${SHIELD_LIST} list ?

@ulfalizer
Copy link
Collaborator

ulfalizer commented Nov 20, 2019

@erwango
No Kconfig-only way to check for a substring (if that's what's needed here), but it could be solved by adding a function in scripts/kconfig/kconfigfunctions.py.

Could look something like this:

def contains(kconf, _, sub, s):
    """
    Returns "y" if 'sub' appears in 's' after split()ing it, and "n" otherwise.
    """
    return "y" if sub in s.split() else "n"


functions = {
    "contains": (contains, 2, 2),
    ...
}

Then it could be used like

config SHIELD_WNC_M14A2A
   def_bool $(contains,wnc_m14a2a,$(SHIELD_LIST))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Shields Shields (add-on boards) Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants