[RFC online/offline atoms PATCH 0/4] modem: add modem state watch

[RFC online/offline atoms PATCH 0/4] modem: add modem state watch

[Pekka.Pessi]

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

Hello all,

Here is a proposal for adding a mechanism for atoms to follow modem
states, with main interest in online and offline.  There are also
examples of two atoms, sms and gprs, using the watch.

The final patch modifies the isigen driver so that it will create sms
and gprs atoms already in offline. Similar patches are needed for most
of the drivers, too.

--Pekka

[RFC online/offline atoms PATCH 1/4] modem: add modem state watch

[Pekka.Pessi]

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

From: Andras Domokos <andras.domokos@nokia.com>

Modem state notifiers get called after modem state has been changed
and atoms have been added or flushed.

The modem state watch benefit the atoms that are active in several
states, such as voicecall, sms, or gprs.
---
 src/modem.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 src/ofono.h |   15 +++++++++++++
 2 files changed, 67 insertions(+), 12 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index 7a29edf..dc7c1b2 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -51,16 +51,9 @@ enum property_type {
 	PROPERTY_TYPE_BOOLEAN,
 };
 
-enum modem_state {
-	MODEM_STATE_POWER_OFF,
-	MODEM_STATE_PRE_SIM,
-	MODEM_STATE_OFFLINE,
-	MODEM_STATE_ONLINE,
-};
-
 struct ofono_modem {
 	char			*path;
-	enum modem_state	modem_state;
+	enum ofono_modem_state	modem_state;
 	GSList			*atoms;
 	struct ofono_watchlist	*atom_watches;
 	GSList			*interface_list;
@@ -72,6 +65,7 @@ struct ofono_modem {
 	ofono_bool_t		powered_pending;
 	guint			timeout;
 	ofono_bool_t		online;
+	struct ofono_watchlist *state_watches;
 	GHashTable		*properties;
 	struct ofono_sim	*sim;
 	unsigned int		sim_watch;
@@ -94,7 +88,7 @@ struct ofono_devinfo {
 
 struct ofono_atom {
 	enum ofono_atom_type type;
-	enum modem_state modem_state;
+	enum ofono_modem_state modem_state;
 	void (*destruct)(struct ofono_atom *atom);
 	void (*unregister)(struct ofono_atom *atom);
 	void *data;
@@ -324,7 +318,8 @@ void __ofono_atom_free(struct ofono_atom *atom)
 	g_free(atom);
 }
 
-static void flush_atoms(struct ofono_modem *modem, enum modem_state new_state)
+static void flush_atoms(struct ofono_modem *modem,
+			enum ofono_modem_state new_state)
 {
 	GSList *cur;
 	GSList *prev;
@@ -360,11 +355,50 @@ static void flush_atoms(struct ofono_modem *modem, enum modem_state new_state)
 	}
 }
 
+static void notify_state_watches(struct ofono_modem *modem)
+{
+	struct ofono_watchlist_item *item;
+	GSList *l;
+	ofono_modem_state_notify_func notify;
+
+	if (modem->state_watches == NULL)
+		return;
+
+	for (l = modem->state_watches->items; l; l = l->next) {
+		item = l->data;
+		notify = item->notify;
+		notify(modem->modem_state, item->notify_data);
+	}
+}
+
+unsigned __ofono_modem_add_state_watch(struct ofono_modem *modem,
+					ofono_modem_state_notify_func notify,
+					void *data, ofono_destroy_func destroy)
+{
+	struct ofono_watchlist_item *item;
+
+	if (modem == NULL || notify == NULL)
+		return 0;
+
+	item = g_new0(struct ofono_watchlist_item, 1);
+
+	item->notify = notify;
+	item->destroy = destroy;
+	item->notify_data = data;
+
+	return __ofono_watchlist_add_item(modem->state_watches, item);
+}
+
+void __ofono_modem_remove_state_watch(struct ofono_modem *modem, unsigned id)
+{
+	__ofono_watchlist_remove_item(modem->state_watches, id);
+}
+
 static void modem_change_state(struct ofono_modem *modem,
-				enum modem_state new_state)
+				enum ofono_modem_state new_state)
 {
 	struct ofono_modem_driver const *driver = modem->driver;
-	enum modem_state old_state = modem->modem_state;
+	enum ofono_modem_state old_state = modem->modem_state;
 	ofono_bool_t new_online = new_state == MODEM_STATE_ONLINE;
 
 	if (old_state == new_state)
@@ -407,6 +441,8 @@ static void modem_change_state(struct ofono_modem *modem,
 			driver->post_online(modem);
 		break;
 	}
+
+	notify_state_watches(modem);
 }
 
 static void sim_state_watch(enum ofono_sim_state new_state, void *user)
@@ -1457,6 +1493,7 @@ int ofono_modem_register(struct ofono_modem *modem)
 	modem->driver_type = NULL;
 
 	modem->atom_watches = __ofono_watchlist_new(g_free);
+	modem->state_watches = __ofono_watchlist_new(g_free);
 
 	emit_modem_added(modem);
 	call_modemwatches(modem, TRUE);
@@ -1488,6 +1525,9 @@ static void modem_unregister(struct ofono_modem *modem)
 	__ofono_watchlist_free(modem->atom_watches);
 	modem->atom_watches = NULL;
 
+	__ofono_watchlist_free(modem->state_watches);
+	modem->state_watches = NULL;
+
 	modem->sim_watch = 0;
 	modem->sim_ready_watch = 0;
 
diff --git a/src/ofono.h b/src/ofono.h
index 6c7f649..9021f51 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -177,6 +177,21 @@ unsigned int __ofono_modemwatch_add(ofono_modemwatch_cb_t cb, void *user,
 					ofono_destroy_func destroy);
 gboolean __ofono_modemwatch_remove(unsigned int id);
 
+enum ofono_modem_state {
+	MODEM_STATE_POWER_OFF,
+	MODEM_STATE_PRE_SIM,
+	MODEM_STATE_OFFLINE,
+	MODEM_STATE_ONLINE,
+};
+
+typedef void (*ofono_modem_state_notify_func)(enum ofono_modem_state state,
+						void *data);
+unsigned int __ofono_modem_add_state_watch(struct ofono_modem *modem,
+				ofono_modem_state_notify_func notify,
+				void *data, ofono_destroy_func destroy);
+void __ofono_modem_remove_state_watch(struct ofono_modem *modem,
+					unsigned int id);
+
 #include <ofono/call-barring.h>
 
 gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);
-- 
1.7.0.4

[RFC online/offline atoms PATCH 2/4] sms: watch modem state

[Pekka.Pessi]

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

From: Pekka Pessi <Pekka.Pessi@nokia.com>

Allow use of SMS atom in online and offline states.  The messages are
queued but not sent in offline mode. Errors occurring when an SMS is
being sent while transition from online to offline are handled
gracefully.
---
 src/sms.c |   46 ++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index aee9a61..f79bb7d 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -72,6 +72,7 @@ struct ofono_sms {
 	guint tx_source;
 	struct ofono_message_waiting *mw;
 	unsigned int mw_watch;
+	unsigned int online_watch;
 	struct ofono_sim *sim;
 	GKeyFile *settings;
 	char *imsi;
@@ -661,6 +662,11 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	DBG("tx_finished");
 
 	if (ok == FALSE) {
+		/* Retry again when back in online mode */
+		/* Note this does not increment retry count */
+		if (!ofono_modem_get_online(modem))
+			return;
+
 		if (!(entry->flags & OFONO_SMS_SUBMIT_FLAG_RETRY))
 			goto next_q;
 
@@ -668,7 +674,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 
 		if (entry->retry < TXQ_MAX_RETRIES) {
 			DBG("Sending failed, retry in %d secs",
-					entry->retry * 5);
+				entry->retry * 5);
 			sms->tx_source = g_timeout_add_seconds(entry->retry * 5,
 								tx_next, sms);
 			return;
@@ -718,6 +724,9 @@ next_q:
 
 	tx_queue_entry_destroy(entry);
 
+	if (!ofono_modem_get_online(modem))
+		return;
+
 	if (g_queue_peek_head(sms->txq)) {
 		DBG("Scheduling next");
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
@@ -727,6 +736,7 @@ next_q:
 static gboolean tx_next(gpointer user_data)
 {
 	struct ofono_sms *sms = user_data;
+	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
 	int send_mms = 0;
 	struct tx_queue_entry *entry = g_queue_peek_head(sms->txq);
 	struct pending_pdu *pdu = &entry->pdus[entry->cur_pdu];
@@ -741,6 +751,9 @@ static gboolean tx_next(gpointer user_data)
 	if (!entry)
 		return FALSE;
 
+	if (!ofono_modem_get_online(modem))
+		return FALSE;
+
 	if (g_queue_get_length(sms->txq) > 1
 			|| (entry->num_pdus - entry->cur_pdu) > 1)
 		send_mms = 1;
@@ -751,6 +764,21 @@ static gboolean tx_next(gpointer user_data)
 	return FALSE;
 }
 
+static void online_watch(enum ofono_modem_state modem_state, void *data)
+{
+
+	struct ofono_sms *sms = data;
+
+	if (modem_state != MODEM_STATE_ONLINE)
+		return;
+
+	if (sms->tx_source > 0)
+		return;
+
+	if (g_queue_get_length(sms->txq))
+		sms->tx_source = g_timeout_add(0, tx_next, sms);
+}
+
 static void set_ref_and_to(GSList *msg_list, guint16 ref, int offset,
 				gboolean use_16bit, const char *to)
 {
@@ -943,14 +971,15 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 
 	g_queue_push_tail(sms->txq, entry);
 
-	if (g_queue_get_length(sms->txq) == 1)
+	modem = __ofono_atom_get_modem(sms->atom);
+
+	if (ofono_modem_get_online(modem) && g_queue_get_length(sms->txq) == 1)
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
 
 	path = message_build_path(sms, m);
 	g_dbus_send_reply(conn, msg, DBUS_TYPE_OBJECT_PATH, &path,
 					DBUS_TYPE_INVALID);
 
-	modem = __ofono_atom_get_modem(sms->atom);
 	__ofono_history_sms_send_pending(modem, &entry->uuid,
 						to, time(NULL), text);
 
@@ -1512,6 +1541,11 @@ static void sms_unregister(struct ofono_atom *atom)
 		sms->mw = NULL;
 	}
 
+	if (sms->online_watch) {
+		__ofono_modem_remove_state_watch(modem, sms->online_watch);
+		sms->online_watch = 0;
+	}
+
 	if (sms->messages) {
 		GHashTableIter iter;
 		struct message *m;
@@ -1714,6 +1748,9 @@ void ofono_sms_register(struct ofono_sms *sms)
 	if (mw_atom && __ofono_atom_get_registered(mw_atom))
 		mw_watch(mw_atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED, sms);
 
+	sms->online_watch = __ofono_modem_add_state_watch(modem,
+					online_watch, sms, NULL);
+
 	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
 
 	/*
@@ -1765,6 +1802,7 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
 				ofono_sms_txq_submit_cb_t cb,
 				void *data, ofono_destroy_func destroy)
 {
+	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
 	struct tx_queue_entry *entry;
 
 	entry = tx_queue_entry_new(list, flags, cb, data, destroy);
@@ -1773,7 +1811,7 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
 
 	g_queue_push_tail(sms->txq, entry);
 
-	if (g_queue_get_length(sms->txq) == 1)
+	if (ofono_modem_get_online(modem) && g_queue_get_length(sms->txq) == 1)
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
 
 	if (uuid)
-- 
1.7.0.4

[RFC online/offline atoms PATCH 3/4] gprs: watch modem state

[Pekka.Pessi]

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

From: Pekka Pessi <Pekka.Pessi@nokia.com>

Indicate detach when modem is in offline state.
---
 src/gprs.c |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 3f085ed..431275c 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -76,6 +76,7 @@ struct ofono_gprs {
 	struct ofono_netreg *netreg;
 	unsigned int netreg_watch;
 	unsigned int status_watch;
+	unsigned int online_watch;
 	GKeyFile *settings;
 	char *imsi;
 	DBusMessage *pending;
@@ -1048,8 +1049,17 @@ static void gprs_attach_callback(const struct ofono_error *error, void *data)
 
 static void gprs_netreg_update(struct ofono_gprs *gprs)
 {
+	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
 	ofono_bool_t attach;
 
+	if (!ofono_modem_get_online(modem)) {
+		gprs->flags &= ~(GPRS_FLAG_RECHECK | GPRS_FLAG_ATTACHING);
+		gprs->netreg_status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
+		gprs->driver_attached = FALSE;
+		gprs_attached_update(gprs);
+		return;
+	}
+
 	attach = gprs->netreg_status == NETWORK_REGISTRATION_STATUS_REGISTERED;
 
 	attach = attach || (gprs->roaming_allowed &&
@@ -1740,6 +1750,11 @@ static void gprs_unregister(struct ofono_atom *atom)
 		gprs->cid_map = NULL;
 	}
 
+	if (gprs->online_watch) {
+		__ofono_modem_remove_state_watch(modem, gprs->online_watch);
+		gprs->online_watch = 0;
+	}
+
 	if (gprs->netreg_watch) {
 		if (gprs->status_watch) {
 			__ofono_netreg_remove_status_watch(gprs->netreg,
@@ -1844,6 +1859,14 @@ static void netreg_watch(struct ofono_atom *atom,
 	gprs_netreg_update(gprs);
 }
 
+static void online_watch(enum ofono_modem_state modem_state, void *data)
+{
+	struct ofono_gprs *gprs = data;
+
+	if (modem_state != MODEM_STATE_ONLINE)
+		gprs_netreg_update(gprs);
+}
+
 static gboolean load_context(struct ofono_gprs *gprs, const char *group)
 {
 	char *name = NULL;
@@ -2041,6 +2064,9 @@ void ofono_gprs_register(struct ofono_gprs *gprs)
 	ofono_modem_add_interface(modem,
 				OFONO_CONNECTION_MANAGER_INTERFACE);
 
+	gprs->online_watch = __ofono_modem_add_state_watch(modem,
+					online_watch, gprs, NULL);
+
 	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
 
 	if (sim_atom) {
-- 
1.7.0.4

[RFC online/offline atoms PATCH 4/4] isigen: create sms and gprs in post_sim

[Pekka.Pessi]

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

From: Pekka Pessi <Pekka.Pessi@nokia.com>

---
 plugins/isigen.c |   20 ++++++++++----------
 1 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/plugins/isigen.c b/plugins/isigen.c
index db61fdb..d550a34 100644
--- a/plugins/isigen.c
+++ b/plugins/isigen.c
@@ -395,22 +395,29 @@ static void isigen_pre_sim(struct ofono_modem *modem)
 static void isigen_post_sim(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_sms_create(isi->modem, 0, "isimodem", isi->idx);
+	gprs = ofono_gprs_create(isi->modem, 0, "isimodem", isi->idx);
+	gc = ofono_gprs_context_create(isi->modem, 0, "isimodem", isi->idx);
+
+	if (gprs && gc)
+		ofono_gprs_add_context(gprs, gc);
+	else
+		DBG("Failed to add context");
 }
 
 static void isigen_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_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);
 	ofono_ssn_create(isi->modem, 0, "isimodem", isi->idx);
 	ofono_ussd_create(isi->modem, 0, "isimodem", isi->idx);
@@ -419,13 +426,6 @@ static void isigen_post_online(struct ofono_modem *modem)
 	ofono_call_barring_create(isi->modem, 0, "isimodem", isi->idx);
 	ofono_call_meter_create(isi->modem, 0, "isimodem", isi->idx);
 	ofono_radio_settings_create(isi->modem, 0, "isimodem", isi->idx);
-	gprs = ofono_gprs_create(isi->modem, 0, "isimodem", isi->idx);
-	gc = ofono_gprs_context_create(isi->modem, 0, "isimodem", isi->idx);
-
-	if (gprs && gc)
-		ofono_gprs_add_context(gprs, gc);
-	else
-		DBG("Failed to add context");
 }
 
 static struct ofono_modem_driver driver = {
-- 
1.7.0.4

Re: [RFC online/offline atoms PATCH 4/4] isigen: create sms and gprs in post_sim

[Marcel Holtmann]

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

Hi Pekka,

> ---
>  plugins/isigen.c |   20 ++++++++++----------
>  1 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/plugins/isigen.c b/plugins/isigen.c
> index db61fdb..d550a34 100644
> --- a/plugins/isigen.c
> +++ b/plugins/isigen.c
> @@ -395,22 +395,29 @@ static void isigen_pre_sim(struct ofono_modem *modem)
>  static void isigen_post_sim(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_sms_create(isi->modem, 0, "isimodem", isi->idx);
> +	gprs = ofono_gprs_create(isi->modem, 0, "isimodem", isi->idx);
> +	gc = ofono_gprs_context_create(isi->modem, 0, "isimodem", isi->idx);

what advantage to we have from moving SMS and GPRS atom into the
post_sim state? I am trying to really understand what this is trying to
do and what is the difference of just doing it when in post_online
state.

Are we expecting SMS and GPRS to just bring the modem online if needed
or what is this about? I don't see how useful this could be.

Regards

Marcel

Re: [RFC online/offline atoms PATCH 4/4] isigen: create sms and gprs in post_sim

[Denis Kenzior]

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

Hi Marcel,

> what advantage to we have from moving SMS and GPRS atom into the
> post_sim state? I am trying to really understand what this is trying to
> do and what is the difference of just doing it when in post_online
> state.

We'd like to be able to toggle GPRS settings even if in offline state.
There's nothing preventing us from creating / deleting / editing
contexts or setting the RoamingAllowed property for instance.

Regards,
-Denis

Re: [RFC online/offline atoms PATCH 4/4] isigen: create sms and gprs in post_sim

[Marcel Holtmann]

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

Hi Denis,

> > what advantage to we have from moving SMS and GPRS atom into the
> > post_sim state? I am trying to really understand what this is trying to
> > do and what is the difference of just doing it when in post_online
> > state.
> 
> We'd like to be able to toggle GPRS settings even if in offline state.
> There's nothing preventing us from creating / deleting / editing
> contexts or setting the RoamingAllowed property for instance.

same as with SMS. Good enough reason for me to do it this way.

Regards

Marcel

Re: [RFC online/offline atoms PATCH 3/4] gprs: watch modem state

[Denis Kenzior]

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

Hi Pekka,

On 10/05/2010 06:35 AM, Pekka.Pessi(a)nokia.com wrote:
> From: Pekka Pessi <Pekka.Pessi@nokia.com>
> 
> Indicate detach when modem is in offline state.
> ---
>  src/gprs.c |   26 ++++++++++++++++++++++++++
>  1 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/src/gprs.c b/src/gprs.c
> index 3f085ed..431275c 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -76,6 +76,7 @@ struct ofono_gprs {
>  	struct ofono_netreg *netreg;
>  	unsigned int netreg_watch;
>  	unsigned int status_watch;
> +	unsigned int online_watch;
>  	GKeyFile *settings;
>  	char *imsi;
>  	DBusMessage *pending;
> @@ -1048,8 +1049,17 @@ static void gprs_attach_callback(const struct ofono_error *error, void *data)
>  
>  static void gprs_netreg_update(struct ofono_gprs *gprs)
>  {
> +	struct ofono_modem *modem = __ofono_atom_get_modem(gprs->atom);
>  	ofono_bool_t attach;
>  
> +	if (!ofono_modem_get_online(modem)) {
> +		gprs->flags &= ~(GPRS_FLAG_RECHECK | GPRS_FLAG_ATTACHING);
> +		gprs->netreg_status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
> +		gprs->driver_attached = FALSE;
> +		gprs_attached_update(gprs);
> +		return;
> +	}
> +
>  	attach = gprs->netreg_status == NETWORK_REGISTRATION_STATUS_REGISTERED;
>  
>  	attach = attach || (gprs->roaming_allowed &&
> @@ -1740,6 +1750,11 @@ static void gprs_unregister(struct ofono_atom *atom)
>  		gprs->cid_map = NULL;
>  	}
>  
> +	if (gprs->online_watch) {
> +		__ofono_modem_remove_state_watch(modem, gprs->online_watch);
> +		gprs->online_watch = 0;
> +	}
> +
>  	if (gprs->netreg_watch) {
>  		if (gprs->status_watch) {
>  			__ofono_netreg_remove_status_watch(gprs->netreg,
> @@ -1844,6 +1859,14 @@ static void netreg_watch(struct ofono_atom *atom,
>  	gprs_netreg_update(gprs);
>  }
>  
> +static void online_watch(enum ofono_modem_state modem_state, void *data)
> +{
> +	struct ofono_gprs *gprs = data;
> +
> +	if (modem_state != MODEM_STATE_ONLINE)
> +		gprs_netreg_update(gprs);

So again, the question is whether it is better to watch for the netreg
atom going away instead of using an online/offline watch...

> +}
> +
>  static gboolean load_context(struct ofono_gprs *gprs, const char *group)
>  {
>  	char *name = NULL;
> @@ -2041,6 +2064,9 @@ void ofono_gprs_register(struct ofono_gprs *gprs)
>  	ofono_modem_add_interface(modem,
>  				OFONO_CONNECTION_MANAGER_INTERFACE);
>  
> +	gprs->online_watch = __ofono_modem_add_state_watch(modem,
> +					online_watch, gprs, NULL);
> +
>  	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
>  
>  	if (sim_atom) {

Regards,
-Denis

Re: [RFC online/offline atoms PATCH 2/4] sms: watch modem state

[Marcel Holtmann]

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

Hi Pekka,

> Allow use of SMS atom in online and offline states.  The messages are
> queued but not sent in offline mode. Errors occurring when an SMS is
> being sent while transition from online to offline are handled
> gracefully.

do we really want this? How is this suppose to work from the user
experience. I think just failing SendMessage with a proper error is a
more clear indication.

I had a quick test with my iPhone and it warns me about being in offline
mode and then sending just fails. And I have to manually trigger of
sending the message again.

So I am bit concerned about blindly spooling messages when in offline
mode.

Regards

Marcel

Re: [RFC online/offline atoms PATCH 2/4] sms: watch modem state

[Denis Kenzior]

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

Hi Marcel,

On 10/05/2010 06:59 AM, Marcel Holtmann wrote:
> Hi Pekka,
> 
>> Allow use of SMS atom in online and offline states.  The messages are
>> queued but not sent in offline mode. Errors occurring when an SMS is
>> being sent while transition from online to offline are handled
>> gracefully.
> 
> do we really want this? How is this suppose to work from the user
> experience. I think just failing SendMessage with a proper error is a
> more clear indication.

That is a valid point, the code should be able to handle this use case,
but from a UI stand point I don't know what is better.

The main reason for moving sms atom from online to offline is the
ability to switch the settings.  In particular the SMSC address which is
stored on the SIM.

Regards,
-Denis

Re: [RFC online/offline atoms PATCH 2/4] sms: watch modem state

[Marcel Holtmann]

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

Hi Denis,

> >> Allow use of SMS atom in online and offline states.  The messages are
> >> queued but not sent in offline mode. Errors occurring when an SMS is
> >> being sent while transition from online to offline are handled
> >> gracefully.
> > 
> > do we really want this? How is this suppose to work from the user
> > experience. I think just failing SendMessage with a proper error is a
> > more clear indication.
> 
> That is a valid point, the code should be able to handle this use case,
> but from a UI stand point I don't know what is better.
> 
> The main reason for moving sms atom from online to offline is the
> ability to switch the settings.  In particular the SMSC address which is
> stored on the SIM.

good enough reason for me. Just blindly spooling SMS is not what I want
right now. We should just fail with a proper error for now. Being able
to send SMS when offline needs a bit more thinking about on how the user
exceptions are and should be handled.

Regards

Marcel

Re: [RFC online/offline atoms PATCH 2/4] sms: watch modem state

[Denis Kenzior]

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

Hi Pekka,

On 10/05/2010 06:35 AM, Pekka.Pessi(a)nokia.com wrote:
> From: Pekka Pessi <Pekka.Pessi@nokia.com>
> 
> Allow use of SMS atom in online and offline states.  The messages are
> queued but not sent in offline mode. Errors occurring when an SMS is
> being sent while transition from online to offline are handled
> gracefully.
> ---
>  src/sms.c |   46 ++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/src/sms.c b/src/sms.c
> index aee9a61..f79bb7d 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -72,6 +72,7 @@ struct ofono_sms {
>  	guint tx_source;
>  	struct ofono_message_waiting *mw;
>  	unsigned int mw_watch;
> +	unsigned int online_watch;
>  	struct ofono_sim *sim;
>  	GKeyFile *settings;
>  	char *imsi;
> @@ -661,6 +662,11 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
>  	DBG("tx_finished");
>  
>  	if (ok == FALSE) {
> +		/* Retry again when back in online mode */
> +		/* Note this does not increment retry count */
> +		if (!ofono_modem_get_online(modem))
> +			return;
> +

So this part needs to be discused some more.  Should we be using an
online watch here, or checking the status of the netreg atom instead?
Since netreg is not available in offline mode, its absence is a good
indicator of being offline as well.  Plus we shouldn't try to send SMSes
when we're not registered / roaming.

>  		if (!(entry->flags & OFONO_SMS_SUBMIT_FLAG_RETRY))
>  			goto next_q;
>  
> @@ -668,7 +674,7 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
>  
>  		if (entry->retry < TXQ_MAX_RETRIES) {
>  			DBG("Sending failed, retry in %d secs",
> -					entry->retry * 5);
> +				entry->retry * 5);

This doesn't seem related...

>  			sms->tx_source = g_timeout_add_seconds(entry->retry * 5,
>  								tx_next, sms);
>  			return;
> @@ -718,6 +724,9 @@ next_q:
>  
>  	tx_queue_entry_destroy(entry);
>  
> +	if (!ofono_modem_get_online(modem))
> +		return;
> +
>  	if (g_queue_peek_head(sms->txq)) {
>  		DBG("Scheduling next");
>  		sms->tx_source = g_timeout_add(0, tx_next, sms);
> @@ -727,6 +736,7 @@ next_q:
>  static gboolean tx_next(gpointer user_data)
>  {
>  	struct ofono_sms *sms = user_data;
> +	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
>  	int send_mms = 0;
>  	struct tx_queue_entry *entry = g_queue_peek_head(sms->txq);
>  	struct pending_pdu *pdu = &entry->pdus[entry->cur_pdu];
> @@ -741,6 +751,9 @@ static gboolean tx_next(gpointer user_data)
>  	if (!entry)
>  		return FALSE;
>  
> +	if (!ofono_modem_get_online(modem))
> +		return FALSE;
> +
>  	if (g_queue_get_length(sms->txq) > 1
>  			|| (entry->num_pdus - entry->cur_pdu) > 1)
>  		send_mms = 1;
> @@ -751,6 +764,21 @@ static gboolean tx_next(gpointer user_data)
>  	return FALSE;
>  }
>  
> +static void online_watch(enum ofono_modem_state modem_state, void *data)
> +{
> +
> +	struct ofono_sms *sms = data;
> +
> +	if (modem_state != MODEM_STATE_ONLINE)
> +		return;
> +
> +	if (sms->tx_source > 0)
> +		return;

If the modem goes offline / netreg atom disappears or gets deregistered,
removing the tx_source might be better.

> +
> +	if (g_queue_get_length(sms->txq))
> +		sms->tx_source = g_timeout_add(0, tx_next, sms);
> +}
> +
>  static void set_ref_and_to(GSList *msg_list, guint16 ref, int offset,
>  				gboolean use_16bit, const char *to)
>  {
> @@ -943,14 +971,15 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  
>  	g_queue_push_tail(sms->txq, entry);
>  
> -	if (g_queue_get_length(sms->txq) == 1)
> +	modem = __ofono_atom_get_modem(sms->atom);
> +
> +	if (ofono_modem_get_online(modem) && g_queue_get_length(sms->txq) == 1)
>  		sms->tx_source = g_timeout_add(0, tx_next, sms);

So here again, perhaps we also need to check the netreg status.

>  
>  	path = message_build_path(sms, m);
>  	g_dbus_send_reply(conn, msg, DBUS_TYPE_OBJECT_PATH, &path,
>  					DBUS_TYPE_INVALID);
>  
> -	modem = __ofono_atom_get_modem(sms->atom);
>  	__ofono_history_sms_send_pending(modem, &entry->uuid,
>  						to, time(NULL), text);
>  
> @@ -1512,6 +1541,11 @@ static void sms_unregister(struct ofono_atom *atom)
>  		sms->mw = NULL;
>  	}
>  
> +	if (sms->online_watch) {
> +		__ofono_modem_remove_state_watch(modem, sms->online_watch);
> +		sms->online_watch = 0;
> +	}
> +
>  	if (sms->messages) {
>  		GHashTableIter iter;
>  		struct message *m;
> @@ -1714,6 +1748,9 @@ void ofono_sms_register(struct ofono_sms *sms)
>  	if (mw_atom && __ofono_atom_get_registered(mw_atom))
>  		mw_watch(mw_atom, OFONO_ATOM_WATCH_CONDITION_REGISTERED, sms);
>  
> +	sms->online_watch = __ofono_modem_add_state_watch(modem,
> +					online_watch, sms, NULL);
> +
>  	sim_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_SIM);
>  
>  	/*
> @@ -1765,6 +1802,7 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  				ofono_sms_txq_submit_cb_t cb,
>  				void *data, ofono_destroy_func destroy)
>  {
> +	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
>  	struct tx_queue_entry *entry;
>  
>  	entry = tx_queue_entry_new(list, flags, cb, data, destroy);
> @@ -1773,7 +1811,7 @@ int __ofono_sms_txq_submit(struct ofono_sms *sms, GSList *list,
>  
>  	g_queue_push_tail(sms->txq, entry);
>  
> -	if (g_queue_get_length(sms->txq) == 1)
> +	if (ofono_modem_get_online(modem) && g_queue_get_length(sms->txq) == 1)
>  		sms->tx_source = g_timeout_add(0, tx_next, sms);
>  
>  	if (uuid)

Regards,
-Denis

Re: [RFC online/offline atoms PATCH 1/4] modem: add modem state watch

[Marcel Holtmann]

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

Hi Pekka,

> Modem state notifiers get called after modem state has been changed
> and atoms have been added or flushed.
> 
> The modem state watch benefit the atoms that are active in several
> states, such as voicecall, sms, or gprs.
> ---
>  src/modem.c |   64 +++++++++++++++++++++++++++++++++++++++++++++++-----------
>  src/ofono.h |   15 +++++++++++++
>  2 files changed, 67 insertions(+), 12 deletions(-)
> 
> diff --git a/src/modem.c b/src/modem.c
> index 7a29edf..dc7c1b2 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -51,16 +51,9 @@ enum property_type {
>  	PROPERTY_TYPE_BOOLEAN,
>  };
>  
> -enum modem_state {
> -	MODEM_STATE_POWER_OFF,
> -	MODEM_STATE_PRE_SIM,
> -	MODEM_STATE_OFFLINE,
> -	MODEM_STATE_ONLINE,
> -};
> -
>  struct ofono_modem {
>  	char			*path;
> -	enum modem_state	modem_state;
> +	enum ofono_modem_state	modem_state;
>  	GSList			*atoms;
>  	struct ofono_watchlist	*atom_watches;
>  	GSList			*interface_list;
> @@ -72,6 +65,7 @@ struct ofono_modem {
>  	ofono_bool_t		powered_pending;
>  	guint			timeout;
>  	ofono_bool_t		online;
> +	struct ofono_watchlist *state_watches;
>  	GHashTable		*properties;
>  	struct ofono_sim	*sim;
>  	unsigned int		sim_watch;
> @@ -94,7 +88,7 @@ struct ofono_devinfo {
>  
>  struct ofono_atom {
>  	enum ofono_atom_type type;
> -	enum modem_state modem_state;
> +	enum ofono_modem_state modem_state;
>  	void (*destruct)(struct ofono_atom *atom);
>  	void (*unregister)(struct ofono_atom *atom);
>  	void *data;
> @@ -324,7 +318,8 @@ void __ofono_atom_free(struct ofono_atom *atom)
>  	g_free(atom);
>  }
>  
> -static void flush_atoms(struct ofono_modem *modem, enum modem_state new_state)
> +static void flush_atoms(struct ofono_modem *modem,
> +			enum ofono_modem_state new_state)
>  {
>  	GSList *cur;
>  	GSList *prev;
> @@ -360,11 +355,50 @@ static void flush_atoms(struct ofono_modem *modem, enum modem_state new_state)
>  	}
>  }
>  
> +static void notify_state_watches(struct ofono_modem *modem)
> +{
> +	struct ofono_watchlist_item *item;
> +	GSList *l;
> +	ofono_modem_state_notify_func notify;
> +
> +	if (modem->state_watches == NULL)
> +		return;
> +
> +	for (l = modem->state_watches->items; l; l = l->next) {
> +		item = l->data;
> +		notify = item->notify;
> +		notify(modem->modem_state, item->notify_data);
> +	}
> +}
> +
> +unsigned __ofono_modem_add_state_watch(struct ofono_modem *modem,
> +					ofono_modem_state_notify_func notify,
> +					void *data, ofono_destroy_func destroy)
> +{
> +	struct ofono_watchlist_item *item;
> +
> +	if (modem == NULL || notify == NULL)
> +		return 0;
> +
> +	item = g_new0(struct ofono_watchlist_item, 1);
> +
> +	item->notify = notify;
> +	item->destroy = destroy;
> +	item->notify_data = data;
> +
> +	return __ofono_watchlist_add_item(modem->state_watches, item);
> +}
> +
> +void __ofono_modem_remove_state_watch(struct ofono_modem *modem, unsigned id)
> +{
> +	__ofono_watchlist_remove_item(modem->state_watches, id);
> +}
> +
>  static void modem_change_state(struct ofono_modem *modem,
> -				enum modem_state new_state)
> +				enum ofono_modem_state new_state)
>  {
>  	struct ofono_modem_driver const *driver = modem->driver;
> -	enum modem_state old_state = modem->modem_state;
> +	enum ofono_modem_state old_state = modem->modem_state;
>  	ofono_bool_t new_online = new_state == MODEM_STATE_ONLINE;
>  
>  	if (old_state == new_state)
> @@ -407,6 +441,8 @@ static void modem_change_state(struct ofono_modem *modem,
>  			driver->post_online(modem);
>  		break;
>  	}
> +
> +	notify_state_watches(modem);
>  }
>  
>  static void sim_state_watch(enum ofono_sim_state new_state, void *user)
> @@ -1457,6 +1493,7 @@ int ofono_modem_register(struct ofono_modem *modem)
>  	modem->driver_type = NULL;
>  
>  	modem->atom_watches = __ofono_watchlist_new(g_free);
> +	modem->state_watches = __ofono_watchlist_new(g_free);
>  
>  	emit_modem_added(modem);
>  	call_modemwatches(modem, TRUE);
> @@ -1488,6 +1525,9 @@ static void modem_unregister(struct ofono_modem *modem)
>  	__ofono_watchlist_free(modem->atom_watches);
>  	modem->atom_watches = NULL;
>  
> +	__ofono_watchlist_free(modem->state_watches);
> +	modem->state_watches = NULL;
> +
>  	modem->sim_watch = 0;
>  	modem->sim_ready_watch = 0;
>  
> diff --git a/src/ofono.h b/src/ofono.h
> index 6c7f649..9021f51 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -177,6 +177,21 @@ unsigned int __ofono_modemwatch_add(ofono_modemwatch_cb_t cb, void *user,
>  					ofono_destroy_func destroy);
>  gboolean __ofono_modemwatch_remove(unsigned int id);
>  
> +enum ofono_modem_state {
> +	MODEM_STATE_POWER_OFF,
> +	MODEM_STATE_PRE_SIM,
> +	MODEM_STATE_OFFLINE,
> +	MODEM_STATE_ONLINE,
> +};

so I am not sure if we wanna have separate modem notifiers or just a
generic one for state. I like to have Denis comment on this one.

However in case we decide to use a state, then just using MODEM_STATE_*
without a proper OFONO_ prefix is not a good idea. It can potentially
clash. Even if this is internal API only, I don't think it is a good
idea to just go without any proper namespacing.

Regards

Marcel

Re: [RFC online/offline atoms PATCH 1/4] modem: add modem state watch

[Denis Kenzior]

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

Hi Andras & Pekka,

<snip>

> diff --git a/src/ofono.h b/src/ofono.h
> index 6c7f649..9021f51 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -177,6 +177,21 @@ unsigned int __ofono_modemwatch_add(ofono_modemwatch_cb_t cb, void *user,
>  					ofono_destroy_func destroy);
>  gboolean __ofono_modemwatch_remove(unsigned int id);
>  
> +enum ofono_modem_state {
> +	MODEM_STATE_POWER_OFF,
> +	MODEM_STATE_PRE_SIM,
> +	MODEM_STATE_OFFLINE,
> +	MODEM_STATE_ONLINE,
> +};

Personally I don't see the need for anything besides an online/offline
watch.  Nobody will ever care about the other states.

> +
> +typedef void (*ofono_modem_state_notify_func)(enum ofono_modem_state state,
> +						void *data);

So I suggest ofono_modem_online_notify_func(gboolean online, void *data)

> +unsigned int __ofono_modem_add_state_watch(struct ofono_modem *modem,
> +				ofono_modem_state_notify_func notify,
> +				void *data, ofono_destroy_func destroy);
> +void __ofono_modem_remove_state_watch(struct ofono_modem *modem,
> +					unsigned int id);
> +

And add/remove online_watch here

>  #include <ofono/call-barring.h>
>  
>  gboolean __ofono_call_barring_is_busy(struct ofono_call_barring *cb);

Regards,
-Denis

Re: [RFC online/offline atoms PATCH 1/4] modem: add modem state watch

[Pekka Pessi]

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

Hi Denis,

2010/10/13 Denis Kenzior <denkenz@gmail.com>:
>> @@ -177,6 +177,21 @@ unsigned int __ofono_modemwatch_add(ofono_modemwatch_cb_t cb, void *user,
>>                                       ofono_destroy_func destroy);
>>  gboolean __ofono_modemwatch_remove(unsigned int id);
>>
>> +enum ofono_modem_state {
>> +     MODEM_STATE_POWER_OFF,
>> +     MODEM_STATE_PRE_SIM,
>> +     MODEM_STATE_OFFLINE,
>> +     MODEM_STATE_ONLINE,
>> +};
>
> Personally I don't see the need for anything besides an online/offline
> watch.  Nobody will ever care about the other states.

In principle, this information would be useful for voicecall atom. On
the other hand, it can follow sim state, too. We'll change this (back)
to online/offline unless we can figure other use for the full modem
state.

-- 
Pekka.Pessi mail at nokia.com