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

[andrzej zaborowski]

Hi,

2009/10/24 Denis Kenzior <denkenz at gmail.com>:
> Ok, so I took the patch and hacked on it for a while.  I disagreed with how
> you split up responsibilities, so much of the logic got moved into the core
> and the driver was simplified.

Yes, there were a couple of possible choices, I chose to align with
how the list of voice calls was managed in voicecall.c .

>
> I also decided to split up GPRS into two atoms to ease integration of hardware
> that supports dedicated network interfaces.  This way the attach / GPRS
> network registration parameters can be reused from the generic 'atmodem'
> driver, while specific context handling can be customized.

That's a good idea.

>
> I'm happy to report that this actually sort of works with my MBM card.  I can
> even define a GPRS context and activate it.  Of course the card crashes as soon
> as I do :)

I'm happy to report it still works on my Nokia phone, too, except for
one test: when you have active contexts and either tell ofono to power
gprs down or are detached by the network for whatever reason,
".active" is never reset and no event emitted, meaning that you can't
remove the context, can't re-activate it or deactivate it.

>
> Some parts I still don't understand:
>
> 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.

> 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)

>
> 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?

This is only for the case of RoamingAllowed = 0.  In my understanding
"Powered" being set means that we're attached or attempting to attach,
which is not the case after we detached because of roaming, so the
D-Bus client would be misled.

>
> In gprs_set_property:
>
> You should really ignore set property requests that set the value to the
> already set value.  Simply return success and don't do anything else.

Good point, the attached patch checks for those cases.

Best regards
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Just-return-success-when-value-already-set-in-SetPro.patch
Type: text/x-patch
Size: 1069 bytes
Desc: not available
URL: <http://lists.ofono.org/pipermail/ofono/attachments/20091024/a0d2b9bd/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Register-gprs-context-on-calypso-modem-phonesim.patch
Type: text/x-patch
Size: 1335 bytes
Desc: not available
URL: <http://lists.ofono.org/pipermail/ofono/attachments/20091024/a0d2b9bd/attachment-0001.bin>