[PATCH 1/3] Add radio access atom and driver API

[Denis Kenzior]

Hi Aki,

On Wed, Feb 3, 2010 at 3:21 AM, Aki Niemi <aki.niemi at nokia.com> wrote:
> ke, 2010-02-03 kello 01:04 +0100, ext Denis Kenzior kirjoitti:
>> It might be helpful to include the API documentation too.  One thing I don't
>> like is the name RadioAccess.  The connotation is a bit too low-level.
>> Perhaps BandSelection? RadioBandSelection?
>
> I thought about this quite a bit, but this was the best name I could
> come up with. RadioAccessSelection, maybe, but then that'd be too
> specific if we wanted to add a property.
>
> And I think we definitely need to add at least one property here, a
> boolean "Active" for when the radio link is active. This is used by
> applications as a power saving measure to time their non-urgent data
> traffic to only when the radio link is in CELL_DCH state already.

How about we keep this simple and not try to overstuff this interface?
 Name it something like RadioPreferences or RadioSettings or
BandSettings, and put two things on here:
- Selection of technology 2G/3G/Dual
- Perhaps selection of band

The DCH state can go to some other interface and discussed in detail there.

>> > +void ofono_radio_access_mode_notify(struct ofono_radio_access
>> >  *radio_access, +                                     int mode);
>>
>> Why do we have this function, can the radio mode be changed outside oFono's
>> control?
>
> This can happen if you are using the phone as a modem on the desktop,
> and you change the setting on the phone.

Then you're opening up a huge can of worms here.  Most interfaces will
be utterly and completely broken if you allow modem access outside
oFono's control.

In fact this whole approach seems wrong to me.  Why should we bother
querying the setting at all?  Simply read it from storage (default it
to something sane, like 2G+3G if no setting exists) and always send
the set_mode at startup.  Then get rid of the notify function, this
has to be taken care of by the ModemEmulator.

>
>> > +static DBusMessage *ra_get_properties(DBusConnection *conn, DBusMessage
>> >  *msg, +                                      void *data)
>> > +{
>> > +     struct ofono_radio_access *ra = data;
>> > +     DBusMessage *reply;
>> > +     DBusMessageIter iter;
>> > +     DBusMessageIter dict;
>> > +
>> > +     const char *mode = radio_access_mode_to_string(ra->mode);
>> > +
>> > +     reply = dbus_message_new_method_return(msg);
>> > +     if (!reply)
>> > +             return NULL;
>> > +
>> > +     dbus_message_iter_init_append(reply, &iter);
>> > +
>> > +     dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
>> > +                                     OFONO_PROPERTIES_ARRAY_SIGNATURE,
>> > +                                     &dict);
>> > +
>> > +     ofono_dbus_dict_append(&dict, "Mode", DBUS_TYPE_STRING, &mode);
>> > +
>> > +     dbus_message_iter_close_container(&iter, &dict);
>> > +
>> > +     return reply;
>>
>> The pattern used in the past was to query the mode from get_properties if it
>> was not cached.  Once the mode was cached, we set a flag and always returned
>> the cached value.  I would do it the same way in this case to minimize flood of
>> queries at modem startup.
>
> I noticed call-forwarding was using it. However, when you have more than
> one property to cache, of which some are optional, that initial chain of
> queries to fill the cache gets a little nasty.

Again, I maintain this is a symptom of trying to overstuff the
interface.  Please keep it simple.

>
> I've not seen problems in practice in filling the cache at modem
> startup. (That's something CSD does.)
>

First of all you're dealing with sane modems.  Most modems are not as
nice and shiny as the Nokia ones.  Secondly, the goal for oFono is
performance, particularly at startup.  If something should not need to
be queries at startup, then don't.

>> Is querying the mode even a good idea?  Perhaps this should be yet another
>> setting, stored by IMSI.
>
> AFAIK, this is not an IMSI specific setting; it persists over a SIM
> swap.

Unless there's a very specific use case for why it should be a
per-modem setting, I suggest to make it per-IMSI.  GPRS settings are
also not stored on a per-SIM basis on most mobile phones.  Quite
frankly that makes no sense.  Lets not step on a rake yet again.

Regards,
-Denis