FW: [RFC 3/3] STE-plugin: Adding STE plugin

FW: [RFC 3/3] STE-plugin: Adding STE plugin

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

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

Hi Denis.

Denis Kenzior <denkenz@gmail.com> wrote:
>> +static void ste_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]; +	struct ofono_call *call;
>> +	GSList *l;
>> +	int ac = 0;
>> +
>> +	if (!req)
>> +		goto error;
>> +
>> +	req->vc = vc;
>> +	req->cb = cb;
>> +	req->data = data;
>> +	req->id = id;
>> +
>> +	/* Count active calls. If there is more than one active call
>> +	 * we cannot use ATH, as it will terminate all calls.
>> +	 * The reason for using ATH and not CHLD is that
>> +	 * emergency calls can not be terminated with AT+CHLD. +	 */
>> +	for (l = vd->calls; l; l = l->next) {
>> +		call = l->data;
>> +
>> +		if (call->status == CALL_STATUS_ACTIVE)
>> +			ac++;
>> +	}
>> +
>> +	l = g_slist_find_custom(vd->calls, GUINT_TO_POINTER(id),
>> +				call_compare_by_id); +	if (l == NULL) {
>> +		ofono_error("Hangup request for unknow call");
>> +		goto error;
>> +	}
>> +	call = l->data;
>> +	/* Check the state of the call, as AT+CHLD does not terminate
>> +	 * calls in state Dialing, Alerting and Incoming */
>> +	if (call->status == CALL_STATUS_DIALING ||
>> +	    call->status == CALL_STATUS_ALERTING ||
>> +	    call->status == CALL_STATUS_INCOMING)
>> +		sprintf(buf, "ATH");
>> +
>> +	/* Waiting call can not be terminated by at+chld=1x,
>> +	 * have to use at+chld = 0, but that will also terminate
>> +	 * other held calls. Bug in STE's AT module.
>> +	 */
>> +	else if (call->status == CALL_STATUS_WAITING)
>> +		sprintf(buf, "AT+CHLD=0");
>> +
>> +	else {
>> +		/* A held call can not be released by ATH, need to use CHLD */
>> +		if (ac > 1 || call->status == CALL_STATUS_HELD)
>> +			sprintf(buf, "AT+CHLD=1%d", id);
>> +		else /* This is the last call, ok to use ATH. */ 
>> +			sprintf(buf,
>> "ATH"); +	}
>> +
>> +	if (g_at_chat_send(vd->chat, buf, none_prefix,
>> +				release_id_cb, req, g_free) > 0)
>> +		return;
>> +
>> +error:
>> +	if (req)
>> +		g_free(req);
>> +
>> +	CALLBACK_WITH_FAILURE(cb, data);
>> +}
> 
> All of this logic needs to go away.  The core already handles all of 
> this, including selection of ATH/CHLD, waiting/held.  Please review 
> src/voicecall.c.
> If the core logic is not sufficient or does not properly handle a 
> certain case, then lets see if we can fix the core first.  Drivers 
> should not concern themselves with this stuff.

OK, we can remove the state logic, but STE modem cannot terminate
calls in state DIALING and ALERTING with CHLD=1X. We need to use 
ATH (or AT+CHUP) instead.

For now I think we will remove the state logic from ste_release_specific as 
you suggest. Terminating calls in state DIALING and ALERTING must then be  
handled by the Core part. The simplest would probably be to use hangup in 
this case, but I suspect hangup work differently for different modems.
So if you cannot use hangup as the general approach, maybe it could be 
implemented by adding callback functions release_dialing and release_alerting 
in struct ofono_voicecall_driver. The STE driver could send ATH from 
release_dialing and release_alerting, other drivers could leave them empty
and this could default to use release_specific instead?

> 
> With this in mind, you might not need to track any state in this 
> driver at all.  See drivers/calypsomodem/voicecall.c for details.
> TI's CPI notifications are almost exactly the same as the STE ECAV.  

The STE ECAV update is giving delta updates on the state information, 
right now I don't see how we can get the ofono_voicall_notify right 
without keeping some state information in ecav_notify. 


BR/Sjur

Re: FW: [RFC 3/3] STE-plugin: Adding STE plugin

[Denis Kenzior]

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

Hi Sjur,

> > All of this logic needs to go away.  The core already handles all of
> > this, including selection of ATH/CHLD, waiting/held.  Please review
> > src/voicecall.c.
> > If the core logic is not sufficient or does not properly handle a
> > certain case, then lets see if we can fix the core first.  Drivers
> > should not concern themselves with this stuff.
> 
> OK, we can remove the state logic, but STE modem cannot terminate
> calls in state DIALING and ALERTING with CHLD=1X. We need to use
> ATH (or AT+CHUP) instead.

oFono already takes care of this for single calls (see src/voicecall.c 
voicecall_hangup.)  So this is only an issue in the case of three way calls, 
is this what you're referring to here?

> 
> For now I think we will remove the state logic from ste_release_specific as
> you suggest. Terminating calls in state DIALING and ALERTING must then be
> handled by the Core part. The simplest would probably be to use hangup in
> this case, but I suspect hangup work differently for different modems.
> So if you cannot use hangup as the general approach, maybe it could be
> implemented by adding callback functions release_dialing and
>  release_alerting in struct ofono_voicecall_driver. The STE driver could
>  send ATH from release_dialing and release_alerting, other drivers could
>  leave them empty and this could default to use release_specific instead?

What I have been considering to take care of this case is to add end_all and 
end_all_active callbacks.  According to 27.007/22.030 ATH should end all calls 
(active + held) except waiting calls, while +CHUP should only end the 
currently active call.  At least on one TI modem I tried this works as 
expected.  Do your modems implement the same behavior?

If so then we can always use end_all_active for dialing/incoming/alerting 
cases.

> 
> > With this in mind, you might not need to track any state in this
> > driver at all.  See drivers/calypsomodem/voicecall.c for details.
> > TI's CPI notifications are almost exactly the same as the STE ECAV.
> 
> The STE ECAV update is giving delta updates on the state information,
> right now I don't see how we can get the ofono_voicall_notify right
> without keeping some state information in ecav_notify.
> 

Yes you're right, I was assuming ECAV gives you more information on whether 
the call was released locally or remotely.

Regards,
-Denis

RE: FW: [RFC 3/3] STE-plugin: Adding STE plugin

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

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

Hi Denis:
Denis Kenzior wrote:
> Hi Sjur,
> 
>>> All of this logic needs to go away.  The core already handles all of
>>> this, including selection of ATH/CHLD, waiting/held.  Please review
>>> src/voicecall.c. If the core logic is not sufficient or does not
>>> properly handle a certain case, then lets see if we can fix the
>>> core first.  Drivers should not concern themselves with this stuff.
>> 
>> OK, we can remove the state logic, but STE modem cannot terminate
>> calls in state DIALING and ALERTING with CHLD=1X. We need to use ATH
>> (or AT+CHUP) instead.
> 
> oFono already takes care of this for single calls (see src/voicecall.c
> voicecall_hangup.)  So this is only an issue in the case of three way
> calls, is this what you're referring to here? 
Kind of. This is very good, it takes care of the situation with emergency call
which cannot be terminated with CHLD commands.

But I think there are more issues. If I am not mistaken STE-modems have 
the following behavior:
CHLD=1X can only terminate call in state ACTIVE or HELD. (I think 
this is as STE interprets the standards).

a) If you are in a active call and receives a new incoming call (ALERTING)
and want to reject the new ALERTING call, then STE modem cannot 
terminate this call with CHLD=1X. It has to be terminated with CHLD=0 
(cause=BUSY) or ATH (possible CHUP). 

b) Or you may have the following situation. One call on HOLD,
another ACTIVE call, and then you receive a new incoming call ALERTING.
If you try to terminate the new incoming (ALERTING) call with CHLD=0,
I think you as a side effect will terminate the call on hold as well.
If I am not mistaken ATH (possible CHUP) would be the correct in this
situation for STE modems

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). 

>> For now I think we will remove the state logic from
>> ste_release_specific as you suggest. Terminating calls in state
>> DIALING and ALERTING must then be handled by the Core part. The
>> simplest would probably be to use hangup in this case, but I suspect
>> hangup work differently for different modems. So if you cannot use
>> hangup as the general approach, maybe it could be 
>> implemented by adding callback functions release_dialing and
>> release_alerting in struct ofono_voicecall_driver. The STE driver
>> could  send ATH from release_dialing and release_alerting, other
>> drivers could  leave them empty and this could default to use
>> release_specific instead? 
What I referred to here was the solution to the situations described above
in a,b,c. 

> 
> What I have been considering to take care of this case is to add
> end_all and end_all_active callbacks.  According to 27.007/22.030 ATH
> should end all calls (active + held) except waiting calls, while
> +CHUP should only end the currently active call.  At least on one TI
> modem I tried this works as expected.  Do your modems implement the
> same behavior?     

No, I don't think so. I think ATH will only terminate one call.
In order to terminate all calls you would probably need to do something
like: AT+CHLD=0;H But I'm not sure this works in all possible scenarios 
either...

> If so then we can always use end_all_active for
> dialing/incoming/alerting cases. 
> 
>> 
>>> With this in mind, you might not need to track any state in this
>>> driver at all.  See drivers/calypsomodem/voicecall.c for details.
>>> TI's CPI notifications are almost exactly the same as the STE ECAV.
>> 
>> The STE ECAV update is giving delta updates on the state information,
>> right now I don't see how we can get the ofono_voicall_notify right
>> without keeping some state information in ecav_notify.
>> 
> 
> Yes you're right, I was assuming ECAV gives you more information on
> whether the call was released locally or remotely. 
> 
> Regards,
> -Denis

Finally I'm not in office so I have taken this from top of my head so
I might have got something wrong here.

BR/Sjur

Re: FW: [RFC 3/3] STE-plugin: Adding STE plugin

[Denis Kenzior]

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

Hi Sjur,

> > oFono already takes care of this for single calls (see src/voicecall.c
> > voicecall_hangup.)  So this is only an issue in the case of three way
> > calls, is this what you're referring to here?
> 
> Kind of. This is very good, it takes care of the situation with emergency
>  call which cannot be terminated with CHLD commands.
> 
> But I think there are more issues. If I am not mistaken STE-modems have
> the following behavior:
> CHLD=1X can only terminate call in state ACTIVE or HELD. (I think
> this is as STE interprets the standards).

The standards specify that CHLD=1X can only terminate an ACTIVE call.  Most 
modems implement it this way.  There are vendor extensions that provide this 
functionality (e.g. CHLD=7X on TI.)  By default oFono assumes that 
release_specific will simply fail if a user attempts to use it on an e.g. HELD 
call with no modem support.

> 
> a) If you are in a active call and receives a new incoming call (ALERTING)
> and want to reject the new ALERTING call, then STE modem cannot
> terminate this call with CHLD=1X. It has to be terminated with CHLD=0
> (cause=BUSY) or ATH (possible CHUP).

Ok, lets get the terminology clear here.  In this case the incoming call is 
not ALERTING, it is WAITING.  WAITING calls are always rejected by using 
CHLD=0.  ALERTING calls are always outgoing calls that transitioned from 
DIALING to alerting the user.

> 
> b) Or you may have the following situation. One call on HOLD,
> another ACTIVE call, and then you receive a new incoming call ALERTING.
> If you try to terminate the new incoming (ALERTING) call with CHLD=0,
> I think you as a side effect will terminate the call on hold as well.
> If I am not mistaken ATH (possible CHUP) would be the correct in this
> situation for STE modems

The standards are quite clear here, the WAITING call always takes precedence 
and thus only the WAITING call is affected. Can you check that STE modems do 
indeed get this wrong?  If the modem is standards compliant, oFono does the 
right thing here.

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

> > What I have been considering to take care of this case is to add
> > end_all and end_all_active callbacks.  According to 27.007/22.030 ATH
> > should end all calls (active + held) except waiting calls, while
> > +CHUP should only end the currently active call.  At least on one TI
> > modem I tried this works as expected.  Do your modems implement the
> > same behavior?
> 
> No, I don't think so. I think ATH will only terminate one call.
> In order to terminate all calls you would probably need to do something
> like: AT+CHLD=0;H But I'm not sure this works in all possible scenarios
> either...

Can you check the behavior of ATH vs CHUP on STE modems?  We need to send the 
right one here or both HELD and ACTIVE/DIALING/ALERTING will be terminated.  
If using CHUP and ATH doesn't work out we'll have to come up with another 
solution.

Regards,
-Denis