Re: [PATCH v2 7/8] voicecall: add +CHUP support for HFP emulator

[Frederic Danis <frederic.danis@linux.intel.com>]

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

Hello Denis,

Le 11/04/2011 18:13, Denis Kenzior a écrit :
> Hi Frédéric,
>
> On 04/07/2011 11:33 AM, Frédéric Danis wrote:
>> ---
>>   src/voicecall.c |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 108 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/voicecall.c b/src/voicecall.c
>> index cf02bb6..8aec0a0 100644
>> --- a/src/voicecall.c
>> +++ b/src/voicecall.c
>> @@ -60,6 +60,7 @@ struct ofono_voicecall {
>>   	GQueue *toneq;
>>   	guint tone_source;
>>   	unsigned int hfp_watch;
>> +	GSList *hfp_release_list;
>
> I'd rather not do this, you're not really avoiding a race condition with
> MPTY hangups or HangupAll here.  I'd rather we re-use release_queue but
> add additional sanity checking.  Perhaps adding a callback parameter to
> voicecalls_release_next might make this easy.  In general we need to
> think some more about how to avoid clashes between the D-Bus API and the
> emulator.
>
>>   };
>>
>>   struct voicecall {
>> @@ -95,6 +96,11 @@ struct tone_queue_entry {
>>   	int id;
>>   };
>>
>> +struct emulator_release {
>> +	struct ofono_voicecall *vc;
>> +	struct ofono_emulator *em;
>> +};
>> +
>>   static const char *default_en_list[] = { "911", "112", NULL };
>>   static const char *default_en_list_no_sim[] = { "119", "118", "999", "110",
>>   						"08", "000", NULL };
>> @@ -102,6 +108,7 @@ static const char *default_en_list_no_sim[] = { "119", "118", "999", "110",
>>   static void generic_callback(const struct ofono_error *error, void *data);
>>   static void multirelease_callback(const struct ofono_error *err, void *data);
>>   static gboolean tone_request_run(gpointer user_data);
>> +static void emulator_release_next(struct emulator_release *er);
>>
>>   static gint call_compare_by_id(gconstpointer a, gconstpointer b)
>>   {
>> @@ -2344,6 +2351,10 @@ static void voicecall_unregister(struct ofono_atom *atom)
>>   						OFONO_ATOM_TYPE_EMULATOR_HFP,
>>   						emulator_remove_handler,
>>   						"A");
>> +	__ofono_modem_foreach_registered_atom(modem,
>> +						OFONO_ATOM_TYPE_EMULATOR_HFP,
>> +						emulator_remove_handler,
>> +						"+CHUP");
>>
>>   	__ofono_modem_remove_atom_watch(modem, vc->hfp_watch);
>>
>> @@ -2558,6 +2569,102 @@ fail:
>>   	};
>>   }
>>
>> +static void emulator_multirelease_callback(const struct ofono_error *error,
>> +						void *data)
>> +{
>> +	struct emulator_release *er = data;
>> +	struct ofono_voicecall *vc = er->vc;
>> +
>> +	if (vc->hfp_release_list != NULL) {
>> +		emulator_release_next(er);
>> +		return;
>> +	}
>> +
>> +	emulator_generic_cb(error, er->em);
>> +
>> +	g_free(er);
>> +}
>> +
>> +static void emulator_release_next(struct emulator_release *er)
>> +{
>> +	struct ofono_voicecall *vc;
>> +	struct voicecall *call;
>> +
>> +	vc = er->vc;
>> +
>> +	call = vc->hfp_release_list->data;
>> +
>> +	vc->hfp_release_list = g_slist_remove(vc->hfp_release_list, call);
>> +
>> +	vc->driver->release_specific(vc, call->call->id,
>> +				emulator_multirelease_callback, er);
>> +}
>> +
>> +static void emulator_chup_cb(struct ofono_emulator *em,
>> +			struct ofono_emulator_request *req, void *userdata)
>> +{
>> +	struct ofono_voicecall *vc = userdata;
>> +	struct ofono_error result;
>> +	struct emulator_release *er;
>> +	GSList *l;
>> +	struct voicecall *call;
>> +
>> +	result.error = 0;
>> +
>> +	switch (ofono_emulator_request_get_type(req)) {
>> +	case OFONO_EMULATOR_REQUEST_TYPE_COMMAND_ONLY:
>> +		if (vc->driver->release_specific == NULL&&
>> +				vc->driver->hangup_active == NULL)
>> +			goto fail;
>> +
>> +		if (vc->driver->hangup_active) {
>> +			vc->driver->hangup_active(vc, emulator_generic_cb, em);
>> +			goto done;
>> +		}
>> +
>> +		/* if there is already a CHUP pending we return an error */
>> +		if (vc->hfp_release_list)
>> +			goto fail;
>
> Why are you checking this here?  Shouldn't you check this before
> potentially sending a hangup_active?
As far as I understand AT+CHUP, it releases all active calls (active + 
incoming + dialing + alerting), so we do not need a call list and are 
not able to know that we already perform one.

Is it correct ?
>
>> +
>> +		er = g_try_new0(struct emulator_release, 1);
>
> For simple structures (e.g. under 1k or so) it is fine to simply use g_new0
>
>> +		if (er == NULL) {
>> +			ofono_error("Unable to allocate release structure");
>> +			goto fail;
>> +		}
>> +
>> +		er->vc = vc;
>> +		er->em = em;
>> +
>> +		for (l = vc->call_list; l; l = l->next) {
>> +			call = l->data;
>> +
>> +			if (call->call->status == CALL_STATUS_WAITING ||
>> +					call->call->status == CALL_STATUS_HELD)
>> +				continue;
>> +
>> +			vc->hfp_release_list = g_slist_prepend(
>> +						vc->hfp_release_list,
>> +						l->data);
>> +		}
>> +
>> +		if (vc->hfp_release_list == NULL) {
>> +			g_free(er);
>> +			goto fail;
>> +		}
>> +
>> +		emulator_release_next(er);
>> +
>> +done:
>> +		dial_request_user_cancel(vc, NULL);
>> +		break;
>> +
>> +	default:
>> +fail:
>> +		result.type = OFONO_ERROR_TYPE_FAILURE;
>> +		ofono_emulator_send_final(em,&result);
>> +	};
>> +}
>> +
>>   static void emulator_hfp_watch(struct ofono_atom *atom,
>>   				enum ofono_atom_watch_condition cond,
>>   				void *data)
>> @@ -2570,6 +2677,7 @@ static void emulator_hfp_watch(struct ofono_atom *atom,
>>   	notify_emulator_call_status(data);
>>
>>   	ofono_emulator_add_handler(em, "A", emulator_ata_cb, data, NULL);
>> +	ofono_emulator_add_handler(em, "+CHUP", emulator_chup_cb, data, NULL);
>>   }
>>
>>   void ofono_voicecall_register(struct ofono_voicecall *vc)
>
> Regards,
> -Denis

Regards

Fred

-- 
Frederic Danis                            Open Source Technology Centre
frederic.danis(a)intel.com                              Intel Corporation