bluetooth: Fix various plm issues #76175

sean-madigan · 2024-07-22T15:04:06Z

callback_list was renamed to conn_cbs in commit 3eb975d. However plm callback was missed due to it not being built anywhere.
Path loss monitoring was not being enabled anywhere, meaning that issues could be merged in without catching the issue. This commit adds a test case to build the shell with path loss monitoring enabled to catch issues.
Path loss monitoring does not work without LE Power Control also enabled in the controller, so update the dependencies in the kconfigs to reflect this.

cvinayak · 2024-07-23T00:30:25Z

subsys/bluetooth/controller/Kconfig

@@ -560,6 +560,7 @@ config BT_CTLR_LE_POWER_CONTROL
 	bool "LE Power Control Request Feature"
 	depends on BT_CTLR_LE_POWER_CONTROL_SUPPORT
 	default y if BT_TRANSMIT_POWER_CONTROL
+	default y if BT_PATH_LOSS_MONITORING


You should instead have depends on BT_CTLR_LE_PATH_LOSS_MONITORING_SUPPORT && BT_CTLR_LE_POWER_CONTROL in config BT_CTLR_LE_PATH_LOSS_MONITORING, right?

Yeah that is probably good to have as well. Worth having this default y if BT_PATH_LOSS_MONITORING here though, otherwise it just silently doesn't work.

otherwise it just silently doesn't work.

The Kconfig depends on should give build errors or warnings

Yeah that's what I thought but without this change if I just enable CONFIG_BT_PATH_LOSS_MONITORING=y then I get no warning about the missing dependency for BT_CTLR_LE_PATH_LOSS_MONITORING. However, if I just try and enable CONFIG_BT_CTLR_LE_PATH_LOSS_MONITORING=y directly, then I do get the warning.

I presume this is for cases where CONFIG_BT_CTLR=n to not gives loads of warnings?

No, not enabled in the .config

Sounds strange that if it doesn't get enabled but it set to =y in your config file and that you don't get any warnings. How are you building?

With these changes cherry picked to NCS, and default y if BT_PATH_LOSS_MONITORING removed, build the zephyr shell with west build -b nrf52840dk/nrf52840 -p -- -DCONFIG_BT_PATH_LOSS_MONITORING=y and can see CONFIG_BT_PATH_LOSS_MONITORING=y in the .config file but not CONFIG_BT_CTLR_LE_PATH_LOSS_MONITORING=y. With no warning printed.

If I do west build -b nrf52840dk/nrf52840 -p -- -DCONFIG_BT_PATH_LOSS_MONITORING=y -DCONFIG_BT_CTLR_LE_PATH_LOSS_MONITORING=y then I get the warning.

Weird. There really should be a warning if you request a Kconfig value that isn't applied. Sounds like a bug somewhere, and shouldn't be fixed by this approach IMO

Looks like we use select elsewhere in the file, so have updated to do this

cvinayak · 2024-07-23T00:32:18Z

tests/bluetooth/shell/testcase.yaml

@@ -29,6 +29,13 @@ tests:
    platform_allow:
      - native_posix
    build_only: true
+  bluetooth.shell.path_loss_monitoring:
+    extra_configs:
+      - CONFIG_BT_PATH_LOSS_MONITORING=y


Suggested change

- CONFIG_BT_PATH_LOSS_MONITORING=y

- CONFIG_BT_TRANSMIT_POWER_CONTROL=y

- CONFIG_BT_PATH_LOSS_MONITORING=y

Did you forget this?

What is the reason for this? Do you mean I should combine with the testcase above?

i thought we were against select BT_CTLR_LE_POWER_CONTROL in favour of depends on.

Refer to: #70251 (comment)

In summary, here are some recommended practices for select:
Avoid selecting symbols with prompts or dependencies. Prefer depends on. If depends on causes annoying bloat in configuration files, consider adding a Kconfig default for the most common value.

@tejlmand FYI

As discussed here, depends on does not have the desired effect - I presume this is why select has been used on other Kconfigs in the file

As discussed here, depends on does not have the desired effect - I presume this is why select has been used on other Kconfigs in the file

"desired effect" has not been a criteria in the "recommended practices", adding new Kconfig default is another recommended practice (does add more Kconfig... but...) and hence will like @tejlmand to provide his comments.

Thalley · 2024-07-29T06:28:05Z

subsys/bluetooth/controller/Kconfig

@@ -560,6 +560,7 @@ config BT_CTLR_LE_POWER_CONTROL
 	bool "LE Power Control Request Feature"
 	depends on BT_CTLR_LE_POWER_CONTROL_SUPPORT
 	default y if BT_TRANSMIT_POWER_CONTROL
+	default y if BT_PATH_LOSS_MONITORING


otherwise it just silently doesn't work.

The Kconfig depends on should give build errors or warnings

subsys/bluetooth/controller/Kconfig

cvinayak · 2024-08-02T07:00:11Z

tests/bluetooth/shell/testcase.yaml

@@ -29,6 +29,13 @@ tests:
    platform_allow:
      - native_posix
    build_only: true
+  bluetooth.shell.path_loss_monitoring:
+    extra_configs:
+      - CONFIG_BT_PATH_LOSS_MONITORING=y


i thought we were against select BT_CTLR_LE_POWER_CONTROL in favour of depends on.

Refer to: #70251 (comment)

In summary, here are some recommended practices for select:
Avoid selecting symbols with prompts or dependencies. Prefer depends on. If depends on causes annoying bloat in configuration files, consider adding a Kconfig default for the most common value.

@tejlmand FYI

As, I myself had used select of Kconfigs with prompt to have dependencies enabled. I am on the side with select being allowed on Kconfigs with prompt, these Kconfig with prompt are optional features when standalone but mandatory when dependee is enabled.

callback_list was renamed to conn_cbs in commit 3eb975d. However plm callback was missed due to it not being built anywhere. Signed-off-by: Sean Madigan <[email protected]>

Path loss monitoring was not being enabled anywhere, meaning that issues could be merged in without catching the issue. This commit adds a test case to build the shell with path loss monitoring enabled to catch issues. Signed-off-by: Sean Madigan <[email protected]>

Path loss monitoring does not work without LE Power Control also enabled in the controller, so update the dependencies in the kconfigs so path loss monitoring selects power control. Signed-off-by: Sean Madigan <[email protected]>

sean-madigan · 2024-08-16T12:38:03Z

Where are we up to with this PR @cvinayak? Does it require any changes do you think?

Thalley · 2024-08-16T13:28:55Z

Where are we up to with this PR @cvinayak? Does it require any changes do you think?

Ideally it would be refactored to use depends on rather than select, but I'll leave it up to @cvinayak

cvinayak · 2024-08-16T14:18:02Z

Where are we up to with this PR @cvinayak? Does it require any changes do you think?

We have not got any comments from @tejlmand, but approving for now as I am ok with use of select for this Kconfig option.

callback_list was renamed to conn_cbs in commit 3eb975d. However plm callback was missed due to it not being built anywhere. Signed-off-by: Sean Madigan <[email protected]> (cherry picked from commit 2e7c488) Upstream PR: zephyrproject-rtos/zephyr#76175 Signed-off-by: Sean Madigan <[email protected]>

…with plm Path loss monitoring was not being enabled anywhere, meaning that issues could be merged in without catching the issue. This commit adds a test case to build the shell with path loss monitoring enabled to catch issues. Signed-off-by: Sean Madigan <[email protected]> (cherry picked from commit 022d339) Upstream PR: zephyrproject-rtos/zephyr#76175 Signed-off-by: Sean Madigan <[email protected]>

…or plm Path loss monitoring does not work without LE Power Control also enabled in the controller, so update the dependencies in the kconfigs so path loss monitoring selects power control. Signed-off-by: Sean Madigan <[email protected]> (cherry picked from commit 2a46ede) Upstream PR: zephyrproject-rtos/zephyr#76175 Signed-off-by: Sean Madigan <[email protected]>

callback_list was renamed to conn_cbs in commit 3eb975d. However plm callback was missed due to it not being built anywhere. Signed-off-by: Sean Madigan <[email protected]> (cherry picked from commit 2e7c488) Upstream PR: zephyrproject-rtos/zephyr#76175 Signed-off-by: Sean Madigan <[email protected]>

…with plm Path loss monitoring was not being enabled anywhere, meaning that issues could be merged in without catching the issue. This commit adds a test case to build the shell with path loss monitoring enabled to catch issues. Signed-off-by: Sean Madigan <[email protected]> (cherry picked from commit 022d339) Upstream PR: zephyrproject-rtos/zephyr#76175 Signed-off-by: Sean Madigan <[email protected]>

…or plm Path loss monitoring does not work without LE Power Control also enabled in the controller, so update the dependencies in the kconfigs so path loss monitoring selects power control. Signed-off-by: Sean Madigan <[email protected]> (cherry picked from commit 2a46ede) Upstream PR: zephyrproject-rtos/zephyr#76175 Signed-off-by: Sean Madigan <[email protected]>

sean-madigan changed the title ~~Fix various plm issues~~ bluwtooth: Fix various plm issues Jul 22, 2024

sean-madigan changed the title ~~bluwtooth: Fix various plm issues~~ bluetooth: Fix various plm issues Jul 22, 2024

sean-madigan force-pushed the fix_plm_issues branch from e0706cb to b50098a Compare July 22, 2024 15:54

sean-madigan marked this pull request as ready for review July 22, 2024 15:58

zephyrbot added area: Bluetooth Controller area: Bluetooth area: Bluetooth Host Bluetooth Host (excluding BR/EDR) labels Jul 22, 2024

zephyrbot requested review from alwa-nordic, carlescufi, cvinayak, erbr-ot, hermabe, jhedberg, jori-nordic, kruithofa, mtpr-ot, sjanc, Thalley, theob-pro, thoh-ot, Vudentz and wopu-ot July 22, 2024 15:59

zephyrbot assigned cvinayak Jul 22, 2024

cvinayak requested changes Jul 23, 2024

View reviewed changes

jori-nordic previously approved these changes Jul 23, 2024

View reviewed changes

sean-madigan dismissed jori-nordic’s stale review via 8dbddf9 July 23, 2024 08:24

sean-madigan force-pushed the fix_plm_issues branch from b50098a to 8dbddf9 Compare July 23, 2024 08:24

Thalley reviewed Jul 29, 2024

View reviewed changes

sean-madigan force-pushed the fix_plm_issues branch 2 times, most recently from 69489cb to 4a260bf Compare July 30, 2024 09:26

Thalley requested a review from cvinayak July 30, 2024 13:25

cvinayak previously requested changes Aug 2, 2024

View reviewed changes

bluetooth: host: conn: Fix renamed callback_list

40913c1

callback_list was renamed to conn_cbs in commit 3eb975d. However plm callback was missed due to it not being built anywhere. Signed-off-by: Sean Madigan <[email protected]>

sean-madigan force-pushed the fix_plm_issues branch from 4a260bf to 54496e5 Compare August 6, 2024 08:03

sean-madigan added 2 commits August 6, 2024 11:49

sean-madigan force-pushed the fix_plm_issues branch from 54496e5 to 3b13880 Compare August 6, 2024 10:49

cvinayak approved these changes Aug 16, 2024

View reviewed changes

Thalley approved these changes Aug 16, 2024

View reviewed changes

nashif merged commit 2a46ede into zephyrproject-rtos:main Aug 17, 2024
25 checks passed

	- CONFIG_BT_PATH_LOSS_MONITORING=y
	- CONFIG_BT_TRANSMIT_POWER_CONTROL=y
	- CONFIG_BT_PATH_LOSS_MONITORING=y

bluetooth: Fix various plm issues #76175

bluetooth: Fix various plm issues #76175

Uh oh!

Conversation

sean-madigan commented Jul 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sean-madigan commented Aug 16, 2024

Uh oh!

Thalley commented Aug 16, 2024

Uh oh!

cvinayak commented Aug 16, 2024

Uh oh!

Uh oh!

Uh oh!