kconfig: BT: Default to using BT_CTLR when BT #9499

SebastianBoe · 2018-08-17T10:44:44Z

As reported in issue #9494, BT samples are using the off-chip
UART-based BT controller on 52810. But the on-chip BT controller is
supported on 52810, so this is the reasonable default when BT is
enabled.

This patch enables BT_CTLR by default when BT is enabled.

This resolves #9494.

Signed-off-by: Sebastian Bøe [email protected]

ulfalizer

This will enable BT_CTLR even if BT isn't enabled, because the default that gets added to BT_CTLR symbol after dependency propagation is default y if BOARD_NRF52810_PCA10040.

That should be fine as long as no code makes decisions based on BT_CTLR without looking at BT first though.

If it's not fine, an if BT could be added. This aspect of Kconfig.defconfig files is a bit wonky. They work more like an OR than an AND.

ulfalizer · 2018-08-17T11:19:43Z

boards/arm/nrf52810_pca10040/Kconfig

@@ -11,4 +11,12 @@ config BOARD_ENABLE_DCDC
        select SOC_DCDC_NRF52X
        default y

+if BT


A space after an if and before an endif looks less cramped, imo.

SebastianBoe · 2018-08-17T11:20:41Z

This will enable BT_CTLR even if BT isn't enabled
If it's not fine, an if BT could be added.

That is not fine, added an if. Thank you for catching this.

I wonder what the cleanest solution here is.

I am not familiar with BT, but if there are multiple implementations of the controller available, e.g. off-chip, or on-chip. And it only makes sense to enable one of them, then I wonder if a choice statement and maybe different defaults depending on the board would be the cleanest solution.

ulfalizer · 2018-08-17T11:20:59Z

Looks like there's also a dependency on BT_HCI on the base definition of BT_CTLR. Don't know if that matters here.

ulfalizer · 2018-08-17T11:23:45Z

That is not fine, added an if. Thank you for catching this.

I wonder what the cleanest solution here is.

I am not familiar with BT, but if there are multiple implementations of the controller available, e.g. off-chip, or on-chip. And it only makes sense to enable one of them, then I wonder if a choice statement and maybe different defaults depending on the board would be the cleanest solution.

If there's only two options, a plain bool is the cleanest solution (Use on-chip Bluetooth controller or the like). Otherwise, a choice might make sense.

SebastianBoe · 2018-08-17T11:24:38Z

If there's only two options

I believe there are several, but a BT expert would have to chime in.

ulfalizer · 2018-08-17T11:26:10Z

boards/arm/nrf52810_pca10040/Kconfig

+# BT_CTLR depends on BT. When BT is enabled we should default to also
+# enabling the controller.
+config BT_CTLR
+	default y


Could also do default y if BT by the way, which saves some lines. No super strong opinions though.

agree, no need of if BT, use default y if BT.

cvinayak · 2018-08-17T11:32:39Z

boards/arm/nrf52810_pca10040/Kconfig

+# BT_CTLR depends on BT. When BT is enabled we should default to also
+# enabling the controller.
+config BT_CTLR
+	default y


agree, no need of if BT, use default y if BT.

As reported in issue zephyrproject-rtos#9494, BT samples are using the off-chip UART-based BT controller on 52810. But the on-chip BT controller is supported on 52810, so this is the reasonable default when BT is enabled. This patch enables BT_CTLR by default when BT is enabled. This resolves zephyrproject-rtos#9494. Signed-off-by: Sebastian Bøe <[email protected]>

SebastianBoe · 2018-08-17T11:42:39Z

@ulfalizer : While we are on the topic.

What is the cleanest way to have boards affect the defaults of the kernel?

E.g. how should a board change the default choice in a Kconfig like this?

config FEATURE
       bool

if FEATURE

choice
	prompt "Feature implementation selection"
	default FEATURE_IMPLEMENTATION_2

config FEATURE_IMPLEMENTATION_1
       bool

config FEATURE_IMPLEMENTATION_2
       bool
       
endchoice

endif # FEATURE

What I have done in this patch is not clean, as you need to duplicate the dependency information of the original config.

ulfalizer · 2018-08-17T12:01:31Z

E.g. how should a board change the default choice in a Kconfig like this?

The choice would have to have a name:

config FEATURE
	bool

if FEATURE

choice FEATURE_IMPLEMENTATION
	prompt "Feature implementation selection"
	default FEATURE_IMPLEMENTATION_2

config FEATURE_IMPLEMENTATION_1
	...

config FEATURE_IMPLEMENTATION_2
	...
       
endchoice

endif # FEATURE

The default could then be changed with an additional definition in a Kconfig.defconfig file:

choice FEATURE_IMPLEMENTATION
	default FEATURE_IMPLEMENTATION_1
endchoice

(In this case, there's no need to repeat the dependency information, because the choice symbols won't be visible unless FEATURE is enabled.)

What I have done in this patch is not clean, as you need to duplicate the dependency information of the original config.

Yeah, that's a drawback of Kconfig.defconfig files in general I think. The extra symbol definitions can't "limit" the base definition of the symbol/choice, only extend it. That means you might have to duplicate a bunch of dependency information.

For symbols that you know will be visible (that have satisfied dependencies), it's cleaner to set them in a *_defconfig file.

Setting a only potentially visible symbol (a symbol with that has a prompt that but might not be visible by default) in a *_defconfig file would also work as a way of giving it a default whenever it becomes visible. That would trigger the was-assigned-the-value-x-got-the-value-y warning if the symbol isn't visible though, so it gets a bit hairy. That warning is useful in other cases.

(Setting a value in a *_defconfig/.config file sets the "user value" of the symbol, which is another input when the symbol value is calculated.)

codecov-io · 2018-08-17T12:14:52Z

Codecov Report

Merging #9499 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9499   +/-   ##
=======================================
  Coverage   52.22%   52.22%           
=======================================
  Files         212      212           
  Lines       25948    25948           
  Branches     5577     5577           
=======================================
  Hits        13551    13551           
  Misses      10140    10140           
  Partials     2257     2257

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaaf20e...d98c2f0. Read the comment docs.

ulfalizer · 2018-08-17T12:22:35Z

Knowing how dependency propagation works might clarify why it works like that.

Say you have a symbol with two definitions:

if DEP_1

config SYM
	bool "sym"
	default DEF_1 if DEP_2
	default DEF_2
	select SEL_1

endif

...

menu "menu"
	depends on DEP_3

config SYM
	default DEF_3
	select SEL_2

endmenu

(I just made it a menu instead of an if in the second example to show that it works the same way.)

Those definitions are logically equivalent to the following definitions (this is what happens internally as well):

config SYM
	bool "sym" if DEP_1  (prompt only visible if DEP_1 is enabled)
	default DEF_1 if DEP_2 && DEP_1
	default DEF_2 if DEP_1
	select SEL_1 if DEP_1

config SYM
	default DEF_3 if DEP_3
	select SEL_2 if DEP_3

Symbols with multiple definitions just get all the properties from the different definition locations, so the symbol finally ends up like this:

config SYM
	bool "sym" if DEP_1
	default DEF_1 if DEP_2 && DEP_1
	default DEF_2 if DEP_1
	default DEF_3 if DEP_3
	select SEL_1 if DEP_1
	select SEL_2 if DEP_3

Note that the properties from the second definition did not get a dependency on DEP_1. They're fully independent.

SebastianBoe · 2018-08-17T12:23:34Z

Thank you for the explanation.

(In this case, there's no need to repeat the dependency information, because the choice symbols won't be visible unless FEATURE is enabled.)

I don't understand the difference between the situation with FEATURE and the situation with BT_CTLR. Is it because one uses 'choice' symbols, and the other uses normal symbols? Because BT_CTLR is also invisible unless BT is enabled.

ulfalizer · 2018-08-17T12:30:04Z

I don't understand the difference between the situation with FEATURE and the situation with BT_CTLR. Is it because one uses 'choice' symbols, and the other uses normal symbols? Because BT_CTLR is also invisible unless BT is enabled.

Yeah, it's just a subtlety specific to choices.

When FEATURE isn't enabled, the choice won't be visible (you get prompt "Feature implementation selection" if FEATURE after dependency propagation, and FEATURE is n). Choices that aren't visible ignore their defaults, so it won't matter that the default FEATURE_IMPLEMENTATION_1 property has a satisfied condition.

Choices are a bit wonky in general. Lots of complexity around them...

SebastianBoe · 2018-08-17T12:31:35Z

Never mind, I understood it now, your explanation was great.

(In this case, there's no need to repeat the dependency information, because the choice symbols won't be visible unless FEATURE is enabled.)

SebastianBoe · 2018-08-17T13:17:20Z

@cvinayak : Review comments have been addressed. Please re-evaluate the review.

ioannisg

Looks OK, just minor suggestions from me.

ioannisg · 2018-08-17T13:39:45Z

boards/arm/nrf52810_pca10040/Kconfig

+# BT_CTLR depends on BT. When BT is enabled we should default to also
+# enabling the controller.
+config BT_CTLR
+	default y if BT


Now this looks better, @SebastianBoe (with the dependence on BT)

ioannisg · 2018-08-17T13:44:25Z

boards/arm/nrf52810_pca10040/Kconfig

@@ -11,4 +11,9 @@ config BOARD_ENABLE_DCDC
        select SOC_DCDC_NRF52X
        default y

+# BT_CTLR depends on BT. When BT is enabled we should default to also


remove "should"

addressed

cvinayak

LGTM

SebastianBoe requested review from ulfalizer, ioannisg and cvinayak August 17, 2018 10:44

SebastianBoe mentioned this pull request Aug 17, 2018

Nordic nrf52810_pca10040 is missing default bluetooth configuration options #9494

Closed

ioannisg requested a review from carlescufi August 17, 2018 10:53

ulfalizer approved these changes Aug 17, 2018

View reviewed changes

SebastianBoe force-pushed the fix_bt_samples_on_nrf_52810 branch from 44ad8ce to 6f0cb5b Compare August 17, 2018 11:17

ulfalizer reviewed Aug 17, 2018

View reviewed changes

SebastianBoe force-pushed the fix_bt_samples_on_nrf_52810 branch from 6f0cb5b to bf91268 Compare August 17, 2018 11:21

ulfalizer reviewed Aug 17, 2018

View reviewed changes

cvinayak previously requested changes Aug 17, 2018

View reviewed changes

SebastianBoe force-pushed the fix_bt_samples_on_nrf_52810 branch from bf91268 to d98c2f0 Compare August 17, 2018 11:35

ioannisg approved these changes Aug 17, 2018

View reviewed changes

cvinayak approved these changes Aug 17, 2018

View reviewed changes

nashif merged commit 9e18b4f into zephyrproject-rtos:master Aug 17, 2018

SebastianBoe deleted the fix_bt_samples_on_nrf_52810 branch August 20, 2018 08:07

SebastianBoe mentioned this pull request Sep 4, 2018

[RFC][DNM] Porting LoRaMAC-node project to zephyr #5764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kconfig: BT: Default to using BT_CTLR when BT #9499

kconfig: BT: Default to using BT_CTLR when BT #9499

SebastianBoe commented Aug 17, 2018

ulfalizer left a comment •

edited

Loading

ulfalizer Aug 17, 2018

SebastianBoe commented Aug 17, 2018

ulfalizer commented Aug 17, 2018

ulfalizer commented Aug 17, 2018

SebastianBoe commented Aug 17, 2018 •

edited

Loading

ulfalizer Aug 17, 2018

cvinayak Aug 17, 2018

SebastianBoe Aug 17, 2018

cvinayak Aug 17, 2018

SebastianBoe commented Aug 17, 2018

ulfalizer commented Aug 17, 2018 •

edited

Loading

codecov-io commented Aug 17, 2018

ulfalizer commented Aug 17, 2018 •

edited

Loading

SebastianBoe commented Aug 17, 2018

ulfalizer commented Aug 17, 2018 •

edited

Loading

SebastianBoe commented Aug 17, 2018

SebastianBoe commented Aug 17, 2018

ioannisg left a comment

ioannisg Aug 17, 2018

ioannisg Aug 17, 2018

cvinayak left a comment

kconfig: BT: Default to using BT_CTLR when BT #9499

kconfig: BT: Default to using BT_CTLR when BT #9499

Conversation

SebastianBoe commented Aug 17, 2018

ulfalizer left a comment • edited Loading

Choose a reason for hiding this comment

ulfalizer Aug 17, 2018

Choose a reason for hiding this comment

SebastianBoe commented Aug 17, 2018

ulfalizer commented Aug 17, 2018

ulfalizer commented Aug 17, 2018

SebastianBoe commented Aug 17, 2018 • edited Loading

ulfalizer Aug 17, 2018

Choose a reason for hiding this comment

cvinayak Aug 17, 2018

Choose a reason for hiding this comment

SebastianBoe Aug 17, 2018

Choose a reason for hiding this comment

cvinayak Aug 17, 2018

Choose a reason for hiding this comment

SebastianBoe commented Aug 17, 2018

ulfalizer commented Aug 17, 2018 • edited Loading

codecov-io commented Aug 17, 2018

Codecov Report

ulfalizer commented Aug 17, 2018 • edited Loading

SebastianBoe commented Aug 17, 2018

ulfalizer commented Aug 17, 2018 • edited Loading

SebastianBoe commented Aug 17, 2018

SebastianBoe commented Aug 17, 2018

ioannisg left a comment

Choose a reason for hiding this comment

ioannisg Aug 17, 2018

Choose a reason for hiding this comment

ioannisg Aug 17, 2018

Choose a reason for hiding this comment

cvinayak left a comment

Choose a reason for hiding this comment

ulfalizer left a comment •

edited

Loading

SebastianBoe commented Aug 17, 2018 •

edited

Loading

ulfalizer commented Aug 17, 2018 •

edited

Loading

ulfalizer commented Aug 17, 2018 •

edited

Loading

ulfalizer commented Aug 17, 2018 •

edited

Loading