[PATCH 1/3] Add multi calls support for HFP voicecall driver

[PATCH 1/3] Add multi calls support for HFP voicecall driver

[Zhenhua Zhang]

[-- Attachment #1: Type: text/plain, Size: 13881 bytes --]

These 3 patches are new multi calls and multiparty call features
support for HFP voicecall driver. So I seperate them with previous
small patches.

I divide big chunk into 3 parts. The first one is multi calls support.
The second is multiparty call support and the latest is send_dtmf
and list_calls implementation.

My test device is iPhone 3G and LG KM900. If you meet problems with
the patch, please paste your log while turning on OFONO_AT_DEBUG and
let me know. Thanks.


This feature is to enable CHLD=0, 1 and 2. We have considered
following issues:

1. Some phones shift call index when a smaller index call is
released. So we use phone number to compare status in clcc_poll_cb.

2. Some phones fail to response after CHLD=0, 1, 2. Need to poll
status after 2 seconds interval. But if we have another dialing
call, we need cancel +CLCC poll in atd_cb because the new call
is not avaiable until callsetup=3 at AG side.

3. In release_specific, if user want to release a hold call, we
first swap the call and then release it.

4. When only one call left in multi calls, we reset ciev_call and
ciev_callsetup status to avoid wrong +CIEV call/callsetup indicator
from previous released call.
---
 drivers/hfpmodem/voicecall.c |  352
+++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 345 insertions(+), 7 deletions(-)

diff --git a/drivers/hfpmodem/voicecall.c b/drivers/hfpmodem/voicecall.c
index b1d144b..43b4c01 100644
--- a/drivers/hfpmodem/voicecall.c
+++ b/drivers/hfpmodem/voicecall.c
@@ -47,6 +47,8 @@
 #define AG_CHLD_3 0x20
 #define AG_CHLD_4 0x40
 
+#define POLL_CLCC_INTERVAL 2000
+
 static const char *none_prefix[] = { NULL };
 static const char *chld_prefix[] = { "+CHLD:", NULL };
 static const char *clcc_prefix[] = { "+CLCC:", NULL };
@@ -61,6 +63,7 @@ struct voicecall_data {
 	int cind_val[HFP_INDICATOR_LAST];
 	unsigned int id_list;
 	unsigned int local_release;
+	unsigned int clcc_source;
 };
 
 struct release_id_req {
@@ -252,6 +255,13 @@ static void atd_cb(gboolean ok, GAtResult *result,
gpointer user_data)
 	if (!ok)
 		goto out;
 
+	/* Disable multiparty call if phone does not support +CLCC */
+	if (vd->call && !(vd->ag_features & AG_FEATURE_ENHANCED_CALL_STATUS))
{
+		ofono_error("Phone does NOT support enhanced "
+				" call status feature, fail to create call");
+		goto out;
+	}
+
 	call = create_call(vd, 0, 0, CALL_STATUS_DIALING,
 					ph.number, ph.type, 0);
 
@@ -276,6 +286,14 @@ static void hfp_dial(struct ofono_voicecall *vc,
 	struct atd_cb_data *cbd;
 	char buf[256];
 
+	/* Remove +CLCC request if we swap the calls before. See comments in
+	 * hfp_hold_all_active.
+	 */
+	if (vd->clcc_source) {
+		g_source_remove(vd->clcc_source);
+		vd->clcc_source = 0;
+	}
+
 	cbd = g_try_new0(struct atd_cb_data, 1);
 
 	if (!cbd)
@@ -348,6 +366,253 @@ static void hfp_hangup(struct ofono_voicecall *vc,
 	hfp_template("AT+CHUP", vc, generic_cb, 0x3f, cb, data);
 }
 
+static void reset_to_single_call(struct voicecall_data *vd)
+{
+	struct ofono_call *oc = vd->calls->data;
+
+	vd->call = oc;
+	vd->cind_val[HFP_INDICATOR_CALL] = 1;
+
+	if (oc->direction == 1)
+		vd->cind_val[HFP_INDICATOR_CALLSETUP] = 1;
+	else
+		vd->cind_val[HFP_INDICATOR_CALLSETUP] = 3;
+}
+
+static void clcc_poll_cb(gboolean ok, GAtResult *result, gpointer
user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GSList *n;
+	GSList *c;
+	GSList *o = vd->calls;
+	struct ofono_call *nc, *oc;
+	struct ofono_error error;
+
+	dump_response("clcc_poll_cb", ok, result);
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok)
+		return;
+
+	n = parse_clcc(result);
+
+	while (o) {
+		oc = o->data;
+
+		/* Call index may be shift when another call is released.
+		 * So we can only find call by phone number.
+		 */
+		c = g_slist_find_custom(n, &oc->phone_number,
+					at_util_call_compare_by_phone_number);
+
+		if (c) {
+			nc = c->data;
+
+			if (memcmp(nc, oc, sizeof(struct ofono_call))) {
+				oc->status = nc->status;
+
+				/* If phone does not support multiparty call,
+				 * there will have only one active call at one
+				 * time. +CHUP only hang up current active call.
+				 */
+				if (oc->status == CALL_STATUS_ACTIVE)
+					vd->call = oc;
+
+				ofono_voicecall_notify(vc, oc);
+			}
+
+			o = o->next;
+		} else {
+			release_call(vc, oc);
+			o = vd->calls;
+		}
+	}
+
+	g_slist_foreach(n, (GFunc) g_free, NULL);
+	g_slist_free(n);
+
+	/* Reset ciev_call and ciev_callsetup based on call info when only one
+	 * call is left
+	 */
+	if (g_slist_length(vd->calls) == 1)
+		reset_to_single_call(vd);
+}
+
+static gboolean poll_clcc(gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	if (vd->ag_features & AG_FEATURE_ENHANCED_CALL_STATUS) {
+		g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix,
+					clcc_poll_cb, vc, NULL);
+
+		vd->clcc_source = 0;
+	}
+
+	return FALSE;
+}
+
+static gboolean check_mpty_feature(struct ofono_voicecall *vc,
+				unsigned int feature,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	if (vd->ag_mpty_features & feature)
+		return TRUE;
+
+	CALLBACK_WITH_FAILURE(cb, data);
+	return FALSE;
+}
+
+static void hfp_hold_all_active(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	if (check_mpty_feature(vc, AG_CHLD_2, cb, data)) {
+		hfp_template("AT+CHLD=2", vc, generic_cb, 0, cb, data);
+
+		/* Some phones fail to send response back after CHLD=2.
+		 * But if we have another dialing out call, we should hold
+		 * +CLCC until AG creates the new call.
+		 */
+		vd->clcc_source = g_timeout_add(POLL_CLCC_INTERVAL,
+						poll_clcc, vc);
+	}
+}
+
+static void hfp_release_all_held(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	if (check_mpty_feature(vc, AG_CHLD_0, cb, data))
+		hfp_template("AT+CHLD=0", vc, generic_cb, 0x10, cb, data);
+}
+
+static void release_id_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct release_id_req *req = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+	struct ofono_error error;
+
+	dump_response("release_id_cb", ok, result);
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (ok)
+		vd->local_release |= (1 << req->id);
+
+	req->cb(&error, req->data);
+
+	/* Some phones fail to send response back after CHLD=1x */
+	vd->clcc_source = g_timeout_add(POLL_CLCC_INTERVAL, poll_clcc,
req->vc);
+}
+
+static void hfp_set_udub(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int incoming_or_waiting = (1 << 4) | (1 << 5);
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct ofono_call *call = vd->call;
+	struct release_id_req *req;
+
+	if (vd->ag_mpty_features & AG_CHLD_0)
+		hfp_template("AT+CHLD=0", vc, generic_cb, incoming_or_waiting,
+					cb, data);
+	else {
+		req = g_try_new0(struct release_id_req, 1);
+
+		if (!req || !call) {
+			CALLBACK_WITH_FAILURE(cb, data);
+			return;
+		}
+
+		req->vc = vc;
+		req->cb = cb;
+		req->data = data;
+		req->id = call->id;
+
+		g_at_chat_send(vd->chat, "AT+CHUP", none_prefix,
+					release_id_cb, req, g_free);
+	}
+}
+
+static void hfp_release_all_active(struct ofono_voicecall *vc,
+					ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	if (check_mpty_feature(vc, AG_CHLD_1, cb, data)) {
+		hfp_template("AT+CHLD=1", vc, generic_cb, 0x1, cb, data);
+
+		vd->clcc_source = g_timeout_add(POLL_CLCC_INTERVAL, poll_clcc,
+						vc);
+	}
+}
+
+static gboolean release_hold_cb(gpointer user_data)
+{
+	struct release_id_req *req = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+
+	vd->local_release |= (1 << req->id);
+
+	hfp_release_all_active(req->vc, req->cb, req->data);
+
+	g_free(req);
+
+	return FALSE;
+}
+
+static void swap_hold_cb(gboolean ok, GAtResult *result, gpointer
user_data)
+{
+	struct release_id_req *req = user_data;
+	struct ofono_error error;
+
+	dump_response("generic_cb", ok, result);
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		CALLBACK_WITH_FAILURE(req->cb, req->data);
+		return;
+	}
+
+	g_timeout_add(POLL_CLCC_INTERVAL, release_hold_cb, req);
+}
+
+static void hfp_release_specific(struct ofono_voicecall *vc, int id,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct release_id_req *req = g_try_new0(struct release_id_req, 1);
+	char buf[32];
+
+	if (!req) {
+		CALLBACK_WITH_FAILURE(cb, data);
+		return;
+	}
+
+	req->vc = vc;
+	req->cb = cb;
+	req->data = data;
+	req->id = id;
+
+	if (vd->ag_features & AG_FEATURE_ENHANCED_CALL_CONTROL) {
+		sprintf(buf, "AT+CHLD=1%d", id);
+
+		g_at_chat_send(vd->chat, buf, none_prefix,
+					release_id_cb, req, g_free);
+	} else if (id == (int) vd->call->id)
+		g_at_chat_send(vd->chat, "AT+CHUP", none_prefix,
+					release_id_cb, req, g_free);
+	else
+		/* If the call is held, swap and then release it */
+		g_at_chat_send(vd->chat, "AT+CHLD=2", none_prefix,
+					swap_hold_cb, req, NULL);
+}
+
 static void ring_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -439,7 +704,10 @@ static void ciev_call_notify(struct ofono_voicecall
*vc,
 
 	switch (value) {
 	case 0:
-		release_call(vc, call);
+		if (g_slist_length(vd->calls) > 1 && !vd->clcc_source)
+			poll_clcc(vc);
+		else
+			release_call(vc, call);
 		break;
 	case 1:
 		call->status = CALL_STATUS_ACTIVE;
@@ -515,7 +783,9 @@ static void ciev_callsetup_notify(struct
ofono_voicecall *vc,
 		/* call=0 and callsetup=1: reject an incoming call
 		 * call=0 and callsetup=2,3: interrupt an outgoing call
 		 */
-		if ((ciev_call == 0) && (ciev_callsetup > 0))
+		if (g_slist_length(vd->calls) > 1 && !vd->clcc_source)
+			poll_clcc(vc);
+		else if ((ciev_call == 0) && (ciev_callsetup > 0))
 			release_call(vc, call);
 		break;
 	case 1:
@@ -538,6 +808,7 @@ static void ciev_callsetup_notify(struct
ofono_voicecall *vc,
 	case 3:
 		call->status = CALL_STATUS_ALERTING;
 		ofono_voicecall_notify(vc, call);
+		break;
 	default:
 		break;
 	}
@@ -545,6 +816,18 @@ static void ciev_callsetup_notify(struct
ofono_voicecall *vc,
 	vd->cind_val[HFP_INDICATOR_CALLSETUP] = value;
 }
 
+static void ciev_callheld_notify(struct ofono_voicecall *vc,
+				struct ofono_call *call,
+				unsigned int value)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	if (!vd->clcc_source)
+		poll_clcc(vc);
+
+	vd->cind_val[HFP_INDICATOR_CALLHELD] = value;
+}
+
 static void ciev_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -569,6 +852,57 @@ static void ciev_notify(GAtResult *result, gpointer
user_data)
 		ciev_call_notify(vc, call, value);
 	else if (index == vd->cind_pos[HFP_INDICATOR_CALLSETUP])
 		ciev_callsetup_notify(vc, call, value);
+	else if (index == vd->cind_pos[HFP_INDICATOR_CALLHELD])
+		ciev_callheld_notify(vc, call, value);
+}
+
+static void ccwa_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GAtResultIter iter;
+	const char *num;
+	int type;
+	struct ofono_call *call;
+
+	dump_response("ccwa_notify", TRUE, result);
+
+	/* CCWA can be repeat, ignore if we already have a waiting call */
+	if (g_slist_find_custom(vd->calls,
+				GINT_TO_POINTER(CALL_STATUS_WAITING),
+				at_util_call_compare_by_status))
+		return;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CCWA:"))
+		return;
+
+	if (!g_at_result_iter_next_string(&iter, &num))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &type))
+		return;
+
+	/* Skip class field */
+	g_at_result_iter_skip_next(&iter);
+
+	/* Skip alpha field */
+	g_at_result_iter_skip_next(&iter);
+
+	/* Skip validity field */
+	g_at_result_iter_skip_next(&iter);
+
+	call = create_call(vd, 0, 1, CALL_STATUS_WAITING,
+				num, type, 0);
+
+	if (!call) {
+		ofono_error("malloc call structfailed."
+				"Call management is fubar");
+		return;
+	}
+
+	ofono_voicecall_notify(vc, call);
 }
 
 static void chld_cb(gboolean ok, GAtResult *result, gpointer user_data)
@@ -624,6 +958,10 @@ static void hfp_voicecall_initialized(gboolean ok,
GAtResult *result,
 	g_at_chat_register(vd->chat, "+CLIP:", clip_notify, FALSE, vc, NULL);
 	g_at_chat_register(vd->chat, "+CIEV:", ciev_notify, FALSE, vc, NULL);
 
+	if (vd->ag_features & AG_FEATURE_ENHANCED_CALL_STATUS)
+		g_at_chat_register(vd->chat, "+CCWA:", ccwa_notify, FALSE, vc,
+					NULL);
+
 	ofono_voicecall_register(vc);
 }
 
@@ -671,11 +1009,11 @@ static struct ofono_voicecall_driver driver = {
 	.answer			= hfp_answer,
 	.hangup			= hfp_hangup,
 	.list_calls		= NULL,
-	.hold_all_active	= NULL,
-	.release_all_held	= NULL,
-	.set_udub		= NULL,
-	.release_all_active	= NULL,
-	.release_specific	= NULL,
+	.hold_all_active	= hfp_hold_all_active,
+	.release_all_held	= hfp_release_all_held,
+	.set_udub		= hfp_set_udub,
+	.release_all_active	= hfp_release_all_active,
+	.release_specific	= hfp_release_specific,
 	.private_chat		= NULL,
 	.create_multiparty	= NULL,
 	.transfer		= NULL,
-- 
1.6.2.5

Re: [PATCH 1/3] Add multi calls support for HFP voicecall driver

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4156 bytes --]

Hi Zhenhua,

>+static void reset_to_single_call(struct voicecall_data *vd)
>+{
>+       struct ofono_call *oc = vd->calls->data;
>+
>+       vd->call = oc;
>+       vd->cind_val[HFP_INDICATOR_CALL] = 1;
>+
>+       if (oc->direction == 1)
>+               vd->cind_val[HFP_INDICATOR_CALLSETUP] = 1;
>+       else
>+               vd->cind_val[HFP_INDICATOR_CALLSETUP] = 3;
>+}
>+

I don't see how this is supposed to work.  What if the last call is a held 
call?

> +		if (c) {
> +			nc = c->data;
> +
> +			if (memcmp(nc, oc, sizeof(struct ofono_call))) {
> +				oc->status = nc->status;
> +
> +				/* If phone does not support multiparty call,
> +				 * there will have only one active call at one
> +				 * time. +CHUP only hang up current active call.
> +				 */
> +				if (oc->status == CALL_STATUS_ACTIVE)
> +					vd->call = oc;

Lets not set this silly variable anymore.  It has way too many meanings and 
uses throughout the code (most of which can be easily simplified) for the logic 
to be understandable.

> +static void hfp_hold_all_active(struct ofono_voicecall *vc,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +
> +	if (check_mpty_feature(vc, AG_CHLD_2, cb, data)) {
> +		hfp_template("AT+CHLD=2", vc, generic_cb, 0, cb, data);
> +
> +		/* Some phones fail to send response back after CHLD=2.
> +		 * But if we have another dialing out call, we should hold
> +		 * +CLCC until AG creates the new call.
> +		 */
> +		vd->clcc_source = g_timeout_add(POLL_CLCC_INTERVAL,
> +						poll_clcc, vc);

This comment makes no sense.  If the phone doesn't respond after CHLD=2 the 
parser is stuck and we're not sending CLCC out anyway.

> +static void hfp_set_udub(struct ofono_voicecall *vc,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	unsigned int incoming_or_waiting = (1 << 4) | (1 << 5);
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct ofono_call *call = vd->call;
> +	struct release_id_req *req;
> +
> +	if (vd->ag_mpty_features & AG_CHLD_0)
> +		hfp_template("AT+CHLD=0", vc, generic_cb, incoming_or_waiting,
> +					cb, data);
> +	else {
> +		req = g_try_new0(struct release_id_req, 1);
> +
> +		if (!req || !call) {
> +			CALLBACK_WITH_FAILURE(cb, data);
> +			return;
> +		}
> +
> +		req->vc = vc;
> +		req->cb = cb;
> +		req->data = data;
> +		req->id = call->id;
> +
> +		g_at_chat_send(vd->chat, "AT+CHUP", none_prefix,
> +					release_id_cb, req, g_free);
> +	}

The core is quite specifically asking to set UDUB, not to hangup the call.  If 
this is not supported, then we should say so.  Please get rid of this.

> +}
> +
> +static void hfp_release_all_active(struct ofono_voicecall *vc,
> +					ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +
> +	if (check_mpty_feature(vc, AG_CHLD_1, cb, data)) {
> +		hfp_template("AT+CHLD=1", vc, generic_cb, 0x1, cb, data);
> +
> +		vd->clcc_source = g_timeout_add(POLL_CLCC_INTERVAL, poll_clcc,
> +						vc);

Why are we scheduling the CLCC before the CHLD returned?  We should wait until 
CHLD fails or succeeds.

> +static void hfp_release_specific(struct ofono_voicecall *vc, int id,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	if (vd->ag_features & AG_FEATURE_ENHANCED_CALL_CONTROL) {
> +		sprintf(buf, "AT+CHLD=1%d", id);

Should we be checking CHLD_1X feature?

> +
> +		g_at_chat_send(vd->chat, buf, none_prefix,
> +					release_id_cb, req, g_free);
> +	} else if (id == (int) vd->call->id)
> +		g_at_chat_send(vd->chat, "AT+CHUP", none_prefix,
> +					release_id_cb, req, g_free);

In the case of a single active call we already send a CHUP.  Is this really 
necessary here?

> +	else
> +		/* If the call is held, swap and then release it */
> +		g_at_chat_send(vd->chat, "AT+CHLD=2", none_prefix,
> +					swap_hold_cb, req, NULL);

This breaks if the held call is a mpty call, since the entire mpty call is 
released.  You also can't do this if a waiting call exists.


Regards,
-Denis

RE: [PATCH 1/3] Add multi calls support for HFP voicecall driver

[Zhang, Zhenhua]

[-- Attachment #1: Type: text/plain, Size: 5490 bytes --]

Hi Denis,

Denis Kenzior wrote:
> Hi Zhenhua,
> 
>> +static void reset_to_single_call(struct voicecall_data *vd) {
>> +       struct ofono_call *oc = vd->calls->data;
>> +
>> +       vd->call = oc;
>> +       vd->cind_val[HFP_INDICATOR_CALL] = 1;
>> +
>> +       if (oc->direction == 1)
>> +               vd->cind_val[HFP_INDICATOR_CALLSETUP] = 1; +      
>> else +               vd->cind_val[HFP_INDICATOR_CALLSETUP] = 3; }
>> +
> 
> I don't see how this is supposed to work.  What if the last
> call is a held call?

Don't see any problem if it is a held call. Still can hang up correctly.
What's the problem you have see.

>> +		if (c) {
>> +			nc = c->data;
>> +
>> +			if (memcmp(nc, oc, sizeof(struct ofono_call))) { +				oc->status
>> = nc->status; +
>> +				/* If phone does not support
> multiparty call,
>> +				 * there will have only one
> active call at one
>> +				 * time. +CHUP only hang up
> current active call.
>> +				 */
>> +				if (oc->status == CALL_STATUS_ACTIVE)
>> +					vd->call = oc;
> 
> Lets not set this silly variable anymore.  It has way too many
> meanings and uses throughout the code (most of which can be
> easily simplified) for the logic to be understandable.

As discussed on IRC, I will rename vd->call to vd->active. But the src/voicecall.c
has the similar variable in struct ofono_voicecall too.

>> +static void hfp_hold_all_active(struct ofono_voicecall *vc,
>> +				ofono_voicecall_cb_t cb, void *data) {
>> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc); +
>> +	if (check_mpty_feature(vc, AG_CHLD_2, cb, data)) {
>> +		hfp_template("AT+CHLD=2", vc, generic_cb, 0, cb, data); +
>> +		/* Some phones fail to send response back after CHLD=2.
>> +		 * But if we have another dialing out call, we should hold
>> +		 * +CLCC until AG creates the new call.
>> +		 */
>> +		vd->clcc_source = g_timeout_add(POLL_CLCC_INTERVAL,
>> +						poll_clcc, vc);
> 
> This comment makes no sense.  If the phone doesn't respond
> after CHLD=2 the parser is stuck and we're not sending CLCC out
> anyway. 

I will update the comments. It means phone should send update notification
Like callheld=1. But some phones doesn't send CIEV update.

>> +static void hfp_set_udub(struct ofono_voicecall *vc,
>> +				ofono_voicecall_cb_t cb, void *data) {
>> +	unsigned int incoming_or_waiting = (1 << 4) | (1 << 5);
>> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
>> +	struct ofono_call *call = vd->call;
>> +	struct release_id_req *req;
>> +
>> +	if (vd->ag_mpty_features & AG_CHLD_0)
>> +		hfp_template("AT+CHLD=0", vc, generic_cb, incoming_or_waiting,
>> +					cb, data); +	else {
>> +		req = g_try_new0(struct release_id_req, 1);
>> +
>> +		if (!req || !call) {
>> +			CALLBACK_WITH_FAILURE(cb, data);
>> +			return;
>> +		}
>> +
>> +		req->vc = vc;
>> +		req->cb = cb;
>> +		req->data = data;
>> +		req->id = call->id;
>> +
>> +		g_at_chat_send(vd->chat, "AT+CHUP", none_prefix,
>> +					release_id_cb, req, g_free);
>> +	}
> 
> The core is quite specifically asking to set UDUB, not to
> hangup the call.  If this is not supported, then we should say
> so.  Please get rid of this.

Ok. Remove it and report errors.

>> +}
>> +
>> +static void hfp_release_all_active(struct ofono_voicecall *vc,
>> +					ofono_voicecall_cb_t
> cb, void *data) {
>> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc); +
>> +	if (check_mpty_feature(vc, AG_CHLD_1, cb, data)) {
>> +		hfp_template("AT+CHLD=1", vc, generic_cb, 0x1, cb, data); +
>> +		vd->clcc_source =
> g_timeout_add(POLL_CLCC_INTERVAL, poll_clcc,
>> +						vc);
> 
> Why are we scheduling the CLCC before the CHLD returned?  We
> should wait until CHLD fails or succeeds.

It doesn't matter if we have 1 extra CLCC even if CHLD=1 is failed. Anyway,
I will move it into generic_cb.

>> +static void hfp_release_specific(struct ofono_voicecall *vc, int id,
>> +				ofono_voicecall_cb_t cb, void *data) {
>> +	if (vd->ag_features & AG_FEATURE_ENHANCED_CALL_CONTROL) {
>> +		sprintf(buf, "AT+CHLD=1%d", id);
> 
> Should we be checking CHLD_1X feature?

You mean checking CHLD_1X instead of enhanced_call_control? In theoriy,
They are the same. I don't think the phone has CHLD_1X without CHLD=2X.
;-)

>> +
>> +		g_at_chat_send(vd->chat, buf, none_prefix,
>> +					release_id_cb, req, g_free);
>> +	} else if (id == (int) vd->call->id)
>> +		g_at_chat_send(vd->chat, "AT+CHUP", none_prefix,
>> +					release_id_cb, req, g_free);
> 
> In the case of a single active call we already send a CHUP.
> Is this really necessary here?

It could be 3-way calls and phone only support CHLD=0, 1 and 2.
So we cannot use CHLD=1x call. If it is 3-way call, we support release
either active call or held call.

>> +	else
>> +		/* If the call is held, swap and then release it */
>> +		g_at_chat_send(vd->chat, "AT+CHLD=2", none_prefix,
>> +					swap_hold_cb, req, NULL);
> 
> This breaks if the held call is a mpty call, since the entire
> mpty call is released.  You also can't do this if a waiting
> call exists.
>

If it is mpty call, phone have already support CHLD=1x. Otherwise, it
could not use CHLD=3 to create mpty call. So this case should never
happen.

> 
> Regards,
> -Denis
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> http://lists.ofono.org/listinfo/ofono

Regards,
Zhenhua