10:56 p.m.
From: Andre Carvalho de Matos <andre.carvalho.matos(a)stericsson.com>
transfer method should fail if you have a multiparty call, according to 22.091
section 8.11
---
src/voicecall.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/voicecall.c b/src/voicecall.c
index f3f6a9b..f959f55 100644
--- a/src/voicecall.c
+++ b/src/voicecall.c
@@ -1339,6 +1339,7 @@ static DBusMessage *manager_transfer(DBusConnection *conn,
struct ofono_voicecall *vc = data;
int numactive;
int numheld;
+ int numconn;
if (vc->pending)
return __ofono_error_busy(msg);
@@ -1350,11 +1351,12 @@ static DBusMessage *manager_transfer(DBusConnection *conn,
* implementing the call transfer operation for a call that is
* still dialing/alerting.
*/
- numactive += voicecalls_num_connecting(vc);
+ numconn = voicecalls_num_connecting(vc);
numheld = voicecalls_num_held(vc);
- if ((numactive != 1) && (numheld != 1))
+ if ((numactive > 1 || numheld > 1) ||
+ ((numheld + numactive + numconn) != 2))
return __ofono_error_failed(msg);
if (vc->driver->transfer == NULL)
--
1.7.0.4
6:46 a.m.
Hi Andre,
@@ -1350,11 +1351,12 @@ static DBusMessage
*manager_transfer(DBusConnection *conn,
* implementing the call transfer operation for a call that is
* still dialing/alerting.
*/
- numactive += voicecalls_num_connecting(vc);
+ numconn = voicecalls_num_connecting(vc);
numheld = voicecalls_num_held(vc);
- if ((numactive != 1) && (numheld != 1))
I don't get it, isn't the above condition taking care of this already?
Its been a while but the precondition of ECT is:
Held Call
Outgoing or an Active call. You cannot have an Outgoing and an Active
call at the same time. Hence numactive == 1 && numheld == 1 should work
just fine.
+ if ((numactive > 1 || numheld > 1) ||
+ ((numheld + numactive + numconn) != 2))
return __ofono_error_failed(msg);
if (vc->driver->transfer == NULL)
Regards,
-Denis
7:37 a.m.
Hi Denis,
On Mon, Feb 7, 2011 at 8:46 PM, Denis Kenzior <denkenz(a)gmail.com> wrote:
Hi Andre,
> @@ -1350,11 +1351,12 @@ static DBusMessage
*manager_transfer(DBusConnection *conn,
> * implementing the call transfer operation for a call that is
> * still dialing/alerting.
> */
> - numactive += voicecalls_num_connecting(vc);
> + numconn = voicecalls_num_connecting(vc);
>
> numheld = voicecalls_num_held(vc);
>
> - if ((numactive != 1) && (numheld != 1))
I don't get it, isn't the above condition taking care of this already?
Its been a while but the precondition of ECT is:
Held Call
Outgoing or an Active call. You cannot have an Outgoing and an Active
call at the same time. Hence numactive == 1 && numheld == 1 should work
just fine.
What you wrote is correct. transfer works when it supposed to work.
The changes i did takes care of the negative cases.
This means calling transfer should fail if we have:
multiparty call active and one held call
one active call and one held multiparty call
only one active call
only one held call
etc...
> + if ((numactive > 1 || numheld > 1) ||
this checks if there is multiparty
+ ((numheld + numactive + numconn) != 2))
this makes sure we have the the two call necessary for a transfer.
return __ofono_error_failed(msg);
>
> if (vc->driver->transfer == NULL)
Regards,
-Denis
With my change oFono doesn't send unnecessary queries to the modem.
Best regards,
André
8:05 a.m.
Hi Andre,
On 02/07/2011 02:37 PM, andre matos wrote:
Hi Denis,
On Mon, Feb 7, 2011 at 8:46 PM, Denis Kenzior <denkenz(a)gmail.com
<mailto:denkenz@gmail.com>> wrote:
Hi Andre,
> @@ -1350,11 +1351,12 @@ static DBusMessage
*manager_transfer(DBusConnection *conn,
> * implementing the call transfer operation for a call that is
> * still dialing/alerting.
> */
> - numactive += voicecalls_num_connecting(vc);
> + numconn = voicecalls_num_connecting(vc);
>
> numheld = voicecalls_num_held(vc);
>
> - if ((numactive != 1) && (numheld != 1))
I don't get it, isn't the above condition taking care of this already?
Its been a while but the precondition of ECT is:
Held Call
Outgoing or an Active call. You cannot have an Outgoing and an Active
call at the same time. Hence numactive == 1 && numheld == 1 should work
just fine.
What you wrote is correct. transfer works when it supposed to work.
The changes i did takes care of the negative cases.
This means calling transfer should fail if we have:
multiparty call active and one held call
if mpty is an active call, then numactive would be > 1 -> fail to invoke ECT
one active call and one held multiparty call
If mpty is a held call, then numheld would be > 1 -> fail to invoke ECT
only one active call
If only active then numheld == 0 -> fail to invoke ECT
only one held call
if only held then numactive == 0 -> fail to invoke ECT
So what condition are we actually trying to solve?
Regards,
-Denis
8:24 a.m.
Hi Denis,
On Mon, Feb 7, 2011 at 10:05 PM, Denis Kenzior <denkenz(a)gmail.com> wrote:
Hi Andre,
On 02/07/2011 02:37 PM, andre matos wrote:
> Hi Denis,
>
> On Mon, Feb 7, 2011 at 8:46 PM, Denis Kenzior <denkenz(a)gmail.com
> <mailto:denkenz@gmail.com>> wrote:
>
> Hi Andre,
>
> > @@ -1350,11 +1351,12 @@ static DBusMessage
> *manager_transfer(DBusConnection *conn,
> > * implementing the call transfer operation for a call that
is
> > * still dialing/alerting.
> > */
> > - numactive += voicecalls_num_connecting(vc);
> > + numconn = voicecalls_num_connecting(vc);
> >
> > numheld = voicecalls_num_held(vc);
> >
> > - if ((numactive != 1) && (numheld != 1))
>
> I don't get it, isn't the above condition taking care of this
already?
> Its been a while but the precondition of ECT is:
>
> Held Call
> Outgoing or an Active call. You cannot have an Outgoing and an
Active
> call at the same time. Hence numactive == 1 && numheld == 1 should
work
> just fine.
>
>
> What you wrote is correct. transfer works when it supposed to work.
>
> The changes i did takes care of the negative cases.
> This means calling transfer should fail if we have:
> multiparty call active and one held call
if mpty is an active call, then numactive would be > 1 -> fail to invoke
ECT
Are we reading the same code?
for this case we have:
numactive != 1 ==> true
numheld != 1 ==> false
(numactive != 1) && (numheld != 1) ==> false
in this case "return __ofono_error_failed(msg);" is not called and the at
command is sent to modem.
I know the correct conditions for ECT. I am avoiding sending to the modem
unnecessary commands. Why should i ask the modem to invoke ECT if i already
know it is going to fail for a multiparty call.
Best regards,
André
8:48 a.m.
Hi Denis,
On Mon, Feb 7, 2011 at 10:30 PM, Denis Kenzior <denkenz(a)gmail.com> wrote:
Hi Andre,
> Are we reading the same code?
>
> for this case we have:
> numactive != 1 ==> true
> numheld != 1 ==> false
>
> (numactive != 1) && (numheld != 1) ==> false
>
Then shouldn't the patch simply be modified to if (numactive != 1 ||
numheld != 1)?
Yes, if you wish to sacrifice readability for simplicity.
I find my version self explanatory.
Best regards,
André
9 a.m.
Hi Andre,
On 02/07/2011 03:48 PM, andre matos wrote:
Hi Denis,
On Mon, Feb 7, 2011 at 10:30 PM, Denis Kenzior <denkenz(a)gmail.com
<mailto:denkenz@gmail.com>> wrote:
Hi Andre,
> Are we reading the same code?
>
> for this case we have:
> numactive != 1 ==> true
> numheld != 1 ==> false
>
> (numactive != 1) && (numheld != 1) ==> false
>
Then shouldn't the patch simply be modified to if (numactive != 1 ||
numheld != 1)?
Yes, if you wish to sacrifice readability for simplicity.
Don't get too offended, the || was intended instead of && in the first
place. My brain just had wired crossed and I wasn't seeing why you
needed a 6 line patch to fix this.
I find my version self explanatory.
Sorry, but I do not find your style more readable, less actually. Also,
when submitting patches please follow our coding style guidelines. You
had at least 1 style violation, namely rule M4.
Regards,
-Denis
9:01 a.m.
Hi Andre,
On 02/07/2011 05:56 AM, Andre Matos wrote:
From: Andre Carvalho de Matos
<andre.carvalho.matos(a)stericsson.com>
transfer method should fail if you have a multiparty call, according to 22.091
section 8.11
---
src/voicecall.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
I pushed my own version of the fix upstream. Thanks for catching this one.
Regards,
-Denis
4241
days inactive
4241
days old
8 comments
3 participants
participants (3)
-
Andre Matos -
andre matos
-
Denis Kenzior