Re: [PATCH] voicecall: fix callheld indicator for PTS

Hello Denis, Le 18/07/2011 18:40, Denis Kenzior a écrit : > Hi Frédéric, > > On 07/18/2011 09:33 AM, Frédéric Danis wrote: >> Fix PTS test TP/TWC/BV-03-I [Call Waiting- Hold Active/Retrieve >> Waiting Call or Held]. >> >> PTS test fails after receiving intermediate update of callheld indicator >> with value 0 (no held call) before it receives update to value 1 >> (active and held calls). This is due to the non-atomic update of calls >> status after call swap (moving first call to active state before moving >> second one to hold state). > > Which scenario fails exactly? > -<1-N> Active + Waiting -> Active + Held > -<1-N> Active + 1 Held -> 1 Active +<1-N> Held > - 1 Active +<1-N> Held -> <1-N> Active + 1 Held > > Also note that you can't really count on the order of the updates, these > are completely up to the implementation. > This test fails for (tested with phonesim) : - 1st call held + 2nd call active -> 1st call active + 2nd call held I understand that order of update for calls will be implementation dependent, this problem occurs when 1st call to be updated was held and become active. >> >> HFP 1.5 spec specifies that an update of callheld indicator to 1 should >> be sent after AT+CHLD=2 command. >> As oFono emulator sends +CIEV only if the indicator value changes, we >> need to use an intermediate state for callheld indicator (2, all >> calls on >> hold). >> >> So, in case of multiple active calls, or an active call with an active >> mutiparty call, force update of callheld indicator to 2. >> --- >> src/voicecall.c | 18 ++++++++++++++---- >> 1 files changed, 14 insertions(+), 4 deletions(-) >> >> diff --git a/src/voicecall.c b/src/voicecall.c >> index 3646951..4b8373e 100644 >> --- a/src/voicecall.c >> +++ b/src/voicecall.c >> @@ -733,7 +733,8 @@ static void emulator_callheld_status_cb(struct >> ofono_atom *atom, void *data) >> static void notify_emulator_call_status(struct ofono_voicecall *vc) >> { >> struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom); >> - gboolean call = FALSE; >> + unsigned int non_mpty = 0; >> + gboolean multiparty = FALSE; >> gboolean held = FALSE; >> gboolean incoming = FALSE; >> gboolean dialing = FALSE; >> @@ -750,7 +751,12 @@ static void notify_emulator_call_status(struct >> ofono_voicecall *vc) >> >> switch (v->call->status) { >> case CALL_STATUS_ACTIVE: >> - call = TRUE; >> + if (g_slist_find_custom(vc->multiparty_list, >> + GINT_TO_POINTER(v->call->id), >> + call_compare_by_id)) >> + multiparty = TRUE; >> + else >> + non_mpty++; >> break; > > What is this trying to achieve? You cannot have multiple calls in active > / held state unless they are part of the mpty. So if you have 3 active > calls, all 3 must be on the mpty list. > As calls status are updated separately, you can have multiple active calls (for a short time) which are not part of a multi-party list : - 1st call held + 2nd call active (callheld indicator = 1) -> 1st call active + 2nd call active (intermediate state, callheld indicator = 0) -> 1st call active + 2nd call held (callheld indicator = 1)

>> >> case CALL_STATUS_HELD: >> @@ -775,7 +781,8 @@ static void notify_emulator_call_status(struct >> ofono_voicecall *vc) >> } >> } >> >> - data.status = call || held ? OFONO_EMULATOR_CALL_ACTIVE : >> + data.status = non_mpty || multiparty || held ? >> + OFONO_EMULATOR_CALL_ACTIVE : >> OFONO_EMULATOR_CALL_INACTIVE; >> >> __ofono_modem_foreach_registered_atom(modem, >> @@ -799,8 +806,11 @@ static void notify_emulator_call_status(struct >> ofono_voicecall *vc) >> &data); >> >> if (held) >> - data.status = call ? OFONO_EMULATOR_CALLHELD_MULTIPLE : >> + data.status = (non_mpty || multiparty) ? >> + OFONO_EMULATOR_CALLHELD_MULTIPLE : >> OFONO_EMULATOR_CALLHELD_ON_HOLD; >> + else if (non_mpty> 1 || (non_mpty&& multiparty)) >> + data.status = OFONO_EMULATOR_CALLHELD_ON_HOLD; >> else >> data.status = OFONO_EMULATOR_CALLHELD_NONE; >> > > I think a different approach is needed. One idea off the top of my > head: Can we mark calls which we foresee being as part of the > transaction and only send the status updates once all of them have > entered the desired state. > I agree with you this should be the best way, but: - currently in voicecall atom during call swap, and as far as I understand, the calls are updated separately from the modem driver and there is no way to know when this update ends. I do not found a way to perform this with certainty that it will not break actual behavior (regarding DBus), This is why I proposed this hack, limited to emulator related code.

- currently HFP indicators are only sent when their value changes, we need to move through an intermediate state (callheld = 2), or add/change API to be able to force the sending of the indicator.