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
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
