Hi Sjur, On 08/04/2010 09:21 AM, Sjur Brændeland wrote: > From: Sjur Brændeland > > 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