Bluetooth: Audio: Update BT_BAP_SCAN_DELEGATOR dependency to GATT_DYN…

[babrsn]

…AMIC_DB

Changed dependency of the BT_BAP_SCAN_DELEGATOR to GATT_DYNAMIC_DB from 'select' to 'depends on' and solved all loop dependencies caused by this change.

Fixes #79108

[PavelVPV]

I guess mesh changed by coincidence?

[sjanc]

this is already set in prj.conf

[babrsn]

Ok.

[omkar3141]

I guess mesh changed by coincidence?

I believe this is very relevant for mesh since we change services at run-time after provisioning (removal of Mesh Provisioning Service and exposing Mesh Proxy Service).

I also compiled 'light' sample on NCSv2.6.0 and I see CONFIG_BT_GATT_DYNAMIC_DB=y in generated .config file. Same observed for Zephyr 'mesh' sample on corresponding zephyr revision.

[omkar3141]

Looks good for mesh.

[PavelVPV]

I guess mesh changed by coincidence?

I believe this is very relevant for mesh since we change services at run-time after provisioning (removal of Mesh Provisioning Service and exposing Mesh Proxy Service).

I also compiled 'light' sample on NCSv2.6.0 and I see CONFIG_BT_GATT_DYNAMIC_DB=y in generated .config file. Same observed for Zephyr 'mesh' sample on corresponding zephyr revision.

No need for explicit selection of Kconfig options that have been enabled by default for ages:

zephyr/subsys/bluetooth/mesh/Kconfig

Lines 37 to 40 in d022d31

	config BT_MESH_GATT_SERVER
	bool
	select BT_MESH_GATT
	select BT_GATT_DYNAMIC_DB

[PavelVPV]

The fact that LE audio cannot use depends on (and thus also no other application) with Mesh enabled is evidence, IMO, that it was not working fine before.

From what I see this doesn't depend on whether mesh enabled or not:

Ok, to describe it in more detail, assume I just change the dependency of ´BT_BAP_SCAN_DELEGATOR´ from ´select´ to ´depends on´ in ´subsys/bluetooth/audio/Kconfig.bap´ (as intended by this issue). Then, for instance, when I build ´bap_broadcast_sink´, I get the dependency loop error (see the attachment file).

This should be explained in the issue more clearly.

[babrsn]

Why do you want to change select BT_GATT_DYNAMIC_DB to depends on BT_GATT_DYNAMIC_DB for BT_BAP_SCAN_DELEGATOR?

Besides being semantically better when dealing with dependencies, our Kconfig page also mentions that depends on is usually better than select for a handful of reason (including these circular dependencies that we have...): https://docs.zephyrproject.org/latest/build/kconfig/tips.html#alternatives-to-select

Since it was working fine before even being semantically not perfect, I'd really like to know what is actually wrong (not worse/better) before approving because now all users will have to explicitly enable BT_GATT_DYNAMIC_DB in their project files.

Since it was working fine before

The fact that LE audio cannot use depends on (and thus also no other application) with Mesh enabled is evidence, IMO, that it was not working fine before. The current usage of select is effectively forcing others to also using select rather than depends on.

@babrsn It would be interesting to see how the changes are if Mesh wasn't changed. You mentioned that the circular dependency could be fixed in another way, without affecting Mesh?

At the moment, I don’t have a ready-to-submit alternative solution. However, I believe there should be more alternatives for this issue. I think the current solution here is the simplest because I have mostly changed the configs which select BT_GATT_DYNAMIC_DB. I suspect that alternative solutions will affect more parameters in more places. If it would be beneficial, I can investigate to see if I can find another good solution that doesn’t affect 'Mesh'. I could do this tomorrow or the day after...

[Thalley]

At the moment, I don’t have a ready-to-submit alternative solution. However, I believe there should be more alternatives for this issue. I think the current solution here is the simplest because I have mostly changed the configs which select BT_GATT_DYNAMIC_DB. I suspect that alternative solutions will affect more parameters in more places. If it would be beneficial, I can investigate to see if I can find another good solution that doesn’t affect 'Mesh'. I could do this tomorrow or the day after...

I think it's worth investigating alternative solutions that doesn't affect mesh (and preferably nothing else besides the LE Audio stack). If you do get something else working, please submit that as a separate commit, either in this PR or another for comparison

[PavelVPV]

Perhaps you want something like this: 39eb100 ?

All audio and mesh upstream samples compile with this change.

But as you see now you (and all who use audio) need explicitly enable BT_GATT_DYNAMIC_DB and some other Bluetooth options.

If you want to proceed with this, I'd recommend go through audio Kconfigs and replace "select BT_" to "depends on BT_" for all upper Bluetooth Kconfig options.

[omkar3141]

If you want to proceed with this, I'd recommend go through audio Kconfigs and replace "select BT_" to "depends on BT_" for all upper Bluetooth Kconfig options.

It is not a bad idea, though it will make all "prj.conf" look quite big and littered with many KConfig options.

Then I think, may be it is worth to not introduce this change everywhere and do it only at a place where it actually creates a problem (like proposed in this PR). This could be middle ground even though it is not so pure and pious.

[Thalley]

Perhaps you want something like this: 39eb100 ?

All audio and mesh upstream samples compile with this change.

But as you see now you (and all who use audio) need explicitly enable BT_GATT_DYNAMIC_DB and some other Bluetooth options.

If you want to proceed with this, I'd recommend go through audio Kconfigs and replace "select BT_" to "depends on BT_" for all upper Bluetooth Kconfig options.

That's closer to the result I had expected from fixing the issue as well. Ideally we want to replace all select with depends on in LE audio in the future

It is not a bad idea, though it will make all "prj.conf" look quite big and littered with many KConfig options.

Then I think, may be it is worth to not introduce this change everywhere and do it only at a place where it actually creates a problem (like proposed in this PR). This could be middle ground even though it is not so pure and pious.

I think this is a subjective matter.
Besides depends on being suggested by Zephyr over select, forcing applications to enable things explicitly does indeed make the .conf files larger, but also clearer in terms of what is enabled.

For example enabling CONFIG_FOO should in many cases perhaps not enable 10s of other Kconfigs what the knowledge of the user. See https://docs.zephyrproject.org/latest/build/kconfig/tips.html#select-pitfalls for reasons why select should not be overused.

[babrsn]

Perhaps you want something like this: 39eb100 ?

All audio and mesh upstream samples compile with this change.

But as you see now you (and all who use audio) need explicitly enable BT_GATT_DYNAMIC_DB and some other Bluetooth options.

If you want to proceed with this, I'd recommend go through audio Kconfigs and replace "select BT_" to "depends on BT_" for all upper Bluetooth Kconfig options.

Thanks. Although it involves a considerable number of changes in the config symbols within the Kconfig files, I think it could be a good solution since it only affects the BT_Audio area. As I still haven’t had time to investigate it and possibly find a better solution, I can cherry-pick it and push an update to this PR...

[Thalley]

Thanks. Although it involves a considerable number of changes in the config symbols within the Kconfig files

As mentioned earlier, these changes are likely ones we want in the future anyways. We should be able to replace all select by depends on in LE Audio.

[babrsn]

Thanks. Although it involves a considerable number of changes in the config symbols within the Kconfig files

As mentioned earlier, these changes are likely ones we want in the future anyways. We should be able to replace all select by depends on in LE Audio.

Indeed

[Thalley]

@babrsn needs a rebase for the CCID changes I believe :)