2:34 p.m.
Hi Jussi,
On 03/14/2011 08:47 AM, Jussi Kangas wrote:
Hi Denis,
On Sat, 2011-03-12 at 00:48 +0200, Denis Kenzior wrote:
>> + } else {
>> + DBG("append only unconditional");
>> + property_append_cf_conditions(&dict,
>> + cf->cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL],
>> + BEARER_CLASS_VOICE,
>> + cf_type_lut[0]);
>> + for (i = 1; i < 4; i++)
>> + property_append_cf_conditions(&dict, NULL,
>> BEARER_CLASS_VOICE,
>> cf_type_lut[i]);
>
> So I'm confused, the TODO entry says we should not return conditional
> entries if the unconditional call forwarding is set as they are now
> quiescent.
>
> Are you proposing we handle this a bit differently?
No. I'm setting the unconditional values to NULL in order to clean them
from API. If only unconditional is updated when there has been
unconditional conditions active before, list-modems would show them
still. After this change information about unconditional conditions
remains in oFono, but they are not shown in the API.
Ok, I think I understand now. You're checking whether any unconditional
entries exist. If they do, you report them and assume all other entries
are quiescent. Right?
So correct me if I'm wrong, but I don't think we can do that. the
current cf_conditions implementation stores all conditions, which could
be unrelated to voice; and if my interpretation of 22.004 is correct you
can have something like this:
Activate CFB for all services to Number 1
Activate CFU for Data services to Number 2
Which results in cf_conditions[CALL_FORWARDING_TYPE_UNCONDITIONAL] not
being NULL and you reporting CFB for voice incorrectly.
You are probably safer using is_cfu_enabled() function.
>
>> + }
>>
>> if ((cf->flags & CALL_FORWARDING_FLAG_CPHS_CFF) ||
>> cf->cfis_record_id > 0)
>> @@ -682,8 +696,23 @@ static void set_query_cf_callback(const struct ofono_error
*error, int total,
>>
>> static void set_query_next_cf_cond(struct ofono_call_forwarding *cf)
>> {
>> - cf->driver->query(cf, cf->query_next, BEARER_CLASS_DEFAULT,
>> - set_query_cf_callback, cf);
>> + DBusMessage *reply;
>> +
>> + while (cf->query_next != CALL_FORWARDING_TYPE_NOT_REACHABLE) {
>> + if (!cf->cf_conditions[cf->query_next]) {
>> + cf->driver->query(cf, cf->query_next,
>> + BEARER_CLASS_DEFAULT,
>> + set_query_cf_callback, cf);
>> + return;
>> + } else {
>> + cf->query_next++;
>> + if (cf->query_next == cf->query_end)
>> + break;
>> + }
>> + }
>> +
>> + reply = dbus_message_new_method_return(cf->pending);
>> + __ofono_dbus_pending_reply(&cf->pending, reply);
>
> Really lost here,
Query_next and query_end tell only what to ask from query_next to
query_end. This causes pointless query if there is a value in the middle
which is already known. What this implementation does it prevents the
query if the condition is already known.
So the reason it is this way is that if we're querying via SS we want to
force the query to proceed anyway. Even if the value is already known.
This is really only a problem for *#002# and *#004# or if the request
of all CF conditions failed as a result of get_properties. In the
latter case we don't set the cached flag and re-get all the cfs again.
Please note that *#002# and *#004# are not valid MMI strings anyway
according to 22.004 ("Interrogation of groups of Supplementary Services
is not supported.") but I threw them in there since the logic was
already implemented. If it makes things easier we can limit this
functionality.
Either way this chunk belongs in a separate patch.
>> static void set_property_callback(const struct ofono_error
*error, void *data)
>> @@ -836,6 +865,7 @@ static void disable_conditional_callback(const struct
ofono_error *error,
>> static void disable_all_callback(const struct ofono_error *error, void *data)
>> {
>> struct ofono_call_forwarding *cf = data;
>> + int i;
>>
>> if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>> DBG("Error occurred during erasure of all");
>> @@ -848,6 +878,10 @@ static void disable_all_callback(const struct ofono_error
*error, void *data)
>> /* Query all cf types */
>> cf->query_next = CALL_FORWARDING_TYPE_UNCONDITIONAL;
>> cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>> +
>> + for (i = 0; i < CALL_FORWARDING_TYPE_ALL; i++)
>> + cf->cf_conditions[i] = 0;
>> +
>
> For pointers please use NULL instead of 0.
>
> Why are we setting them to NULL here in the first place? And shouldn't
> you free the data?
This is the other half of the implementation that prevents the querying
already known conditions. What I'm doing here is that I'm using the
cf_conditions as a flag to tell what is known. And since this is the
response for clearing all conditions, we don't know anything and
everything has to be queried in order to see if the request was
succesful or not. What comes to freeing the data I think you are right.
I think totally new parameter inside cf would be a better solution. That
would be less heavy. I'll check that out.
So see my comment above, this optimization might belong in a separate patch.
>> @@ -1180,8 +1236,14 @@ static gboolean cf_ss_control(int
type, const char *sc,
>> cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>> break;
>> case CALL_FORWARDING_TYPE_ALL_CONDITIONAL:
>> - cf->query_next = CALL_FORWARDING_TYPE_BUSY;
>> - cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>
> I don't see how handling both these cases with the same code can work...
Well at least in my testings if there is break and separate query_next
and query_end for CALL_FORWARDING_TYPE_ALL_CONDITIONAL erasing the
condition by using ss API does not remove conditions from API.
Sorry I'm still confused with this change. Maybe a more detailed trace
would help?
>> + case CALL_FORWARDING_TYPE_UNCONDITIONAL:
>> + if (type == SS_CONTROL_TYPE_REGISTRATION) {
>> + cf->query_next = cf->ss_req->cf_type;
>> + cf->query_end = cf->ss_req->cf_type;
>> + } else {
>> + cf->query_next = cf->ss_req->cf_type;
>> + cf->query_end = CALL_FORWARDING_TYPE_NOT_REACHABLE;
>> + }
>
> I'm a bit lost here, can you explain some more what you're trying to
> accomplish?
I was looking for this: If the user sets call forwarding unconditional
or all conditional on, there is no reason to ask anything else since
implementation in cf_get_properties_reply prevents the showing. If user
clears the call forwarding unconditional all have to be asked.
I see, so again I'm not sure this optimization is going to work.
Suppose I run an registration to CFall to Number 1 via USSD. Something
like:
**002*+12345#
I do want all affected services to show up in my USSD.Initiate return
dictionary with the results that the network is giving us. Not anything
that is cached by oFono. Think of MMI codes as completely bypassing the
cache but oFono peeking at them to update its own properties.
It might be helpful to think of this as two caches. The conditional
cache and the unconditional cache. If the conditional values are
updated when CFU is on, then we should clear the conditional cache and
re-query them next time they are needed (e.g. when CFU is turned off)
Regards,
-Denis