Hi Sjur, On 08/06/2010 07:59 AM, Sjur Brændeland wrote: > From: Sjur Brændeland > > 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