Re: [RFC][PATCH] Add: online method to isimodem

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7288 bytes --]

Hi Pekka,

> ---
>  drivers/isimodem/isimodem.c |   38 +++++++---
>  gatchat/gatppp.c            |   16 ++++
>  gatchat/ppp.h               |    2 +
>  gatchat/ppp_lcp.c           |    4 +
>  gatchat/ppp_net.c           |   13 +++-
>  include/modem.h             |    6 ++
>  src/modem.c                 |  183
>  +++++++++++++++++++++++++++++++++++++++++++ src/ofono.h                 | 
>   11 +++
>  8 files changed, 260 insertions(+), 13 deletions(-)

In general it is a good idea to break up your patches when you're touching 
multiple areas at once.  So in this case this needs to be a set of patches for 
PPP support, set of patches for core support and finally a set of patches for 
ISI modem.  The core changes should come before driver changes.

And why exactly are you including ppp patches along with isimodem?  I'm 
confused.

> diff --git a/include/modem.h b/include/modem.h
> index d502640..34cac7b 100644
> --- a/include/modem.h
> +++ b/include/modem.h
> @@ -50,6 +50,9 @@ void ofono_modem_remove(struct ofono_modem *modem);
>  void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t
>  powered); ofono_bool_t ofono_modem_get_powered(struct ofono_modem *modem);
> 
> +void ofono_modem_set_online(struct ofono_modem *modem, ofono_bool_t

This is assuming the driver can set online states outside of oFono control.  
Tell me a use case where this makes sense please.  If this is meant to be used 
only by ofono core, then this function should not be declared here.

>  online); +ofono_bool_t ofono_modem_get_online(struct ofono_modem *modem);
> +

>  void ofono_modem_set_name(struct ofono_modem *modem, const char *name);
> 
>  int ofono_modem_set_string(struct ofono_modem *modem,
> @@ -85,6 +88,9 @@ struct ofono_modem_driver {
> 
>  	/* Populate the atoms that are available with SIM / Unlocked SIM*/
>  	void (*post_sim)(struct ofono_modem *modem);
> +
> +	/* Enable or disable cellular radio */
> +	int (*online)(struct ofono_modem *modem, ofono_bool_t online);

Only the core should drive the online state, so I suggest a callback function 
/ user data here.

>  };
> 
>  int ofono_modem_driver_register(const struct ofono_modem_driver *);
> diff --git a/src/modem.c b/src/modem.c
> index 8319702..05b1fed 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -61,6 +61,11 @@ struct ofono_modem {
>  	ofono_bool_t		powered;
>  	ofono_bool_t		powered_pending;
>  	guint			timeout;
> +	DBusMessage		*pending_online;

We allow only one outstanding call / interface, so there's really no need for 
this member.

> +	ofono_bool_t            online;
> +	ofono_bool_t            requested_online;

Lets be consistent with how powered is done at least...

> +	ofono_bool_t            online_timeout;

Is this supposed to be a guint?

> +	struct ofono_watchlist	*online_watches;
>  	GHashTable		*properties;
>  	struct ofono_sim	*sim;
>  	unsigned int		sim_watch;
> @@ -361,6 +366,9 @@ static DBusMessage *modem_get_properties(DBusConnection
>  *conn, OFONO_PROPERTIES_ARRAY_SIGNATURE,
>  					&dict);
> 
> +	ofono_dbus_dict_append(&dict, "Online", DBUS_TYPE_BOOLEAN,
> +				&modem->online);
> +

I think FlightMode will be more recognizable for the UI guys.

>  	ofono_dbus_dict_append(&dict, "Powered", DBUS_TYPE_BOOLEAN,
>  				&modem->powered);
> 
> @@ -478,6 +486,158 @@ static gboolean set_powered_timeout(gpointer user)
>  	return FALSE;
>  }
> 
> +static int set_online(struct ofono_modem *modem, ofono_bool_t online)
> +{
> +	const struct ofono_modem_driver *driver = modem->driver;
> +	int err = -EINVAL;
> +
> +	if (modem->requested_online == online)
> +		return -EALREADY;
> +
> +	modem->requested_online = online;
> +
> +	if (driver && driver->online)
> +		err = driver->online(modem, online);
> +	else if (powering_down && !online)
> +		err = 0;
> +
> +	if (err == 0)
> +		ofono_modem_set_online(modem, online);
> +	else if (err != -EINPROGRESS)
> +		modem->requested_online = modem->online;
> +
> +	return err;
> +}
> +
> +static gboolean set_online_timeout(gpointer user)
> +{
> +	struct ofono_modem *modem = user;
> +
> +	DBG("modem: %p", modem);
> +
> +	modem->online_timeout = 0;
> +	modem->requested_online = modem->online;
> +
> +	if (modem->pending)
> +		__ofono_dbus_pending_reply(&modem->pending,
> +				__ofono_error_timed_out(modem->pending));
> +
> +	return FALSE;
> +}
> +
> +static DBusMessage *set_property_online(struct ofono_modem *modem,
> +					DBusMessage *msg,
> +					DBusMessageIter *var)
> +{
> +	ofono_bool_t online;
> +	int err;
> +
> +	if (dbus_message_iter_get_arg_type(var) != DBUS_TYPE_BOOLEAN)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_get_basic(var, &online);
> +
> +	if (modem->pending_online != NULL)
> +		return __ofono_error_busy(msg);
> +
> +	if (modem->online == online)
> +		return dbus_message_new_method_return(msg);
> +
> +	modem->pending_online = dbus_message_ref(msg);
> +
> +	err = set_online(modem, online);
> +	if (err < 0) {
> +		if (err != -EINPROGRESS && err != -EALREADY)

Why are you checking for EALREADY again, you just checked this case 3 
statements above...

> +			return __ofono_error_failed(msg);
> +
> +		modem->online_timeout = g_timeout_add_seconds(20,
> +						set_online_timeout, modem);
> +	}
> +
> +	return NULL;
> +}
> +
> +void ofono_modem_set_online(struct ofono_modem *modem, ofono_bool_t
>  online) +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	GSList *l;
> +	struct ofono_watchlist_item *watch;
> +	ofono_online_watch_func notify;
> +
> +	if (modem->online_timeout) {
> +		g_source_remove(modem->online_timeout);
> +		modem->online_timeout = 0;
> +	}
> +
> +	if (modem->pending_online) {
> +		DBusMessage *reply;
> +
> +		if (online == modem->requested_online)
> +			reply = dbus_message_new_method_return(modem->pending_online);
> +		else
> +			reply = __ofono_error_failed(modem->pending_online);
> +
> +		__ofono_dbus_pending_reply(&modem->pending_online, reply);
> +	}
> +
> +	modem->requested_online = online;
> +
> +	if (modem->online == online)
> +		return;
> +
> +	modem->online = online;
> +
> +	if (!modem->driver) {
> +		ofono_error("Calling ofono_modem_set_online on a"
> +				"modem with no driver is not valid, "
> +				"please fix the modem driver.");
> +		return;
> +	}
> +
> +	ofono_dbus_signal_property_changed(conn, modem->path,
> +					OFONO_MODEM_INTERFACE, "Online",
> +					DBUS_TYPE_BOOLEAN, &online);
> +
> +	for (l = modem->online_watches->items; l; l = l->next) {
> +		watch = l->data;
> +		notify = watch->notify;
> +		notify(modem, online, watch->notify_data);
> +	}
> +}
> +

<snip>

> +	if (modem->online == TRUE)
> +		set_online(modem, FALSE);
> +

My overall feeling here is that there's no need to make FlightMode state so 
complicated.  Can we assume that powering down the modem from 'online' state 
will take care of bringing the hardware down cleanly and avoid explicitly 
calling the driver to bring it 'offline' first?

There's also the question of who stores the FlightMode user setting, and 
whether we store it by device ID or IMSI.

Regards,
-Denis