Re: [RFC] GPRS context setup and teardown (doc/dataconnectionmanager-api.txt)

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4045 bytes --]

Hi Andrew,
> > in set_registration_status:
> >        attached = (status != NETWORK_REGISTRATION_STATUS_REGISTERED &&
> >                        status != NETWORK_REGISTRATION_STATUS_ROAMING);
> >        if (gprs->attached != (int) attached &&
> >                        !(gprs->flags & GPRS_FLAG_ATTACHING)) {
> >                gprs->attached = (int) attached;
> >
> >                ofono_dbus_signal_property_changed(conn, path,
> >                                DATA_CONNECTION_MANAGER_INTERFACE,
> >                                "Attached", DBUS_TYPE_BOOLEAN,
> >                                &attached);
> >
> >                gprs_netreg_update(gprs);
> >        }
> >
> > I do not understand this logic at all.  Can't we always call
> > gprs_netreg_update here?
>
> Sure, but it won't do anything, if either "attached" state hasn't
> changed at all or it's already busy executing +CGATT.

So the issue here is that I can actually understand what gprs_netreg_update 
does.  This function I'm having trouble with.

Lets suppose status=registered arrives and we're already attached.  Here 
attached will be set to FALSE, Attached property will be set to false and 
emitted.  gprs_netreg_update will presumably reset Attached back to True.  Why 
do this for a potentially spurious indication?  Remember your status might be 
indicated when just the network access technology has changed.

Another case.  We're detached (Powered=True, RoamingAllowed=False) and are 
currently Roaming.  Then status=searching arrives.  We report Attached as 
True.

Am I properly outlining the logic or am I missing something?  Again, explain 
the intention here.  Either way, the current logic is way too complicated and 
needs to be simplified.

>
> > In my opinion Attached should only be emitted once
> > it really has changed (e.g. in the callback)
>
> Often there's no other notification telling us that we were detached,
> so we must take any sign of it into account (it might be broken modems
> but I've seen such situations on both modems that I've tested it with)

For situations where we are forcefully detached and/or go into a non-
registered state, the core should take care of this.  If it is a modem not 
reporting this properly, then I'd argue this is something the driver needs to 
take care of.  If the core starts trying to guess / outwit every single broken 
hardware out there it will be unmaintainable.  Lets at least try to keep the 
core clean and only responsible for the things mandated by the relevant 
standards.

>
> > gprs_netreg_update:
> >                /* Prevent further attempts to attach */
> >                if (!attach && gprs->powered) {
> >                        gprs->powered = 0;
> >
> >                        path = __ofono_atom_get_path(gprs->atom);
> >                        ofono_dbus_signal_property_changed(conn, path,
> >                                        DATA_CONNECTION_MANAGER_INTERFACE,
> >                                        "Powered", DBUS_TYPE_BOOLEAN,
> > &value); }
> >
> > This is just too evil.  Can't we set another flag that attached
> > conditions have changed while we were attaching/detaching and that we
> > should recheck those conditions when we return from attach/detach?
>
> Is it evil because we change a property that is writable?

Yes, and also because this is a hack :)

>
> This is only for the case of RoamingAllowed = 0.  In my understanding
> "Powered" being set means that we're attached or attempting to attach,

No, Powered means GPRS is on assuming all conditions are fulfilled (e.g. 
registration, roaming, network resources)  This is a user setting that will be 
persisted (in IMSI based storage.)  It should not be spuriously changed by the 
daemon.

> which is not the case after we detached because of roaming, so the
> D-Bus client would be misled.

No, 'Attached' is what is used for this purpose.

Both patches have been applied.  Thanks!

Regards,
-Denis