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

[Denis Kenzior <denkenz@gmail.com>]

[-- 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