Skip to content

Conversation

@Thalley
Copy link
Contributor

@Thalley Thalley commented Nov 12, 2024

Add validation that the device supports the role
set by bt_tmap_register.

fixes #60495

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
LOG_DBG("Device does not support the CY role");
LOG_DBG("Device does not support the CT role");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not specific for this PR, but also shows up other places where CHECKIF is used, and will be compiler dependent:
If the KCONFIG option NO_RUNTIME_CHECKS is set to 'y' a compiler may emit the following warning: static bool valid_tmap_role defined but not used

suggestions for fixing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two ways we can apply is to guard this call with an IS_ENABLED, or instead of having CHECKIF in the call, we can change all the checks in the functions to use CHECKIF to that if NO_RUNTIME_CHECKS then the function is just a no-op.

But you have a good point that we should do that here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the macro it is defined as

#elif defined(CONFIG_NO_RUNTIME_CHECKS)
#define CHECKIF(...) \
	if (0)
#else

With if (0) the code is still compiled, but not linked, and should thus not have any issues. Will leave as is

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the code compiles and works fine as is, the only issue is that we get a compiler warning, that shouldn't be there.

kruithofa
kruithofa previously approved these changes Nov 14, 2024
@Thalley Thalley force-pushed the tmap_role_check branch 2 times, most recently from f8c3f61 to 149a400 Compare November 22, 2024 20:56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to wrap this very long line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Add validation that the device supports the role
set by bt_tmap_register.

Signed-off-by: Emil Gydesen <[email protected]>
@kartben kartben merged commit 8c38d1b into zephyrproject-rtos:main Dec 27, 2024
27 checks passed
@Thalley Thalley deleted the tmap_role_check branch December 27, 2024 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

LE audio TMAP: Add role verification

5 participants