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

2010-02-02 Thread Denis Kenzior
Hi Aki,

 This atom provides access to the modem's radio access properties. It
 currently includes a single rw property, namely the radio access
 selection mode setting.

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?

 
 This allows the user to query and select the used radio access
 technology preference. In dual mode, either 2G or 3G is used depending
 on reception. 2G only mode or 3G only mode force selection of the
 respective access only.
 
 In the future, this atom could be extended, if necessary, to handle
 other radio access related modem features such as CELL_DCH status,
 UTRAN channel, or frequency band.
 ---
  Makefile.am|6 +-
  include/radio-access.h |   72 ++
  src/ofono.h|2 +
  src/radio-access.c |  354
   4 files changed, 432
  insertions(+), 2 deletions(-)
  create mode 100644 include/radio-access.h
  create mode 100644 src/radio-access.c
 
 diff --git a/Makefile.am b/Makefile.am
 index 9df..a18e4b9 100644
 --- a/Makefile.am
 +++ b/Makefile.am
 @@ -11,7 +11,8 @@ include_HEADERS = include/log.h include/plugin.h
  include/history.h \ include/sms.h include/sim.h include/message-waiting.h
  \
   include/netreg.h include/voicecall.h include/devinfo.h \
   include/cbs.h include/call-volume.h \
 - include/gprs.h include/gprs-context.h
 + include/gprs.h include/gprs-context.h \
 + include/radio-access.h
 
  nodist_include_HEADERS = include/version.h
 
 @@ -224,7 +225,8 @@ src_ofonod_SOURCES = $(gdbus_sources)
  $(builtin_sources) \ src/phonebook.c src/history.c src/message-waiting.c \
   src/simutil.h src/simutil.c src/storage.h \
   src/storage.c src/cbs.c src/watch.c src/call-volume.c \
 - src/gprs.c src/idmap.h src/idmap.c
 + src/gprs.c src/idmap.h src/idmap.c \
 + src/radio-access.c
 
  src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ -ldl
 
 diff --git a/include/radio-access.h b/include/radio-access.h
 new file mode 100644
 index 000..6677573
 --- /dev/null
 +++ b/include/radio-access.h
 @@ -0,0 +1,72 @@
 +/*
 + *
 + *  oFono - Open Source Telephony
 + *
 + *  Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
 + *
 + *  This program is free software; you can redistribute it and/or modify
 + *  it under the terms of the GNU General Public License version 2 as
 + *  published by the Free Software Foundation.
 + *
 + *  This program is distributed in the hope that it will be useful,
 + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
 + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 + *  GNU General Public License for more details.
 + *
 + *  You should have received a copy of the GNU General Public License
 + *  along with this program; if not, write to the Free Software
 + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 
  USA + *
 + */
 +
 +#ifndef __OFONO_RADIO_ACCESS_H
 +#define __OFONO_RADIO_ACCESS_H
 +
 +#ifdef __cplusplus
 +extern C {
 +#endif
 +
 +#include ofono/types.h
 +
 +struct ofono_radio_access;
 +
 +typedef void (*ofono_radio_access_mode_set_cb_t)(const struct ofono_error
  *error, +void *data);
 +typedef void (*ofono_radio_access_mode_query_cb_t)(const struct
  ofono_error *error, +
 int mode, void *data);

I really prefer mode to be an enum here.  There's no standard to reference 
here...

 +
 +struct ofono_radio_access_driver {
 + const char *name;
 + int (*probe)(struct ofono_radio_access *radio_access,
 + unsigned int vendor,
 + void *data);
 + void (*remove)(struct ofono_radio_access *radio_access);
 + void (*query_mode)(struct ofono_radio_access *radio_access,
 + ofono_radio_access_mode_query_cb_t cb, void 
 *data);
 + void (*set_mode)(struct ofono_radio_access *radio_access, int mode,
 + ofono_radio_access_mode_set_cb_t cb, void 
 *data);

Same here, mode should be an enum.

 +};
 +
 +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?

 +
 +int ofono_radio_access_driver_register(const struct
  ofono_radio_access_driver *d); +void
  ofono_radio_access_driver_unregister(const struct
  ofono_radio_access_driver *d); +
 +struct ofono_radio_access *ofono_radio_access_create(struct ofono_modem
  *modem, +unsigned int vendor,
 + 

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

2010-02-02 Thread Bastian, Waldo
  This atom provides access to the modem's radio access properties. It
  currently includes a single rw property, namely the radio access
  selection mode setting.
 
 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?

Radio access is a pretty broad high level concept. It's one of those black 
boxes you draw somewhere near the bottom of a phone architecture diagram, so in 
that sense it is low-level but that comes with the territory. BandSelection in 
my mind implies frequency band selection, which is a narrow aspect of radio 
access. The 2G/3G selection covered with this patch is about radio access 
_technology_, which is distinct from the actual frequency bands used by those 
technologies. I think RadioAccess is a fairly good name for an interface that 
has the potential to cover various radio related settings.

Use case for 2G/3G selection:
http://androidcommunity.com/forums/archive/index.php/t-11857.html

Use case for band selection:
http://support.t-mobile.com/doc/tm53294.xml?docid=5222referring%20topicid=67A2L.SERVICE=Referring%20TopicID/DocID%20List%20Index=ynavtypeid=2pagetypeid=26prevPageIndex=3

Cheers,
Waldo
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


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

2010-02-02 Thread Denis Kenzior
Hi Waldo,

   This atom provides access to the modem's radio access properties. It
   currently includes a single rw property, namely the radio access
   selection mode setting.
 
  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?
 
 Radio access is a pretty broad high level concept. It's one of those black
  boxes you draw somewhere near the bottom of a phone architecture diagram,
  so in that sense it is low-level but that comes with the territory.
  BandSelection in my mind implies frequency band selection, which is a
  narrow aspect of radio access. The 2G/3G selection covered with this patch
  is about radio access _technology_, which is distinct from the actual
  frequency bands used by those technologies. I think RadioAccess is a
  fairly good name for an interface that has the potential to cover various
  radio related settings.

People on this list keep forgetting two things:
1. We're not designing a kitchen sink API here.  Most of the 'radio related 
settings' will simply never be exposed, nobody really cares what UMTS channel 
he/she is currently on.
2. We're designing the API to be easy to use for everyone, not just GSM geeks.

In your AM/FM Tuner the 'Band' button switches between AM and FM.  You 
immediately know what the button does.  The interface name in oFono should 
ideally give the user an immediate idea of what it is for.  In my view 
RadioAccess does not fit this ideal.  Perhaps BandSelection is the wrong 
name too, lets come up with a better one.

Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono


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

2010-02-02 Thread Bastian, Waldo
 People on this list keep forgetting two things:
 1. We're not designing a kitchen sink API here.  Most of the 'radio
 related
 settings' will simply never be exposed, nobody really cares what UMTS
 channel
 he/she is currently on.
 2. We're designing the API to be easy to use for everyone, not just GSM
 geeks.
 
 In your AM/FM Tuner the 'Band' button switches between AM and FM.  You
 immediately know what the button does.  The interface name in oFono should
 ideally give the user an immediate idea of what it is for.  In my view
 RadioAccess does not fit this ideal.  Perhaps BandSelection is the
 wrong name too, lets come up with a better one.

StuffThatCustomerServiceTellsYouToUseWhenThingsDontQuiteWork

Cheers,
Waldo
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono