Skip to content

Bluetooth: CCP: Server: Add support for get UCI #79642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Bluetooth: CCP: Server: Add support for get UCI #79642

wants to merge 1 commit into from
+141 −3

Conversation

@Thalley
Copy link
Contributor

@Thalley Thalley commented Oct 10, 2024

Add support for getting the UCI.
For now the UCI will be duplicated by the TBS
implementation, but will be optimizied in the future
so only one copy of the UCI exists.

@Thalley Thalley force-pushed the ccp_server_uci branch 3 times, most recently from bd06274 to a3d25a7 Compare October 22, 2024 13:09
@Thalley Thalley force-pushed the ccp_server_uci branch 2 times, most recently from 33653a0 to 59bce07 Compare November 12, 2024 10:56
@Thalley Thalley force-pushed the ccp_server_uci branch 3 times, most recently from 36c53fd to f7802fb Compare November 22, 2024 21:10
@Thalley Thalley self-assigned this Nov 26, 2024
@Thalley Thalley requested a review from Copilot April 7, 2025 08:49
Copilot AI reviewed Apr 7, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 10 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • doc/connectivity/bluetooth/shell/audio/ccp.rst: Language not supported
  • subsys/bluetooth/audio/Kconfig.ccp: Language not supported
  • subsys/bluetooth/audio/Kconfig.tbs: Language not supported
Comments suppressed due to low confidence (1)

subsys/bluetooth/audio/shell/ccp_call_control_server.c:112

  • [nitpick] There is an inconsistency between the configuration macros used; consider using CONFIG_BT_CCP_CALL_CONTROL_SERVER_BEARER_COUNT instead of CONFIG_BT_TBS_BEARER_COUNT for index validation to avoid potential mismatches.
index = validate_and_get_index(sh, argv[1]);

subsys/bluetooth/audio/shell/ccp_call_control_server.c Outdated
return -ENOEXEC;
}

if (index > CONFIG_BT_TBS_BEARER_COUNT) {
Copy link

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

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

Index validation should use '>= CONFIG_BT_TBS_BEARER_COUNT' to correctly ensure that the index is within the bounds of the bearer array.

Suggested change
if (index > CONFIG_BT_TBS_BEARER_COUNT) {
if (index >= CONFIG_BT_TBS_BEARER_COUNT) {

Copilot uses AI. Check for mistakes.
@Thalley Thalley requested a review from Copilot April 7, 2025 09:04
Copilot AI reviewed Apr 7, 2025
View reviewed changes
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 7 out of 10 changed files in this pull request and generated 1 comment.

Files not reviewed (3)
  • doc/connectivity/bluetooth/shell/audio/ccp.rst: Language not supported
  • subsys/bluetooth/audio/Kconfig.ccp: Language not supported
  • subsys/bluetooth/audio/Kconfig.tbs: Language not supported

include/zephyr/bluetooth/audio/ccp.h Outdated
* @param[out] uci Pointer that will be updated to be the bearer uci.
*
* @retval 0 Success
* @retval -EINVAL @p bearer or @p name is NULL
Copy link

Copilot AI Apr 7, 2025

Choose a reason for hiding this comment

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

In the documentation for bt_ccp_call_control_server_get_bearer_uci, the parameter is mistakenly referred to as 'name' instead of 'uci'. Consider updating it to '@p uci' for clarity.

Suggested change
* @retval -EINVAL @p bearer or @p name is NULL
* @retval -EINVAL @p bearer or @p uci is NULL

Copilot uses AI. Check for mistakes.
@Thalley Thalley force-pushed the ccp_server_uci branch 2 times, most recently from ae12b78 to 1ea821c Compare April 9, 2025 13:58
@Thalley Thalley force-pushed the ccp_server_uci branch 2 times, most recently from 89c3a4e to d3b066d Compare September 8, 2025 07:59
@Thalley Thalley marked this pull request as ready for review September 8, 2025 07:59
@zephyrbot zephyrbot added the area: Tests Issues related to a particular existing or missing test label Oct 13, 2025
@zephyrbot zephyrbot requested a review from nashif October 13, 2025 12:06
fredrikdanebjer
fredrikdanebjer reviewed Nov 19, 2025
View reviewed changes
doc/connectivity/bluetooth/shell/audio/ccp.rst

.. code-block:: console
uart:~$ ccp_call_control_server get_bearer_uci
Copy link
Contributor

@fredrikdanebjer fredrikdanebjer Nov 19, 2025

Choose a reason for hiding this comment

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

Do we normally assume that omitting index will default the index to 0, instead of the whole list?

Copy link
Contributor Author

@Thalley Thalley Nov 26, 2025

Choose a reason for hiding this comment

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

For now at least we always default to bearer[0] if no index is provided.

Do you think it makes more sense to perform the action on all bearers if index is omitted?

It could work, especially for all "get/read" operations, but probably doesn't make sense for "set/write" operations.

It's following the implementation of the TBS shell, but we can change that no problem.

We can also just modify it so that index is a mandatory argument

fredrikdanebjer
fredrikdanebjer reviewed Nov 19, 2025
View reviewed changes
Copy link
Contributor

@fredrikdanebjer fredrikdanebjer left a comment

Choose a reason for hiding this comment

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

LGTM. I had one minor comment you are free to answer before I approve :)

@Thalley
Bluetooth: CCP: Server: Add support for get UCI
87a7198 
Add support for getting the UCI.
For now the UCI will be duplicated by the TBS
implementation, but will be optimizied in the future
so only one copy of the UCI exists.

Signed-off-by: Emil Gydesen <[email protected]>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Nov 28, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@larsgk larsgk larsgk approved these changes

@asbjornsabo asbjornsabo Awaiting requested review from asbjornsabo

@babrsn babrsn Awaiting requested review from babrsn

@jhedberg jhedberg Awaiting requested review from jhedberg

@jthm-ot jthm-ot Awaiting requested review from jthm-ot

@niym-ot niym-ot Awaiting requested review from niym-ot

@sjanc sjanc Awaiting requested review from sjanc

@nashif nashif Awaiting requested review from nashif

@fredrikdanebjer fredrikdanebjer Awaiting requested review from fredrikdanebjer

At least 2 approving reviews are required to merge this pull request.

Assignees

@Thalley Thalley

Labels

area: Bluetooth Audio area: Bluetooth area: Tests Issues related to a particular existing or missing test

Projects

Bluetooth LE Audio
Status: In Review

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Thalley @larsgk @fredrikdanebjer @zephyrbot