[RFC] STE-driver: Terminating voice call in state DIALING/ALERTING.

[RFC] STE-driver: Terminating voice call in state DIALING/ALERTING.

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch splits the callback hangup into hangup_all and hangup_active.

---
Hi Denis,
I'm picking up a really old thread here...

> Denis Kenzior wrote:
>>>> c) If you have an call on hold and initiate a new call, but want to
>>>> terminate the newly initiated call (DIALING), then this call cannot
>>>> be terminated with CHLD=1X, but you would have to use ATH (or
>>>> possible CHUP).
>>>
>>> Yes, so this is the case that we do need to take care of in the core.
>>> Most
>>> modems let us get away with sending release_specific up to this point.
>>
>> For the STE modem, calls in state DIALING and ALERTING will have to be
>>  terminated with ATH or AT+CHUP, AT+CHLD=1x does not work. This means that
>>  the current implementation, using release_specific (and thus AT+CHLD=1x)
>>  will not work.
>
> Yep, so please critique my earlier suggestion of splitting up hangup
> into two methods: hangup_all and hangup_active.  Modem drivers will need to
> provide one or the other or both.
> The core can then use the hangup_active (if available) in those cases where
> release_specific might not work.  The condition for hangup_active will be that
> it does not affect held or waiting calls.  For ST-E the hangup_active
> implementation will simply be +CHUP. For modems that do not have this
> available we will fall back to release_specific and assume that on those
> CHLD=1X or equivalent can affect dialing/alerting calls.

If I understand you correctly STE driver should implement
hangup_active using +CUP. Core should then use hangup_active on calls
in state DIALING and ALERTING - Yes, I think this would work.

> hangup_all can also be used for cases where we loop release_specific
> over all active/dialing/alerting/held/incoming calls.  For ST-E the hangup_all
> implementation might be +CHUP;CHLD=1n;...;+CHLD=1m where n, m, etc are
> ids of the HELD calls.  We should not hangup waiting calls to be compliant
> with section 6.5.5.1 of 3GPP 22.030:
> "Entering END	-Releases the subscriber from all calls
> (except a possible waiting call)."

I guess the stedriver will not implement the hangup_all because the STE
modem does not have an AT command for terminating all calls. (I assume you
don't want the driver to iterate over calls constructing a command like:
"+CHUP;CHLD=1n;...;+CHLD=1m").

I've probably not understood your proposal fully, but I have tried to
put together some code based on it. Please have a look and check if 
you had something like this in mind.

However, I think we can solve this even simpler if we just
call hangup for any ALERTING/DIALING call..

Regards
Sjur

diff --git a/drivers/stemodem/voicecall.c b/drivers/stemodem/voicecall.c
index a56709a..2ddae05 100644
--- a/drivers/stemodem/voicecall.c
+++ b/drivers/stemodem/voicecall.c
@@ -260,7 +260,7 @@ static void ste_answer(struct ofono_voicecall *vc,
 	ste_template("ATA", vc, ste_generic_cb, 0, cb, data);
 }
 
-static void ste_hangup(struct ofono_voicecall *vc,
+static void ste_hangup_active(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data)
 {
 	ste_template("AT+CHUP", vc, ste_generic_cb, 0x3f, cb, data);
@@ -569,7 +569,7 @@ static struct ofono_voicecall_driver driver = {
 	.remove			= ste_voicecall_remove,
 	.dial			= ste_dial,
 	.answer			= ste_answer,
-	.hangup			= ste_hangup,
+	.hangup_active		= ste_hangup_active,
 	.hold_all_active	= ste_hold_all_active,
 	.release_all_held	= ste_release_all_held,
 	.set_udub		= ste_set_udub,
diff --git a/include/voicecall.h b/include/voicecall.h
index 6ceb3d8..2500e5f 100644
--- a/include/voicecall.h
+++ b/include/voicecall.h
@@ -67,7 +67,11 @@ struct ofono_voicecall_driver {
 			ofono_voicecall_cb_t cb, void *data);
 	void (*answer)(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data);
-	void (*hangup)(struct ofono_voicecall *vc,
+	void (*hangup_all)(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data);
+/* HACK: for temporary backwards compability */
+#define hangup_active hangup
+	void (*hangup_active)(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data);
 	void (*hold_all_active)(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data);
diff --git a/src/voicecall.c b/src/voicecall.c
index a30aaa5..a420777 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -264,12 +264,12 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
 	/* According to various specs, other than 27.007, +CHUP is used
 	 * to reject an incoming call
 	 */
-	if (call->status == CALL_STATUS_INCOMING) {
+	if (vc->driver->hangup_active && call->status == CALL_STATUS_INCOMING) {
 		if (vc->driver->hangup == NULL)
 			return __ofono_error_not_implemented(msg);
 
 		vc->pending = dbus_message_ref(msg);
-		vc->driver->hangup(vc, generic_callback, vc);
+		vc->driver->hangup_active(vc, generic_callback, vc);
 
 		return NULL;
 	}
@@ -286,12 +286,12 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
 
 	num_calls = g_slist_length(vc->call_list);
 
-	if (num_calls == 1 && vc->driver->hangup &&
+	if (vc->driver->hangup_active && num_calls == 1 && vc->driver->hangup &&
 			(call->status == CALL_STATUS_ACTIVE ||
 				call->status == CALL_STATUS_DIALING ||
 				call->status == CALL_STATUS_ALERTING)) {
 		vc->pending = dbus_message_ref(msg);
-		vc->driver->hangup(vc, generic_callback, vc);
+		vc->driver->hangup_active(vc, generic_callback, vc);
 
 		return NULL;
 	}
@@ -304,6 +304,16 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
 		return NULL;
 	}
 
+	if (vc->driver->hangup_active != NULL &&
+		(call->status == CALL_STATUS_ALERTING ||
+			call->status == CALL_STATUS_DIALING)) {
+
+		vc->pending = dbus_message_ref(msg);
+		vc->driver->hangup_active(vc, generic_callback, vc);
+
+		return NULL;
+	}
+
 	if (vc->driver->release_specific == NULL)
 		return __ofono_error_not_implemented(msg);
 
@@ -762,7 +772,14 @@ static void voicecalls_release_next(struct ofono_voicecall *vc)
 
 	vc->release_list = g_slist_remove(vc->release_list, call);
 
-	vc->driver->release_specific(vc, call->call->id,
+	if (vc->driver->hangup_active != NULL &&
+		(call->call->status == CALL_STATUS_ALERTING ||
+			call->call->status == CALL_STATUS_DIALING ||
+			call->call->status == CALL_STATUS_INCOMING))
+
+		vc->driver->hangup_active(vc, multirelease_callback, vc);
+	else
+		vc->driver->release_specific(vc, call->call->id,
 						multirelease_callback, vc);
 }
 
@@ -1131,9 +1148,12 @@ static DBusMessage *manager_hangup_all(DBusConnection *conn,
 
 	vc->pending = dbus_message_ref(msg);
 
-	voicecalls_release_queue(vc, vc->call_list);
-	voicecalls_release_next(vc);
-
+	if (vc->driver->hangup_all != NULL)
+		vc->driver->hangup_all(vc, generic_callback, vc);
+	else {
+		voicecalls_release_queue(vc, vc->call_list);
+		voicecalls_release_next(vc);
+	}
 	return NULL;
 }
 
-- 
1.6.3.3

Re: [RFC] STE-driver: Terminating voice call in state DIALING/ALERTING.

[Denis Kenzior]

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

Hi Sjur,

On 08/04/2010 09:21 AM, Sjur Brændeland wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> This patch splits the callback hangup into hangup_all and hangup_active.
> 
> ---
> Hi Denis,
> I'm picking up a really old thread here...

Yes a really old thread ;)

> 
>> Denis Kenzior wrote:
>>>>> c) If you have an call on hold and initiate a new call, but want to
>>>>> terminate the newly initiated call (DIALING), then this call cannot
>>>>> be terminated with CHLD=1X, but you would have to use ATH (or
>>>>> possible CHUP).
>>>>
>>>> Yes, so this is the case that we do need to take care of in the core.
>>>> Most
>>>> modems let us get away with sending release_specific up to this point.
>>>
>>> For the STE modem, calls in state DIALING and ALERTING will have to be
>>>  terminated with ATH or AT+CHUP, AT+CHLD=1x does not work. This means that
>>>  the current implementation, using release_specific (and thus AT+CHLD=1x)
>>>  will not work.
>>
>> Yep, so please critique my earlier suggestion of splitting up hangup
>> into two methods: hangup_all and hangup_active.  Modem drivers will need to
>> provide one or the other or both.
>> The core can then use the hangup_active (if available) in those cases where
>> release_specific might not work.  The condition for hangup_active will be that
>> it does not affect held or waiting calls.  For ST-E the hangup_active
>> implementation will simply be +CHUP. For modems that do not have this
>> available we will fall back to release_specific and assume that on those
>> CHLD=1X or equivalent can affect dialing/alerting calls.
> 
> If I understand you correctly STE driver should implement
> hangup_active using +CUP. Core should then use hangup_active on calls
> in state DIALING and ALERTING - Yes, I think this would work.
> 
>> hangup_all can also be used for cases where we loop release_specific
>> over all active/dialing/alerting/held/incoming calls.  For ST-E the hangup_all
>> implementation might be +CHUP;CHLD=1n;...;+CHLD=1m where n, m, etc are
>> ids of the HELD calls.  We should not hangup waiting calls to be compliant
>> with section 6.5.5.1 of 3GPP 22.030:
>> "Entering END	-Releases the subscriber from all calls
>> (except a possible waiting call)."
> 
> I guess the stedriver will not implement the hangup_all because the STE
> modem does not have an AT command for terminating all calls. (I assume you
> don't want the driver to iterate over calls constructing a command like:
> "+CHUP;CHLD=1n;...;+CHLD=1m").
> 
> I've probably not understood your proposal fully, but I have tried to
> put together some code based on it. Please have a look and check if 
> you had something like this in mind.
> 
> However, I think we can solve this even simpler if we just
> call hangup for any ALERTING/DIALING call..

This is a real gray area.  On some devices this will work, on others it
might have un-intentional consequences.  At least on the calypso,
sending CHUP/ATH will terminate all calls not in the WAITING state.

> 
> Regards
> Sjur
> 
> diff --git a/drivers/stemodem/voicecall.c b/drivers/stemodem/voicecall.c
> index a56709a..2ddae05 100644
> --- a/drivers/stemodem/voicecall.c
> +++ b/drivers/stemodem/voicecall.c
> @@ -260,7 +260,7 @@ static void ste_answer(struct ofono_voicecall *vc,
>  	ste_template("ATA", vc, ste_generic_cb, 0, cb, data);
>  }
>  
> -static void ste_hangup(struct ofono_voicecall *vc,
> +static void ste_hangup_active(struct ofono_voicecall *vc,
>  			ofono_voicecall_cb_t cb, void *data)
>  {
>  	ste_template("AT+CHUP", vc, ste_generic_cb, 0x3f, cb, data);
> @@ -569,7 +569,7 @@ static struct ofono_voicecall_driver driver = {
>  	.remove			= ste_voicecall_remove,
>  	.dial			= ste_dial,
>  	.answer			= ste_answer,
> -	.hangup			= ste_hangup,
> +	.hangup_active		= ste_hangup_active,
>  	.hold_all_active	= ste_hold_all_active,
>  	.release_all_held	= ste_release_all_held,
>  	.set_udub		= ste_set_udub,
> diff --git a/include/voicecall.h b/include/voicecall.h
> index 6ceb3d8..2500e5f 100644
> --- a/include/voicecall.h
> +++ b/include/voicecall.h
> @@ -67,7 +67,11 @@ struct ofono_voicecall_driver {
>  			ofono_voicecall_cb_t cb, void *data);
>  	void (*answer)(struct ofono_voicecall *vc,
>  			ofono_voicecall_cb_t cb, void *data);
> -	void (*hangup)(struct ofono_voicecall *vc,
> +	void (*hangup_all)(struct ofono_voicecall *vc,
> +			ofono_voicecall_cb_t cb, void *data);
> +/* HACK: for temporary backwards compability */
> +#define hangup_active hangup
> +	void (*hangup_active)(struct ofono_voicecall *vc,
>  			ofono_voicecall_cb_t cb, void *data);
>  	void (*hold_all_active)(struct ofono_voicecall *vc,
>  			ofono_voicecall_cb_t cb, void *data);
> diff --git a/src/voicecall.c b/src/voicecall.c
> index a30aaa5..a420777 100644
> --- a/src/voicecall.c
> +++ b/src/voicecall.c
> @@ -264,12 +264,12 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
>  	/* According to various specs, other than 27.007, +CHUP is used
>  	 * to reject an incoming call
>  	 */
> -	if (call->status == CALL_STATUS_INCOMING) {
> +	if (vc->driver->hangup_active && call->status == CALL_STATUS_INCOMING) {

You're breaking the logic somewhat here.  If the call is INCOMING, we
shouldn't skip the rest of the block if hangup_active is not implemented.

>  		if (vc->driver->hangup == NULL)
>  			return __ofono_error_not_implemented(msg);
>  
>  		vc->pending = dbus_message_ref(msg);
> -		vc->driver->hangup(vc, generic_callback, vc);
> +		vc->driver->hangup_active(vc, generic_callback, vc);
>  
>  		return NULL;
>  	}

Here we need to check whether hangup_active or hangup_all are
implemented. If both are, then prefer hangup_all.  This would make it
easier to keep compatibility with current drivers.

> @@ -286,12 +286,12 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
>  
>  	num_calls = g_slist_length(vc->call_list);
>  
> -	if (num_calls == 1 && vc->driver->hangup &&
> +	if (vc->driver->hangup_active && num_calls == 1 && vc->driver->hangup &&

This should probably be a condition something like:

if (num_calls == 1 && (vc->driver->hangup || vc->driver->hangup_active)
...

>  			(call->status == CALL_STATUS_ACTIVE ||
>  				call->status == CALL_STATUS_DIALING ||
>  				call->status == CALL_STATUS_ALERTING)) {
>  		vc->pending = dbus_message_ref(msg);
> -		vc->driver->hangup(vc, generic_callback, vc);
> +		vc->driver->hangup_active(vc, generic_callback, vc);

And again prefer hangup_all over hangup_active to keep compatibility
with old drivers.

>  
>  		return NULL;
>  	}
> @@ -304,6 +304,16 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
>  		return NULL;
>  	}
>  
> +	if (vc->driver->hangup_active != NULL &&
> +		(call->status == CALL_STATUS_ALERTING ||
> +			call->status == CALL_STATUS_DIALING)) {
> +
> +		vc->pending = dbus_message_ref(msg);
> +		vc->driver->hangup_active(vc, generic_callback, vc);
> +
> +		return NULL;
> +	}
> +

This seems reasonable

>  	if (vc->driver->release_specific == NULL)
>  		return __ofono_error_not_implemented(msg);
>  
> @@ -762,7 +772,14 @@ static void voicecalls_release_next(struct ofono_voicecall *vc)
>  
>  	vc->release_list = g_slist_remove(vc->release_list, call);
>  
> -	vc->driver->release_specific(vc, call->call->id,
> +	if (vc->driver->hangup_active != NULL &&
> +		(call->call->status == CALL_STATUS_ALERTING ||
> +			call->call->status == CALL_STATUS_DIALING ||
> +			call->call->status == CALL_STATUS_INCOMING))

This reminds me that we should treat INCOMING calls in HangupAll
specially.  It should not be handled here.  HangupAll should also skip
waiting calls.

> +
> +		vc->driver->hangup_active(vc, multirelease_callback, vc);
> +	else
> +		vc->driver->release_specific(vc, call->call->id,
>  						multirelease_callback, vc);
>  }
>  
> @@ -1131,9 +1148,12 @@ static DBusMessage *manager_hangup_all(DBusConnection *conn,
>  
>  	vc->pending = dbus_message_ref(msg);
>  
> -	voicecalls_release_queue(vc, vc->call_list);
> -	voicecalls_release_next(vc);
> -
> +	if (vc->driver->hangup_all != NULL)
> +		vc->driver->hangup_all(vc, generic_callback, vc);
> +	else {
> +		voicecalls_release_queue(vc, vc->call_list);
> +		voicecalls_release_next(vc);
> +	}

This looks reasonable.

>  	return NULL;
>  }
>  

Regards,
-Denis

Re: [RFC] STE-driver: Terminating voice call in state DIALING/ALERTING.

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Denis,

Denis Kenzior wrote:
>> I'm picking up a really old thread here...
>Yes a really old thread ;)
Better late than never, right? :-)

...
>> However, I think we can solve this even simpler if we just
>> call hangup for any ALERTING/DIALING call..
>This is a real gray area.  On some devices this will work, on others it
>might have un-intentional consequences.  At least on the calypso,
>sending CHUP/ATH will terminate all calls not in the WAITING state.

Ok, so we should go forward with this proposal then.
I'll try to send a new RFC within the next couple of days.

My initial intention here was not to submit patches
on src/voicecall.c, but to make sure I understood your proposal properly.
Let me know how you think we should go forward with this, as this
impacts all drivers/*/voicecall.c

>> -	 if (call->status == CALL_STATUS_INCOMING) {
>> +	 if (vc->driver->hangup_active && call->status == CALL_STATUS_INCOMING) {
>
>You're breaking the logic somewhat here.  If the call is INCOMING, we
>shouldn't skip the rest of the block if hangup_active is not implemented.
>
>>		 if (vc->driver->hangup == NULL)
>>			 return __ofono_error_not_implemented(msg);
>>
>>		 vc->pending = dbus_message_ref(msg);
>> -		 vc->driver->hangup(vc, generic_callback, vc);
>> +		 vc->driver->hangup_active(vc, generic_callback, vc);
>>
>>		 return NULL;
>>	 }
>
>Here we need to check whether hangup_active or hangup_all are
>implemented. If both are, then prefer hangup_all.  This would make it
>easier to keep compatibility with current drivers.

Did you have something like this in mind then:

	if (call->status == CALL_STATUS_INCOMING) {
		vc->pending = dbus_message_ref(msg);
		if (vc->driver->hangup_all)
			vc->driver->hangup_all(vc, generic_callback, vc);
		else if (vc->driver->hangup_active)
			vc->driver->hangup_active(vc, generic_callback, vc);
		else
			return __ofono_error_not_implemented(msg);

		return NULL;
	}

Should we do not_implemented here or did you intend the drivers to be allowed
to not implement hangup_active nor hangup_all, and fall through to
release_specific?

>
>> @@ -286,12 +286,12 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
>>
>>	 num_calls = g_slist_length(vc->call_list);
>>
>> -	 if (num_calls == 1 && vc->driver->hangup &&
>> +	 if (vc->driver->hangup_active && num_calls == 1 && vc->driver->hangup &&
>
>This should probably be a condition something like:
>
>if (num_calls == 1 && (vc->driver->hangup || vc->driver->hangup_active)
>...
>
>>			 (call->status == CALL_STATUS_ACTIVE ||
>>				 call->status == CALL_STATUS_DIALING ||
>>				 call->status == CALL_STATUS_ALERTING)) {
>>		 vc->pending = dbus_message_ref(msg);
>> -		 vc->driver->hangup(vc, generic_callback, vc);
>> +		 vc->driver->hangup_active(vc, generic_callback, vc);
>
>And again prefer hangup_all over hangup_active to keep compatibility
>with old drivers.


Something like this then:
if (num_calls == 1 && (vc->driver->hangup_all || vc->driver->hangup_active) &&
			(call->status == CALL_STATUS_ACTIVE ||
				call->status == CALL_STATUS_DIALING ||
				call->status == CALL_STATUS_ALERTING)) {
		vc->pending = dbus_message_ref(msg);
		 if (vc->driver->hangup_all)
			vc->driver->hangup_all(vc, generic_callback, vc);
		 else if (vc->driver->hangup_active)
			vc->driver->hangup_active(vc, generic_callback, vc);
		 else
			return __ofono_error_not_implemented(msg);

>This reminds me that we should treat INCOMING calls in HangupAll
>specially. It should not be handled here.

What did you have in mind?

>HangupAll should also skip waiting calls.

voicecalls_release_queue and voicecalls_release_next are used for
multiparty_hangup as well, I assume you want the same behaviour for multi-party
so that we can do these changes in voicecalls_release_queue, right?

Regards
Sjur

Re: [RFC] STE-driver: Terminating voice call in state DIALING/ALERTING.

[Denis Kenzior]

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

Hi Sjur,

On 08/05/2010 02:04 PM, Sjur Brændeland wrote:
> Hi Denis,
> 
> Denis Kenzior wrote:
>>> I'm picking up a really old thread here...
>> Yes a really old thread ;)
> Better late than never, right? :-)
> 
> ...

Definitely :)  I'm very glad you brought this back up.

>>> However, I think we can solve this even simpler if we just
>>> call hangup for any ALERTING/DIALING call..
>> This is a real gray area.  On some devices this will work, on others it
>> might have un-intentional consequences.  At least on the calypso,
>> sending CHUP/ATH will terminate all calls not in the WAITING state.
> 
> Ok, so we should go forward with this proposal then.
> I'll try to send a new RFC within the next couple of days.
> 
> My initial intention here was not to submit patches
> on src/voicecall.c, but to make sure I understood your proposal properly.
> Let me know how you think we should go forward with this, as this
> impacts all drivers/*/voicecall.c
> 

Yes, I think this definitely makes sense.  There might be some other
modems we don't cover properly (some don't allow HELD calls to be
terminated using CHLD=1X), but we cross that bridge when we come to it.

>>> -	 if (call->status == CALL_STATUS_INCOMING) {
>>> +	 if (vc->driver->hangup_active && call->status == CALL_STATUS_INCOMING) {
>>
>> You're breaking the logic somewhat here.  If the call is INCOMING, we
>> shouldn't skip the rest of the block if hangup_active is not implemented.
>>
>>> 		 if (vc->driver->hangup == NULL)
>>> 			 return __ofono_error_not_implemented(msg);
>>>
>>> 		 vc->pending = dbus_message_ref(msg);
>>> -		 vc->driver->hangup(vc, generic_callback, vc);
>>> +		 vc->driver->hangup_active(vc, generic_callback, vc);
>>>
>>> 		 return NULL;
>>> 	 }
>>
>> Here we need to check whether hangup_active or hangup_all are
>> implemented. If both are, then prefer hangup_all.  This would make it
>> easier to keep compatibility with current drivers.
> 
> Did you have something like this in mind then:
> 
> 	if (call->status == CALL_STATUS_INCOMING) {
> 		vc->pending = dbus_message_ref(msg);
> 		if (vc->driver->hangup_all)
> 			vc->driver->hangup_all(vc, generic_callback, vc);
> 		else if (vc->driver->hangup_active)
> 			vc->driver->hangup_active(vc, generic_callback, vc);
> 		else
> 			return __ofono_error_not_implemented(msg);
> 
> 		return NULL;
> 	}
> 
> Should we do not_implemented here or did you intend the drivers to be allowed
> to not implement hangup_active nor hangup_all, and fall through to
> release_specific?

I think doing not_implemented here is a good idea.  However, you should
not take the ref of the message if you're returning not_implemented.
Something like this would be better:

if (call->status == CALL_STATUS_INCOMING) {
	if (vc->driver->hangup_all == NULL &&
			vc->driver->hangup_active == NULL)
		return __ofono_error_not_implemented(msg);

	vc->pending = dbus_message_ref(msg);

	if (vc->driver->hangup_all)
		....
	else
		....

	return NULL;
}

> 
>>
>>> @@ -286,12 +286,12 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
>>>
>>> 	 num_calls = g_slist_length(vc->call_list);
>>>
>>> -	 if (num_calls == 1 && vc->driver->hangup &&
>>> +	 if (vc->driver->hangup_active && num_calls == 1 && vc->driver->hangup &&
>>
>> This should probably be a condition something like:
>>
>> if (num_calls == 1 && (vc->driver->hangup || vc->driver->hangup_active)
>> ...
>>
>>> 			 (call->status == CALL_STATUS_ACTIVE ||
>>> 				 call->status == CALL_STATUS_DIALING ||
>>> 				 call->status == CALL_STATUS_ALERTING)) {
>>> 		 vc->pending = dbus_message_ref(msg);
>>> -		 vc->driver->hangup(vc, generic_callback, vc);
>>> +		 vc->driver->hangup_active(vc, generic_callback, vc);
>>
>> And again prefer hangup_all over hangup_active to keep compatibility
>> with old drivers.
> 
> 
> Something like this then:
> if (num_calls == 1 && (vc->driver->hangup_all || vc->driver->hangup_active) &&
> 			(call->status == CALL_STATUS_ACTIVE ||
> 				call->status == CALL_STATUS_DIALING ||
> 				call->status == CALL_STATUS_ALERTING)) {
> 		vc->pending = dbus_message_ref(msg);
> 		 if (vc->driver->hangup_all)
> 			vc->driver->hangup_all(vc, generic_callback, vc);
> 		 else if (vc->driver->hangup_active)
> 			vc->driver->hangup_active(vc, generic_callback, vc);
> 		 else
> 			return __ofono_error_not_implemented(msg);
> 

Yep, but see the comment about dbus_message_ref above.

>> This reminds me that we should treat INCOMING calls in HangupAll
>> specially. It should not be handled here.
> 
> What did you have in mind?

I'm thinking of simply checking if there is an INCOMING call.  If so, it
is assumed to be a single call and using hangup_all / hangup_active is
sufficient.  This would also be more consistent with voicecall_hangup
implementation above.

> 
>> HangupAll should also skip waiting calls.
> 
> voicecalls_release_queue and voicecalls_release_next are used for
> multiparty_hangup as well, I assume you want the same behaviour for multi-party
> so that we can do these changes in voicecalls_release_queue, right?

Correct, however multiparty calls cannot be in the WAITING state.
Essentially we can just skip these by not queuing them on the release list.

> 
> Regards
> Sjur

Regards,
-Denis

[RFCv2] STE-driver: Terminating voice call in state DIALING/ALERTING.

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

From: Sjur Brændeland <sjur.brandeland@stericsson.com>

This patch fixes problem with terminating calls in state DIALING/ALERTING
for STE-modem. The main change is that voicecall-callback function
hangup is split into the functions hangup_all and hangup_active.

CHANGES:
include/voicecall.h:
o hangup is replaced with hangup_active and hangup_all.
o As a tmp hack to provide backwards portability
  the previous hangup is defined to hangup_all

drivers/atmodem/voicecall.c:
o hangup_all uses ATH
o hangup_active uses CHUP

drivers/stemodem/voicecall.c:
o hangup_active uses CHUP while hangup_all is not implemented.

src/voicecall.c:
o In cases where hangup previously was used, hangup_all is used if implemented
  otherwise hangup_active is used.
o Call in state DIALING/ALERTING is released with hangup_active if implemented.
o manager_hangup_all will no longer release calls in state waiting.
o manager_hangup_all will simply call hangup_all if implemented.
o manager_hangup_all will release calls in state ALERTING/DIALING/INCOMING
  using hangup_active otherwise release_specific.
---
 drivers/atmodem/voicecall.c  |   14 ++++++--
 drivers/stemodem/voicecall.c |    4 +-
 include/voicecall.h          |    6 +++-
 src/voicecall.c              |   76 +++++++++++++++++++++++++++++++++--------
 4 files changed, 79 insertions(+), 21 deletions(-)

diff --git a/drivers/atmodem/voicecall.c b/drivers/atmodem/voicecall.c
index fce9144..80ce897 100644
--- a/drivers/atmodem/voicecall.c
+++ b/drivers/atmodem/voicecall.c
@@ -406,10 +406,17 @@ static void at_answer(struct ofono_voicecall *vc,
 	at_template("ATA", vc, generic_cb, 0, cb, data);
 }
 
-static void at_hangup(struct ofono_voicecall *vc,
+static void at_hangup_all(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data)
 {
-	/* Hangup all calls */
+	/* Hangup all calls, except waiting */
+	at_template("ATH", vc, generic_cb, 0x3f, cb, data);
+}
+
+static void at_hangup_active(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	/* Hangup  currently active call */
 	at_template("AT+CHUP", vc, generic_cb, 0x3f, cb, data);
 }
 
@@ -874,7 +881,8 @@ static struct ofono_voicecall_driver driver = {
 	.remove			= at_voicecall_remove,
 	.dial			= at_dial,
 	.answer			= at_answer,
-	.hangup			= at_hangup,
+	.hangup_active		= at_hangup_active,
+	.hangup_all		= at_hangup_all,
 	.hold_all_active	= at_hold_all_active,
 	.release_all_held	= at_release_all_held,
 	.set_udub		= at_set_udub,
diff --git a/drivers/stemodem/voicecall.c b/drivers/stemodem/voicecall.c
index a56709a..2ddae05 100644
--- a/drivers/stemodem/voicecall.c
+++ b/drivers/stemodem/voicecall.c
@@ -260,7 +260,7 @@ static void ste_answer(struct ofono_voicecall *vc,
 	ste_template("ATA", vc, ste_generic_cb, 0, cb, data);
 }
 
-static void ste_hangup(struct ofono_voicecall *vc,
+static void ste_hangup_active(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data)
 {
 	ste_template("AT+CHUP", vc, ste_generic_cb, 0x3f, cb, data);
@@ -569,7 +569,7 @@ static struct ofono_voicecall_driver driver = {
 	.remove			= ste_voicecall_remove,
 	.dial			= ste_dial,
 	.answer			= ste_answer,
-	.hangup			= ste_hangup,
+	.hangup_active		= ste_hangup_active,
 	.hold_all_active	= ste_hold_all_active,
 	.release_all_held	= ste_release_all_held,
 	.set_udub		= ste_set_udub,
diff --git a/include/voicecall.h b/include/voicecall.h
index 6ceb3d8..d43954c 100644
--- a/include/voicecall.h
+++ b/include/voicecall.h
@@ -67,7 +67,11 @@ struct ofono_voicecall_driver {
 			ofono_voicecall_cb_t cb, void *data);
 	void (*answer)(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data);
-	void (*hangup)(struct ofono_voicecall *vc,
+/* HACK: for temporary backwards compatibility */
+#define hangup_all hangup
+	void (*hangup_all)(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data);
+	void (*hangup_active)(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data);
 	void (*hold_all_active)(struct ofono_voicecall *vc,
 			ofono_voicecall_cb_t cb, void *data);
diff --git a/src/voicecall.c b/src/voicecall.c
index a30aaa5..6ba6bb7 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -261,15 +261,18 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
 	if (vc->pending)
 		return __ofono_error_busy(msg);
 
-	/* According to various specs, other than 27.007, +CHUP is used
-	 * to reject an incoming call
-	 */
 	if (call->status == CALL_STATUS_INCOMING) {
-		if (vc->driver->hangup == NULL)
+
+		if (vc->driver->hangup_all == NULL &&
+				vc->driver->hangup_active == NULL)
 			return __ofono_error_not_implemented(msg);
 
 		vc->pending = dbus_message_ref(msg);
-		vc->driver->hangup(vc, generic_callback, vc);
+
+		if (vc->driver->hangup_all)
+			vc->driver->hangup_all(vc, generic_callback, vc);
+		else
+			vc->driver->hangup_active(vc, generic_callback, vc);
 
 		return NULL;
 	}
@@ -286,12 +289,19 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
 
 	num_calls = g_slist_length(vc->call_list);
 
-	if (num_calls == 1 && vc->driver->hangup &&
+	if (num_calls == 1 &&
+			(vc->driver->hangup_all != NULL ||
+				vc->driver->hangup_active != NULL) &&
 			(call->status == CALL_STATUS_ACTIVE ||
 				call->status == CALL_STATUS_DIALING ||
 				call->status == CALL_STATUS_ALERTING)) {
+
 		vc->pending = dbus_message_ref(msg);
-		vc->driver->hangup(vc, generic_callback, vc);
+
+		if (vc->driver->hangup_all)
+			vc->driver->hangup_all(vc, generic_callback, vc);
+		else
+			vc->driver->hangup_active(vc, generic_callback, vc);
 
 		return NULL;
 	}
@@ -304,6 +314,15 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
 		return NULL;
 	}
 
+	if (vc->driver->hangup_active != NULL &&
+		(call->status == CALL_STATUS_ALERTING ||
+			call->status == CALL_STATUS_DIALING)) {
+		vc->pending = dbus_message_ref(msg);
+		vc->driver->hangup_active(vc, generic_callback, vc);
+
+		return NULL;
+	}
+
 	if (vc->driver->release_specific == NULL)
 		return __ofono_error_not_implemented(msg);
 
@@ -743,12 +762,18 @@ static void emit_multiparty_call_list_changed(struct ofono_voicecall *vc)
 static void voicecalls_release_queue(struct ofono_voicecall *vc, GSList *calls)
 {
 	GSList *l;
+	struct voicecall *call;
 
 	g_slist_free(vc->release_list);
 	vc->release_list = NULL;
 
-	for (l = calls; l; l = l->next)
-		vc->release_list = g_slist_prepend(vc->release_list, l->data);
+	for (l = calls; l; l = l->next) {
+		call = l->data;
+
+		if (call->call->status != CALL_STATUS_WAITING)
+			vc->release_list =
+				g_slist_prepend(vc->release_list, call);
+	}
 }
 
 static void voicecalls_release_next(struct ofono_voicecall *vc)
@@ -762,7 +787,14 @@ static void voicecalls_release_next(struct ofono_voicecall *vc)
 
 	vc->release_list = g_slist_remove(vc->release_list, call);
 
-	vc->driver->release_specific(vc, call->call->id,
+	if (vc->driver->hangup_active != NULL &&
+			(call->call->status == CALL_STATUS_ALERTING ||
+				call->call->status == CALL_STATUS_DIALING ||
+				call->call->status == CALL_STATUS_INCOMING))
+
+		vc->driver->hangup_active(vc, multirelease_callback, vc);
+	else
+		vc->driver->release_specific(vc, call->call->id,
 						multirelease_callback, vc);
 }
 
@@ -1119,7 +1151,9 @@ static DBusMessage *manager_hangup_all(DBusConnection *conn,
 	if (vc->pending)
 		return __ofono_error_busy(msg);
 
-	if (!vc->driver->release_specific)
+	if (vc->driver->hangup_all == NULL &&
+		(vc->driver->release_specific == NULL ||
+			vc->driver->hangup_active == NULL))
 		return __ofono_error_not_implemented(msg);
 
 	if (vc->call_list == NULL) {
@@ -1131,9 +1165,17 @@ static DBusMessage *manager_hangup_all(DBusConnection *conn,
 
 	vc->pending = dbus_message_ref(msg);
 
-	voicecalls_release_queue(vc, vc->call_list);
-	voicecalls_release_next(vc);
+	if (vc->driver->hangup_all != NULL)
+		vc->driver->hangup_all(vc, generic_callback, vc);
 
+	/* In case of INCOMING call, assume we only have this one call */
+	else if (voicecalls_have_incoming(vc) &&
+			vc->driver->hangup_active != NULL)
+		vc->driver->hangup_active(vc, generic_callback, vc);
+	else {
+		voicecalls_release_queue(vc, vc->call_list);
+		voicecalls_release_next(vc);
+	}
 	return NULL;
 }
 
@@ -1370,8 +1412,12 @@ static DBusMessage *multiparty_hangup(DBusConnection *conn,
 
 	/* Fall back to the old-fashioned way */
 	vc->flags |= VOICECALLS_FLAG_MULTI_RELEASE;
-	voicecalls_release_queue(vc, vc->multiparty_list);
-	voicecalls_release_next(vc);
+	if (vc->driver->hangup_all != NULL)
+		vc->driver->hangup_all(vc, generic_callback, vc);
+	else {
+		voicecalls_release_queue(vc, vc->call_list);
+		voicecalls_release_next(vc);
+	}
 
 out:
 	return NULL;
-- 
1.6.3.3

Re: [RFCv2] STE-driver: Terminating voice call in state DIALING/ALERTING.

[Denis Kenzior]

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

Hi Sjur,

On 08/06/2010 07:59 AM, Sjur Brændeland wrote:
> From: Sjur Brændeland <sjur.brandeland@stericsson.com>
> 
> This patch fixes problem with terminating calls in state DIALING/ALERTING
> for STE-modem. The main change is that voicecall-callback function
> hangup is split into the functions hangup_all and hangup_active.
> 
> CHANGES:
> include/voicecall.h:
> o hangup is replaced with hangup_active and hangup_all.
> o As a tmp hack to provide backwards portability
>   the previous hangup is defined to hangup_all
> 
> drivers/atmodem/voicecall.c:
> o hangup_all uses ATH
> o hangup_active uses CHUP
> 
> drivers/stemodem/voicecall.c:
> o hangup_active uses CHUP while hangup_all is not implemented.
> 
> src/voicecall.c:
> o In cases where hangup previously was used, hangup_all is used if implemented
>   otherwise hangup_active is used.
> o Call in state DIALING/ALERTING is released with hangup_active if implemented.
> o manager_hangup_all will no longer release calls in state waiting.
> o manager_hangup_all will simply call hangup_all if implemented.
> o manager_hangup_all will release calls in state ALERTING/DIALING/INCOMING
>   using hangup_active otherwise release_specific.
> ---
>  drivers/atmodem/voicecall.c  |   14 ++++++--
>  drivers/stemodem/voicecall.c |    4 +-
>  include/voicecall.h          |    6 +++-
>  src/voicecall.c              |   76 +++++++++++++++++++++++++++++++++--------
>  4 files changed, 79 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/atmodem/voicecall.c b/drivers/atmodem/voicecall.c
> index fce9144..80ce897 100644
> --- a/drivers/atmodem/voicecall.c
> +++ b/drivers/atmodem/voicecall.c
> @@ -406,10 +406,17 @@ static void at_answer(struct ofono_voicecall *vc,
>  	at_template("ATA", vc, generic_cb, 0, cb, data);
>  }
>  
> -static void at_hangup(struct ofono_voicecall *vc,
> +static void at_hangup_all(struct ofono_voicecall *vc,
>  			ofono_voicecall_cb_t cb, void *data)
>  {
> -	/* Hangup all calls */
> +	/* Hangup all calls, except waiting */
> +	at_template("ATH", vc, generic_cb, 0x3f, cb, data);
> +}
> +
> +static void at_hangup_active(struct ofono_voicecall *vc,
> +			ofono_voicecall_cb_t cb, void *data)
> +{
> +	/* Hangup  currently active call */

It might be safer to only implement hangup_active in the generic driver.
 However, since this driver is only for testing anyway, the change is fine.

>  	at_template("AT+CHUP", vc, generic_cb, 0x3f, cb, data);
>  }
>  
> @@ -874,7 +881,8 @@ static struct ofono_voicecall_driver driver = {
>  	.remove			= at_voicecall_remove,
>  	.dial			= at_dial,
>  	.answer			= at_answer,
> -	.hangup			= at_hangup,
> +	.hangup_active		= at_hangup_active,
> +	.hangup_all		= at_hangup_all,
>  	.hold_all_active	= at_hold_all_active,
>  	.release_all_held	= at_release_all_held,
>  	.set_udub		= at_set_udub,
> diff --git a/drivers/stemodem/voicecall.c b/drivers/stemodem/voicecall.c
> index a56709a..2ddae05 100644
> --- a/drivers/stemodem/voicecall.c
> +++ b/drivers/stemodem/voicecall.c
> @@ -260,7 +260,7 @@ static void ste_answer(struct ofono_voicecall *vc,
>  	ste_template("ATA", vc, ste_generic_cb, 0, cb, data);
>  }
>  
> -static void ste_hangup(struct ofono_voicecall *vc,
> +static void ste_hangup_active(struct ofono_voicecall *vc,
>  			ofono_voicecall_cb_t cb, void *data)
>  {
>  	ste_template("AT+CHUP", vc, ste_generic_cb, 0x3f, cb, data);
> @@ -569,7 +569,7 @@ static struct ofono_voicecall_driver driver = {
>  	.remove			= ste_voicecall_remove,
>  	.dial			= ste_dial,
>  	.answer			= ste_answer,
> -	.hangup			= ste_hangup,
> +	.hangup_active		= ste_hangup_active,
>  	.hold_all_active	= ste_hold_all_active,
>  	.release_all_held	= ste_release_all_held,
>  	.set_udub		= ste_set_udub,
> diff --git a/include/voicecall.h b/include/voicecall.h
> index 6ceb3d8..d43954c 100644
> --- a/include/voicecall.h
> +++ b/include/voicecall.h
> @@ -67,7 +67,11 @@ struct ofono_voicecall_driver {
>  			ofono_voicecall_cb_t cb, void *data);
>  	void (*answer)(struct ofono_voicecall *vc,
>  			ofono_voicecall_cb_t cb, void *data);
> -	void (*hangup)(struct ofono_voicecall *vc,
> +/* HACK: for temporary backwards compatibility */
> +#define hangup_all hangup
> +	void (*hangup_all)(struct ofono_voicecall *vc,
> +			ofono_voicecall_cb_t cb, void *data);
> +	void (*hangup_active)(struct ofono_voicecall *vc,

We'll need to figure out how to migrate to the new driver API a bit more
sanely.

>  			ofono_voicecall_cb_t cb, void *data);
>  	void (*hold_all_active)(struct ofono_voicecall *vc,
>  			ofono_voicecall_cb_t cb, void *data);
> diff --git a/src/voicecall.c b/src/voicecall.c
> index a30aaa5..6ba6bb7 100644
> --- a/src/voicecall.c
> +++ b/src/voicecall.c
> @@ -261,15 +261,18 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
>  	if (vc->pending)
>  		return __ofono_error_busy(msg);
>  
> -	/* According to various specs, other than 27.007, +CHUP is used
> -	 * to reject an incoming call
> -	 */
>  	if (call->status == CALL_STATUS_INCOMING) {
> -		if (vc->driver->hangup == NULL)
> +
> +		if (vc->driver->hangup_all == NULL &&
> +				vc->driver->hangup_active == NULL)
>  			return __ofono_error_not_implemented(msg);
>  
>  		vc->pending = dbus_message_ref(msg);
> -		vc->driver->hangup(vc, generic_callback, vc);
> +
> +		if (vc->driver->hangup_all)
> +			vc->driver->hangup_all(vc, generic_callback, vc);
> +		else
> +			vc->driver->hangup_active(vc, generic_callback, vc);

Looks good.

>  
>  		return NULL;
>  	}
> @@ -286,12 +289,19 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
>  
>  	num_calls = g_slist_length(vc->call_list);
>  
> -	if (num_calls == 1 && vc->driver->hangup &&
> +	if (num_calls == 1 &&
> +			(vc->driver->hangup_all != NULL ||
> +				vc->driver->hangup_active != NULL) &&
>  			(call->status == CALL_STATUS_ACTIVE ||
>  				call->status == CALL_STATUS_DIALING ||
>  				call->status == CALL_STATUS_ALERTING)) {

This if statement is getting a little out of hand, but I can take care
of style issues later.

> +

Don't need the empty line here.

>  		vc->pending = dbus_message_ref(msg);
> -		vc->driver->hangup(vc, generic_callback, vc);
> +
> +		if (vc->driver->hangup_all)
> +			vc->driver->hangup_all(vc, generic_callback, vc);
> +		else
> +			vc->driver->hangup_active(vc, generic_callback, vc);

Looks good.

>  
>  		return NULL;
>  	}
> @@ -304,6 +314,15 @@ static DBusMessage *voicecall_hangup(DBusConnection *conn,
>  		return NULL;
>  	}
>  
> +	if (vc->driver->hangup_active != NULL &&
> +		(call->status == CALL_STATUS_ALERTING ||
> +			call->status == CALL_STATUS_DIALING)) {
> +		vc->pending = dbus_message_ref(msg);
> +		vc->driver->hangup_active(vc, generic_callback, vc);
> +
> +		return NULL;
> +	}
> +

Looks good.

>  	if (vc->driver->release_specific == NULL)
>  		return __ofono_error_not_implemented(msg);
>  
> @@ -743,12 +762,18 @@ static void emit_multiparty_call_list_changed(struct ofono_voicecall *vc)
>  static void voicecalls_release_queue(struct ofono_voicecall *vc, GSList *calls)
>  {
>  	GSList *l;
> +	struct voicecall *call;
>  
>  	g_slist_free(vc->release_list);
>  	vc->release_list = NULL;
>  
> -	for (l = calls; l; l = l->next)
> -		vc->release_list = g_slist_prepend(vc->release_list, l->data);
> +	for (l = calls; l; l = l->next) {
> +		call = l->data;
> +
> +		if (call->call->status != CALL_STATUS_WAITING)
> +			vc->release_list =
> +				g_slist_prepend(vc->release_list, call);
> +	}

Actually I prefer this to be done in manager_hangup_all.  This function
is used by the multiparty release path which can never include waiting
calls.

>  }
>  
>  static void voicecalls_release_next(struct ofono_voicecall *vc)
> @@ -762,7 +787,14 @@ static void voicecalls_release_next(struct ofono_voicecall *vc)
>  
>  	vc->release_list = g_slist_remove(vc->release_list, call);
>  
> -	vc->driver->release_specific(vc, call->call->id,
> +	if (vc->driver->hangup_active != NULL &&
> +			(call->call->status == CALL_STATUS_ALERTING ||
> +				call->call->status == CALL_STATUS_DIALING ||
> +				call->call->status == CALL_STATUS_INCOMING))
> +
> +		vc->driver->hangup_active(vc, multirelease_callback, vc);
> +	else
> +		vc->driver->release_specific(vc, call->call->id,
>  						multirelease_callback, vc);

Looks good.

>  }
>  
> @@ -1119,7 +1151,9 @@ static DBusMessage *manager_hangup_all(DBusConnection *conn,
>  	if (vc->pending)
>  		return __ofono_error_busy(msg);
>  
> -	if (!vc->driver->release_specific)
> +	if (vc->driver->hangup_all == NULL &&
> +		(vc->driver->release_specific == NULL ||
> +			vc->driver->hangup_active == NULL))
>  		return __ofono_error_not_implemented(msg);

Looks good.

>  
>  	if (vc->call_list == NULL) {
> @@ -1131,9 +1165,17 @@ static DBusMessage *manager_hangup_all(DBusConnection *conn,
>  
>  	vc->pending = dbus_message_ref(msg);
>  
> -	voicecalls_release_queue(vc, vc->call_list);
> -	voicecalls_release_next(vc);
> +	if (vc->driver->hangup_all != NULL)
> +		vc->driver->hangup_all(vc, generic_callback, vc);
>  
> +	/* In case of INCOMING call, assume we only have this one call */
> +	else if (voicecalls_have_incoming(vc) &&
> +			vc->driver->hangup_active != NULL)
> +		vc->driver->hangup_active(vc, generic_callback, vc);

Now that I think about it, the else if clause is not needed.  You take
care of incoming calls with hangup_active in voicecalls_release_next.
Right?

> +	else {
> +		voicecalls_release_queue(vc, vc->call_list);
> +		voicecalls_release_next(vc);
> +	}
>  	return NULL;
>  }
>  
> @@ -1370,8 +1412,12 @@ static DBusMessage *multiparty_hangup(DBusConnection *conn,
>  
>  	/* Fall back to the old-fashioned way */
>  	vc->flags |= VOICECALLS_FLAG_MULTI_RELEASE;
> -	voicecalls_release_queue(vc, vc->multiparty_list);
> -	voicecalls_release_next(vc);
> +	if (vc->driver->hangup_all != NULL)
> +		vc->driver->hangup_all(vc, generic_callback, vc);
> +	else {
> +		voicecalls_release_queue(vc, vc->call_list);
> +		voicecalls_release_next(vc);
> +	}

This doesn't look right.  If we get to this part then we either have
waiting + active, waiting + held, waiting + active + held or active +
held.  Calling hangup_all would be all right for the first two cases,
but not the other two.

For multiparty we need to fall back to release_specific.  The original
code was fine.

>  
>  out:
>  	return NULL;

Regards,
-Denis

Re: [RFCv2] STE-driver: Terminating voice call in state DIALING/ALERTING.

[Sjur =?unknown-8bit?q?Br=C3=A6ndeland?=]

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

Hi Denis.
...
>> +static void at_hangup_active(struct ofono_voicecall *vc,
>> +                     ofono_voicecall_cb_t cb, void *data)
>> +{
>> +     /* Hangup  currently active call */
>
> It might be safer to only implement hangup_active in the generic driver.
>  However, since this driver is only for testing anyway, the change is fine.

Ok, I might change that then.

...
>> +/* HACK: for temporary backwards compatibility */
>> +#define hangup_all hangup
>> +     void (*hangup_all)(struct ofono_voicecall *vc,
>> +                     ofono_voicecall_cb_t cb, void *data);
>> +     void (*hangup_active)(struct ofono_voicecall *vc,
>
> We'll need to figure out how to migrate to the new driver API a bit more
> sanely.

Should we s/hangup/hangup_active in all drivers and skip this hack then?
Or did you have something else in mind?
....
>  static void voicecalls_release_queue(struct ofono_voicecall *vc, GSList *calls)
>>  {
>>       GSList *l;
>> +     struct voicecall *call;
>>
>>       g_slist_free(vc->release_list);
>>       vc->release_list = NULL;
>>
>> -     for (l = calls; l; l = l->next)
>> -             vc->release_list = g_slist_prepend(vc->release_list, l->data);
>> +     for (l = calls; l; l = l->next) {
>> +             call = l->data;
>> +
>> +             if (call->call->status != CALL_STATUS_WAITING)
>> +                     vc->release_list =
>> +                             g_slist_prepend(vc->release_list, call);
>> +     }
>
> Actually I prefer this to be done in manager_hangup_all.  This function
> is used by the multiparty release path which can never include waiting
> calls.

OK, I'll look into that.
...

>> @@ -1131,9 +1165,17 @@ static DBusMessage *manager_hangup_all(DBusConnection *conn,
>>
>>       vc->pending = dbus_message_ref(msg);
>>
>> -     voicecalls_release_queue(vc, vc->call_list);
>> -     voicecalls_release_next(vc);
>> +     if (vc->driver->hangup_all != NULL)
>> +             vc->driver->hangup_all(vc, generic_callback, vc);
>>
>> +     /* In case of INCOMING call, assume we only have this one call */
>> +     else if (voicecalls_have_incoming(vc) &&
>> +                     vc->driver->hangup_active != NULL)
>> +             vc->driver->hangup_active(vc, generic_callback, vc);
>
> Now that I think about it, the else if clause is not needed.  You take
> care of incoming calls with hangup_active in voicecalls_release_next.
> Right?

Yes, agree.

...
>> @@ -1370,8 +1412,12 @@ static DBusMessage *multiparty_hangup(DBusConnection *conn,
>>
>>       /* Fall back to the old-fashioned way */
>>       vc->flags |= VOICECALLS_FLAG_MULTI_RELEASE;
>> -     voicecalls_release_queue(vc, vc->multiparty_list);
>> -     voicecalls_release_next(vc);
>> +     if (vc->driver->hangup_all != NULL)
>> +             vc->driver->hangup_all(vc, generic_callback, vc);
>> +     else {
>> +             voicecalls_release_queue(vc, vc->call_list);
>> +             voicecalls_release_next(vc);
>> +     }
>
> This doesn't look right.  If we get to this part then we either have
> waiting + active, waiting + held, waiting + active + held or active +
> held.  Calling hangup_all would be all right for the first two cases,
> but not the other two.
>
> For multiparty we need to fall back to release_specific.  The original
> code was fine.

OK, I'll revert this.


How do we proceede with this change. I am very happy if you carry on from here,
or would you like me to to submit a patch-set with these changes,
including s/hangup/hangup_active in drivers/*/voicecall.c?

Regards
Sjur

Re: [RFCv2] STE-driver: Terminating voice call in state DIALING/ALERTING.

[Denis Kenzior]

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

Hi Sjur,

>>> +/* HACK: for temporary backwards compatibility */
>>> +#define hangup_all hangup
>>> +     void (*hangup_all)(struct ofono_voicecall *vc,
>>> +                     ofono_voicecall_cb_t cb, void *data);
>>> +     void (*hangup_active)(struct ofono_voicecall *vc,
>>
>> We'll need to figure out how to migrate to the new driver API a bit more
>> sanely.
> 
> Should we s/hangup/hangup_active in all drivers and skip this hack then?
> Or did you have something else in mind?

That sounds good.  Break this up into two patches, one renaming hangup
to hangup_active and the second one introducing hangup_all.

> 
> How do we proceede with this change. I am very happy if you carry on from here,
> or would you like me to to submit a patch-set with these changes,
> including s/hangup/hangup_active in drivers/*/voicecall.c?

Go ahead and re-submit the fixed up patch set and I will take care of
the rest.

Regards,
-Denis