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: net: Update overlay-bt.conf in various samples #14674

Merged
merged 1 commit into from
Mar 20, 2019

Conversation

rlubos
Copy link
Contributor

@rlubos rlubos commented Mar 19, 2019

CONFIG_NET_L2_BT no longer forces required BT configuration, but
depends on a user to set a valid configuration instead. Hence, we
need to select a proper configuration in the overlay config file.

Signed-off-by: Robert Lubos [email protected]

`CONFIG_NET_L2_BT` no longer forces required BT configuration, but
depends on a user to set a valid configuration instead. Hence, we
need to select a proper configuration in the overlay config file.

Signed-off-by: Robert Lubos <[email protected]>
@rlubos rlubos requested review from jukkar and mike-scott March 19, 2019 13:04
@jukkar jukkar requested a review from Vudentz March 19, 2019 13:05
@jukkar jukkar added bug The issue is a bug, or the PR is fixing a bug area: Networking labels Mar 19, 2019
@jukkar jukkar added this to the v1.14.0 milestone Mar 19, 2019
Copy link
Contributor

@Vudentz Vudentz left a comment

Choose a reason for hiding this comment

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

LGTM

@pfalcon
Copy link
Contributor

pfalcon commented Mar 19, 2019

CONFIG_NET_L2_BT no longer forces required BT configuration, but
depends on a user to set a valid configuration instead.

I wonder why that happened, looks rather verbose. IMHO, good enough defaults should have been used instead.

@galak galak added the area: Samples Samples label Mar 19, 2019
@mike-scott
Copy link
Contributor

@rlubos Thank you for submitting this fix. I hit this yesterday and had just begun debugging it. This looks good from a bugfix point of view.

I was wondering 2 things (general question):

  1. How does a Kconfig change go in which breaks 4 samples?
  2. Seems like Zephyr has a problem enabling configurations in a consistent manner. See Expanding "shield" functionality (example uses WNC-M14A2A and SARA modem) #14057 (comment) for more discussion, but it's reflected here where we now have 4 identical overlay-bt.conf files which need to be maintained. Then we have sort of the wild west of per board Kconfig.defconfig conditional settings (where if xxx config is enabled we set some defaults). Each board choosing how they want to setup various defaults and under what circumstances. Also complicating matters, is we are moving away from using "select" in Kconfig to "depends", rather than looking at how "imply" would solve the same issues w/o forcing broken defaults.

For 1.15 we should look at how/why and where we are setting up default configurations and possibly setup some kind of boads/common-overlays/* if we feel that's the right solution.

Note, that I didn't even bring up the complication of SHIELD configurations .. we probably need to solve the base Zephyr issues first.

@pfalcon
Copy link
Contributor

pfalcon commented Mar 20, 2019

For 1.15 we should look at how/why and where we are setting up default configurations and possibly setup some kind of boads/common-overlays/* if we feel that's the right solution.

Reading thru all this, I'd say we should have a good criteria what can go into board config/overlays and what shouldn't. E.g., the changes in this PR fine-tune BT config, and BT is in no way board-specific, so should not appear in board common configs. Of course, there's going to be (unfortunate) exceptions. (E.g. if one board doesn't/can't implement some generic features, and those need to be overridden just for it.)

@jukkar
Copy link
Member

jukkar commented Mar 20, 2019

We should have a way to have "global" config overlays. For example in qemu_x86 it would be nice to use overlay-e1000.conf when compiling zephyr. This overlay is found only in couple of samples which is quite annoying actually. The same overlay-e1000.conf could be used in all the samples as it only contains generic options.

@pfalcon
Copy link
Contributor

pfalcon commented Mar 20, 2019

We should have a way to have "global" config overlays.

One way to achieve that would be add specially defined options (in special Kconfig.* files) which would select needed other options. E.g. CONFIG_ADHOC_GIMME_E1000 (the whole point is that they should be named distinctly and clearly).

@jukkar
Copy link
Member

jukkar commented Mar 20, 2019

CONFIG_ADHOC_GIMME_E1000

I did not meant that. In order to select that symbol you would need to have this set in your overlay file anyway.
I was thinking something like what we do right now so

cmake .. -DBOARD=qemu_x86 -DOVERLAY_CONFIG=overlay-e1000.conf

but if the overlay-e1000.conf would not be found in local sample directory, a global directory would be searched and the overlay would be used if found there.

@pfalcon
Copy link
Contributor

pfalcon commented Mar 20, 2019

Yep, that would sound good, though probably would take more changes to do.

@jukkar
Copy link
Member

jukkar commented Mar 20, 2019

Created enhancement #14740 for the overlay issue.

@mike-scott
Copy link
Contributor

the changes in this PR fine-tune BT config, and BT is in no way board-specific, so should not appear in board common configs. Of course, there's going to be (unfortunate) exceptions

@pfalcon my idea was identical to @jukkar .. I confused the issue by mentioning boards/common-overlay dir, but common overlay dir could be anywhere.

@galak galak merged commit e39aed5 into zephyrproject-rtos:master Mar 20, 2019
@rlubos rlubos deleted the fix-bt-overlay branch October 8, 2019 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking area: Samples Samples bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants