Bluetooth: Controller: Only show one prompt for experimental configs #42319

rugeGerritsen · 2022-01-31T14:50:50Z

When browsing through the tree, you will now no longer see two entries
for each symbol that is experimental for BT_LL_SW_SPLIT

The prompt message for experimental messages have now been rephrased to
include (Split Link Layer)

See

subsys/bluetooth/controller/Kconfig

alwa-nordic

Removing the label "[EXPERIMENTAL]" from the feature name is fine in my opinion. I agree it's better than showing it twice in search results.

alwa-nordic · 2022-02-01T10:14:44Z

Adding label area DX to discuss:

The symbol appearing twice in search results, one with "[EXPERIMENTAL]" and one without is confusing to developers.
Arguably, it is the combination of two symbols that is "[EXPERIMENTAL]" here. Eg. BT_LL_SW_SPLIT (an implementation selection) and BT_CTLR_ADV_EXT (a feature). Is this well communicated to the developer by conditionally adding "[EXPERIMENTAL]" to the prompt of the feature symbol?

cvinayak · 2022-02-01T11:01:26Z

subsys/bluetooth/controller/Kconfig

@@ -470,20 +470,17 @@ config BT_CTLR_CHAN_SEL_2
 	  the Controller.

 config BT_CTLR_ADV_EXT
-	bool "LE Advertising Extensions" if !BT_LL_SW_SPLIT
+	bool "LE Advertising Extensions"


Decoupling out the prompt solves the duplicate menu item.

Suggested change

bool "LE Advertising Extensions"

prompt "LE Advertising Extensions [EXPERIMENTAL]" if BT_LL_SW_SPLIT

config BT_CTLR_ADV_EXT

prompt "LE Advertising Extensions" if !BT_LL_SW_SPLIT

config BT_CTLR_ADV_EXT

bool

Wouldn't this still result in multiple entries of BT_CTLR_ADV_EXT when you search for it?

Not sure about the search, but it will definitely break menuconfig. The rendering logic that produces the current duplicate entry thing in menuconfig is caused by menuconfig forcing menus with visible children into existence, even if they should be invisible. As the menu parent is always the last node before the if, the [EXPERIMENTAL] node is forced into existence, as it holds all the child nodes.

IMO, the right variant of this suggestion would be to keep the two entries, but remove the prompts for both, then create a single prompt entry below them that conditionally adds both prompts:

config BT_CTLR_ADV_EXT prompt "LE Advertising Extensions [EXPERIMENTAL]" if BT_LL_SW_SPLIT prompt "LE Advertising Extensions"

This entry will only serve as a toggleable menu heading, really, but it should keep the logic of the other properties as-is.

Wouldn't this still result in multiple entries of BT_CTLR_ADV_EXT when you search for it?

There will be multiple entries when searched, but only one is enabled based on the if condition that is satisfied. This is similar to how all hidden Kconfig get listed when in search.

My suggestions do not have the original problem of duplicate entries when traversing using menuconfig/guiconfig.

the right variant of this suggestion would be to keep the two entries, but remove the prompts for both, then create a single prompt entry below them that conditionally adds both prompts

This (multiple conditional prompt under an entry) is not permitted by Kconfig and fail to build with a warning.

So @cvinayak , @alwa-nordic, and @trond-snekvik . The remaining question is: Are we ok with having two entries? It looks a bit messy IMO. But the proposed solution is better than the existing one

It is ok to have see two prompt messages, and is consistent with the Link Layer selection wherein only one is selectable based on the Link Layer enabled.

alwa-nordic · 2022-02-07T08:51:36Z

@cvinayak You NAK-ed this PR, but it's unclear to both the author and me what your objections are, and so we don't know how to proceed. Could you have a look at this again?

cvinayak · 2022-02-08T09:08:45Z

@cvinayak You NAK-ed this PR, but it's unclear to both the author and me what your objections are, and so we don't know how to proceed. Could you have a look at this again?

@alwa-nordic The title of the PR and the description do not describe the entirety of the problem. One, duplicate prompt when traversing through the Kconfig menu items, and second, display of duplicate prompt when searching. My suggestion in this PR addresses the former. The later, to have duplicate prompts when searching is acceptable and is inline with other hidden Kconfig that get listed.

tejlmand

Having both prompts appearing in the search results is unfortunate, but the side-effects of this proposal are worse, when considering UI interface of items in menuconfig and doc generation.

Unfortunately I don't have a good solution to the search issue.

tejlmand · 2022-02-09T10:50:09Z

subsys/bluetooth/controller/Kconfig

 	depends on BT_BROADCASTER && BT_CTLR_ADV_PERIODIC_SUPPORT
 	select BT_CTLR_CHAN_SEL_2
 	default y if BT_PER_ADV
+	select EXPERIMENTAL if BT_LL_SW_SPLIT


We are now missing [EXPERIMENTAL] in the prompt text.

It should be visible to users that uses menuconfig / guiconfig that a particular item is considered experimental.

Also it goes to the docset, where it becomes visible that this symbol is experimental, for example like here:
https://docs.zephyrproject.org/latest/reference/kconfig/CONFIG_BT_LL_SW_LLCP.html#config-bt-ll-sw-llcp

Note, this is also the reason why the help text is important, as it provides information to the reader as to why this feature is considered experimental.

With this proposal, it becomes very hidden that this setting under some circumstances are considered experimental, which is worse than both prompts appearing in a search.

rugeGerritsen · 2022-02-09T11:38:57Z

One proposed construct that will make it look nicer is the following:

# Alternative 1

config BT_CTLR_ADV_EXT
	bool "LE Advertising Extensions"
	depends on BT_CTLR_ADV_EXT_SUPPORT

if BT_LL_SW_SPLIT
config BT_CTLR_ADV_EXT
	prompt "LE Advertising Extensions [EXPERIMENTAL]"
	select EXPERIMENTAL
endif

This ensures that the "EXPERIMENTAL" prompt is only visible when BT_LL_SW_SPLIT is selected. When BT_LL_SW_SPLIT is selected, both the non-experimental and experimental prompt is visible. This can further be solved by adding even more if statements (Option 2), but that is messy IMO.

# Alternative 2

if !BT_LL_SW_SPLIT
config BT_CTLR_ADV_EXT
	bool "LE Advertising Extensions"
	depends on BT_CTLR_ADV_EXT_SUPPORT
endif

if BT_LL_SW_SPLIT
config BT_CTLR_ADV_EXT
	prompt "LE Advertising Extensions [EXPERIMENTAL]"
	select EXPERIMENTAL
endif

@tejlmand , @cvinayak , @Thalley , @trond-snekvik , @alwa-nordic are you ok with implementing alternative 1?

cvinayak · 2022-02-09T15:41:20Z

When BT_LL_SW_SPLIT is selected, both the non-experimental and experimental prompt is visible.

Alternative 1, does not solve anything but instead adds duplicates in the menuconfig/guiconfig contrary to the title of this PR.

Have you tried my suggestion here: #42319 (comment)
which does not have duplicates in menuconfig/guiconfig and updates correctly realtime when Link Layer choice is changed in menuconfig/guiconfig, help shows the correct dependencies values and the Kconfig file is clean.

rugeGerritsen · 2022-02-09T16:03:49Z

@cvinayak , thanks, that works out nicely. I must have missed something when I tried it the first time

cvinayak

~~With the latest changes, none of these Kconfigs are selectable in menuconfig with upstream repo.~~ Now the duplicate entry are far down the list in the upstream repo.

tejlmand · 2022-02-16T11:36:06Z

subsys/bluetooth/controller/Kconfig.ll_sw_split

@@ -62,6 +62,40 @@ config BT_LLL_VENDOR_OPENISA
 	help
 	  Use OpenISA Lower Link Layer implementation.

+# First redefine the experimental symbols.
+# We define them here due to avoid declaring them right below the non-experimental


Although I initially was in favor of moving those setting re-declarations into Kconfig.ll_sw_split, then the new behavior in menuconfig is quite confusing.

For example, when BT_CTLR_ADV_EXT is not enabled, the menu look as:

but then enabling this setting causes the menu to be updated (which is normal behavior), but in contrast to old behavior the new available settings appear above the current setting, at what to some might look at random location (although it's not).

This is a bit confusing, as it is not easy to spot the new settings (unless you know that you should look for the (NEW) postfix).

Good point. Should we rather decline this PR and resolve the Kconfig issue then?

Should we rather decline this PR and resolve the Kconfig issue then?

at this point, i'm not confident that the is a easy solution to the Kconfiglib issue.
Cause trying to solve that issue might easily break this functionality:

# Show invisible symbols if they have visible children. This # can happen for an m/y-valued symbol with an optional prompt # ('prompt "foo" is COND') that is currently disabled. Note # that it applies to both 'config' and 'menuconfig' symbols.

in which case we might end-up changing one kind of misbehaving with another kind.

However, I also posted a workaround here: ulfalizer/Kconfiglib#117
which you can try out, and see if it creates the desired result.

I tried it out, it didn't work (pushed the changes to demonstrate it)

because you place the DUMMY at the wrong location.

It needs to be moved downwards, like this:

--- a/subsys/bluetooth/controller/Kconfig +++ b/subsys/bluetooth/controller/Kconfig @@ -529,14 +529,14 @@ config BT_CTLR_ADV_PERIODIC Enable support for Bluetooth 5.0 LE Periodic Advertising in the Controller. -# Workaround for https://github.com/ulfalizer/Kconfiglib/issues/117 -config DUMMY - bool - config BT_CTLR_ADV_PERIODIC bool "LE Periodic Advertising in Advertising State (Split Link Layer) [EXPERIMENTAL]" if BT_LL_SW_SPLIT select EXPERIMENTAL if BT_LL_SW_SPLIT +# Workaround for https://github.com/ulfalizer/Kconfiglib/issues/117 +config DUMMY + bool + config BT_CTLR_ADV_PERIODIC_ADI_SUPPORT bool "Periodic Advertising ADI support" depends on BT_CTLR_ADV_PERIODIC

Note, we probably should name DUMMY something else, I only used that name here ulfalizer/Kconfiglib#117 for showing the workaround.

Secondly, I think the first DUMMY should have a help text similar to this.

config PROMPT_ADJUSTMENT_WORKAROUND bool help Unused and hidden Kconfig symbol to allow prompt adjustment. Work around for https://github.com/ulfalizer/Kconfiglib/issues/117

tejlmand

could you please checkup whether the workaround is needed in all cases ?

From what I can tell, only a few places suffers from the problem.
Please read ulfalizer/Kconfiglib#117 carefully to understand under which circumstances this problem manifests itself.

tejlmand · 2022-02-21T11:15:25Z

subsys/bluetooth/controller/Kconfig

+	help
+	  Unused and hidden Kconfig symbol to allow prompt adjustment.
+	  Work around for https://github.com/ulfalizer/Kconfiglib/issues/117


Only first occurrence should contain the help text, else this will be seen in menuconfig.

Suggested change

help

Unused and hidden Kconfig symbol to allow prompt adjustment.

Work around for https://github.com/ulfalizer/Kconfiglib/issues/117

tejlmand · 2022-02-21T11:16:01Z

subsys/bluetooth/controller/Kconfig

+	help
+	  Unused and hidden Kconfig symbol to allow prompt adjustment.
+	  Work around for https://github.com/ulfalizer/Kconfiglib/issues/117


Suggested change

help

Unused and hidden Kconfig symbol to allow prompt adjustment.

Work around for https://github.com/ulfalizer/Kconfiglib/issues/117

tejlmand · 2022-02-21T11:17:55Z

subsys/bluetooth/controller/Kconfig

+config PROMPT_ADJUSTMENT_WORKAROUND
+	bool
+	help
+	  Unused and hidden Kconfig symbol to allow prompt adjustment.
+	  Work around for https://github.com/ulfalizer/Kconfiglib/issues/117
+


I don't believe prompt adjustment is needed here, cause the following setting does not have depends on BT_CTLR_ADV_ISO or is guarded by if BT_CTLR_ADV_ISO .

Could you please if prompt adjustment is really needed here.

tejlmand · 2022-02-21T11:19:19Z

subsys/bluetooth/controller/Kconfig

+config PROMPT_ADJUSTMENT_WORKAROUND
+	bool
+	help
+	  Unused and hidden Kconfig symbol to allow prompt adjustment.
+	  Work around for https://github.com/ulfalizer/Kconfiglib/issues/117
+


Could you please if prompt adjustment is really needed here.

BT_CTLR_BROADCAST_ISO is not depending on BT_CTLR_SYNC_ISO from what I can tell.

tejlmand · 2022-02-21T11:20:24Z

subsys/bluetooth/controller/Kconfig

+config PROMPT_ADJUSTMENT_WORKAROUND
+	bool
+	help
+	  Unused and hidden Kconfig symbol to allow prompt adjustment.
+	  Work around for https://github.com/ulfalizer/Kconfiglib/issues/117
+


is prompt adjustment really needed here ?

tejlmand · 2022-02-21T11:27:52Z

subsys/bluetooth/controller/Kconfig

+config PROMPT_ADJUSTMENT_WORKAROUND
+	bool
+	help
+	  Unused and hidden Kconfig symbol to allow prompt adjustment.
+	  Work around for https://github.com/ulfalizer/Kconfiglib/issues/117
+


is prompt adjustment really needed here ?

tejlmand · 2022-02-21T11:29:13Z

subsys/bluetooth/controller/Kconfig

@@ -682,15 +712,21 @@ config BT_CTLR_SET_HOST_FEATURE
 config BT_CTLR_CENTRAL_ISO
 	bool "LE Connected Isochronous Stream Central" if !BT_LL_SW_SPLIT
 	depends on BT_CTLR_CENTRAL_ISO_SUPPORT && BT_CENTRAL
+	select BT_CTLR_SET_HOST_FEATURE


curious. what does this change has to do with ?

Only show one prompt for experimental configs

tejlmand

A little too much was remove in last push.

Please notice I only asked for removal of the help text those two places:
#42319 (comment)
#42319 (comment)

The

config PROMPT_ADJUSTMENT_WORKAROUND
	bool

must still be there.

tejlmand · 2022-02-25T21:33:21Z

subsys/bluetooth/controller/Kconfig

@@ -525,7 +531,7 @@ config BT_CTLR_ADV_PERIODIC
 	  Controller.

 config BT_CTLR_ADV_PERIODIC
-	bool "LE Periodic Advertising in Advertising State [EXPERIMENTAL]" if BT_LL_SW_SPLIT
+	bool "LE Periodic Advertising in Advertising State (Split Link Layer) [EXPERIMENTAL]" if BT_LL_SW_SPLIT


comment not directly related to this line, but cannot comment below.

Why was this removed in last update ?
https://github.com/zephyrproject-rtos/zephyr/blob/65a659e7f7e2ad76a2406f8d6db57bca99ee25b3/subsys/bluetooth/controller/Kconfig#L537-L538

Only the help text was supposed to be remove if you read this comment #42319 (comment).

This code part still suffers the double prompt issue, as there are two config BT_CTLR_ADV_PERIODIC after one another, and then a depends on BT_CTLR_ADV_PERIODIC here:
https://github.com/zephyrproject-rtos/zephyr/blob/e4e8b78f93a7871c4494bf6b2e5596e97fcf7c12/subsys/bluetooth/controller/Kconfig#L524-L539

Especially pay attention to this line:
https://github.com/zephyrproject-rtos/zephyr/blob/e4e8b78f93a7871c4494bf6b2e5596e97fcf7c12/subsys/bluetooth/controller/Kconfig#L539

So you still need

config PROMPT_ADJUSTMENT_WORKAROUND bool

just no help text.

tejlmand · 2022-02-25T21:36:36Z

subsys/bluetooth/controller/Kconfig

@@ -545,7 +551,7 @@ config BT_CTLR_SYNC_PERIODIC
 	  Synchronization state in the Controller.

 config BT_CTLR_SYNC_PERIODIC
-	bool "LE Periodic Advertising in Synchronization State [EXPERIMENTAL]" if BT_LL_SW_SPLIT
+	bool "LE Periodic Advertising in Synchronization State (Split Link Layer) [EXPERIMENTAL]" if BT_LL_SW_SPLIT


same here. not related to this line, but below.

You still need:
https://github.com/zephyrproject-rtos/zephyr/blob/65a659e7f7e2ad76a2406f8d6db57bca99ee25b3/subsys/bluetooth/controller/Kconfig#L563-L564

just without the help text.

Notice the two config BT_CTLR_SYNC_PERIODIC after one another, and then a depends on BT_CTLR_SYNC_PERIODIC here:
https://github.com/zephyrproject-rtos/zephyr/blob/e4e8b78f93a7871c4494bf6b2e5596e97fcf7c12/subsys/bluetooth/controller/Kconfig#L544-L559

Uses the proposed workaround in ulfalizer/Kconfiglib#117 Signed-off-by: Rubin Gerritsen <[email protected]>

This makes the it more intuitive when searching. Signed-off-by: Rubin Gerritsen <[email protected]>

rugeGerritsen · 2022-03-15T08:22:18Z

@tejlmand , thanks for the thorough review

tejlmand

lgtm - Thanks for the effort and patience.

This sure was a tricky issue, but the end-result looks good.

cvinayak

Approved the prompt text changes. Not reviewed/tested for the prompt workaround.

rugeGerritsen · 2022-03-22T14:38:46Z

@kruithofa , @thoh-ot , are you ok with merging this PR as is?

rugeGerritsen · 2022-03-28T08:36:23Z

I believe this PR can be merged now

rugeGerritsen · 2022-03-30T08:20:18Z

@carlescufi , can you merge this PR?

nashif · 2022-03-30T14:12:38Z

this is marked as DNM, it will not be merged with this label.

rugeGerritsen · 2022-03-30T14:15:45Z

Removed the DNM I added some time back

rugeGerritsen requested review from Thalley and cvinayak January 31, 2022 14:51

github-actions bot added area: Bluetooth area: Bluetooth Controller labels Jan 31, 2022

rugeGerritsen added RFC Request For Comments: want input from the community and removed area: Bluetooth labels Jan 31, 2022

rugeGerritsen marked this pull request as ready for review January 31, 2022 14:56

rugeGerritsen requested review from carlescufi, kruithofa and thoh-ot as code owners January 31, 2022 14:56

rugeGerritsen requested a review from alwa-nordic January 31, 2022 14:56

Thalley reviewed Jan 31, 2022

View reviewed changes

subsys/bluetooth/controller/Kconfig Outdated Show resolved Hide resolved

alwa-nordic previously approved these changes Jan 31, 2022

View reviewed changes

alwa-nordic added area: Kconfig area: DX Developer and User Experience labels Feb 1, 2022

alwa-nordic requested a review from tejlmand February 1, 2022 10:06

cvinayak requested changes Feb 1, 2022

View reviewed changes

cvinayak added area: Bluetooth bug The issue is a bug, or the PR is fixing a bug labels Feb 1, 2022

cvinayak self-assigned this Feb 1, 2022

alwa-nordic requested a review from cvinayak February 7, 2022 08:51

tejlmand requested changes Feb 9, 2022

View reviewed changes

rugeGerritsen requested a review from tejlmand February 9, 2022 11:39

rugeGerritsen force-pushed the single_controller_kconfigs branch from 3f62ee9 to 851898d Compare February 9, 2022 16:09

cvinayak requested changes Feb 16, 2022

View reviewed changes

tejlmand reviewed Feb 17, 2022

View reviewed changes

rugeGerritsen force-pushed the single_controller_kconfigs branch 3 times, most recently from fbcd008 to 65a659e Compare February 18, 2022 15:01

rugeGerritsen requested review from tejlmand and cvinayak February 18, 2022 15:01

tejlmand requested changes Feb 21, 2022

View reviewed changes

rugeGerritsen force-pushed the single_controller_kconfigs branch from 65a659e to e4e8b78 Compare February 22, 2022 15:59

rugeGerritsen requested a review from tejlmand February 22, 2022 16:00

tejlmand requested changes Feb 25, 2022

View reviewed changes

rugeGerritsen added 2 commits March 15, 2022 09:16

Bluetooth: Controller: Only show one prompt for experimental configs

8b9ff8e

Uses the proposed workaround in ulfalizer/Kconfiglib#117 Signed-off-by: Rubin Gerritsen <[email protected]>

Bluetooth: Controller: Enhance experimental SW_SPLIT Kconfigs names

3b5d83c

This makes the it more intuitive when searching. Signed-off-by: Rubin Gerritsen <[email protected]>

rugeGerritsen force-pushed the single_controller_kconfigs branch from e4e8b78 to 3b5d83c Compare March 15, 2022 08:21

rugeGerritsen requested a review from tejlmand March 15, 2022 08:22

tejlmand approved these changes Mar 15, 2022

View reviewed changes

carlescufi approved these changes Mar 21, 2022

View reviewed changes

cvinayak approved these changes Mar 21, 2022

View reviewed changes

rugeGerritsen requested a review from Thalley March 22, 2022 14:38

thoh-ot approved these changes Mar 28, 2022

View reviewed changes

kruithofa approved these changes Mar 28, 2022

View reviewed changes

alwa-nordic approved these changes Mar 28, 2022

View reviewed changes

rugeGerritsen removed the DNM This PR should not be merged (Do Not Merge) label Mar 30, 2022

carlescufi merged commit b523975 into zephyrproject-rtos:main Mar 31, 2022

	help
	Unused and hidden Kconfig symbol to allow prompt adjustment.
	Work around for https://github.com/ulfalizer/Kconfiglib/issues/117

Bluetooth: Controller: Only show one prompt for experimental configs #42319

Bluetooth: Controller: Only show one prompt for experimental configs #42319

Conversation

rugeGerritsen commented Jan 31, 2022 • edited Loading

alwa-nordic left a comment

Choose a reason for hiding this comment

alwa-nordic commented Feb 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alwa-nordic commented Feb 7, 2022 • edited Loading

cvinayak commented Feb 8, 2022

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugeGerritsen commented Feb 9, 2022

cvinayak commented Feb 9, 2022

rugeGerritsen commented Feb 9, 2022

cvinayak left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tejlmand left a comment

Choose a reason for hiding this comment

tejlmand Feb 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rugeGerritsen commented Mar 15, 2022

tejlmand left a comment

Choose a reason for hiding this comment

cvinayak left a comment

Choose a reason for hiding this comment

rugeGerritsen commented Mar 22, 2022

rugeGerritsen commented Mar 28, 2022

rugeGerritsen commented Mar 30, 2022

nashif commented Mar 30, 2022

rugeGerritsen commented Mar 30, 2022

rugeGerritsen commented Jan 31, 2022 •

edited

Loading

alwa-nordic commented Feb 7, 2022 •

edited

Loading

cvinayak left a comment •

edited

Loading

tejlmand Feb 25, 2022 •

edited

Loading