Hi Andrew,
> Hi,
> here's a one-big-patch that adds the DataConnectionManager interface
> according to doc/dataconnectionmanager-api.txt without any glue for
> the multiplexer or PPP setup. Only AT backend is implemented.
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. Some parts might have been lost along the way.
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.
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 :)
>
> One issue with the AT implementation of the api is that "Powered" (a
> read-write property) can be set independently of "Attached" (read-only
> property) and remain set when "Attached" is clear. The semantics would
> be that the network doesn't have resources to let the modem attach,
> but the modem waits for the resources to become available and then
> attaches. On AT the modem is in this state only when executing +CGATT,
> so currently the code will rerun +CGATT as soon as the previous one
> returns with error, probably starving other commands. A possible
> workaround would be for "Powered" to flip back to False after the modem
> fails to attach once, or give up on having separate properties.
> Alternatively we could re-try to attach periodically but on one modem
> I've tried +CGATT fails after about 1 minute (that's the Calypso) and
> on another only about 0.5s (Nokia phones with AT emulation).
We should probably do both with some intelligence. Right now we lose GPRS
registration when we run an operator scan (the modem turns off GPRS
automagically), so running CGATT afterwards is OK. But then we should do it
in increasing timeouts. E.g. 5 seconds, 10 seconds, etc and turn off entirely
once we have given up.
I don't think this is necessary, all voice dial strings are suffixed by ';', so
dialing *99***1#; should result in an error.
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? In my opinion Attached should only be emitted once
it really has changed (e.g. in the callback)
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?
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.
Regards,
-Denis
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
