Bluetooth: Classic: HFP: Support ongoing calls before SLC established #88331

lylezhu2012 · 2025-04-09T04:13:36Z

Bluetooth: Classic: HFP_AG: Support ongoing calls before SLC

Support the case that there are some calls existed before SLC established.

Add a callback to get the ongoing calls one by one from upper layer when the response of the AT command AT+CIND=? from HF has been sent.

And set the Call, Call Setup, and Held Call indicators and report the values int the response of AT command AT+CIND?. Then report all ongoing calls in the +CLCC response.

Bluetooth: Classic: HFP_HF: Support ongoing calls before SLC

If the any value of Call, Call Setup, and Held Call indicators is not zero in the response of AT+CIND?, get all calls via AT+CLCC.

Bluetooth: Classic: Shell: Add command ongoing_calls

Add shell command ongoing_calls to set the ongoing calls.

Bluetooth: Classic: HGP_AG: change get_ongoing_call() to async mode

Change the callback get_ongoing_call() of the AG from synchronous to asynchronous mode. It will help to avoid the Bluetooth host stack be blocked in the context of callback get_ongoing_call().

Add a function bt_hfp_ag_ongoing_calls() to set the ongoing calls and reply the AT command AT+CIND? after the callback get_ongoing_call() has been notified.

Add a delayable worker to avoid the AT command AT+CIND? never being replied. After the time exceeds @kconfig{CONFIG_BT_HFP_AG_GET_ONGOING_CALL_TIMEOUT}, the response of the AT command AT+CIND? will be replied.

lylezhu2012 · 2025-04-09T07:14:24Z

@gzh-terry , please try the changes.

jhedberg · 2025-04-10T07:59:40Z

include/zephyr/bluetooth/classic/hfp_ag.h

+ *
+ *  @param number Phone number of the call.
+ *  @param type Specify the format of the phone number.
+ *  @param dir Call direction (true for incoming, false for outgoing).


This is not a very intuitive way to use or name a boolean. At the very least its name should reflect its meaning, i.e. it should be called incoming (that way the code using it will look saner). However, probably an enum with two possible values would be even better.

Yes. An enum is more clear.

jhedberg · 2025-04-10T08:00:12Z

subsys/bluetooth/host/classic/hfp_ag_internal.h

+	/* existing calls */
+	struct bt_hfp_ag_existing_call existing_calls[CONFIG_BT_HFP_AG_MAX_CALLS];


Out of curiosity, is "existing" the term used in the spec for such calls? I'd have otherwise used "ongoing" or "active".

Yes. I tend to use ongoing.

jhedberg · 2025-04-10T08:01:13Z

subsys/bluetooth/host/classic/hfp_hf.c

+	struct bt_hfp_hf_call *call;
+	int count = 0;
+
+	for (size_t index = 0; index < ARRAY_SIZE(hf->calls); index++) {


Could you please just start using ARRAY_FOR_EACH() for any new array iteration, so we gradually move toward a common and consistent pattern for this throughout the stack.

gzh-terry · 2025-04-10T08:12:16Z

subsys/bluetooth/host/classic/shell/hfp.c

@@ -1229,6 +1257,7 @@ static struct bt_hfp_ag_cb ag_cb = {
 	.disconnected = ag_disconnected,
 	.sco_connected = ag_sco_connected,
 	.sco_disconnected = ag_sco_disconnected,
+	.get_existing_call = ag_get_existing_call,


Hi @lylezhu2012

It looks like the AG stack itself would invoke a callback to acquire all call information upon SLC connected.

I've checked the implementations on the other stacks, and they look a little bit different. The HFP service (an app in Zephyr's view) will query the call status from the telecom (also called telephony) and notify the stack (Zephyr) as if new calls arrived.

See queryPhoneState and phoneStateChanged

As a result, nothing to do at Zephyr's side.

The HFP service (an app in Zephyr's view) will query the call status from the telecom (also called telephony) and notify the stack (Zephyr) as if new calls arrived.

I am not sure whether there is a subsystem/module/driver likes as telecom in zephyr. But I think even if there is, it should be integrated at a higher level with telecom and hfp ag.

As a result, nothing to do at Zephyr's side.

I added a command ~~existing_calls~~ ongoing_calls in hfp ag to simulate there are some calles ongoing. Please refer to the changes of shell.

gzh-terry · 2025-04-10T08:53:13Z

include/zephyr/bluetooth/classic/hfp_ag.h

+	 *
+	 *  @return 0 in case of success and will get next one or negative value in case of error.
+	 */
+	int (*get_existing_call)(struct bt_hfp_ag *ag, struct bt_hfp_ag_existing_call *call);


For the AT+CIND=? command, I would recommend separating the callback and the CIND response. You can refer to the Android code where a CIND callback only indicates that the AT+CIND=? command is received, and another method is used to send the response.

If we combine them into one callback, it would be very difficult to transfer the work to another loop/thread (which is actually the majority of cases).

Maybe it is not clear. I mean the callback will be notified after the command AT+CIND=? has been handled. The result of the callback will be replied in the response of AT+CIND?.

Hi @lylezhu2012

I thought your code works like this:

We are at AG side.

We received an AT+CIND? command to check the current call status.

We post this callback to ask app about current calls.

App should return the call status directly via *call in this callback.

We transfer these call status to HF via +CIND:...

My concern is, the upper layer can not return immediately in step 4. Because in most cases the app need to get these information in another loop/thread, and sometimes another core.

So what app did is to hold this loop, and post a message to somewhere else, and wait until the call informations are collected. In these period, all activities in zephyr are blocked (>100ms in case of IPC).

Besides, here comes another issue: the app may call some zephyr's interface again during this waiting, and can lead to deadlock issue.

So a typicall method is to make them happen asynchronously:

One callback to inform AT+CIND? is received.

Another API to respond +CIND:...

Same issue happens in A2DP, where the Start/Suspend request needs to be responded in a synchronous call (return 0 to accept, otherwise reject).

I have updated the PR. Please try it again.

@gzh-terry , Did you try the patch?

makeshi · 2025-04-14T01:10:35Z

subsys/bluetooth/host/classic/hfp_ag.c

@@ -280,7 +280,11 @@ static struct bt_hfp_ag_call *get_new_call(struct bt_hfp_ag *ag, const char *num

 		if (!atomic_test_and_set_bit(call->flags, BT_HFP_AG_CALL_IN_USING)) {
 			/* Copy number to ag->number including null-character */
-			strcpy(call->number, number);
+			if (number != NULL) {
+				strcpy(call->number, number);


It is better use strncpy to avoid buffer overflow

There shouldn't be such a situation. Before calling get_new_call(), the length of number has been checked.

Even then, it's possible this will trigger warnings from static analyzers like Coverity. Ideally we should be using strlcpy(). I don't see many uses of it in the main tree, perhaps because minimal libc lacks support for it, however that should be easy to fix.

Yes. I will try use strncpy() to replace strcpy().

makeshi · 2025-04-14T01:23:22Z

subsys/bluetooth/host/classic/hfp_ag.c

+
+		err = bt_ag->get_ongoing_call(ag, call);
+		if (err != 0) {
+			LOG_DBG("No ongoing call retrieved");


It is a normal case for err != 0. Instead, I think the err == 0 is a corner case.

makeshi · 2025-04-14T01:27:12Z

subsys/bluetooth/host/classic/hfp_ag.c

@@ -1042,20 +1197,439 @@ static int bt_hfp_ag_cind_handler(struct bt_hfp_ag *ag, struct net_buf *buf)
 			ag_ind[BT_HFP_AG_ROAM_IND].max, ag_ind[BT_HFP_AG_BATTERY_IND].name,
 			ag_ind[BT_HFP_AG_BATTERY_IND].min, ag_ind[BT_HFP_AG_BATTERY_IND].connector,
 			ag_ind[BT_HFP_AG_BATTERY_IND].max);
+
+		bt_hfp_ag_get_ongoing_calls(ag);


bt_hfp_ag_get_ongoing_calls can be return int function? otherwise "err" need initialized 0.

err is used to save the reture value of hfp_ag_send_data(). bt_hfp_ag_get_ongoing_calls() only tries to get the ongoing calls. If there are ongoing calls, save them.

makeshi · 2025-04-14T01:27:18Z

subsys/bluetooth/host/classic/hfp_ag.c

+					ag->indicator_value[BT_HFP_AG_BATTERY_IND]);
+	}
+
+	for (size_t index = 0; index < ag->ongoing_call_count; index++) {


similar with hf, also can use ARRAY_FOR_EACH

ARRAY_FOR_EACH() is used to iterate over an array. The MAX is ARRAY_SIZE(). In current case, the MAX is ag->ongoing_call_count.

gzh-terry · 2025-04-29T03:33:58Z

subsys/bluetooth/host/classic/hfp_hf.c

+	case BT_HFP_CLCC_STATUS_ACTIVE:
+		hf_call_state_update(call, BT_HFP_HF_CALL_STATE_ACTIVE);
+		if (bt_hf->accept) {
+			bt_hf->accept(call);


May change this name to active?

There is a case that incoming held is also active status.

gzh-terry · 2025-04-29T03:41:46Z

subsys/bluetooth/host/classic/hfp_hf.c

 		incoming = true;
 	}

-	if (incoming != !!dir) {


Shoud generate a callback as well.
This usually happens when a call is taken over by another call (i.e., from VoIP to normal call)
We may receive two different calls with the same index in consecutive AT+CLCC procedure.
For example:

AT+CLCC +CLCC: 1,0,0,0,0,"123456",129 OK AT+CLCC +CLCC: 1,1,0,0,0,"654321",129 OK

Shoud generate a callback as well. This usually happens when a call is taken over by another call (i.e., from VoIP to normal call) We may receive two different calls with the same index in consecutive AT+CLCC procedure. For example:

AT+CLCC +CLCC: 1,0,0,0,0,"123456",129 OK AT+CLCC +CLCC: 1,1,0,0,0,"654321",129 OK

In the above case, we need to report the second call as a new, and the previous one is considered terminated.

Logically, the previous call should be removed first by AG, and then the new call should be notified.
Or, AG report these two calls at same time. And than remove the first one.

It seems that the phenomena you describe are not within the scope of HFP protocol.

However, the exchange of AT commands is considerably slower compared to the speed at which call state changes. This scenario frequently occurs in real products.

But it's still fine in this PR since it does not change the logic.

Yes. I think we can add this case down the road.

Support the case that there are some calls existed before SLC established. Add a callback to get the ongoing calls one by one from upper layer when the response of the AT command `AT+CIND=?` from HF has been sent. And set the Call, Call Setup, and Held Call indicators and report the values int the response of AT command `AT+CIND?`. Then report all ongoing calls in the `+CLCC` response. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

For read-only access to fields, `lock/unlock` is unnecessary. Remove unnecessary `lock/unlock` protection for read-only access fields of AG. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

If the any value of Call, Call Setup, and Held Call indicators is not zero in the response of `AT+CIND?`, get all calls via `AT+CLCC`. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

Add shell command `ongoing_calls` to set the ongoing calls. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

@kconfig

Change the callback `get_ongoing_call()` of the AG from synchronous to asynchronous mode. It will help to avoid the Bluetooth host stack be blocked in the context of callback `get_ongoing_call()`. Add a function `bt_hfp_ag_ongoing_calls()` to set the ongoing calls and reply the AT command `AT+CIND?` after the callback `get_ongoing_call()` has been notified. Add a delayable worker to avoid the AT command `AT+CIND?` never being replied. After the time exceeds @kconfig{CONFIG_BT_HFP_AG_GET_ONGOING_CALL_TIMEOUT}, the response of the AT command `AT+CIND?` will be replied. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

Add shell command `auto_select_codec` to select codec automatically when codec negotiation callback is notified. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

github-actions bot added area: Bluetooth area: Bluetooth Classic labels Apr 9, 2025

github-actions bot requested review from gzh-terry, jhedberg, makeshi and MarkWangChinese April 9, 2025 04:14

github-actions bot assigned lylezhu2012 Apr 9, 2025

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch 2 times, most recently from d8d4f65 to ad0cdab Compare April 9, 2025 07:11

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch 2 times, most recently from 4a55c15 to ae848c8 Compare April 10, 2025 02:41

jhedberg reviewed Apr 10, 2025

View reviewed changes

gzh-terry reviewed Apr 10, 2025

View reviewed changes

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch from ae848c8 to eae65b2 Compare April 10, 2025 12:09

lylezhu2012 requested review from gzh-terry and jhedberg April 10, 2025 12:22

lylezhu2012 changed the title ~~Bluetooth: Classic: HFP: Support existing calls before SLC~~ Bluetooth: Classic: HFP: Support ongoing calls before SLC Apr 10, 2025

lylezhu2012 changed the title ~~Bluetooth: Classic: HFP: Support ongoing calls before SLC~~ Bluetooth: Classic: HFP: Support ongoing calls before SLC established Apr 10, 2025

makeshi reviewed Apr 14, 2025

View reviewed changes

lylezhu2012 requested a review from makeshi April 16, 2025 02:27

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch 2 times, most recently from 433726d to bd619cf Compare April 17, 2025 12:18

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch 3 times, most recently from f7a1305 to 4e24f7c Compare April 24, 2025 08:42

gzh-terry reviewed Apr 29, 2025

View reviewed changes

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch from 4e24f7c to 7353e63 Compare April 29, 2025 07:03

gzh-terry previously approved these changes Apr 29, 2025

View reviewed changes

makeshi previously approved these changes Apr 30, 2025

View reviewed changes

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch from 7353e63 to c6b67df Compare April 30, 2025 06:39

lylezhu2012 added 5 commits April 30, 2025 14:41

Bluetooth: Classic: HFP_AG: Remove unnecessary lock/unlock

af2b133

For read-only access to fields, `lock/unlock` is unnecessary. Remove unnecessary `lock/unlock` protection for read-only access fields of AG. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

Bluetooth: Classic: HFP_HF: Support ongoing calls before SLC

bfc3992

If the any value of Call, Call Setup, and Held Call indicators is not zero in the response of `AT+CIND?`, get all calls via `AT+CLCC`. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

Bluetooth: Classic: Shell: Add command ongoing_calls

72f89a7

Add shell command `ongoing_calls` to set the ongoing calls. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

lylezhu2012 dismissed stale reviews from makeshi and gzh-terry via 25750ee April 30, 2025 08:41

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch from c6b67df to 25750ee Compare April 30, 2025 08:41

lylezhu2012 requested review from makeshi and gzh-terry April 30, 2025 08:41

makeshi previously approved these changes Apr 30, 2025

View reviewed changes

gzh-terry previously approved these changes May 7, 2025

View reviewed changes

Bluetooth: Class: Shell: HFP_HF: Add auto_select_codec command

218e709

Add shell command `auto_select_codec` to select codec automatically when codec negotiation callback is notified. Signed-off-by: Lyle Zhu <lyle.zhu@nxp.com>

lylezhu2012 dismissed stale reviews from gzh-terry and makeshi via 218e709 May 7, 2025 08:21

lylezhu2012 force-pushed the classic_hfp_ag_get_existing_callmain branch from 25750ee to 218e709 Compare May 7, 2025 08:21

lylezhu2012 requested review from makeshi and gzh-terry May 7, 2025 08:37

makeshi approved these changes May 20, 2025

View reviewed changes

gzh-terry approved these changes May 29, 2025

View reviewed changes

kartben merged commit ff41c71 into zephyrproject-rtos:main May 29, 2025
27 checks passed

		/* existing calls */
		struct bt_hfp_ag_existing_call existing_calls[CONFIG_BT_HFP_AG_MAX_CALLS];

Bluetooth: Classic: HFP: Support ongoing calls before SLC established #88331

Bluetooth: Classic: HFP: Support ongoing calls before SLC established #88331

Uh oh!

Conversation

lylezhu2012 commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lylezhu2012 commented Apr 9, 2025

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!

gzh-terry Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lylezhu2012 Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gzh-terry Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzh-terry Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gzh-terry Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lylezhu2012 Apr 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

lylezhu2012 Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

lylezhu2012 commented Apr 9, 2025 •

edited

Loading

gzh-terry Apr 10, 2025 •

edited

Loading

lylezhu2012 Apr 10, 2025 •

edited

Loading

gzh-terry Apr 16, 2025 •

edited

Loading

gzh-terry Apr 16, 2025 •

edited

Loading

gzh-terry Apr 16, 2025 •

edited

Loading

lylezhu2012 Apr 24, 2025 •

edited

Loading

lylezhu2012 Apr 14, 2025 •

edited

Loading