5:19 a.m.
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