[PATCH 1/3] Add: Online property to modem

[PATCH 1/3] Add: Online property to modem

[Pekka Pessi]

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

The online/offline state is changed with the new online() modem driver
method.

In order to track atoms, there are modem states as follows:
- OFONO_MODEM_STATE_POWER_OFF
- OFONO_MODEM_STATE_PRE_SIM
- OFONO_MODEM_STATE_OFFLINE
- OFONO_MODEM_STATE_ONLINE

Atoms are removed by modem core according to the state. Atoms are added with
driver methods pre_sim(), post_sim() and post_online().
---
 include/modem.h |   12 +++
 src/modem.c     |  205 ++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 179 insertions(+), 38 deletions(-)

diff --git a/include/modem.h b/include/modem.h
index d502640..2e9bd87 100644
--- a/include/modem.h
+++ b/include/modem.h
@@ -50,6 +50,8 @@ 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);
 
+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,
@@ -64,6 +66,9 @@ int ofono_modem_set_boolean(struct ofono_modem *modem,
 				const char *key, bool value);
 bool ofono_modem_get_boolean(struct ofono_modem *modem, const char *key);
 
+typedef void (*ofono_modem_online_cb)(const struct ofono_error *error,
+				void *data);
+
 struct ofono_modem_driver {
 	const char *name;
 
@@ -80,11 +85,18 @@ struct ofono_modem_driver {
 	/* Power down device */
 	int (*disable)(struct ofono_modem *modem);
 
+	/* Enable or disable cellular radio */
+	void (*online)(struct ofono_modem *modem, ofono_bool_t online,
+		ofono_modem_online_cb callback, void *data);
+
 	/* Populate the atoms available without SIM / Locked SIM */
 	void (*pre_sim)(struct ofono_modem *modem);
 
 	/* Populate the atoms that are available with SIM / Unlocked SIM*/
 	void (*post_sim)(struct ofono_modem *modem);
+
+	/* Populate the atoms available online */
+	void (*post_online)(struct ofono_modem *modem);
 };
 
 int ofono_modem_driver_register(const struct ofono_modem_driver *);
diff --git a/src/modem.c b/src/modem.c
index 04fba15..bf78183 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -49,10 +49,17 @@ enum ofono_property_type {
 	OFONO_PROPERTY_TYPE_BOOLEAN,
 };
 
+enum ofono_modem_state {
+	OFONO_MODEM_STATE_POWER_OFF,
+	OFONO_MODEM_STATE_PRE_SIM,
+	OFONO_MODEM_STATE_OFFLINE,
+	OFONO_MODEM_STATE_ONLINE,
+};
+
 struct ofono_modem {
 	char			*path;
+	enum ofono_modem_state   modem_state;
 	GSList			*atoms;
-	GSList			*pre_sim_atoms;
 	struct ofono_watchlist	*atom_watches;
 	GSList			*interface_list;
 	unsigned int		call_ids;
@@ -61,6 +68,8 @@ struct ofono_modem {
 	ofono_bool_t		powered;
 	ofono_bool_t		powered_pending;
 	guint			timeout;
+	ofono_bool_t		online;
+	ofono_bool_t            online_pending;
 	GHashTable		*properties;
 	struct ofono_sim	*sim;
 	unsigned int		sim_watch;
@@ -83,6 +92,7 @@ struct ofono_devinfo {
 
 struct ofono_atom {
 	enum ofono_atom_type type;
+	enum ofono_modem_state modem_state;
 	void (*destruct)(struct ofono_atom *atom);
 	void (*unregister)(struct ofono_atom *atom);
 	void *data;
@@ -160,6 +170,7 @@ struct ofono_atom *__ofono_modem_add_atom(struct ofono_modem *modem,
 	atom = g_new0(struct ofono_atom, 1);
 
 	atom->type = type;
+	atom->modem_state = modem->modem_state;
 	atom->destruct = destruct;
 	atom->data = data;
 	atom->modem = modem;
@@ -275,7 +286,6 @@ struct ofono_atom *__ofono_modem_find_atom(struct ofono_modem *modem,
 		return NULL;
 
 	FIND_ATOM_IN_LIST(modem->atoms)
-	FIND_ATOM_IN_LIST(modem->pre_sim_atoms);
 
 	return NULL;
 }
@@ -301,7 +311,6 @@ void __ofono_modem_foreach_atom(struct ofono_modem *modem,
 		return;
 
 	FOREACH_ATOM_IN_LIST(modem->atoms)
-	FOREACH_ATOM_IN_LIST(modem->pre_sim_atoms)
 }
 
 void __ofono_atom_free(struct ofono_atom *atom)
@@ -309,7 +318,6 @@ void __ofono_atom_free(struct ofono_atom *atom)
 	struct ofono_modem *modem = atom->modem;
 
 	modem->atoms = g_slist_remove(modem->atoms, atom);
-	modem->pre_sim_atoms = g_slist_remove(modem->pre_sim_atoms, atom);
 
 	__ofono_atom_unregister(atom);
 
@@ -319,13 +327,23 @@ void __ofono_atom_free(struct ofono_atom *atom)
 	g_free(atom);
 }
 
-static void remove_all_atoms(GSList **atoms)
+static void flush_atoms(struct ofono_modem *modem,
+			enum ofono_modem_state new_state)
 {
 	GSList *l;
 	struct ofono_atom *atom;
 
-	for (l = *atoms; l; l = l->next) {
-		atom = l->data;
+	for (;;) {
+		for (l = modem->atoms; l; l = l->next) {
+			atom = l->data;
+			if (atom->modem_state > new_state)
+				break;
+		}
+
+		if (!l)
+			return;
+
+		modem->atoms = g_slist_delete_link(modem->atoms, l);
 
 		__ofono_atom_unregister(atom);
 
@@ -334,9 +352,133 @@ static void remove_all_atoms(GSList **atoms)
 
 		g_free(atom);
 	}
+}
+
+static void modem_change_state(struct ofono_modem *modem,
+			enum ofono_modem_state new_state)
+{
+	struct ofono_modem_driver const *driver = modem->driver;
+	enum ofono_modem_state old_state = modem->modem_state;
+	ofono_bool_t online_changed = 0;
+
+	if (old_state == new_state)
+		return;
+
+	modem->modem_state = new_state;
+
+	if (old_state > new_state)
+		flush_atoms(modem, new_state);
+
+	switch (new_state) {
+	case OFONO_MODEM_STATE_POWER_OFF:
+		modem->call_ids = 0;
+		if (modem->online)
+			modem->online = 0, online_changed = 1;
+		break;
+
+	case OFONO_MODEM_STATE_PRE_SIM:
+		if (modem->online)
+			modem->online = 0, online_changed = 1;
+		if (new_state > old_state) {
+			if (driver->pre_sim)
+				driver->pre_sim(modem);
+		} else if (online_changed) {
+			if (driver->online)
+				driver->online(modem, 0, NULL, NULL);
+		}
+		break;
+
+	case OFONO_MODEM_STATE_OFFLINE:
+		if (modem->online)
+			modem->online = 0, online_changed = 1;
+		if (new_state > old_state) {
+			if (driver->post_sim)
+				driver->post_sim(modem);
+			__ofono_history_probe_drivers(modem);
+			__ofono_nettime_probe_drivers(modem);
+		}
+		break;
+
+	case OFONO_MODEM_STATE_ONLINE:
+		if (!modem->online)
+			modem->online = 1, online_changed = 1;
+		if (driver->post_online)
+			driver->post_online(modem);
+		break;
+	}
 
-	g_slist_free(*atoms);
-	*atoms = NULL;
+	if (online_changed) {
+		DBusConnection *conn = ofono_dbus_get_connection();
+		ofono_dbus_signal_property_changed(conn, modem->path,
+					OFONO_MODEM_INTERFACE, "Online",
+					DBUS_TYPE_BOOLEAN, &modem->online);
+	}
+}
+
+static void set_online_callback(const struct ofono_error *error,
+				void *data)
+{
+	struct ofono_modem *modem = data;
+	DBusMessage *reply = NULL;
+	ofono_bool_t online = modem->online_pending;
+
+	if (error && error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		reply = __ofono_error_failed(modem->pending);
+		online = modem->online;
+	} else if (online && modem->modem_state < OFONO_MODEM_STATE_OFFLINE) {
+		reply = __ofono_error_failed(modem->pending);
+		online = FALSE;
+	} else
+		reply = dbus_message_new_method_return(modem->pending);
+
+	__ofono_dbus_pending_reply(&modem->pending, reply);
+
+	modem->online_pending = online;
+
+	if (modem->online == online)
+		return;
+
+	if (online)
+		modem_change_state(modem, OFONO_MODEM_STATE_ONLINE);
+	else
+		modem_change_state(modem, OFONO_MODEM_STATE_OFFLINE);
+}
+
+static DBusMessage *set_property_online(struct ofono_modem *modem,
+					DBusMessage *msg,
+					DBusMessageIter *var)
+{
+	ofono_bool_t online;
+	const struct ofono_modem_driver *driver = modem->driver;
+
+	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->online == online)
+		return dbus_message_new_method_return(msg);
+
+	if (!driver || !driver->online)
+		return __ofono_error_failed(msg);
+
+	if (modem->pending != NULL)
+		return __ofono_error_busy(msg);
+
+	modem->pending = dbus_message_ref(msg);
+	modem->online_pending = online;
+
+	driver->online(modem, online, set_online_callback, modem);
+
+	return NULL;
+}
+
+ofono_bool_t ofono_modem_get_online(struct ofono_modem *modem)
+{
+	if (modem == NULL)
+		return FALSE;
+
+	return modem->online;
 }
 
 static DBusMessage *modem_get_properties(DBusConnection *conn,
@@ -361,6 +503,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);
 
@@ -420,11 +565,8 @@ static int set_powered(struct ofono_modem *modem, ofono_bool_t powered)
 		return -EALREADY;
 
 	/* Remove the atoms even if the driver is no longer available */
-	if (powered == FALSE) {
-		remove_all_atoms(&modem->atoms);
-		remove_all_atoms(&modem->pre_sim_atoms);
-		modem->call_ids = 0;
-	}
+	if (powered == FALSE)
+		modem_change_state(modem, OFONO_MODEM_STATE_POWER_OFF);
 
 	modem->powered_pending = powered;
 
@@ -502,6 +644,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;
@@ -535,10 +680,8 @@ static DBusMessage *modem_set_property(DBusConnection *conn,
 						"Powered", DBUS_TYPE_BOOLEAN,
 						&powered);
 
-		if (powered) {
-			if (modem->driver->pre_sim)
-				modem->driver->pre_sim(modem);
-		}
+		if (powered)
+			modem_change_state(modem, OFONO_MODEM_STATE_PRE_SIM);
 
 		return NULL;
 	}
@@ -596,14 +739,10 @@ void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
 						"Powered", DBUS_TYPE_BOOLEAN,
 						&dbus_powered);
 
-		if (powered) {
-			if (modem->driver->pre_sim)
-				modem->driver->pre_sim(modem);
-		} else {
-			remove_all_atoms(&modem->atoms);
-			remove_all_atoms(&modem->pre_sim_atoms);
-			modem->call_ids = 0;
-		}
+		if (powered)
+			modem_change_state(modem, OFONO_MODEM_STATE_PRE_SIM);
+		else
+			modem_change_state(modem, OFONO_MODEM_STATE_POWER_OFF);
 	}
 
 	if (powering_down && powered == FALSE) {
@@ -1132,22 +1271,12 @@ static void modem_sim_ready(void *user, enum ofono_sim_state new_state)
 
 	switch (new_state) {
 	case OFONO_SIM_STATE_NOT_PRESENT:
-		if (modem->pre_sim_atoms != NULL)
-			remove_all_atoms(&modem->atoms);
+		modem_change_state(modem, OFONO_MODEM_STATE_PRE_SIM);
 		break;
 	case OFONO_SIM_STATE_INSERTED:
 		break;
 	case OFONO_SIM_STATE_READY:
-		if (modem->pre_sim_atoms == NULL) {
-			modem->pre_sim_atoms = modem->atoms;
-			modem->atoms = NULL;
-		}
-
-		if (modem->driver->post_sim)
-			modem->driver->post_sim(modem);
-
-		__ofono_history_probe_drivers(modem);
-		__ofono_nettime_probe_drivers(modem);
+		modem_change_state(modem, OFONO_MODEM_STATE_OFFLINE);
 	}
 }
 
-- 
1.6.3.3

[PATCH 2/3] Add: test scripts online-modem and offline-modem

[Pekka Pessi]

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

---
 test/offline-modem |   17 +++++++++++++++++
 test/online-modem  |   17 +++++++++++++++++
 2 files changed, 34 insertions(+), 0 deletions(-)
 create mode 100755 test/offline-modem
 create mode 100755 test/online-modem

diff --git a/test/offline-modem b/test/offline-modem
new file mode 100755
index 0000000..16325ad
--- /dev/null
+++ b/test/offline-modem
@@ -0,0 +1,17 @@
+#!/usr/bin/python
+
+import dbus, sys
+
+bus = dbus.SystemBus()
+
+if len(sys.argv) == 2:
+	path = sys.argv[1]
+else:
+	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+			'org.ofono.Manager')
+	properties = manager.GetProperties()
+	path = properties["Modems"][0]
+
+print "Setting modem %s offline..." % path
+modem = dbus.Interface(bus.get_object('org.ofono', path), 'org.ofono.Modem')
+modem.SetProperty("Online", dbus.Boolean(0))
diff --git a/test/online-modem b/test/online-modem
new file mode 100755
index 0000000..db0b9f5
--- /dev/null
+++ b/test/online-modem
@@ -0,0 +1,17 @@
+#!/usr/bin/python
+
+import dbus, sys
+
+bus = dbus.SystemBus()
+
+if len(sys.argv) == 2:
+	path = sys.argv[1]
+else:
+	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+			'org.ofono.Manager')
+	properties = manager.GetProperties()
+	path = properties["Modems"][0]
+
+print "Setting modem %s online..." % path
+modem = dbus.Interface(bus.get_object('org.ofono', path), 'org.ofono.Modem')
+modem.SetProperty("Online", dbus.Boolean(1))
-- 
1.6.3.3

[PATCH 3/3] Add: online and post_online methods to isimodem driver

[Pekka Pessi]

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

---
 drivers/isimodem/isimodem.c |  120 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 115 insertions(+), 5 deletions(-)

diff --git a/drivers/isimodem/isimodem.c b/drivers/isimodem/isimodem.c
index 4085a55..99ba440 100644
--- a/drivers/isimodem/isimodem.c
+++ b/drivers/isimodem/isimodem.c
@@ -68,6 +68,11 @@ struct isi_data {
 	unsigned interval;
 	int reported;
 	int mtc_state;
+	int online;
+	struct {
+		ofono_modem_online_cb callback;
+		void *data;
+	} online_request;
 };
 
 static void report_powered(struct isi_data *isi, ofono_bool_t powered)
@@ -76,9 +81,33 @@ static void report_powered(struct isi_data *isi, ofono_bool_t powered)
 		ofono_modem_set_powered(isi->modem, isi->reported = powered);
 }
 
-static void set_power_by_mtc_state(struct isi_data *isi, int state)
+static void report_online(struct isi_data *isi, uint8_t mtc_state)
 {
-	switch (isi->mtc_state = state) {
+	ofono_modem_online_cb cb = isi->online_request.callback;
+	struct ofono_error error = {
+		(isi->online
+			? mtc_state == MTC_NORMAL
+			: mtc_state != MTC_NORMAL)
+		? OFONO_ERROR_TYPE_NO_ERROR
+		: OFONO_ERROR_TYPE_FAILURE,
+	};
+	void *data = isi->online_request.data;
+
+	isi->online_request.callback = 0;
+	isi->online_request.data = 0;
+
+	if (cb)
+		cb(&error, data);
+}
+
+
+static void set_power_by_mtc_state(struct isi_data *isi, int mtc_state)
+{
+	isi->mtc_state = mtc_state;
+
+	report_online(isi, mtc_state);
+
+	switch (mtc_state) {
 	case MTC_STATE_NONE:
 	case MTC_POWER_OFF:
 	case MTC_CHARGING:
@@ -87,6 +116,9 @@ static void set_power_by_mtc_state(struct isi_data *isi, int state)
 		break;
 
 	case MTC_RF_INACTIVE:
+		report_powered(isi, 1);
+		break;
+
 	case MTC_NORMAL:
 		report_powered(isi, 1);
 		break;
@@ -96,6 +128,7 @@ static void set_power_by_mtc_state(struct isi_data *isi, int state)
 	}
 }
 
+
 static void mtc_state_ind_cb(GIsiClient *client, const void *restrict data,
 				size_t len, uint16_t object, void *opaque)
 {
@@ -155,7 +188,8 @@ static bool mtc_poll_query_cb(GIsiClient *client, const void *restrict data,
 	DBG("target modem state: %s (0x%02X)",
 		mtc_modem_state_name(msg[2]), msg[2]);
 
-	set_power_by_mtc_state(isi, msg[1]);
+	if (msg[1] == msg[2])
+		set_power_by_mtc_state(isi, msg[1]);
 
 	return true;
 }
@@ -179,7 +213,8 @@ static bool mtc_query_cb(GIsiClient *client, const void *restrict data,
 	DBG("target modem state: %s (0x%02X)",
 		mtc_modem_state_name(msg[2]), msg[2]);
 
-	set_power_by_mtc_state(isi, msg[1]);
+	if (msg[1] == msg[2])
+		set_power_by_mtc_state(isi, msg[1]);
 
 	return true;
 }
@@ -276,6 +311,7 @@ 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->reported = -1;
 
 	return 0;
 }
@@ -292,6 +328,70 @@ static void isi_modem_remove(struct ofono_modem *modem)
 	g_free(isi);
 }
 
+static bool mtc_state_cb(GIsiClient *client, const void *restrict data,
+			size_t len, uint16_t object, void *opaque)
+{
+	struct isi_data *isi = opaque;
+	const unsigned char *msg = data;
+
+	if (!msg) {
+		DBG("ISI client error: %d", g_isi_client_error(client));
+		goto err;
+	}
+
+	if (len < 3 || msg[0] != MTC_STATE_RESP)
+		return false;
+	DBG("cause: %s (0x%02X)", mtc_isi_cause_name(msg[1]), msg[1]);
+	if (msg[1] == MTC_OK)
+		return true;
+
+err:
+	if (isi->online_request.callback) {
+		ofono_modem_online_cb cb = isi->online_request.callback;
+		struct ofono_error error = {
+			msg && msg[1] == MTC_ALREADY_ACTIVE
+			? OFONO_ERROR_TYPE_NO_ERROR
+			: OFONO_ERROR_TYPE_FAILURE,
+		};
+		void *cbdata = isi->online_request.data;
+		isi->online_request.callback = 0;
+		isi->online_request.data = 0;
+		cb(&error, cbdata);
+	}
+
+	return true;
+}
+
+static int mtc_state_req(struct isi_data *isi, uint8_t state)
+{
+	const unsigned char req[] = {
+		MTC_STATE_REQ, state, 0x00
+	};
+
+	if (g_isi_request_make(isi->client, req, sizeof(req), MTC_TIMEOUT,
+					mtc_state_cb, isi))
+		return 0;
+	else
+		return -errno;
+}
+
+static void isi_modem_online(struct ofono_modem *modem, ofono_bool_t online,
+			ofono_modem_online_cb callback, void *data)
+{
+	struct isi_data *isi = ofono_modem_get_data(modem);
+	int err;
+
+	DBG("(%p) with %s", modem, isi->ifname);
+	isi->online = online;
+	isi->online_request.callback = callback;
+	isi->online_request.data = data;
+
+	err = mtc_state_req(isi, online ? MTC_NORMAL : MTC_RF_INACTIVE);
+
+	if (err)
+		report_online(isi, MTC_STATE_NONE);
+}
+
 static void isi_modem_pre_sim(struct ofono_modem *modem)
 {
 	struct isi_data *isi = ofono_modem_get_data(modem);
@@ -306,12 +406,20 @@ static void isi_modem_pre_sim(struct ofono_modem *modem)
 static void isi_modem_post_sim(struct ofono_modem *modem)
 {
 	struct isi_data *isi = ofono_modem_get_data(modem);
+
+	DBG("(%p) with %s", modem, isi->ifname);
+
+	ofono_phonebook_create(isi->modem, 0, "isimodem", isi->idx);
+}
+
+static void isi_modem_post_online(struct ofono_modem *modem)
+{
+	struct isi_data *isi = ofono_modem_get_data(modem);
 	struct ofono_gprs *gprs;
 	struct ofono_gprs_context *gc;
 
 	DBG("(%p) with %s", modem, isi->ifname);
 
-	ofono_phonebook_create(isi->modem, 0, "isimodem", isi->idx);
 	ofono_netreg_create(isi->modem, 0, "isimodem", isi->idx);
 	ofono_sms_create(isi->modem, 0, "isimodem", isi->idx);
 	ofono_cbs_create(isi->modem, 0, "isimodem", isi->idx);
@@ -335,8 +443,10 @@ static struct ofono_modem_driver driver = {
 	.name = "isimodem",
 	.probe = isi_modem_probe,
 	.remove = isi_modem_remove,
+	.online = isi_modem_online,
 	.pre_sim = isi_modem_pre_sim,
 	.post_sim = isi_modem_post_sim,
+	.post_online = isi_modem_post_online,
 };
 
 static int isimodem_init(void)
-- 
1.6.3.3

Re: [PATCH 1/3] Add: Online property to modem

[Aki Niemi]

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

Hi Pekka,

On Fri, 2010-05-21 at 16:25 +0200, Pessi Pekka (Nokia-D/Helsinki) wrote:
> The online/offline state is changed with the new online() modem driver
> method.
> 
> In order to track atoms, there are modem states as follows:
> - OFONO_MODEM_STATE_POWER_OFF
> - OFONO_MODEM_STATE_PRE_SIM
> - OFONO_MODEM_STATE_OFFLINE
> - OFONO_MODEM_STATE_ONLINE

Also, doc/modem-api.txt needs to describe the D-Bus API of this new
Online property.

> Atoms are removed by modem core according to the state. Atoms are added with
> driver methods pre_sim(), post_sim() and post_online().
> ---
>  include/modem.h |   12 +++
>  src/modem.c     |  205 ++++++++++++++++++++++++++++++++++++++++++++----------
>  2 files changed, 179 insertions(+), 38 deletions(-)
> 
> diff --git a/include/modem.h b/include/modem.h
> index d502640..2e9bd87 100644
> --- a/include/modem.h
> +++ b/include/modem.h
> @@ -50,6 +50,8 @@ 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);
>  
> +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,
> @@ -64,6 +66,9 @@ int ofono_modem_set_boolean(struct ofono_modem *modem,
>  				const char *key, bool value);
>  bool ofono_modem_get_boolean(struct ofono_modem *modem, const char *key);
>  
> +typedef void (*ofono_modem_online_cb)(const struct ofono_error *error,
> +				void *data);
> +
>  struct ofono_modem_driver {
>  	const char *name;
>  
> @@ -80,11 +85,18 @@ struct ofono_modem_driver {
>  	/* Power down device */
>  	int (*disable)(struct ofono_modem *modem);
>  
> +	/* Enable or disable cellular radio */
> +	void (*online)(struct ofono_modem *modem, ofono_bool_t online,
> +		ofono_modem_online_cb callback, void *data);
> +

Let's call this set_online.

>  	/* Populate the atoms available without SIM / Locked SIM */
>  	void (*pre_sim)(struct ofono_modem *modem);
>  
>  	/* Populate the atoms that are available with SIM / Unlocked SIM*/
>  	void (*post_sim)(struct ofono_modem *modem);
> +
> +	/* Populate the atoms available online */
> +	void (*post_online)(struct ofono_modem *modem);
>  };
>  
>  int ofono_modem_driver_register(const struct ofono_modem_driver *);
> diff --git a/src/modem.c b/src/modem.c
> index 04fba15..bf78183 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -49,10 +49,17 @@ enum ofono_property_type {
>  	OFONO_PROPERTY_TYPE_BOOLEAN,
>  };
>  
> +enum ofono_modem_state {
> +	OFONO_MODEM_STATE_POWER_OFF,
> +	OFONO_MODEM_STATE_PRE_SIM,
> +	OFONO_MODEM_STATE_OFFLINE,
> +	OFONO_MODEM_STATE_ONLINE,
> +};
> +
>  struct ofono_modem {
>  	char			*path;
> +	enum ofono_modem_state   modem_state;
>  	GSList			*atoms;
> -	GSList			*pre_sim_atoms;
>  	struct ofono_watchlist	*atom_watches;
>  	GSList			*interface_list;
>  	unsigned int		call_ids;
> @@ -61,6 +68,8 @@ struct ofono_modem {
>  	ofono_bool_t		powered;
>  	ofono_bool_t		powered_pending;
>  	guint			timeout;
> +	ofono_bool_t		online;
> +	ofono_bool_t            online_pending;
>  	GHashTable		*properties;
>  	struct ofono_sim	*sim;
>  	unsigned int		sim_watch;
> @@ -83,6 +92,7 @@ struct ofono_devinfo {
>  
>  struct ofono_atom {
>  	enum ofono_atom_type type;
> +	enum ofono_modem_state modem_state;
>  	void (*destruct)(struct ofono_atom *atom);
>  	void (*unregister)(struct ofono_atom *atom);
>  	void *data;
> @@ -160,6 +170,7 @@ struct ofono_atom *__ofono_modem_add_atom(struct ofono_modem *modem,
>  	atom = g_new0(struct ofono_atom, 1);
>  
>  	atom->type = type;
> +	atom->modem_state = modem->modem_state;
>  	atom->destruct = destruct;
>  	atom->data = data;
>  	atom->modem = modem;
> @@ -275,7 +286,6 @@ struct ofono_atom *__ofono_modem_find_atom(struct ofono_modem *modem,
>  		return NULL;
>  
>  	FIND_ATOM_IN_LIST(modem->atoms)
> -	FIND_ATOM_IN_LIST(modem->pre_sim_atoms);
>  
>  	return NULL;
>  }
> @@ -301,7 +311,6 @@ void __ofono_modem_foreach_atom(struct ofono_modem *modem,
>  		return;
>  
>  	FOREACH_ATOM_IN_LIST(modem->atoms)
> -	FOREACH_ATOM_IN_LIST(modem->pre_sim_atoms)
>  }
>  
>  void __ofono_atom_free(struct ofono_atom *atom)
> @@ -309,7 +318,6 @@ void __ofono_atom_free(struct ofono_atom *atom)
>  	struct ofono_modem *modem = atom->modem;
>  
>  	modem->atoms = g_slist_remove(modem->atoms, atom);
> -	modem->pre_sim_atoms = g_slist_remove(modem->pre_sim_atoms, atom);
>  
>  	__ofono_atom_unregister(atom);
>  
> @@ -319,13 +327,23 @@ void __ofono_atom_free(struct ofono_atom *atom)
>  	g_free(atom);
>  }
>  
> -static void remove_all_atoms(GSList **atoms)
> +static void flush_atoms(struct ofono_modem *modem,
> +			enum ofono_modem_state new_state)
>  {
>  	GSList *l;
>  	struct ofono_atom *atom;
>  
> -	for (l = *atoms; l; l = l->next) {
> -		atom = l->data;
> +	for (;;) {
> +		for (l = modem->atoms; l; l = l->next) {
> +			atom = l->data;
> +			if (atom->modem_state > new_state)
> +				break;
> +		}
> +
> +		if (!l)
> +			return;
> +
> +		modem->atoms = g_slist_delete_link(modem->atoms, l);
>  
>  		__ofono_atom_unregister(atom);

Let's refactor this. It's not obvious why we need both these for loops.
How about something like this instead:

> 	struct ofono_atom *atom;
> 	GSList *l = modem->atoms;
> 	GSList *next;
> 	
> 	for (l = modem->atoms; l; l = next) {
> 
> 		atom = l->data;
> 		next = l->next;
> 
> 		if (atom->modem_state > new_state)
> 			continue;
> 
> 		modem->atoms = g_slist_delete_link(modem->atoms, l);
> 
> 		__ofono_atom_unregister(atom);
> 
> 		if (atom->destruct)
> 			atom->destruct(atom);
> 
> 		g_free(atom);
> 	}

Or is there a reason you'd want to go to the head of the list again
after removing a node?

> @@ -334,9 +352,133 @@ static void remove_all_atoms(GSList **atoms)
>  
>  		g_free(atom);
>  	}
> +}
> +
> +static void modem_change_state(struct ofono_modem *modem,
> +			enum ofono_modem_state new_state)
> +{
> +	struct ofono_modem_driver const *driver = modem->driver;
> +	enum ofono_modem_state old_state = modem->modem_state;
> +	ofono_bool_t online_changed = 0;
> +
> +	if (old_state == new_state)
> +		return;
> +
> +	modem->modem_state = new_state;
> +
> +	if (old_state > new_state)
> +		flush_atoms(modem, new_state);
> +
> +	switch (new_state) {
> +	case OFONO_MODEM_STATE_POWER_OFF:
> +		modem->call_ids = 0;
> +		if (modem->online)
> +			modem->online = 0, online_changed = 1;

Only one assignment per line, so this needs curly brackets.

> +		break;
> +
> +	case OFONO_MODEM_STATE_PRE_SIM:
> +		if (modem->online)
> +			modem->online = 0, online_changed = 1;

Same as above.

> +		if (new_state > old_state) {

I'd prefer this for readability:

if (old_state < OFONO_MODEM_STATE_PRE_SIM) {

> +			if (driver->pre_sim)
> +				driver->pre_sim(modem);
> +		} else if (online_changed) {
> +			if (driver->online)
> +				driver->online(modem, 0, NULL, NULL);
> +		}
> +		break;
> +
> +	case OFONO_MODEM_STATE_OFFLINE:
> +		if (modem->online)
> +			modem->online = 0, online_changed = 1;

Split into two lines.

> +		if (new_state > old_state) {

Same as above; this looks better:

if (old_state < OFONO_MODEM_STATE_OFFLINE) {

> +			if (driver->post_sim)
> +				driver->post_sim(modem);
> +			__ofono_history_probe_drivers(modem);
> +			__ofono_nettime_probe_drivers(modem);
> +		}
> +		break;
> +
> +	case OFONO_MODEM_STATE_ONLINE:

Line feed before if statement.

> +		if (!modem->online)
> +			modem->online = 1, online_changed = 1;

Split lines as above, and add line feed before the next if statement.

> +		if (driver->post_online)
> +			driver->post_online(modem);
> +		break;
> +	}
>  
> -	g_slist_free(*atoms);
> -	*atoms = NULL;
> +	if (online_changed) {
> +		DBusConnection *conn = ofono_dbus_get_connection();
> +		ofono_dbus_signal_property_changed(conn, modem->path,
> +					OFONO_MODEM_INTERFACE, "Online",
> +					DBUS_TYPE_BOOLEAN, &modem->online);
> +	}
> +}
> +
> +static void set_online_callback(const struct ofono_error *error,
> +				void *data)
> +{
> +	struct ofono_modem *modem = data;
> +	DBusMessage *reply = NULL;
> +	ofono_bool_t online = modem->online_pending;
> +
> +	if (error && error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		reply = __ofono_error_failed(modem->pending);
> +		online = modem->online;
> +	} else if (online && modem->modem_state < OFONO_MODEM_STATE_OFFLINE) {
> +		reply = __ofono_error_failed(modem->pending);
> +		online = FALSE;
> +	} else
> +		reply = dbus_message_new_method_return(modem->pending);
> +
> +	__ofono_dbus_pending_reply(&modem->pending, reply);
> +
> +	modem->online_pending = online;
> +
> +	if (modem->online == online)
> +		return;
> +
> +	if (online)
> +		modem_change_state(modem, OFONO_MODEM_STATE_ONLINE);
> +	else
> +		modem_change_state(modem, OFONO_MODEM_STATE_OFFLINE);
> +}
> +
> +static DBusMessage *set_property_online(struct ofono_modem *modem,
> +					DBusMessage *msg,
> +					DBusMessageIter *var)
> +{
> +	ofono_bool_t online;
> +	const struct ofono_modem_driver *driver = modem->driver;
> +
> +	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->online == online)
> +		return dbus_message_new_method_return(msg);
> +
> +	if (!driver || !driver->online)
> +		return __ofono_error_failed(msg);
> +
> +	if (modem->pending != NULL)
> +		return __ofono_error_busy(msg);
> +
> +	modem->pending = dbus_message_ref(msg);
> +	modem->online_pending = online;
> +
> +	driver->online(modem, online, set_online_callback, modem);
> +
> +	return NULL;
> +}
> +
> +ofono_bool_t ofono_modem_get_online(struct ofono_modem *modem)
> +{
> +	if (modem == NULL)
> +		return FALSE;
> +
> +	return modem->online;
>  }
>  
>  static DBusMessage *modem_get_properties(DBusConnection *conn,
> @@ -361,6 +503,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);
>  
> @@ -420,11 +565,8 @@ static int set_powered(struct ofono_modem *modem, ofono_bool_t powered)
>  		return -EALREADY;
>  
>  	/* Remove the atoms even if the driver is no longer available */
> -	if (powered == FALSE) {
> -		remove_all_atoms(&modem->atoms);
> -		remove_all_atoms(&modem->pre_sim_atoms);
> -		modem->call_ids = 0;
> -	}
> +	if (powered == FALSE)
> +		modem_change_state(modem, OFONO_MODEM_STATE_POWER_OFF);
>  
>  	modem->powered_pending = powered;
>  
> @@ -502,6 +644,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) {

Should probably split this out into a set_property_powered(), too.

>  		ofono_bool_t powered;
>  		int err;
> @@ -535,10 +680,8 @@ static DBusMessage *modem_set_property(DBusConnection *conn,
>  						"Powered", DBUS_TYPE_BOOLEAN,
>  						&powered);
>  
> -		if (powered) {
> -			if (modem->driver->pre_sim)
> -				modem->driver->pre_sim(modem);
> -		}
> +		if (powered)
> +			modem_change_state(modem, OFONO_MODEM_STATE_PRE_SIM);
>  
>  		return NULL;
>  	}
> @@ -596,14 +739,10 @@ void ofono_modem_set_powered(struct ofono_modem *modem, ofono_bool_t powered)
>  						"Powered", DBUS_TYPE_BOOLEAN,
>  						&dbus_powered);
>  
> -		if (powered) {
> -			if (modem->driver->pre_sim)
> -				modem->driver->pre_sim(modem);
> -		} else {
> -			remove_all_atoms(&modem->atoms);
> -			remove_all_atoms(&modem->pre_sim_atoms);
> -			modem->call_ids = 0;
> -		}
> +		if (powered)
> +			modem_change_state(modem, OFONO_MODEM_STATE_PRE_SIM);
> +		else
> +			modem_change_state(modem, OFONO_MODEM_STATE_POWER_OFF);
>  	}
>  
>  	if (powering_down && powered == FALSE) {
> @@ -1132,22 +1271,12 @@ static void modem_sim_ready(void *user, enum ofono_sim_state new_state)
>  
>  	switch (new_state) {
>  	case OFONO_SIM_STATE_NOT_PRESENT:
> -		if (modem->pre_sim_atoms != NULL)
> -			remove_all_atoms(&modem->atoms);
> +		modem_change_state(modem, OFONO_MODEM_STATE_PRE_SIM);
>  		break;
>  	case OFONO_SIM_STATE_INSERTED:
>  		break;
>  	case OFONO_SIM_STATE_READY:
> -		if (modem->pre_sim_atoms == NULL) {
> -			modem->pre_sim_atoms = modem->atoms;
> -			modem->atoms = NULL;
> -		}
> -
> -		if (modem->driver->post_sim)
> -			modem->driver->post_sim(modem);
> -
> -		__ofono_history_probe_drivers(modem);
> -		__ofono_nettime_probe_drivers(modem);
> +		modem_change_state(modem, OFONO_MODEM_STATE_OFFLINE);
>  	}
>  }
>  

Cheers,
Aki

Re: [PATCH 1/3] Add: Online property to modem

[Aki Niemi]

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

On Tue, 2010-05-25 at 15:48 +0200, Niemi Aki (Nokia-D/Helsinki) wrote:
> Let's refactor this. It's not obvious why we need both these for loops.
> How about something like this instead:
> 
> >       struct ofono_atom *atom;
> >       GSList *l = modem->atoms;
> >       GSList *next;
> >
> >       for (l = modem->atoms; l; l = next) {
> >
> >               atom = l->data;
> >               next = l->next;
> >
> >               if (atom->modem_state > new_state)
> >                       continue;

Hmm... This probably should've been:

if (atom->modem_state <= new_state)
	continue;

Cheers,
Aki

Re: [PATCH 3/3] Add: online and post_online methods to isimodem driver

[Aki Niemi]

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

Hi Pekka,

On Fri, 2010-05-21 at 16:25 +0200, Pessi Pekka (Nokia-D/Helsinki) wrote:
> ---
>  drivers/isimodem/isimodem.c |  120 +++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 115 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/isimodem/isimodem.c b/drivers/isimodem/isimodem.c
> index 4085a55..99ba440 100644
> --- a/drivers/isimodem/isimodem.c
> +++ b/drivers/isimodem/isimodem.c
> @@ -68,6 +68,11 @@ struct isi_data {
>  	unsigned interval;
>  	int reported;
>  	int mtc_state;
> +	int online;
> +	struct {
> +		ofono_modem_online_cb callback;
> +		void *data;
> +	} online_request;
>  };
>  
>  static void report_powered(struct isi_data *isi, ofono_bool_t powered)
> @@ -76,9 +81,33 @@ static void report_powered(struct isi_data *isi, ofono_bool_t powered)
>  		ofono_modem_set_powered(isi->modem, isi->reported = powered);
>  }
>  
> -static void set_power_by_mtc_state(struct isi_data *isi, int state)
> +static void report_online(struct isi_data *isi, uint8_t mtc_state)
>  {
> -	switch (isi->mtc_state = state) {
> +	ofono_modem_online_cb cb = isi->online_request.callback;
> +	struct ofono_error error = {
> +		(isi->online
> +			? mtc_state == MTC_NORMAL
> +			: mtc_state != MTC_NORMAL)
> +		? OFONO_ERROR_TYPE_NO_ERROR
> +		: OFONO_ERROR_TYPE_FAILURE,
> +	};

I can't parse this.

> +	void *data = isi->online_request.data;
> +
> +	isi->online_request.callback = 0;
> +	isi->online_request.data = 0;
> +
> +	if (cb)
> +		cb(&error, data);
> +}
> +
> +
> +static void set_power_by_mtc_state(struct isi_data *isi, int mtc_state)
> +{
> +	isi->mtc_state = mtc_state;
> +
> +	report_online(isi, mtc_state);

Should we check first for isi->mtc_state == mtc_state, and return?
Otherwise, what's the point in storing mtc_state?

> +	switch (mtc_state) {
>  	case MTC_STATE_NONE:
>  	case MTC_POWER_OFF:
>  	case MTC_CHARGING:
> @@ -87,6 +116,9 @@ static void set_power_by_mtc_state(struct isi_data *isi, int state)
>  		break;
>  
>  	case MTC_RF_INACTIVE:
> +		report_powered(isi, 1);
> +		break;
> +
>  	case MTC_NORMAL:
>  		report_powered(isi, 1);
>  		break;

Can't we fold all of these cases, including default, into one single
report_powered() call?

> @@ -96,6 +128,7 @@ static void set_power_by_mtc_state(struct isi_data *isi, int state)
>  	}
>  }
>  
> +

Extra line feed here.

>  static void mtc_state_ind_cb(GIsiClient *client, const void *restrict data,
>  				size_t len, uint16_t object, void *opaque)
>  {
> @@ -155,7 +188,8 @@ static bool mtc_poll_query_cb(GIsiClient *client, const void *restrict data,
>  	DBG("target modem state: %s (0x%02X)",
>  		mtc_modem_state_name(msg[2]), msg[2]);
>  
> -	set_power_by_mtc_state(isi, msg[1]);
> +	if (msg[1] == msg[2])
> +		set_power_by_mtc_state(isi, msg[1]);
>  
>  	return true;
>  }
> @@ -179,7 +213,8 @@ static bool mtc_query_cb(GIsiClient *client, const void *restrict data,
>  	DBG("target modem state: %s (0x%02X)",
>  		mtc_modem_state_name(msg[2]), msg[2]);
>  
> -	set_power_by_mtc_state(isi, msg[1]);
> +	if (msg[1] == msg[2])
> +		set_power_by_mtc_state(isi, msg[1]);
>  
>  	return true;
>  }
> @@ -276,6 +311,7 @@ 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->reported = -1;
>  
>  	return 0;
>  }
> @@ -292,6 +328,70 @@ static void isi_modem_remove(struct ofono_modem *modem)
>  	g_free(isi);
>  }
>  
> +static bool mtc_state_cb(GIsiClient *client, const void *restrict data,
> +			size_t len, uint16_t object, void *opaque)
> +{
> +	struct isi_data *isi = opaque;
> +	const unsigned char *msg = data;
> +
> +	if (!msg) {
> +		DBG("ISI client error: %d", g_isi_client_error(client));
> +		goto err;
> +	}
> +
> +	if (len < 3 || msg[0] != MTC_STATE_RESP)
> +		return false;

Add line feed here.

> +	DBG("cause: %s (0x%02X)", mtc_isi_cause_name(msg[1]), msg[1]);

And here.

> +	if (msg[1] == MTC_OK)
> +		return true;
> +
> +err:
> +	if (isi->online_request.callback) {
> +		ofono_modem_online_cb cb = isi->online_request.callback;
> +		struct ofono_error error = {
> +			msg && msg[1] == MTC_ALREADY_ACTIVE
> +			? OFONO_ERROR_TYPE_NO_ERROR
> +			: OFONO_ERROR_TYPE_FAILURE,
> +		};

This is hard to parse. Maybe use the CALLBACK_WITH_* macros, or separate
success/failure reporting to separate functions, or both.

> +		void *cbdata = isi->online_request.data;
> +		isi->online_request.callback = 0;
> +		isi->online_request.data = 0;
> +		cb(&error, cbdata);
> +	}

BTW, why aren't you using struct isi_cb_data? It can hold user data,
too, if a reference to struct isi_data is needed (although it isn't
needed here).

(If it is to avoid heap allocation for outgoing requests, then that is
futile anyway, because the current GIsiClient implementation already
does that.)

> +	return true;
> +}
> +
> +static int mtc_state_req(struct isi_data *isi, uint8_t state)
> +{
> +	const unsigned char req[] = {
> +		MTC_STATE_REQ, state, 0x00
> +	};
> +
> +	if (g_isi_request_make(isi->client, req, sizeof(req), MTC_TIMEOUT,
> +					mtc_state_cb, isi))
> +		return 0;
> +	else
> +		return -errno;
> +}

Why doesn't this simply return a boolean?

> +static void isi_modem_online(struct ofono_modem *modem, ofono_bool_t online,
> +			ofono_modem_online_cb callback, void *data)
> +{
> +	struct isi_data *isi = ofono_modem_get_data(modem);
> +	int err;
> +
> +	DBG("(%p) with %s", modem, isi->ifname);
> +	isi->online = online;
> +	isi->online_request.callback = callback;
> +	isi->online_request.data = data;
> +
> +	err = mtc_state_req(isi, online ? MTC_NORMAL : MTC_RF_INACTIVE);
> +
> +	if (err)
> +		report_online(isi, MTC_STATE_NONE);
> +}

...especially since the errno isn't even logged here.

>  static void isi_modem_pre_sim(struct ofono_modem *modem)
>  {
>  	struct isi_data *isi = ofono_modem_get_data(modem);
> @@ -306,12 +406,20 @@ static void isi_modem_pre_sim(struct ofono_modem *modem)
>  static void isi_modem_post_sim(struct ofono_modem *modem)
>  {
>  	struct isi_data *isi = ofono_modem_get_data(modem);
> +
> +	DBG("(%p) with %s", modem, isi->ifname);
> +
> +	ofono_phonebook_create(isi->modem, 0, "isimodem", isi->idx);
> +}
> +
> +static void isi_modem_post_online(struct ofono_modem *modem)
> +{
> +	struct isi_data *isi = ofono_modem_get_data(modem);
>  	struct ofono_gprs *gprs;
>  	struct ofono_gprs_context *gc;
>  
>  	DBG("(%p) with %s", modem, isi->ifname);
>  
> -	ofono_phonebook_create(isi->modem, 0, "isimodem", isi->idx);
>  	ofono_netreg_create(isi->modem, 0, "isimodem", isi->idx);
>  	ofono_sms_create(isi->modem, 0, "isimodem", isi->idx);
>  	ofono_cbs_create(isi->modem, 0, "isimodem", isi->idx);
> @@ -335,8 +443,10 @@ static struct ofono_modem_driver driver = {
>  	.name = "isimodem",
>  	.probe = isi_modem_probe,
>  	.remove = isi_modem_remove,
> +	.online = isi_modem_online,
>  	.pre_sim = isi_modem_pre_sim,
>  	.post_sim = isi_modem_post_sim,
> +	.post_online = isi_modem_post_online,
>  };
>  
>  static int isimodem_init(void)

Cheers,
Aki

Re: [PATCH 3/3] Add: online and post_online methods to isimodem driver

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

Le mardi 25 mai 2010 23:24:21 Aki Niemi, vous avez écrit :
> > @@ -76,9 +81,33 @@ static void report_powered(struct isi_data *isi,
> > ofono_bool_t powered)
> > 
> >  		ofono_modem_set_powered(isi->modem, isi->reported = powered);
> >  
> >  }
> > 
> > -static void set_power_by_mtc_state(struct isi_data *isi, int state)
> > +static void report_online(struct isi_data *isi, uint8_t mtc_state)
> > 
> >  {
> > 
> > -	switch (isi->mtc_state = state) {
> > +	ofono_modem_online_cb cb = isi->online_request.callback;
> > +	struct ofono_error error = {
> > +		(isi->online
> > +			? mtc_state == MTC_NORMAL
> > +			: mtc_state != MTC_NORMAL)
> > +		? OFONO_ERROR_TYPE_NO_ERROR
> > +		: OFONO_ERROR_TYPE_FAILURE,
> > +	};
> 
> I can't parse this.

This is an intricate way to compare two boolean values:

((!isi->online) == (mtc_state != MTC_NORMAL))
	? OFONO_ERROR_TYPE_NO_ERROR
	: OFONO_ERROR_TYPE_FAILURE;

i.e. if ISI online status equals to MTC state normal, then no error, 
otherwise, error. C does not provide a *logical* XOR operator that would 
simplify this.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis

Re: [PATCH 3/3] Add: online and post_online methods to isimodem driver

[Aki Niemi]

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

Hi Rémi,

On Tue, 2010-05-25 at 22:32 +0200, ext Rémi Denis-Courmont wrote:
> > > -	switch (isi->mtc_state = state) {
> > > +	ofono_modem_online_cb cb = isi->online_request.callback;
> > > +	struct ofono_error error = {
> > > +		(isi->online
> > > +			? mtc_state == MTC_NORMAL
> > > +			: mtc_state != MTC_NORMAL)
> > > +		? OFONO_ERROR_TYPE_NO_ERROR
> > > +		: OFONO_ERROR_TYPE_FAILURE,
> > > +	};
> > 
> > I can't parse this.
> 
> This is an intricate way to compare two boolean values:
> 
> ((!isi->online) == (mtc_state != MTC_NORMAL))
> 	? OFONO_ERROR_TYPE_NO_ERROR
> 	: OFONO_ERROR_TYPE_FAILURE;
> 
> i.e. if ISI online status equals to MTC state normal, then no error, 
> otherwise, error. C does not provide a *logical* XOR operator that would 
> simplify this.

It just isn't very readable.

Write that open, and I am able to parse it, so I think something like
this would be better:

bool not_online = isi->online != TRUE;
bool not_normal = mtc_state != MTC_NORMAL;

struct ofono_error error = {
	not_normal && not_online ? NO_ERROR : FAILURE;
};

Cheers,
Aki

Re: [PATCH 3/3] Add: online and post_online methods to isimodem driver

[=?unknown-8bit?q?R=C3=A9mi?= Denis-Courmont]

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

Le mardi 25 mai 2010 23:51:09 Aki Niemi, vous avez écrit :
> > i.e. if ISI online status equals to MTC state normal, then no error,
> > otherwise, error. C does not provide a *logical* XOR operator that would
> > simplify this.
> 
> It just isn't very readable.
> 
> Write that open, and I am able to parse it, so I think something like
> this would be better:
> 
> bool not_online = isi->online != TRUE;
> bool not_normal = mtc_state != MTC_NORMAL;
> 
> struct ofono_error error = {
> 	not_normal && not_online ? NO_ERROR : FAILURE;
> };

Err no. You've put a logical AND where Pekka had a logical NOT XOR.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis

Re: [PATCH 3/3] Add: online and post_online methods to isimodem driver

[Aki Niemi]

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

On Tue, 2010-05-25 at 23:01 +0200, ext Rémi Denis-Courmont wrote:
> > bool not_online = isi->online != TRUE;
> > bool not_normal = mtc_state != MTC_NORMAL;
> > 
> > struct ofono_error error = {
> > 	not_normal && not_online ? NO_ERROR : FAILURE;
> > };
> 
> Err no. You've put a logical AND where Pekka had a logical NOT XOR.

Ah, so maybe it should have been s/&&/==, but my point remains.
Chaining ? operators like that is confusing.

Cheers,
Aki