Bluetooth: Controller: Fix incorrect assertion check

[MeisterBob]

Definition of adv and connectable variables where guarded by an ifdef and then used without a guard leading to build errors when CONFIG_ASSERT was disabled and CONFIG_BT_PERIPHERAL enabled.

fixes #86855

[cvinayak]

Thank you for the pull request. Changes requested.

Also, update commit title to Bluetooth: Controller: Fix incorrect assertion check

[MeisterBob]

I removed the assertion ifdef

[MeisterBob]

Apparently LL_ASSERT becomes an empty macro when asserts are disabled and connectable becomes an unused variable. I can add __maybe_unused to the connectable or guard that whole block with defined(CONFIG_BT_ASSERT) || defined(CONFIG_ASSERT). Which do you prefer @cvinayak

[cvinayak]

Apparently LL_ASSERT becomes an empty macro when asserts are disabled and connectable becomes an unused variable. I can add __maybe_unused to the connectable or guard that whole block with defined(CONFIG_BT_ASSERT) || defined(CONFIG_ASSERT). Which do you prefer @cvinayak

Something went wrong again, LL_ASSERT should not become empty macro, this is essential. I will have to bisect and figure out when did LL_ASSERT start being empty macro :-(

[MeisterBob]

when CONFIG_BT_CTLR_ASSERT_HANDLER = n

zephyr/subsys/bluetooth/controller/hal/debug.h

Line 25 in 2e23dfd

BT_ASSERT(cond)

and CONFIG_BT_ASSERT = n

zephyr/subsys/bluetooth/common/assert.h

Line 39 in 2e23dfd

#define BT_ASSERT(cond) __ASSERT_NO_MSG(cond)

and finally CONFIG_ASSERT = n

zephyr/include/zephyr/sys/__assert.h

Line 142 in 2e23dfd

#define __ASSERT_NO_MSG(test) { }

[cvinayak]

Yes, I looked into these. And, we always default CONFIG_BT_ASSERT=y here:

zephyr/subsys/bluetooth/common/Kconfig

Lines 295 to 300 in 2e23dfd

	config BT_ASSERT
	bool "Custom Bluetooth assert implementation"
	default y
	help
	Use a custom Bluetooth assert implementation instead of the
	kernel-wide __ASSERT() when CONFIG_ASSERT is disabled.

Hence, all is well as CONFIG_BT_ASSERT_VERBOSE=y and uses __ASSERT_LOC() which is not dependent on CONFIG_ASSERT=y. If an application explicitly uses CONFIG_BT_ASSERT=n then they are on their own and an explicit choice that we permit.

[cvinayak]

An application can always keep CONFIG_BT_ASSERT=y, as there is only a handful in the Zephyr Bluetooth LE Host stack that is fatal. (But for the Controller, it is required to keep LL_ASSERT due to the errors occurring in ISR execution context.

subsys/bluetooth/host/hci_common.c:19:  BT_ASSERT(buf);
subsys/bluetooth/host/hci_core.c:252:   BT_ASSERT_MSG(buf, "Unable to alloc for Host NCP");
subsys/bluetooth/host/hci_core.c:262:   BT_ASSERT_MSG(err == 0, "Unable to send Host NCP (err %d)", err);
subsys/bluetooth/host/hci_core.c:433:                   BT_ASSERT_MSG(success, "command opcode 0x%04x timeout", opcode);
subsys/bluetooth/host/hci_core.c:439:   BT_ASSERT_MSG(err == 0,
subsys/bluetooth/host/hci_core.c:3034:  BT_ASSERT(bt_hci_evt_get_flags(hdr->evt) & BT_HCI_EVT_FLAG_RECV);
subsys/bluetooth/host/hci_core.c:3049:  BT_ASSERT(buf);
subsys/bluetooth/host/hci_core.c:4090:  BT_ASSERT(evt_flags & BT_HCI_EVT_FLAG_RECV_PRIO);

We can review and reduce those as well to return error code if they are in the API call path in thread execution context, right @jhedberg and @alwa-nordic ?

[jhedberg]

We can review and reduce those as well to return error code if they are in the API call path in thread execution context, right @jhedberg and @alwa-nordic ?

Semantically, most of those are justifiably asserts. There maybe one or two that could be converted into runtime checks, but not all. One valid question is why do we have BT_ASSERT_*() rather than using __ASSERT*(). Would converting to the latter help at all with the issue being addressed in this PR?

[github-actions]

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.