[RFC] Proposal for flight mode

[RFC] Proposal for flight mode

[Pekka Pessi]

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

Hello all,

Here is the first step in adding proper flight mode to ofono. The patch
adds a boolean property "Online" to modem; if that is false, modem is in
flight mode and it has killed its cellular radio.

If the modem driver does not implement online method, the value of the
property is initially true. This feature is intended as temporary. It just
makes the new property as non-intrusive as possible.

There is also a watch list for online/offline state.

The second patch adds the online method to isimodem. The online property of
the isimodem follows the online/offline state of Nokia modems (however, this
has only been tested with N95).

In the next phase, I'll add separate atom lists for flight and online modes.
The old-fashioned post_sim will become post_online. The new method
post_sim_flight will request modem driver to add flight-mode atoms.

--Pekka

BTW, if you have applied the patch I sent earlier today, "usbpnmodem:
configure usbpn interfaces automatically", you can actually try Nokia phones
via USB. They should just work provided you have kernel 2.6.31 or newer
without any manual configuration. Select the "PC suite mode" from the phone
when you connect the USB cable.

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

[Pekka Pessi]

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

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

diff --git a/drivers/isimodem/isimodem.c b/drivers/isimodem/isimodem.c
index e6e0bc1..733ac96 100644
--- a/drivers/isimodem/isimodem.c
+++ b/drivers/isimodem/isimodem.c
@@ -64,16 +64,20 @@ struct isi_data {
 	GIsiModem *idx;
 	GIsiClient *client;
 	GPhonetNetlink *link;
+	GPhonetLinkState linkstate;
 	unsigned interval;
-	int reported;
+	int powered;
 	int mtc_state;
-	int iface_up;
+	int online;
 };
 
-static void report_powered(struct isi_data *isi, ofono_bool_t powered)
+static void report_powered(struct isi_data *isi, ofono_bool_t powered,
+				ofono_bool_t online)
 {
-	if (powered != isi->reported)
-		ofono_modem_set_powered(isi->modem, isi->reported = powered);
+	if (powered != isi->powered)
+		ofono_modem_set_powered(isi->modem, isi->powered = powered);
+	if (online != isi->online)
+		ofono_modem_set_online(isi->modem, isi->online = online);
 }
 
 static void set_power_by_mtc_state(struct isi_data *isi, int state)
@@ -84,16 +88,19 @@ static void set_power_by_mtc_state(struct isi_data *isi, int state)
 	case MTC_POWER_OFF:
 	case MTC_CHARGING:
 	case MTC_SELFTEST_FAIL:
-		report_powered(isi, 0);
+		report_powered(isi, 0, 0);
 		break;
 
 	case MTC_RF_INACTIVE:
+		report_powered(isi, 1, 0);
+		break;
+
 	case MTC_NORMAL:
-		report_powered(isi, 1);
+		report_powered(isi, 1, 1);
 		break;
 
 	default:
-		report_powered(isi, 1);
+		report_powered(isi, 1, 1);
 	}
 }
 
@@ -132,7 +139,7 @@ static bool mtc_poll_query_cb(GIsiClient *client, const void *restrict data,
 			MTC_STATE_QUERY_REQ, 0x00, 0x00
 		};
 
-		if (!isi-> iface_up)
+		if (isi->linkstate != PN_LINK_UP)
 			return true;
 
 		isi->interval *= 2;
@@ -198,7 +205,7 @@ static void reachable_cb(GIsiClient *client, bool alive, uint16_t object,
 	if (!alive) {
 		DBG("MTC client: %s", strerror(-g_isi_client_error(client)));
 
-		if (isi->iface_up)
+		if (isi->linkstate == PN_LINK_UP)
 			g_isi_request_make(client, msg, sizeof(msg),
 						isi->interval = MTC_TIMEOUT,
 						mtc_poll_query_cb, opaque);
@@ -228,7 +235,7 @@ static void phonet_status_cb(GIsiModem *idx,
 		st == PN_LINK_REMOVED ? "removed" :
 		st == PN_LINK_DOWN ? "down" : "up");
 
-	isi->iface_up = st == PN_LINK_UP;
+	isi->linkstate = st;
 
 	if (st == PN_LINK_UP)
 		g_isi_verify(isi->client, reachable_cb, isi);
@@ -278,6 +285,8 @@ static int isi_modem_probe(struct ofono_modem *modem)
 	isi->ifname = ifname;
 	isi->link = link;
 	isi->client = g_isi_client_create(isi->idx, PN_MTC);
+	isi->powered = -1;
+	isi->online = -1;
 
 	return 0;
 }
@@ -334,12 +343,19 @@ static void isi_modem_post_sim(struct ofono_modem *modem)
 		DBG("Failed to add context");
 }
 
+static int isi_modem_online(struct ofono_modem *modem, ofono_bool_t online)
+{
+	/* isi_modem follows modem online/offline state */
+	return -ENOTSUP;
+}
+
 static struct ofono_modem_driver driver = {
 	.name = "isimodem",
 	.probe = isi_modem_probe,
 	.remove = isi_modem_remove,
 	.pre_sim = isi_modem_pre_sim,
 	.post_sim = isi_modem_post_sim,
+	.online = isi_modem_online,
 };
 
 static int isimodem_init(void)
diff --git a/gatchat/gatppp.c b/gatchat/gatppp.c
index e1e49e6..b7834e3 100644
--- a/gatchat/gatppp.c
+++ b/gatchat/gatppp.c
@@ -40,6 +40,7 @@
 #include "ppp.h"
 
 #define DEFAULT_MRU	1500
+#define DEFAULT_MTU	1500
 
 #define BUFFERSZ	(DEFAULT_MRU * 2)
 
@@ -58,6 +59,7 @@ struct _GAtPPP {
 	guint8 buffer[BUFFERSZ];
 	int index;
 	gint mru;
+	gint mtu;
 	char username[256];
 	char password[256];
 	guint32 xmit_accm[8];
@@ -407,6 +409,8 @@ void ppp_net_up_notify(GAtPPP *ppp, const char *ip,
 {
 	ppp->net = ppp_net_new(ppp);
 
+	ppp_net_set_mtu(ppp->net, ppp->mtu);
+
 	if (ppp->connect_cb == NULL)
 		return;
 
@@ -435,6 +439,17 @@ void ppp_set_xmit_accm(GAtPPP *ppp, guint32 accm)
 	ppp->xmit_accm[0] = accm;
 }
 
+/*
+ * The only time we use other than default MTU is when we are in
+ * the network phase.
+ */
+void ppp_set_mtu(GAtPPP *ppp, const guint8 *data)
+{
+	guint16 mtu = get_host_short(data);
+
+	ppp->mtu = mtu;
+}
+
 /* Administrative Open */
 void g_at_ppp_open(GAtPPP *ppp)
 {
@@ -576,6 +591,7 @@ GAtPPP *g_at_ppp_new(GIOChannel *modem)
 
 	/* set options to defaults */
 	ppp->mru = DEFAULT_MRU;
+	ppp->mtu = DEFAULT_MTU;
 	ppp->recv_accm = ~0U;
 	ppp->xmit_accm[0] = ~0U;
 	ppp->xmit_accm[3] = 0x60000000; /* 0x7d, 0x7e */
diff --git a/gatchat/ppp.h b/gatchat/ppp.h
index a8a0486..07483a9 100644
--- a/gatchat/ppp.h
+++ b/gatchat/ppp.h
@@ -103,6 +103,7 @@ struct ppp_net *ppp_net_new(GAtPPP *ppp);
 const char *ppp_net_get_interface(struct ppp_net *net);
 void ppp_net_process_packet(struct ppp_net *net, guint8 *packet);
 void ppp_net_free(struct ppp_net *net);
+void ppp_net_set_mtu(struct ppp_net *net, guint16 mtu);
 
 /* PPP functions related to main GAtPPP object */
 void ppp_debug(GAtPPP *ppp, const char *str);
@@ -115,3 +116,4 @@ void ppp_net_up_notify(GAtPPP *ppp, const char *ip,
 void ppp_net_down_notify(GAtPPP *ppp);
 void ppp_set_recv_accm(GAtPPP *ppp, guint32 accm);
 void ppp_set_xmit_accm(GAtPPP *ppp, guint32 accm);
+void ppp_set_mtu(GAtPPP *ppp, const guint8 *data);
diff --git a/gatchat/ppp_lcp.c b/gatchat/ppp_lcp.c
index 5cf5656..8639c6c 100644
--- a/gatchat/ppp_lcp.c
+++ b/gatchat/ppp_lcp.c
@@ -197,6 +197,7 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
 		case ACCM:
 		case PFC:
 		case ACFC:
+		case MRU:
 			break;
 
 		case MAGIC_NUMBER:
@@ -226,6 +227,9 @@ static enum rcr_result lcp_rcr(struct pppcp_data *pppcp,
 		case AUTH_PROTO:
 			ppp_set_auth(ppp, ppp_option_iter_get_data(&iter));
 			break;
+		case MRU:
+			ppp_set_mtu(ppp, ppp_option_iter_get_data(&iter));
+			break;
 		case MAGIC_NUMBER:
 		case PFC:
 		case ACFC:
diff --git a/gatchat/ppp_net.c b/gatchat/ppp_net.c
index 325e859..c1f2eb4 100644
--- a/gatchat/ppp_net.c
+++ b/gatchat/ppp_net.c
@@ -38,7 +38,6 @@
 #include "gatppp.h"
 #include "ppp.h"
 
-/* XXX should be maximum IP Packet size */
 #define MAX_PACKET 1500
 
 struct ppp_net {
@@ -46,8 +45,17 @@ struct ppp_net {
 	char *if_name;
 	GIOChannel *channel;
 	gint watch;
+	gint mtu;
 };
 
+void ppp_net_set_mtu(struct ppp_net *net, guint16 mtu)
+{
+	if (net == NULL)
+		return;
+
+	net->mtu = mtu;
+}
+
 void ppp_net_process_packet(struct ppp_net *net, guint8 *packet)
 {
 	GError *error = NULL;
@@ -80,7 +88,7 @@ static gboolean ppp_net_callback(GIOChannel *channel, GIOCondition cond,
 
 	if (cond & G_IO_IN) {
 		/* leave space to add PPP protocol field */
-		status = g_io_channel_read_chars(channel, buf + 2, MAX_PACKET,
+		status = g_io_channel_read_chars(channel, buf + 2, net->mtu,
 				&bytes_read, &error);
 		if (bytes_read > 0) {
 			ppp->proto = htons(PPP_IP_PROTO);
@@ -140,6 +148,7 @@ struct ppp_net *ppp_net_new(GAtPPP *ppp)
 			ppp_net_callback, net);
 	net->ppp = ppp;
 
+	net->mtu = MAX_PACKET;
 	return net;
 
 error:
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 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);
 };
 
 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;
+	ofono_bool_t            online;
+	ofono_bool_t            requested_online;
+	ofono_bool_t            online_timeout;
+	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);
+
 	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)
+			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);
+	}
+}
+
+unsigned int __ofono_modem_add_online_watch(struct ofono_modem *modem,
+					ofono_online_watch_func notify,
+					void *data, ofono_destroy_func destroy)
+{
+	struct ofono_watchlist_item *watch;
+
+	if (notify == NULL)
+		return 0;
+
+	watch = g_new0(struct ofono_watchlist_item, 1);
+
+	watch->notify = notify;
+	watch->destroy = destroy;
+	watch->notify_data = data;
+
+	return __ofono_watchlist_add_item(modem->online_watches, watch);
+}
+
+gboolean __ofono_modem_remove_online_watch(struct ofono_modem *modem,
+						unsigned int id)
+{
+	return __ofono_watchlist_remove_item(modem->online_watches, id);
+}
+
+
+ofono_bool_t ofono_modem_get_online(struct ofono_modem *modem)
+{
+	if (modem == NULL)
+		return FALSE;
+
+	return modem->online;
+}
+
 static DBusMessage *modem_set_property(DBusConnection *conn,
 					DBusMessage *msg, void *data)
 {
@@ -502,6 +662,9 @@ static DBusMessage *modem_set_property(DBusConnection *conn,
 
 	dbus_message_iter_recurse(&iter, &var);
 
+	if (g_str_equal(name, "Online"))
+		return set_property_online(modem, msg, &var);
+
 	if (g_str_equal(name, "Powered") == TRUE) {
 		ofono_bool_t powered;
 		int err;
@@ -1210,6 +1373,7 @@ int ofono_modem_register(struct ofono_modem *modem)
 	modem->driver_type = NULL;
 
 	modem->atom_watches = __ofono_watchlist_new(g_free);
+	modem->online_watches = __ofono_watchlist_new(g_free);
 
 	emit_modems();
 
@@ -1217,6 +1381,9 @@ int ofono_modem_register(struct ofono_modem *modem)
 					OFONO_ATOM_TYPE_SIM,
 					sim_watch, modem, NULL);
 
+	if (modem->driver->online == NULL)
+		ofono_modem_set_online(modem, TRUE);
+
 	return 0;
 }
 
@@ -1224,12 +1391,18 @@ static void modem_unregister(struct ofono_modem *modem)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 
+	if (modem->online == TRUE)
+		set_online(modem, FALSE);
+
 	if (modem->powered == TRUE)
 		set_powered(modem, FALSE);
 
 	__ofono_watchlist_free(modem->atom_watches);
 	modem->atom_watches = NULL;
 
+	__ofono_watchlist_free(modem->online_watches);
+	modem->online_watches = NULL;
+
 	modem->sim_watch = 0;
 	modem->sim_ready_watch = 0;
 
@@ -1247,6 +1420,16 @@ static void modem_unregister(struct ofono_modem *modem)
 		modem->pending = NULL;
 	}
 
+	if (modem->online_timeout) {
+		g_source_remove(modem->online_timeout);
+		modem->online_timeout = 0;
+	}
+
+	if (modem->pending_online) {
+		dbus_message_unref(modem->pending_online);
+		modem->pending_online = NULL;
+	}
+
 	if (modem->interface_update) {
 		g_source_remove(modem->interface_update);
 		modem->interface_update = 0;
diff --git a/src/ofono.h b/src/ofono.h
index 7b13cce..b122f5e 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -159,6 +159,17 @@ gboolean __ofono_modem_remove_atom_watch(struct ofono_modem *modem,
 
 void __ofono_atom_free(struct ofono_atom *atom);
 
+typedef void (*ofono_online_watch_func)(struct ofono_modem *modem,
+					ofono_bool_t online,
+					void *data);
+
+unsigned int __ofono_modem_add_online_watch(struct ofono_modem *modem,
+					ofono_online_watch_func notify,
+					void *data, ofono_destroy_func destroy);
+
+gboolean __ofono_modem_remove_online_watch(struct ofono_modem *modem,
+						unsigned int id);
+
 #include <ofono/call-barring.h>
 #include <ofono/call-forwarding.h>
 #include <ofono/call-meter.h>
-- 
1.6.3.3

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

[Denis Kenzior]

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

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

[Pekka Pessi]

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

2010/4/23 Denis Kenzior <denkenz@gmail.com>:
>>  gatchat/gatppp.c            |   16 ++++
>>  gatchat/ppp.h               |    2 +
>>  gatchat/ppp_lcp.c           |    4 +
>>  gatchat/ppp_net.c           |   13 +++-

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

I'm as confused as you. It seems to me I squished somebody else's
patch here during rebase. ;-o

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

I'm not assuming only core being able to drive the online state. If
you are using a PC with an external handset as modem, it will have its
own flight mode settings.


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

It depends on how the flight mode is driven. If ofono will follow user
changing online/offline directly on external handset, the
ofono_modem_set_online needs to know which SetProperty call is
pending.

>> +     ofono_bool_t            online_timeout;
>
> Is this supposed to be a guint?

Sure.

>> +     ofono_dbus_dict_append(&dict, "Online", DBUS_TYPE_BOOLEAN,
>> +                             &modem->online);

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

It depends. Our UI people want to use "Offline" and "Online" in UI
(while the values used in D-Bus are "flight" and "normal"). It is
probably because they have plenty of use cases for "Offline" outside
aeroplanes.

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

You are probably right, I did not think the underlying mechanism through.

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

Unless we provide multiple settings, it is best left to some
application. Using my favourite phone as an example, the N900
application controlling the modem keeps track of Call UI state and two
UI settings, system mode (Online/offline, or if you prefer,
normal/flight) and cellular enable/disable. Only if Call UI is ready,
system mode is online and cellular radio is enabled, it will let modem
out of flight mode.

-- 
Pekka.Pessi mail at nokia.com

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

[Denis Kenzior]

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

Hi Pekka,

> >> +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.
> 
> I'm not assuming only core being able to drive the online state. If
> you are using a PC with an external handset as modem, it will have its
> own flight mode settings.

We've had this argument many times before, the bottom line is oFono is not 
setup to share the modem with another stack.  oFono caches results of just 
about every setting on the modem, including call settings, call barring, call 
forwarding, sms, cell broadcast, etc, etc, etc.  Just because you can make it 
work for the online state doesn't mean you will ever get this right in the 
general case.

This usecase is the realm of Gammu or Gnokii, not oFono.

> >> +     ofono_dbus_dict_append(&dict, "Online", DBUS_TYPE_BOOLEAN,
> >> +                             &modem->online);
> >
> > I think FlightMode will be more recognizable for the UI guys.
> 
> It depends. Our UI people want to use "Offline" and "Online" in UI
> (while the values used in D-Bus are "flight" and "normal"). It is
> probably because they have plenty of use cases for "Offline" outside
> aeroplanes.
> 
> ...

I'm fine with either actually, maybe others have a stronger preference they'd 
like to argue for or against...

> > There's also the question of who stores the FlightMode user setting, and
> > whether we store it by device ID or IMSI.
> 
> Unless we provide multiple settings, it is best left to some
> application. Using my favourite phone as an example, the N900
> application controlling the modem keeps track of Call UI state and two
> UI settings, system mode (Online/offline, or if you prefer,
> normal/flight) and cellular enable/disable. Only if Call UI is ready,
> system mode is online and cellular radio is enabled, it will let modem
> out of flight mode.

I'm fine with an external entity controlling this, as we do the same for the 
Powered setting.  However this means that by default oFono (and assuming 
offline/online is supported) should only get as far as offline state.  Online 
state has to be enabled by someone else.

Lets make sure there are no other concerns here, particularly from our 
connection management guys.

Regards,
-Denis