[PATCH 01/11] In drivers/atmodem/sms.c:at_cmgl_cpms_cb() there is a temporary fix for Siemens TC65. If AT+CMGL=4 is sent to TC65, the AT command queue jams.

[PATCH 01/11] In drivers/atmodem/sms.c:at_cmgl_cpms_cb() there is a temporary fix for Siemens TC65. If AT+CMGL=4 is sent to TC65, the AT command queue jams.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 drivers/atmodem/sms.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index 8049df6..27de77b 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -544,6 +544,10 @@ static void at_cmgl_cb(gboolean ok, GAtResult *result, gpointer user_data)
 
 static void at_cmgl_cpms_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
+
+	DBG("PASS AT+CMGL=4!!!");
+	return;
+
 	struct cpms_request *req = user_data;
 	struct ofono_sms *sms = req->sms;
 	struct sms_data *data = ofono_sms_get_data(sms);
-- 
1.6.0.4

[PATCH 02/11] Made needed changes to at_cds_notify() for status report and corrected at_cmgl_cpms_cb() to meet the ISO standard.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 drivers/atmodem/sms.c |   26 +++++++++++++++++++-------
 1 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index 27de77b..a1c885b 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -283,16 +283,28 @@ static void at_cds_notify(GAtResult *result, gpointer user_data)
 {
 	struct ofono_sms *sms = user_data;
 	struct sms_data *data = ofono_sms_get_data(sms);
-	int pdulen;
-	const char *pdu;
+	long pdu_len;
+	int tpdu_len;
+	const char *hexpdu;
+	unsigned char pdu[176];
 	char buf[256];
 
-	if (!at_parse_pdu_common(result, "+CDS:", &pdu, &pdulen)) {
+	if (!at_parse_pdu_common(result, "+CDS:", &hexpdu, &tpdu_len)) {
 		ofono_error("Unable to parse CDS notification");
 		return;
 	}
 
-	DBG("Got new Status-Report PDU via CDS: %s, %d", pdu, pdulen);
+	/*Is this necessary?*/
+	if (strlen(hexpdu) > sizeof(pdu) * 2) {
+		ofono_error("Bad PDU length in CMT notification");
+		return;
+	}
+
+	DBG("Got new Status-Report PDU via CDS: %s, %d", hexpdu, tpdu_len);
+
+	/*Decode pdu and notify about new SMS status report*/
+	decode_hex_own_buf(hexpdu, -1, &pdu_len, 0, pdu);
+	ofono_sms_status_report_notify(sms, pdu, pdu_len, tpdu_len);
 
 	/* We must acknowledge the PDU using CNMA */
 	if (data->cnma_ack_pdu)
@@ -545,13 +557,13 @@ static void at_cmgl_cb(gboolean ok, GAtResult *result, gpointer user_data)
 static void at_cmgl_cpms_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 
-	DBG("PASS AT+CMGL=4!!!");
-	return;
-
 	struct cpms_request *req = user_data;
 	struct ofono_sms *sms = req->sms;
 	struct sms_data *data = ofono_sms_get_data(sms);
 
+	DBG("PASS AT+CMGL=4!!!");
+	return;
+
 	if (!ok) {
 		ofono_error("Initial CPMS request failed");
 		at_cmgl_done(sms);
-- 
1.6.0.4

[PATCH 03/11] Enable status report.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 src/smsutil.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index e634764..2679cc2 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2596,7 +2596,7 @@ GSList *sms_text_prepare(const char *utf8, guint16 ref,
 	template.submit.rd = FALSE;
 	template.submit.vpf = SMS_VALIDITY_PERIOD_FORMAT_RELATIVE;
 	template.submit.rp = FALSE;
-	template.submit.srr = FALSE;
+	template.submit.srr = TRUE;
 	template.submit.mr = 0;
 	template.submit.vp.relative = 0xA7; /* 24 Hours */
 
-- 
1.6.0.4

[PATCH 04/11] Changes for SMS statur report.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 include/sms.h |    2 +-
 src/sms.c     |  124 ++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 119 insertions(+), 7 deletions(-)

diff --git a/include/sms.h b/include/sms.h
index daaec37..c007675 100644
--- a/include/sms.h
+++ b/include/sms.h
@@ -54,7 +54,7 @@ struct ofono_sms_driver {
 
 void ofono_sms_deliver_notify(struct ofono_sms *sms, unsigned char *pdu,
 				int len, int tpdu_len);
-void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
+void ofono_sms_status_report_notify(struct ofono_sms *sms, unsigned char *pdu,
 				int len, int tpdu_len);
 
 int ofono_sms_driver_register(const struct ofono_sms_driver *d);
diff --git a/src/sms.c b/src/sms.c
index 855bef8..63e0190 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -459,9 +459,10 @@ static GDBusMethodTable sms_manager_methods[] = {
 };
 
 static GDBusSignalTable sms_manager_signals[] = {
-	{ "PropertyChanged",	"sv"		},
-	{ "IncomingMessage",	"sa{sv}"	},
-	{ "ImmediateMessage",	"sa{sv}"	},
+	{ "PropertyChanged",		"sv"		},
+	{ "IncomingMessage",		"sa{sv}"	},
+	{ "ImmediateMessage",		"sa{sv}"	},
+	{ "IncomingStatusReport",	"{sv}"		},
 	{ }
 };
 
@@ -471,6 +472,7 @@ static void dispatch_app_datagram(struct ofono_sms *sms, int dst, int src,
 	DBG("Got app datagram for dst port: %d, src port: %d",
 			dst, src);
 	DBG("Contents-Len: %ld", len);
+	//DBG("buf: %s", buf);
 }
 
 static void dispatch_text_message(struct ofono_sms *sms,
@@ -539,6 +541,74 @@ static void dispatch_text_message(struct ofono_sms *sms,
 	}
 }
 
+static void dispatch_sms_delivery_report(struct ofono_sms *sms,
+					const enum sms_st *st,
+					const struct sms_address *raddr,
+					const struct sms_scts *scts)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = __ofono_atom_get_path(sms->atom);
+	DBusMessage *signal;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+	char buf[128];
+	const char *signal_name;
+	time_t ts;
+	struct tm remote;
+	struct tm local;
+	const char *str = buf;
+
+	if (!st){
+		DBG("status unavailable");
+		return;
+	}
+
+	signal_name = "IncomingStatusReport";
+
+	signal = dbus_message_new_signal(path, OFONO_SMS_MANAGER_INTERFACE,
+						signal_name);
+
+	if (!signal)
+		return;
+
+
+	/*Start assembling dbus-message*/
+	dbus_message_iter_init_append(signal, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+						&dict);
+
+	/*This is the time when sender sent the message,
+	  should be delivery time?*/
+	ts = sms_scts_to_time(scts, &remote);
+	localtime_r(&ts, &local);
+
+	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
+	buf[127] = '\0';
+	ofono_dbus_dict_append(&dict, "LocalSentTime", DBUS_TYPE_STRING, &str);
+
+	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &remote);
+	buf[127] = '\0';
+	ofono_dbus_dict_append(&dict, "SentTime", DBUS_TYPE_STRING, &str);
+
+	/*Status*/
+	if(*st==0x00){
+		str = sms_address_to_string(raddr);
+		ofono_dbus_dict_append(&dict, "Message was delivered to", DBUS_TYPE_STRING, &str);
+	}
+	else{
+		str = sms_address_to_string(raddr);
+		ofono_dbus_dict_append(&dict, "Message was not delivered to", DBUS_TYPE_STRING, &str);
+	}
+
+	/*dbus-message assembled*/
+	dbus_message_iter_close_container(&iter, &dict);
+
+	g_dbus_send_message(conn, signal);
+
+}
+
 static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
 {
 	GSList *l;
@@ -646,6 +716,18 @@ static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
 	}
 }
 
+static void sms_status_report_dispatch(struct ofono_sms *sms, GSList *sms_list)
+{
+	const struct sms *s;
+	enum sms_charset uninitialized_var(old_charset);
+
+	s = sms_list->data;
+	dispatch_sms_delivery_report(sms, &s->status_report.st,
+					&s->status_report.raddr,
+					&s->status_report.scts);
+
+}
+
 static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 {
 	GSList *l;
@@ -679,6 +761,19 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 	g_slist_free(l);
 }
 
+static void handle_sms_status_report(struct ofono_sms *sms, const struct sms *incoming)
+{
+	GSList *l;
+
+	/*TODO:
+	fragmented SMS delivery report? check handle_deliver()
+	*/
+
+	l = g_slist_append(NULL, (void *)incoming);
+	sms_status_report_dispatch(sms, l);
+	g_slist_free(l);
+}
+
 static inline gboolean handle_mwi(struct ofono_sms *sms, struct sms *s)
 {
 	gboolean discard;
@@ -696,7 +791,7 @@ void ofono_sms_deliver_notify(struct ofono_sms *sms, unsigned char *pdu,
 {
 	struct sms s;
 	enum sms_class cls;
-
+	DBG("ofono_sms_deliver_notify");
 	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
 		ofono_error("Unable to decode PDU");
 		return;
@@ -807,10 +902,27 @@ out:
 	handle_deliver(sms, &s);
 }
 
-void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
+void ofono_sms_status_report_notify(struct ofono_sms *sms, unsigned char *pdu,
 				int len, int tpdu_len)
 {
-	ofono_error("SMS Status-Report not yet handled");
+	struct sms s;
+	enum sms_class cls;
+
+	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
+		ofono_error("Unable to decode PDU");
+		return;
+	}
+
+	if (s.type != SMS_TYPE_STATUS_REPORT) {
+		ofono_error("Expecting a STATUS REPORT pdu");
+	}
+
+	if (!sms_dcs_decode(s.deliver.dcs, &cls, NULL, NULL, NULL)) {
+		ofono_error("Unknown / Reserved DCS.  Ignoring");
+		return;
+	}
+
+	handle_sms_status_report(sms, &s);
 }
 
 int ofono_sms_driver_register(const struct ofono_sms_driver *d)
-- 
1.6.0.4

[PATCH 05/11] Added message delivery time to dbus message.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 src/sms.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 63e0190..b4a5364 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -579,8 +579,7 @@ static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 					OFONO_PROPERTIES_ARRAY_SIGNATURE,
 						&dict);
 
-	/*This is the time when sender sent the message,
-	  should be delivery time?*/
+	/*This is the time when sender sent the message*/
 	ts = sms_scts_to_time(scts, &remote);
 	localtime_r(&ts, &local);
 
@@ -592,6 +591,14 @@ static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 	buf[127] = '\0';
 	ofono_dbus_dict_append(&dict, "SentTime", DBUS_TYPE_STRING, &str);
 
+	/*This is the time when the message was delivered to the recipient*/
+	ts = sms_scts_to_time(dt, &remote);
+	localtime_r(&ts, &local);
+
+	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
+	buf[127] = '\0';
+	ofono_dbus_dict_append(&dict, "LocalDeliverTime", DBUS_TYPE_STRING, &str);
+
 	/*Status*/
 	if(*st==0x00){
 		str = sms_address_to_string(raddr);
-- 
1.6.0.4

[PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

Also made some repairs for dispatch_sms_delivery_report() and
sms_status_report_dispatch() to survive the make :)
---
 src/sms.c       |   13 ++++++++-----
 src/smsutil.c   |    5 +++--
 src/smsutil.h   |    3 ++-
 unit/test-sms.c |   10 +++++-----
 4 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index b4a5364..d079ece 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -400,6 +400,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	struct ofono_sms *sms = data;
 	const char *to;
 	const char *text;
+	gboolean ask_status_report;
 	GSList *msg_list;
 	int ref_offset;
 	struct tx_queue_entry *entry;
@@ -407,13 +408,14 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 
 	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
 					DBUS_TYPE_STRING, &text,
+					DBUS_TYPE_BOOLEAN, &ask_status_report,
 					DBUS_TYPE_INVALID))
 		return __ofono_error_invalid_args(msg);
 
 	if (valid_phone_number_format(to) == FALSE)
 		return __ofono_error_invalid_format(msg);
 
-	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset);
+	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset, ask_status_report);
 
 	if (!msg_list)
 		return __ofono_error_invalid_format(msg);
@@ -453,7 +455,7 @@ static GDBusMethodTable sms_manager_methods[] = {
 							G_DBUS_METHOD_FLAG_ASYNC },
 	{ "SetProperty",	"sv",	"",		sms_set_property,
 							G_DBUS_METHOD_FLAG_ASYNC },
-	{ "SendMessage",	"ss",	"",		sms_send_message,
+	{ "SendMessage",	"ssb",	"",		sms_send_message,
 							G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
 };
@@ -472,7 +474,6 @@ static void dispatch_app_datagram(struct ofono_sms *sms, int dst, int src,
 	DBG("Got app datagram for dst port: %d, src port: %d",
 			dst, src);
 	DBG("Contents-Len: %ld", len);
-	//DBG("buf: %s", buf);
 }
 
 static void dispatch_text_message(struct ofono_sms *sms,
@@ -544,7 +545,8 @@ static void dispatch_text_message(struct ofono_sms *sms,
 static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 					const enum sms_st *st,
 					const struct sms_address *raddr,
-					const struct sms_scts *scts)
+					const struct sms_scts *scts,
+					const struct sms_scts *dt)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path = __ofono_atom_get_path(sms->atom);
@@ -731,7 +733,8 @@ static void sms_status_report_dispatch(struct ofono_sms *sms, GSList *sms_list)
 	s = sms_list->data;
 	dispatch_sms_delivery_report(sms, &s->status_report.st,
 					&s->status_report.raddr,
-					&s->status_report.scts);
+					&s->status_report.scts,
+					&s->status_report.dt);
 
 }
 
diff --git a/src/smsutil.c b/src/smsutil.c
index 2679cc2..b627458 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -2580,7 +2580,8 @@ static inline GSList *sms_list_append(GSList *l, const struct sms *in)
  * if no concatenation took place.
  */
 GSList *sms_text_prepare(const char *utf8, guint16 ref,
-				gboolean use_16bit, int *ref_offset)
+				gboolean use_16bit, int *ref_offset,
+				const gboolean ask_status_report)
 {
 	struct sms template;
 	int offset = 0;
@@ -2596,7 +2597,7 @@ GSList *sms_text_prepare(const char *utf8, guint16 ref,
 	template.submit.rd = FALSE;
 	template.submit.vpf = SMS_VALIDITY_PERIOD_FORMAT_RELATIVE;
 	template.submit.rp = FALSE;
-	template.submit.srr = TRUE;
+	template.submit.srr = ask_status_report;
 	template.submit.mr = 0;
 	template.submit.vp.relative = 0xA7; /* 24 Hours */
 
diff --git a/src/smsutil.h b/src/smsutil.h
index a060c1b..d55b83a 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -479,7 +479,8 @@ GSList *sms_assembly_add_fragment(struct sms_assembly *assembly,
 void sms_assembly_expire(struct sms_assembly *assembly, time_t before);
 
 GSList *sms_text_prepare(const char *utf8, guint16 ref,
-				gboolean use_16bit, int *ref_offset);
+				gboolean use_16bit, int *ref_offset,
+				const gboolean ask_status_report);
 
 gboolean cbs_dcs_decode(guint8 dcs, gboolean *udhi, enum sms_class *cls,
 			enum sms_charset *charset, gboolean *compressed,
diff --git a/unit/test-sms.c b/unit/test-sms.c
index b242913..5a1daa1 100644
--- a/unit/test-sms.c
+++ b/unit/test-sms.c
@@ -685,7 +685,7 @@ static void test_assembly()
 	if (g_test_verbose())
 		g_printf("Text:\n%s\n", utf8);
 
-	l = sms_text_prepare(utf8, ref, TRUE, NULL);
+	l = sms_text_prepare(utf8, ref, TRUE, NULL, FALSE);
 	g_assert(l);
 	g_assert(g_slist_length(l) == 3);
 
@@ -715,7 +715,7 @@ static void test_prepare_7bit()
 	int encoded_tpdu_len;
 	char *encoded_pdu;
 
-	r = sms_text_prepare(test_no_fragmentation_7bit, 0, FALSE, NULL);
+	r = sms_text_prepare(test_no_fragmentation_7bit, 0, FALSE, NULL, FALSE);
 
 	g_assert(r != NULL);
 
@@ -788,7 +788,7 @@ static void test_prepare_concat()
 	if (g_test_verbose())
 		g_print("strlen: %zd\n", strlen(pad1));
 
-	r = sms_text_prepare(pad1, 0, TRUE, NULL);
+	r = sms_text_prepare(pad1, 0, TRUE, NULL, FALSE);
 
 	g_assert(r);
 	g_assert(g_slist_length(r) == 2);
@@ -865,7 +865,7 @@ static void test_limit(gunichar uni, int target_size, gboolean use_16bit)
 
 	utf8[i] = '\0';
 
-	l = sms_text_prepare(utf8, 0, use_16bit, NULL);
+	l = sms_text_prepare(utf8, 0, use_16bit, NULL, FALSE);
 
 	g_assert(l);
 	g_assert(g_slist_length(l) == 255);
@@ -878,7 +878,7 @@ static void test_limit(gunichar uni, int target_size, gboolean use_16bit)
 	memcpy(utf8 + i, utf8_char, stride);
 	utf8[i+stride] = '\0';
 
-	l = sms_text_prepare(utf8, 0, use_16bit, NULL);
+	l = sms_text_prepare(utf8, 0, use_16bit, NULL, FALSE);
 
 	g_assert(l == NULL);
 	g_free(utf8);
-- 
1.6.0.4

[PATCH 07/11] Removed Siemens TC65 specific code.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 drivers/atmodem/sms.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index a1c885b..adaf4f2 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -561,9 +561,6 @@ static void at_cmgl_cpms_cb(gboolean ok, GAtResult *result, gpointer user_data)
 	struct ofono_sms *sms = req->sms;
 	struct sms_data *data = ofono_sms_get_data(sms);
 
-	DBG("PASS AT+CMGL=4!!!");
-	return;
-
 	if (!ok) {
 		ofono_error("Initial CPMS request failed");
 		at_cmgl_done(sms);
-- 
1.6.0.4

[PATCH 08/11] Some style corrections.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 src/sms.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index d079ece..2d9088b 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -542,7 +542,7 @@ static void dispatch_text_message(struct ofono_sms *sms,
 	}
 }
 
-static void dispatch_sms_delivery_report(struct ofono_sms *sms,
+static void dispatch_sms_status_report(struct ofono_sms *sms,
 					const enum sms_st *st,
 					const struct sms_address *raddr,
 					const struct sms_scts *scts,
@@ -604,11 +604,13 @@ static void dispatch_sms_delivery_report(struct ofono_sms *sms,
 	/*Status*/
 	if(*st==0x00){
 		str = sms_address_to_string(raddr);
-		ofono_dbus_dict_append(&dict, "Message was delivered to", DBUS_TYPE_STRING, &str);
+		ofono_dbus_dict_append(&dict, "Message was delivered to",
+							DBUS_TYPE_STRING, &str);
 	}
 	else{
 		str = sms_address_to_string(raddr);
-		ofono_dbus_dict_append(&dict, "Message was not delivered to", DBUS_TYPE_STRING, &str);
+		ofono_dbus_dict_append(&dict, "Message was not delivered to",
+							DBUS_TYPE_STRING, &str);
 	}
 
 	/*dbus-message assembled*/
@@ -731,7 +733,7 @@ static void sms_status_report_dispatch(struct ofono_sms *sms, GSList *sms_list)
 	enum sms_charset uninitialized_var(old_charset);
 
 	s = sms_list->data;
-	dispatch_sms_delivery_report(sms, &s->status_report.st,
+	dispatch_sms_status_report(sms, &s->status_report.st,
 					&s->status_report.raddr,
 					&s->status_report.scts,
 					&s->status_report.dt);
@@ -775,10 +777,6 @@ static void handle_sms_status_report(struct ofono_sms *sms, const struct sms *in
 {
 	GSList *l;
 
-	/*TODO:
-	fragmented SMS delivery report? check handle_deliver()
-	*/
-
 	l = g_slist_append(NULL, (void *)incoming);
 	sms_status_report_dispatch(sms, l);
 	g_slist_free(l);
@@ -801,7 +799,7 @@ void ofono_sms_deliver_notify(struct ofono_sms *sms, unsigned char *pdu,
 {
 	struct sms s;
 	enum sms_class cls;
-	DBG("ofono_sms_deliver_notify");
+
 	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
 		ofono_error("Unable to decode PDU");
 		return;
-- 
1.6.0.4

[PATCH 09/11] Support for concatenated SMS status report. And ofono_history_sms is now updated according to received status reports.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 include/history.h |    2 +
 src/sms.c         |  157 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 src/smsutil.h     |    6 ++
 3 files changed, 163 insertions(+), 2 deletions(-)

diff --git a/include/history.h b/include/history.h
index 300a4fb..17445f0 100644
--- a/include/history.h
+++ b/include/history.h
@@ -33,6 +33,8 @@ enum ofono_history_sms_status {
 	OFONO_HISTORY_SMS_STATUS_PENDING,
 	OFONO_HISTORY_SMS_STATUS_SUBMITTED,
 	OFONO_HISTORY_SMS_STATUS_SUBMIT_FAILED,
+	OFONO_HISTORY_SMS_STATUS_DELIVERED,
+	OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED,
 };
 
 struct ofono_history_context {
diff --git a/src/sms.c b/src/sms.c
index 2d9088b..0a8bf05 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -67,6 +67,17 @@ struct ofono_sms {
 	const struct ofono_sms_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
+	GSList *pending_status_reports;
+};
+
+struct mr_number {
+	guint8 mr;
+	enum sms_status_report_state delivery_status;
+};
+
+struct pending_status_report_mr_numbers {
+	unsigned int ofono_msg_id;
+	GSList *pending_mr_numbers;
 };
 
 struct pending_pdu {
@@ -82,6 +93,7 @@ struct tx_queue_entry {
 	unsigned int msg_id;
 	unsigned int retry;
 	DBusMessage *msg;
+	gboolean status_report;
 };
 
 static void set_sca(struct ofono_sms *sms,
@@ -263,6 +275,47 @@ static DBusMessage *sms_set_property(DBusConnection *conn, DBusMessage *msg,
 	return __ofono_error_invalid_args(msg);
 }
 
+static void add_pending_status_report(struct ofono_sms *sms, int mr,
+							unsigned int msg_id)
+{
+	struct pending_status_report_mr_numbers *status_report_mr_numbers;
+	GSList *l;
+	struct mr_number *new_mr;
+
+	for (l = sms->pending_status_reports; l; l = l->next) {
+		status_report_mr_numbers = l->data;
+
+		if (status_report_mr_numbers->ofono_msg_id == msg_id) {
+			/*Relate new MR-number to existing ofono_msg_id*/
+			new_mr = g_new0(struct mr_number, 1);
+			new_mr->mr = mr;
+			new_mr->delivery_status = SMS_STATUS_REPORT_PENDING;
+			status_report_mr_numbers->pending_mr_numbers =
+				g_slist_append(status_report_mr_numbers-> \
+						pending_mr_numbers, new_mr);
+			return;
+		}
+	}
+
+	/*Create new ofono_msg_id and relate MR-number to it*/
+	status_report_mr_numbers =
+			g_new0(struct pending_status_report_mr_numbers, 1);
+
+	status_report_mr_numbers->ofono_msg_id = msg_id;
+
+	new_mr = g_new0(struct mr_number, 1);
+	new_mr->mr = mr;
+	new_mr->delivery_status = SMS_STATUS_REPORT_PENDING;
+	status_report_mr_numbers->pending_mr_numbers =
+		g_slist_append(status_report_mr_numbers-> \
+					pending_mr_numbers, new_mr);
+
+	sms->pending_status_reports =
+		g_slist_append(sms->pending_status_reports,
+					status_report_mr_numbers);
+
+}
+
 static void tx_finished(const struct ofono_error *error, int mr, void *data)
 {
 	struct ofono_sms *sms = data;
@@ -306,6 +359,9 @@ static void tx_finished(const struct ofono_error *error, int mr, void *data)
 	entry->cur_pdu += 1;
 	entry->retry = 0;
 
+	if(entry->status_report)
+		add_pending_status_report(sms, mr, entry->msg_id);
+
 	if (entry->cur_pdu < entry->num_pdus) {
 		sms->tx_source = g_timeout_add(0, tx_next, sms);
 		return;
@@ -437,6 +493,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 
 	entry->msg = dbus_message_ref(msg);
 	entry->msg_id = sms->next_msg_id++;
+	entry->status_report = ask_status_report;
 
 	g_queue_push_tail(sms->txq, entry);
 
@@ -542,11 +599,102 @@ static void dispatch_text_message(struct ofono_sms *sms,
 	}
 }
 
+static void update_pending_status_report_mr_number(struct ofono_sms *sms,
+		guint8 mr, enum sms_status_report_state status_report_state)
+{
+	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
+	struct pending_status_report_mr_numbers *status_report_mr_numbers;
+	GSList *l;
+	GSList *i;
+	struct mr_number *current_mr;
+	gboolean delivery_successful;
+
+	/*Each ofono_msg_id can relate to 1-n mr_numbers*/
+	for (l = sms->pending_status_reports; l; l = l->next) {
+		delivery_successful = TRUE;
+		status_report_mr_numbers = l->data;
+
+		for(i = status_report_mr_numbers->pending_mr_numbers; i;
+								i = i->next) {
+			current_mr = i->data;
+
+			/*Message with current MR-number was delivered*/
+			if(current_mr->mr == mr &&
+				status_report_state ==
+				SMS_STATUS_REPORT_SENDING_SUCCEEDED) {
+				current_mr->delivery_status =
+					SMS_STATUS_REPORT_SENDING_SUCCEEDED;
+			}
+
+			if(current_mr->delivery_status ==
+				SMS_STATUS_REPORT_PENDING)
+				/*msg_id still relates to pending status report*/
+				delivery_successful = FALSE;
+
+			/*If one part of the fragmented message is undelivered,
+			  whole message is declared undelivered*/
+			if(current_mr->mr == mr &&
+				status_report_state ==
+					SMS_STATUS_REPORT_SENDING_FAILED) {
+
+				DBG("SMS-STATUS-REPORT: message %d not "\
+				"delivered", status_report_mr_numbers-> \
+								ofono_msg_id);
+
+				__ofono_history_sms_send_status(modem,
+				     status_report_mr_numbers->ofono_msg_id,
+				     time(NULL),
+				     OFONO_HISTORY_SMS_STATUS_DELIVER_FAILED);
+
+				/*Free msg_id and relating MR-numbers*/
+				g_slist_foreach(status_report_mr_numbers-> \
+					pending_mr_numbers, (GFunc)g_free,
+					NULL);
+				g_slist_free(status_report_mr_numbers-> \
+					pending_mr_numbers);
+				sms->pending_status_reports =
+					g_slist_remove(sms-> \
+						pending_status_reports,
+						status_report_mr_numbers);
+				g_free(status_report_mr_numbers);
+
+				/*Received MR is handled*/
+				return;
+			}
+		}
+
+		/*all parts of the message succesfully delivered*/
+		if(delivery_successful) {
+			DBG("SMS-STATUS-REPORT: message %d delivered",
+				status_report_mr_numbers->ofono_msg_id);
+
+			__ofono_history_sms_send_status(modem,
+					status_report_mr_numbers->ofono_msg_id,
+					time(NULL),
+					OFONO_HISTORY_SMS_STATUS_DELIVERED);
+
+			/*Free msg_id and relating MR-numbers*/
+			g_slist_foreach(status_report_mr_numbers-> \
+				pending_mr_numbers, (GFunc)g_free, NULL);
+			g_slist_free(status_report_mr_numbers-> \
+				pending_mr_numbers);
+			sms->pending_status_reports =
+				g_slist_remove(sms->pending_status_reports,
+						status_report_mr_numbers);
+			g_free(status_report_mr_numbers);
+
+			/*Received MR is handled*/
+			return;
+		}
+	}
+}
+
 static void dispatch_sms_status_report(struct ofono_sms *sms,
 					const enum sms_st *st,
 					const struct sms_address *raddr,
 					const struct sms_scts *scts,
-					const struct sms_scts *dt)
+					const struct sms_scts *dt,
+					const guint8 mr)
 {
 	DBusConnection *conn = ofono_dbus_get_connection();
 	const char *path = __ofono_atom_get_path(sms->atom);
@@ -606,11 +754,15 @@ static void dispatch_sms_status_report(struct ofono_sms *sms,
 		str = sms_address_to_string(raddr);
 		ofono_dbus_dict_append(&dict, "Message was delivered to",
 							DBUS_TYPE_STRING, &str);
+		update_pending_status_report_mr_number(sms, mr,
+					SMS_STATUS_REPORT_SENDING_SUCCEEDED);
 	}
 	else{
 		str = sms_address_to_string(raddr);
 		ofono_dbus_dict_append(&dict, "Message was not delivered to",
 							DBUS_TYPE_STRING, &str);
+		update_pending_status_report_mr_number(sms, mr,
+					SMS_STATUS_REPORT_SENDING_FAILED);
 	}
 
 	/*dbus-message assembled*/
@@ -736,7 +888,8 @@ static void sms_status_report_dispatch(struct ofono_sms *sms, GSList *sms_list)
 	dispatch_sms_status_report(sms, &s->status_report.st,
 					&s->status_report.raddr,
 					&s->status_report.scts,
-					&s->status_report.dt);
+					&s->status_report.dt,
+					s->status_report.mr);
 
 }
 
diff --git a/src/smsutil.h b/src/smsutil.h
index d55b83a..c5dc819 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -178,6 +178,12 @@ enum sms_pid_type {
 	SMS_PID_TYPE_USIM_DOWNLOAD = 0x7f,
 };
 
+enum sms_status_report_state {
+	SMS_STATUS_REPORT_PENDING = 0,
+	SMS_STATUS_REPORT_SENDING_FAILED =1,
+	SMS_STATUS_REPORT_SENDING_SUCCEEDED = 2,
+};
+
 enum cbs_language {
 	CBS_LANGUAGE_GERMAN = 0x0,
 	CBS_LANGUAGE_ENGLISH = 0x1,
-- 
1.6.0.4

[PATCH 10/11] Some naming changes to enum sms_status_report_result.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 src/sms.c     |   21 +++++++++++----------
 src/smsutil.h |    8 ++++----
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index 0a8bf05..de2d367 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -72,7 +72,7 @@ struct ofono_sms {
 
 struct mr_number {
 	guint8 mr;
-	enum sms_status_report_state delivery_status;
+	enum sms_status_report_result delivery_status;
 };
 
 struct pending_status_report_mr_numbers {
@@ -289,7 +289,8 @@ static void add_pending_status_report(struct ofono_sms *sms, int mr,
 			/*Relate new MR-number to existing ofono_msg_id*/
 			new_mr = g_new0(struct mr_number, 1);
 			new_mr->mr = mr;
-			new_mr->delivery_status = SMS_STATUS_REPORT_PENDING;
+			new_mr->delivery_status =
+				SMS_STATUS_REPORT_STATUS_PENDING;
 			status_report_mr_numbers->pending_mr_numbers =
 				g_slist_append(status_report_mr_numbers-> \
 						pending_mr_numbers, new_mr);
@@ -305,7 +306,7 @@ static void add_pending_status_report(struct ofono_sms *sms, int mr,
 
 	new_mr = g_new0(struct mr_number, 1);
 	new_mr->mr = mr;
-	new_mr->delivery_status = SMS_STATUS_REPORT_PENDING;
+	new_mr->delivery_status = SMS_STATUS_REPORT_STATUS_PENDING;
 	status_report_mr_numbers->pending_mr_numbers =
 		g_slist_append(status_report_mr_numbers-> \
 					pending_mr_numbers, new_mr);
@@ -600,7 +601,7 @@ static void dispatch_text_message(struct ofono_sms *sms,
 }
 
 static void update_pending_status_report_mr_number(struct ofono_sms *sms,
-		guint8 mr, enum sms_status_report_state status_report_state)
+		guint8 mr, enum sms_status_report_result status_report_state)
 {
 	struct ofono_modem *modem = __ofono_atom_get_modem(sms->atom);
 	struct pending_status_report_mr_numbers *status_report_mr_numbers;
@@ -621,13 +622,13 @@ static void update_pending_status_report_mr_number(struct ofono_sms *sms,
 			/*Message with current MR-number was delivered*/
 			if(current_mr->mr == mr &&
 				status_report_state ==
-				SMS_STATUS_REPORT_SENDING_SUCCEEDED) {
+				SMS_STATUS_REPORT_MESSAGE_DELIVERED) {
 				current_mr->delivery_status =
-					SMS_STATUS_REPORT_SENDING_SUCCEEDED;
+					SMS_STATUS_REPORT_MESSAGE_DELIVERED;
 			}
 
 			if(current_mr->delivery_status ==
-				SMS_STATUS_REPORT_PENDING)
+				SMS_STATUS_REPORT_STATUS_PENDING)
 				/*msg_id still relates to pending status report*/
 				delivery_successful = FALSE;
 
@@ -635,7 +636,7 @@ static void update_pending_status_report_mr_number(struct ofono_sms *sms,
 			  whole message is declared undelivered*/
 			if(current_mr->mr == mr &&
 				status_report_state ==
-					SMS_STATUS_REPORT_SENDING_FAILED) {
+					SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED) {
 
 				DBG("SMS-STATUS-REPORT: message %d not "\
 				"delivered", status_report_mr_numbers-> \
@@ -755,14 +756,14 @@ static void dispatch_sms_status_report(struct ofono_sms *sms,
 		ofono_dbus_dict_append(&dict, "Message was delivered to",
 							DBUS_TYPE_STRING, &str);
 		update_pending_status_report_mr_number(sms, mr,
-					SMS_STATUS_REPORT_SENDING_SUCCEEDED);
+					SMS_STATUS_REPORT_MESSAGE_DELIVERED);
 	}
 	else{
 		str = sms_address_to_string(raddr);
 		ofono_dbus_dict_append(&dict, "Message was not delivered to",
 							DBUS_TYPE_STRING, &str);
 		update_pending_status_report_mr_number(sms, mr,
-					SMS_STATUS_REPORT_SENDING_FAILED);
+					SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED);
 	}
 
 	/*dbus-message assembled*/
diff --git a/src/smsutil.h b/src/smsutil.h
index c5dc819..40ceead 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -178,10 +178,10 @@ enum sms_pid_type {
 	SMS_PID_TYPE_USIM_DOWNLOAD = 0x7f,
 };
 
-enum sms_status_report_state {
-	SMS_STATUS_REPORT_PENDING = 0,
-	SMS_STATUS_REPORT_SENDING_FAILED =1,
-	SMS_STATUS_REPORT_SENDING_SUCCEEDED = 2,
+enum sms_status_report_result {
+	SMS_STATUS_REPORT_STATUS_PENDING = 0,
+	SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED = 1,
+	SMS_STATUS_REPORT_MESSAGE_DELIVERED = 2,
 };
 
 enum cbs_language {
-- 
1.6.0.4

[PATCH 11/11] Style corrections.

[pasi.miettinen]

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

From: Pasi Miettinen <pasi.miettinen@ixonos.com>

---
 src/sms.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/src/sms.c b/src/sms.c
index de2d367..6bca8cd 100644
--- a/src/sms.c
+++ b/src/sms.c
@@ -472,7 +472,8 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 	if (valid_phone_number_format(to) == FALSE)
 		return __ofono_error_invalid_format(msg);
 
-	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset, ask_status_report);
+	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset,
+						ask_status_report);
 
 	if (!msg_list)
 		return __ofono_error_invalid_format(msg);
@@ -509,12 +510,12 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
 }
 
 static GDBusMethodTable sms_manager_methods[] = {
-	{ "GetProperties",	"",	"a{sv}",	sms_get_properties,
-							G_DBUS_METHOD_FLAG_ASYNC },
-	{ "SetProperty",	"sv",	"",		sms_set_property,
-							G_DBUS_METHOD_FLAG_ASYNC },
-	{ "SendMessage",	"ssb",	"",		sms_send_message,
-							G_DBUS_METHOD_FLAG_ASYNC },
+	{ "GetProperties",	"",	"a{sv}", sms_get_properties,
+						 G_DBUS_METHOD_FLAG_ASYNC },
+	{ "SetProperty",	"sv",	"",	 sms_set_property,
+						 G_DBUS_METHOD_FLAG_ASYNC },
+	{ "SendMessage",	"ssb",	"",	 sms_send_message,
+						 G_DBUS_METHOD_FLAG_ASYNC },
 	{ }
 };
 
@@ -636,7 +637,7 @@ static void update_pending_status_report_mr_number(struct ofono_sms *sms,
 			  whole message is declared undelivered*/
 			if(current_mr->mr == mr &&
 				status_report_state ==
-					SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED) {
+				SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED) {
 
 				DBG("SMS-STATUS-REPORT: message %d not "\
 				"delivered", status_report_mr_numbers-> \
@@ -748,7 +749,8 @@ static void dispatch_sms_status_report(struct ofono_sms *sms,
 
 	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
 	buf[127] = '\0';
-	ofono_dbus_dict_append(&dict, "LocalDeliverTime", DBUS_TYPE_STRING, &str);
+	ofono_dbus_dict_append(&dict, "LocalDeliverTime",
+					DBUS_TYPE_STRING, &str);
 
 	/*Status*/
 	if(*st==0x00){
@@ -763,7 +765,7 @@ static void dispatch_sms_status_report(struct ofono_sms *sms,
 		ofono_dbus_dict_append(&dict, "Message was not delivered to",
 							DBUS_TYPE_STRING, &str);
 		update_pending_status_report_mr_number(sms, mr,
-					SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED);
+				SMS_STATUS_REPORT_MESSAGE_NOT_DELIVERED);
 	}
 
 	/*dbus-message assembled*/
@@ -927,7 +929,8 @@ static void handle_deliver(struct ofono_sms *sms, const struct sms *incoming)
 	g_slist_free(l);
 }
 
-static void handle_sms_status_report(struct ofono_sms *sms, const struct sms *incoming)
+static void handle_sms_status_report(struct ofono_sms *sms,
+					const struct sms *incoming)
 {
 	GSList *l;
 
-- 
1.6.0.4

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Aki Niemi]

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

On Thu, 2010-05-27 at 12:54 +0200, ext pasi.miettinen(a)ixonos.com wrote:
> diff --git a/src/sms.c b/src/sms.c
> index b4a5364..d079ece 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -400,6 +400,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  	struct ofono_sms *sms = data;
>  	const char *to;
>  	const char *text;
> +	gboolean ask_status_report;
>  	GSList *msg_list;
>  	int ref_offset;
>  	struct tx_queue_entry *entry;
> @@ -407,13 +408,14 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
>  
>  	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
>  					DBUS_TYPE_STRING, &text,
> +					DBUS_TYPE_BOOLEAN, &ask_status_report,
>  					DBUS_TYPE_INVALID))
>  		return __ofono_error_invalid_args(msg);
>  
>  	if (valid_phone_number_format(to) == FALSE)
>  		return __ofono_error_invalid_format(msg);
>  
> -	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset);
> +	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset, ask_status_report);
>  
>  	if (!msg_list)
>  		return __ofono_error_invalid_format(msg);
> @@ -453,7 +455,7 @@ static GDBusMethodTable sms_manager_methods[] = {
>  							G_DBUS_METHOD_FLAG_ASYNC },
>  	{ "SetProperty",	"sv",	"",		sms_set_property,
>  							G_DBUS_METHOD_FLAG_ASYNC },
> -	{ "SendMessage",	"ss",	"",		sms_send_message,
> +	{ "SendMessage",	"ssb",	"",		sms_send_message,
>  							G_DBUS_METHOD_FLAG_ASYNC },
>  	{ }
>  };

I don't like this being an argument to SendMessage(). I think it needs
to be exposed, but as a property instead. Is there a use case for
setting this per message? I think majority of current phones either
provide a global setting for this, or set it on by default.

Cheers,
Aki

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Marcel Holtmann]

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

Hi Aki,

> > diff --git a/src/sms.c b/src/sms.c
> > index b4a5364..d079ece 100644
> > --- a/src/sms.c
> > +++ b/src/sms.c
> > @@ -400,6 +400,7 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
> >  	struct ofono_sms *sms = data;
> >  	const char *to;
> >  	const char *text;
> > +	gboolean ask_status_report;
> >  	GSList *msg_list;
> >  	int ref_offset;
> >  	struct tx_queue_entry *entry;
> > @@ -407,13 +408,14 @@ static DBusMessage *sms_send_message(DBusConnection *conn, DBusMessage *msg,
> >  
> >  	if (!dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &to,
> >  					DBUS_TYPE_STRING, &text,
> > +					DBUS_TYPE_BOOLEAN, &ask_status_report,
> >  					DBUS_TYPE_INVALID))
> >  		return __ofono_error_invalid_args(msg);
> >  
> >  	if (valid_phone_number_format(to) == FALSE)
> >  		return __ofono_error_invalid_format(msg);
> >  
> > -	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset);
> > +	msg_list = sms_text_prepare(text, 0, TRUE, &ref_offset, ask_status_report);
> >  
> >  	if (!msg_list)
> >  		return __ofono_error_invalid_format(msg);
> > @@ -453,7 +455,7 @@ static GDBusMethodTable sms_manager_methods[] = {
> >  							G_DBUS_METHOD_FLAG_ASYNC },
> >  	{ "SetProperty",	"sv",	"",		sms_set_property,
> >  							G_DBUS_METHOD_FLAG_ASYNC },
> > -	{ "SendMessage",	"ss",	"",		sms_send_message,
> > +	{ "SendMessage",	"ssb",	"",		sms_send_message,
> >  							G_DBUS_METHOD_FLAG_ASYNC },
> >  	{ }
> >  };
> 
> I don't like this being an argument to SendMessage(). I think it needs
> to be exposed, but as a property instead. Is there a use case for
> setting this per message? I think majority of current phones either
> provide a global setting for this, or set it on by default.

our idea is actually that every new SMS has its own object path for its
lifetime. So we can have then properties easily on them.

Regards

Marcel

Re: [PATCH 04/11] Changes for SMS statur report.

[Aki Niemi]

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

On Thu, 2010-05-27 at 12:54 +0200, ext pasi.miettinen(a)ixonos.com wrote:
> diff --git a/include/sms.h b/include/sms.h
> index daaec37..c007675 100644
> --- a/include/sms.h
> +++ b/include/sms.h
> @@ -54,7 +54,7 @@ struct ofono_sms_driver {
>  
>  void ofono_sms_deliver_notify(struct ofono_sms *sms, unsigned char *pdu,
>  				int len, int tpdu_len);
> -void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
> +void ofono_sms_status_report_notify(struct ofono_sms *sms, unsigned char *pdu,
>  				int len, int tpdu_len);

Actually, I think we should have a single ofono_sms_pdu_notify(), and
let the core handle demuxing the different message types after parsing
the PDU.

ISI modems don't have different messages for deliver and status report;
the modem just sends a TPDU.

Cheers,
Aki

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Denis Kenzior]

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

Hi Aki,

> > -	{ "SendMessage",	"ss",	"",		sms_send_message,
> > +	{ "SendMessage",	"ssb",	"",		sms_send_message,
> >  							G_DBUS_METHOD_FLAG_ASYNC },
> >  	{ }
> >  };
> 
> I don't like this being an argument to SendMessage(). I think it needs
> to be exposed, but as a property instead. Is there a use case for
> setting this per message? I think majority of current phones either
> provide a global setting for this, or set it on by default.
> 

I agree, we should expose this as the 'UseDeliveryReports' property on 
SmsManager or alternatively use a global oFono setting read at startup.

Regards,
-Denis

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Aki Niemi]

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

On Thu, 2010-05-27 at 13:39 +0200, ext Marcel Holtmann wrote: 
> > I don't like this being an argument to SendMessage(). I think it needs
> > to be exposed, but as a property instead. Is there a use case for
> > setting this per message? I think majority of current phones either
> > provide a global setting for this, or set it on by default.
> 
> our idea is actually that every new SMS has its own object path for its
> lifetime. So we can have then properties easily on them.

Sure, but there should still be a property in SmsManager to control
whether srr is to be set on outgoing messages.

Another property in the actual SmsMessage (residing on its own object
path) could then indicate whether srr *was* set for that particular
message when it was sent.

Cheers,
Aki

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Marcel Holtmann]

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

Hi Denis,

> > > -	{ "SendMessage",	"ss",	"",		sms_send_message,
> > > +	{ "SendMessage",	"ssb",	"",		sms_send_message,
> > >  							G_DBUS_METHOD_FLAG_ASYNC },
> > >  	{ }
> > >  };
> > 
> > I don't like this being an argument to SendMessage(). I think it needs
> > to be exposed, but as a property instead. Is there a use case for
> > setting this per message? I think majority of current phones either
> > provide a global setting for this, or set it on by default.
> > 
> 
> I agree, we should expose this as the 'UseDeliveryReports' property on 
> SmsManager or alternatively use a global oFono setting read at startup.

I think just a global setting in /etc/ofono/main.conf would be better. I
don't really see the point in making this an option for the user. My
personal opinion here is to always request delivery reports by default
and handle them internally to acknowledge SMS messages.

Regards

Marcel

VS: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Miettinen Pasi]

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

>Hi Denis,
>
> > > > - { "SendMessage",        "ss",   "",             sms_send_message,
> > > > + { "SendMessage",        "ssb",  "",             sms_send_message,
> > > >                                                   G_DBUS_METHOD_FLAG_ASYNC },
> > > >  { }
> > > >  };
> > > 
> > > I don't like this being an argument to SendMessage(). I think it needs
> > > to be exposed, but as a property instead. Is there a use case for
> > > setting this per message? I think majority of current phones either
> > > provide a global setting for this, or set it on by default.
> > >
> >
> > I agree, we should expose this as the 'UseDeliveryReports' property on
> > SmsManager or alternatively use a global oFono setting read at startup.
>
> I think just a global setting in /etc/ofono/main.conf would be better. I
> don't really see the point in making this an option for the user. My
> personal opinion here is to always request delivery reports by default
> and handle them internally to acknowledge SMS messages.
>
> Regards
>
> Marcel

Thank you for your opinions. We already hoped that you would comment
this part because we also had conversation here about how should this
matter be solved. Current solution is mostly done this way because it
seemed easiest way to get things working.

Have you thought this from the operator point of view. I mean, if you
always request  delivery report, it is going to increase the load of the
operators net and that is something that operator is not going to like.

But when you do the final decision about this matter, please let me know
and I can then implement it.

-Pasi


_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono

VS: [PATCH 04/11] Changes for SMS statur report.

[Miettinen Pasi]

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

> On Thu, 2010-05-27 at 12:54 +0200, ext pasi.miettinen(a)ixonos.com wrote:
> > diff --git a/include/sms.h b/include/sms.h
> > index daaec37..c007675 100644
> > --- a/include/sms.h
> > +++ b/include/sms.h
> > @@ -54,7 +54,7 @@ struct ofono_sms_driver {
> >
> > void ofono_sms_deliver_notify(struct ofono_sms *sms, unsigned char *pdu,
> >                              int len, int tpdu_len);
> > -void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
> > +void ofono_sms_status_report_notify(struct ofono_sms *sms, unsigned char *pdu,
> >                              int len, int tpdu_len);
>
> Actually, I think we should have a single ofono_sms_pdu_notify(), and
> let the core handle demuxing the different message types after parsing
> the PDU.
>
> ISI modems don't have different messages for deliver and status report;
> the modem just sends a TPDU.
>
> Cheers,
> Aki

I made one possible solution for this and planning to test how it works after
the weekend. Hopefully I am able to send a fixing patch early next week.

Br,
Pasi

_______________________________________________
ofono mailing list
ofono(a)ofono.org
http://lists.ofono.org/listinfo/ofono

Re: [PATCH 01/11] In drivers/atmodem/sms.c:at_cmgl_cpms_cb() there is a temporary fix for Siemens TC65. If AT+CMGL=4 is sent to TC65, the AT command queue jams.

[Denis Kenzior]

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

Hi Pasi,

> From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> 
> ---
>  drivers/atmodem/sms.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 

Please keep the header to less than 50 characters.  Include the meat of the 
description in the commit description itself or the comment in the code.

> diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
> index 8049df6..27de77b 100644
> --- a/drivers/atmodem/sms.c
> +++ b/drivers/atmodem/sms.c
> @@ -544,6 +544,10 @@ static void at_cmgl_cb(gboolean ok, GAtResult *result,
>  gpointer user_data)
> 
>  static void at_cmgl_cpms_cb(gboolean ok, GAtResult *result, gpointer
>  user_data) {
> +
> +	DBG("PASS AT+CMGL=4!!!");
> +	return;
> +

Please use the vendor quirks facility.  See drivers/atmodem/vendor.h and the 
various uses of it in plugins/ and drivers/atmodem.  You might have to create 
a proper modem driver for the TC65.

Regards,
-Denis

Re: [PATCH 02/11] Made needed changes to at_cds_notify() for status report and corrected at_cmgl_cpms_cb() to meet the ISO standard.

[Denis Kenzior]

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

Hi Pasi,

> From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> 
> ---
>  drivers/atmodem/sms.c |   26 +++++++++++++++++++-------
>  1 files changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
> index 27de77b..a1c885b 100644
> --- a/drivers/atmodem/sms.c
> +++ b/drivers/atmodem/sms.c
> @@ -283,16 +283,28 @@ static void at_cds_notify(GAtResult *result, gpointer
>  user_data) {
>  	struct ofono_sms *sms = user_data;
>  	struct sms_data *data = ofono_sms_get_data(sms);
> -	int pdulen;
> -	const char *pdu;
> +	long pdu_len;
> +	int tpdu_len;
> +	const char *hexpdu;
> +	unsigned char pdu[176];
>  	char buf[256];
> 
> -	if (!at_parse_pdu_common(result, "+CDS:", &pdu, &pdulen)) {
> +	if (!at_parse_pdu_common(result, "+CDS:", &hexpdu, &tpdu_len)) {
>  		ofono_error("Unable to parse CDS notification");
>  		return;
>  	}
> 
> -	DBG("Got new Status-Report PDU via CDS: %s, %d", pdu, pdulen);
> +	/*Is this necessary?*/

This is necessary to keep from overflowing the buffer.  Better safe than sorry, 
so I'm fine leaving this check in.  Remove the comment though.

> +	if (strlen(hexpdu) > sizeof(pdu) * 2) {
> +		ofono_error("Bad PDU length in CMT notification");
> +		return;
> +	}
> +
> +	DBG("Got new Status-Report PDU via CDS: %s, %d", hexpdu, tpdu_len);
> +
> +	/*Decode pdu and notify about new SMS status report*/
> +	decode_hex_own_buf(hexpdu, -1, &pdu_len, 0, pdu);
> +	ofono_sms_status_report_notify(sms, pdu, pdu_len, tpdu_len);
> 
>  	/* We must acknowledge the PDU using CNMA */
>  	if (data->cnma_ack_pdu)
> @@ -545,13 +557,13 @@ static void at_cmgl_cb(gboolean ok, GAtResult
>  *result, gpointer user_data) static void at_cmgl_cpms_cb(gboolean ok,
>  GAtResult *result, gpointer user_data) {
> 
> -	DBG("PASS AT+CMGL=4!!!");
> -	return;
> -
>  	struct cpms_request *req = user_data;
>  	struct ofono_sms *sms = req->sms;
>  	struct sms_data *data = ofono_sms_get_data(sms);
> 
> +	DBG("PASS AT+CMGL=4!!!");
> +	return;
> +

This belongs in a separate patch, please break up the patches appropriately.

Regards,
-Denis

Re: [PATCH 03/11] Enable status report.

[Denis Kenzior]

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

Hi Pasi,

> From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> 
> ---
>  src/smsutil.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/src/smsutil.c b/src/smsutil.c
> index e634764..2679cc2 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -2596,7 +2596,7 @@ GSList *sms_text_prepare(const char *utf8, guint16
>  ref, template.submit.rd = FALSE;
>  	template.submit.vpf = SMS_VALIDITY_PERIOD_FORMAT_RELATIVE;
>  	template.submit.rp = FALSE;
> -	template.submit.srr = FALSE;
> +	template.submit.srr = TRUE;
>  	template.submit.mr = 0;
>  	template.submit.vp.relative = 0xA7; /* 24 Hours */
> 

I suggest that this be made a parameter passed to sms_text_prepare.

Regards,
-Denis

Re: [PATCH 04/11] Changes for SMS statur report.

[Denis Kenzior]

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

Hi Pasi,

> From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> 
> ---
>  include/sms.h |    2 +-
>  src/sms.c     |  124
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 2 files 
changed,
>  119 insertions(+), 7 deletions(-)
> 
> diff --git a/include/sms.h b/include/sms.h
> index daaec37..c007675 100644
> --- a/include/sms.h
> +++ b/include/sms.h
> @@ -54,7 +54,7 @@ struct ofono_sms_driver {
> 
>  void ofono_sms_deliver_notify(struct ofono_sms *sms, unsigned char *pdu,
>  				int len, int tpdu_len);
> -void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
> +void ofono_sms_status_report_notify(struct ofono_sms *sms, unsigned char
>  *pdu, int len, int tpdu_len);

Please leave the rename out for now.

> 
>  int ofono_sms_driver_register(const struct ofono_sms_driver *d);
> diff --git a/src/sms.c b/src/sms.c
> index 855bef8..63e0190 100644
> --- a/src/sms.c
> +++ b/src/sms.c
> @@ -459,9 +459,10 @@ static GDBusMethodTable sms_manager_methods[] = {
>  };
> 
>  static GDBusSignalTable sms_manager_signals[] = {
> -	{ "PropertyChanged",	"sv"		},
> -	{ "IncomingMessage",	"sa{sv}"	},
> -	{ "ImmediateMessage",	"sa{sv}"	},
> +	{ "PropertyChanged",		"sv"		},
> +	{ "IncomingMessage",		"sa{sv}"	},
> +	{ "ImmediateMessage",		"sa{sv}"	},
> +	{ "IncomingStatusReport",	"{sv}"		},

Your patch does too much here.  If you want to reformat this table, send a 
separate patch please.  The current patch should only add the new entry.

>  	{ }
>  };
> 
> @@ -471,6 +472,7 @@ static void dispatch_app_datagram(struct ofono_sms
>  *sms, int dst, int src, DBG("Got app datagram for dst port: %d, src port:
>  %d",
>  			dst, src);
>  	DBG("Contents-Len: %ld", len);
> +	//DBG("buf: %s", buf);

Submissions should never contain commented-out code, you either need it or you 
don't.

>  }
> 
>  static void dispatch_text_message(struct ofono_sms *sms,
> @@ -539,6 +541,74 @@ static void dispatch_text_message(struct ofono_sms
>  *sms, }
>  }
> 
> +static void dispatch_sms_delivery_report(struct ofono_sms *sms,
> +					const enum sms_st *st,
> +					const struct sms_address *raddr,
> +					const struct sms_scts *scts)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(sms->atom);
> +	DBusMessage *signal;
> +	DBusMessageIter iter;
> +	DBusMessageIter dict;
> +	char buf[128];
> +	const char *signal_name;
> +	time_t ts;
> +	struct tm remote;
> +	struct tm local;
> +	const char *str = buf;
> +
> +	if (!st){
> +		DBG("status unavailable");
> +		return;
> +	}
> +
> +	signal_name = "IncomingStatusReport";
> +
> +	signal = dbus_message_new_signal(path, OFONO_SMS_MANAGER_INTERFACE,
> +						signal_name);

This variable and assignment is not needed, simply use the string in the 
dbus_message_new_signal call.

> +
> +	if (!signal)
> +		return;
> +
> +

Two consecutive empty lines is a no-no ;)

> +	/*Start assembling dbus-message*/
> +	dbus_message_iter_init_append(signal, &iter);
> +
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +						&dict);
> +

Your signature here does not match the IncomingStatusReport signature.

> +	/*This is the time when sender sent the message,
> +	  should be delivery time?*/
> +	ts = sms_scts_to_time(scts, &remote);
> +	localtime_r(&ts, &local);
> +
> +	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &local);
> +	buf[127] = '\0';
> +	ofono_dbus_dict_append(&dict, "LocalSentTime", DBUS_TYPE_STRING, &str);
> +
> +	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", &remote);
> +	buf[127] = '\0';
> +	ofono_dbus_dict_append(&dict, "SentTime", DBUS_TYPE_STRING, &str);

SentTime and LocalSentTime?

> +
> +	/*Status*/
> +	if(*st==0x00){
> +		str = sms_address_to_string(raddr);
> +		ofono_dbus_dict_append(&dict, "Message was delivered to",
>  DBUS_TYPE_STRING, &str); +	}
> +	else{
> +		str = sms_address_to_string(raddr);
> +		ofono_dbus_dict_append(&dict, "Message was not delivered to",
>  DBUS_TYPE_STRING, &str); +	}

I suggest using simple string types, like 'delivered', 'undeliverable', etc.  
Also, the status report enum has nice definitions on whether this is a final / 
non-final result, etc.  These should be handled appropriately.

> +
> +	/*dbus-message assembled*/
> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	g_dbus_send_message(conn, signal);
> +
> +}
> +
>  static void sms_dispatch(struct ofono_sms *sms, GSList *sms_list)
>  {
>  	GSList *l;
> @@ -646,6 +716,18 @@ static void sms_dispatch(struct ofono_sms *sms, GSList
>  *sms_list) }
>  }
> 
> +static void sms_status_report_dispatch(struct ofono_sms *sms, GSList
>  *sms_list) +{
> +	const struct sms *s;
> +	enum sms_charset uninitialized_var(old_charset);

What is the purpose of this variable?

> +
> +	s = sms_list->data;
> +	dispatch_sms_delivery_report(sms, &s->status_report.st,
> +					&s->status_report.raddr,
> +					&s->status_report.scts);
> +
> +}
> +

Not quite sure why all of this is in a separate function...

>  static void handle_deliver(struct ofono_sms *sms, const struct sms
>  *incoming) {
>  	GSList *l;
> @@ -679,6 +761,19 @@ static void handle_deliver(struct ofono_sms *sms,
>  const struct sms *incoming) g_slist_free(l);
>  }
> 
> +static void handle_sms_status_report(struct ofono_sms *sms, const struct
>  sms *incoming) +{
> +	GSList *l;
> +
> +	/*TODO:
> +	fragmented SMS delivery report? check handle_deliver()
> +	*/

I suggest that we figure this part out first actually :)

> +
> +	l = g_slist_append(NULL, (void *)incoming);

Casting is not required.

> +	sms_status_report_dispatch(sms, l);
> +	g_slist_free(l);
> +}
> +
>  static inline gboolean handle_mwi(struct ofono_sms *sms, struct sms *s)
>  {
>  	gboolean discard;
> @@ -696,7 +791,7 @@ void ofono_sms_deliver_notify(struct ofono_sms *sms,
>  unsigned char *pdu, {
>  	struct sms s;
>  	enum sms_class cls;
> -
> +	DBG("ofono_sms_deliver_notify");

You should use a newline to separate the variable definitions and code.  If 
statement blocks should generally be preceded and followed by an empty line.

>  	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
>  		ofono_error("Unable to decode PDU");
>  		return;
> @@ -807,10 +902,27 @@ out:
>  	handle_deliver(sms, &s);
>  }
> 
> -void ofono_sms_status_notify(struct ofono_sms *sms, unsigned char *pdu,
> +void ofono_sms_status_report_notify(struct ofono_sms *sms, unsigned char
>  *pdu, int len, int tpdu_len)
>  {
> -	ofono_error("SMS Status-Report not yet handled");
> +	struct sms s;
> +	enum sms_class cls;
> +
> +	if (!sms_decode(pdu, len, FALSE, tpdu_len, &s)) {
> +		ofono_error("Unable to decode PDU");
> +		return;
> +	}
> +
> +	if (s.type != SMS_TYPE_STATUS_REPORT) {
> +		ofono_error("Expecting a STATUS REPORT pdu");
> +	}
> +
> +	if (!sms_dcs_decode(s.deliver.dcs, &cls, NULL, NULL, NULL)) {
> +		ofono_error("Unknown / Reserved DCS.  Ignoring");
> +		return;
> +	}
> +
> +	handle_sms_status_report(sms, &s);
>  }
> 
>  int ofono_sms_driver_register(const struct ofono_sms_driver *d)
> 

Regards,
-Denis

Re: [PATCH 07/11] Removed Siemens TC65 specific code.

[Denis Kenzior]

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

Hi Pasi,

> From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> 
> ---
>  drivers/atmodem/sms.c |    3 ---
>  1 files changed, 0 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
> index a1c885b..adaf4f2 100644
> --- a/drivers/atmodem/sms.c
> +++ b/drivers/atmodem/sms.c
> @@ -561,9 +561,6 @@ static void at_cmgl_cpms_cb(gboolean ok, GAtResult
>  *result, gpointer user_data) struct ofono_sms *sms = req->sms;
>  	struct sms_data *data = ofono_sms_get_data(sms);
> 
> -	DBG("PASS AT+CMGL=4!!!");
> -	return;
> -
>  	if (!ok) {
>  		ofono_error("Initial CPMS request failed");
>  		at_cmgl_done(sms);
> 

In the future please don't send patches where one patch adds a piece of code 
and a later patch removes it.  It is very confusing :)

git rebase --interactive is very helpful tool for restructuring / re-ordering 
your patches for submission.

Regards,
-Denis

Re: [PATCH 11/11] Style corrections.

[Denis Kenzior]

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

Hi Pasi,

> From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> 
> ---
>  src/sms.c |   25 ++++++++++++++-----------
>  1 files changed, 14 insertions(+), 11 deletions(-)
> 

Style correction patches for code introduced in the same series should simply 
be squished into the previous changes.  This makes it much easier for us to 
review.  e.g. this patch should not exist and any corrections made to the 
patches that originally introduced the code.

Regards,
-Denis

Re: [PATCH 09/11] Support for concatenated SMS status report. And ofono_history_sms is now updated according to received status reports.

[Denis Kenzior]

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

Hi Pasi,

> From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> 
> ---
>  include/history.h |    2 +
>  src/sms.c         |  157
>  ++++++++++++++++++++++++++++++++++++++++++++++++++++- src/smsutil.h     
| 
>    6 ++
>  3 files changed, 163 insertions(+), 2 deletions(-)
> 

Some general comments:

I'd like to see a status_report assembly similar to how the sms_assembly is 
done.  This should exist in smsutil.c/.h so that we can easily unit test this 
functionality (which I would also like to see.)

Another desirable feature for the status report assembly is to save off the 
status report fragments on disk, so that status reports (for multi-fragment 
SMSes) received across oFono / system reboots and crashes are handled 
appropriately.

Regards,
-Denis

Re: VS: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Denis Kenzior]

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

Hi Pasi,

> >Hi Denis,
> >
> > > > > - { "SendMessage",        "ss",   "",             sms_send_message,
> > > > > + { "SendMessage",        "ssb",  "",             sms_send_message,
> > > > >                                                  
> > > > > G_DBUS_METHOD_FLAG_ASYNC }, { }
> > > > >  };
> > > >
> > > > I don't like this being an argument to SendMessage(). I think it
> > > > needs to be exposed, but as a property instead. Is there a use case
> > > > for setting this per message? I think majority of current phones
> > > > either provide a global setting for this, or set it on by default.
> > >
> > > I agree, we should expose this as the 'UseDeliveryReports' property on
> > > SmsManager or alternatively use a global oFono setting read at startup.
> >
> > I think just a global setting in /etc/ofono/main.conf would be better. I
> > don't really see the point in making this an option for the user. My
> > personal opinion here is to always request delivery reports by default
> > and handle them internally to acknowledge SMS messages.
> >
> > Regards
> >
> > Marcel
> 
> Thank you for your opinions. We already hoped that you would comment
> this part because we also had conversation here about how should this
> matter be solved. Current solution is mostly done this way because it
> seemed easiest way to get things working.
> 
> Have you thought this from the operator point of view. I mean, if you
> always request  delivery report, it is going to increase the load of the
> operators net and that is something that operator is not going to like.

Most operators / phones do expose this setting in the UI and leave it off by 
default.

> 
> But when you do the final decision about this matter, please let me know
> and I can then implement it.

I suggest using a Property (UseDeliveryReports) for this setting and storing 
it in the sms imsi-keyed settings file for now.  

Regards,
-Denis

VS: [PATCH 09/11] Support for concatenated SMS status report. And ofono_history_sms is now updated according to received status reports.

[Miettinen Pasi]

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

> Hi Pasi,
>
> > From: Pasi Miettinen <pasi.miettinen@ixonos.com>
> >
> > ---
> >  include/history.h |    2 +
> >  src/sms.c         |  157
> >  ++++++++++++++++++++++++++++++++++++++++++++++++++++- src/smsutil.h
> |
> >    6 ++
> >  3 files changed, 163 insertions(+), 2 deletions(-)
> >
>
> Some general comments:
>
> I'd like to see a status_report assembly similar to how the sms_assembly is
> done.  This should exist in smsutil.c/.h so that we can easily unit test this
> functionality (which I would also like to see.)
>
> Another desirable feature for the status report assembly is to save off the
> status report fragments on disk, so that status reports (for multi-fragment
> SMSes) received across oFono / system reboots and crashes are handled
> appropriately.
>
> Regards,
> -Denis

Hi Denis,

Could you clarify a bit what do you mean by "status_report assembly silmilar
to how the sms_assembly is done"?

Currently I have a belief that status-report, which relates to sent concatenated
message, does not know anything about other status-reports which relate to
the same concatenated message. I have to admit that I am not sure if this
belief is correct. Is it?

If the status-reports of the concatenated message know about each other,
then I might have some kind of picture what you are looking for concidering
the status_report assembly. Otherwise I am little bit lost what you are after
if my current solution does not satisfy you at all.

Br,
-Pasi

Re: VS: [PATCH 09/11] Support for concatenated SMS status report. And ofono_history_sms is now updated according to received status reports.

[Denis Kenzior]

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

Hi Pasi,

> > I'd like to see a status_report assembly similar to how the sms_assembly
> > is done.  This should exist in smsutil.c/.h so that we can easily unit
> > test this functionality (which I would also like to see.)
> >
> > Another desirable feature for the status report assembly is to save off
> > the status report fragments on disk, so that status reports (for
> > multi-fragment SMSes) received across oFono / system reboots and crashes
> > are handled appropriately.
> 
> Could you clarify a bit what do you mean by "status_report assembly
>  silmilar to how the sms_assembly is done"?

The function of sms_assembly is twofold:
- It takes care of all the de-fragmentation logic and only gives back a list 
of sms fragments once the entire list is received.  This is done outside of 
src/sms.c so that we can easily unit test this functionality and to keep 
clutter out of the main core logic.
- It takes care of writing fragments out to disk, so that in case of reset / 
reboot the received fragments are not lost.  This storage is keyed by the IMSI 
of SIM card, so even if the SIM is removed and a new inserted things just 
work.

For status reports I'd like something similar.  E.g. if I send a concatenated 
message with 3 fragments, and only 2 status reports come in before reboot, the 
next time I startup and receive the 3rd status report, the message state 
should be updated appropriately.

> 
> Currently I have a belief that status-report, which relates to sent
>  concatenated message, does not know anything about other status-reports
>  which relate to the same concatenated message. I have to admit that I am
>  not sure if this belief is correct. Is it?

You are correct in that each status report is handled / delivered separately.  
However, just relying on MR is not enough.  If you're sending lots of messages 
it is quite easy to overflow the MR.  So you must at least take the destination 
address into account.

Also, for sms fragments containing a udh, you can set some bits indicating to 
the SMSC to preserve the outgoing headers.  This way you can actually 
correlate the outgoing fragmentation information to the incoming status 
reports.  See Service Center Control Parameters IEI.  Whether this is useful 
in practice, I'm not sure...

> 
> If the status-reports of the concatenated message know about each other,
> then I might have some kind of picture what you are looking for concidering
> the status_report assembly. Otherwise I am little bit lost what you are
>  after if my current solution does not satisfy you at all.

If the SMSC sends you the original headers, then you have a pretty good chance 
of reliably correlating between sent message fragments and incoming status 
reports.  If you don't, then fall back to the regular MR + destination address 
approach.

The status_report_assembly can actually take some information as input.  Such 
as the fragment concatentation header, message reference, outgoing address and 
expiration time.

So in summary:
- should be in src/smsutil.c
- store intermediate state on disk and restore the state as needed on startup
- takes mr + destination address into account
- only notifies once all fragments are received or first fails

Regards,
-Denis

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Inaky Perez-Gonzalez]

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

On Thu, 2010-05-27 at 14:56 +0300, Aki Niemi wrote: 
> On Thu, 2010-05-27 at 13:39 +0200, ext Marcel Holtmann wrote: 
> > > I don't like this being an argument to SendMessage(). I think it needs
> > > to be exposed, but as a property instead. Is there a use case for
> > > setting this per message? I think majority of current phones either
> > > provide a global setting for this, or set it on by default.
> > 
> > our idea is actually that every new SMS has its own object path for its
> > lifetime. So we can have then properties easily on them.
> 
> Sure, but there should still be a property in SmsManager to control
> whether srr is to be set on outgoing messages.
> 
> Another property in the actual SmsMessage (residing on its own object
> path) could then indicate whether srr *was* set for that particular
> message when it was sent.

I tend to disagree with that; by making it an SmsManager property, you
are creating an API that is not reentrant. If more than one application
is sending SMS's at the same time and they have different requirements
wrt to status-reports, they would be trumping each other:

app A                                   app-B

smsmanager.status-reports = TRUE
                                      smsmanager.status-reports = FALSE
smsmanager.SendMessage(msg)  <--- OOPS
                                      smsmanager.SendMessage(msg)

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Denis Kenzior]

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

Hi Inaky,

> On Thu, 2010-05-27 at 14:56 +0300, Aki Niemi wrote:
> > On Thu, 2010-05-27 at 13:39 +0200, ext Marcel Holtmann wrote:
> > > > I don't like this being an argument to SendMessage(). I think it
> > > > needs to be exposed, but as a property instead. Is there a use case
> > > > for setting this per message? I think majority of current phones
> > > > either provide a global setting for this, or set it on by default.
> > >
> > > our idea is actually that every new SMS has its own object path for its
> > > lifetime. So we can have then properties easily on them.
> >
> > Sure, but there should still be a property in SmsManager to control
> > whether srr is to be set on outgoing messages.
> >
> > Another property in the actual SmsMessage (residing on its own object
> > path) could then indicate whether srr *was* set for that particular
> > message when it was sent.
> 
> I tend to disagree with that; by making it an SmsManager property, you
> are creating an API that is not reentrant. If more than one application
> is sending SMS's at the same time and they have different requirements
> wrt to status-reports, they would be trumping each other:

While you're 100% correct about a possible race condition, the reality is that 
nobody actually uses it this way.  It is just a setting buried somewhere in 
the Settings UI that the user maybe changes once in his lifetime.

There's no reason to burden everyone with sending a value (and storing / 
looking up its setting needlessly externally) that nobody really cares about.

Regards,
-Denis

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Inaky Perez-Gonzalez]

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

On Wed, 2010-06-09 at 17:47 -0500, Denis Kenzior wrote: 
> Hi Inaky,
> 
> > On Thu, 2010-05-27 at 14:56 +0300, Aki Niemi wrote:
> > > On Thu, 2010-05-27 at 13:39 +0200, ext Marcel Holtmann wrote:
> > > > > I don't like this being an argument to SendMessage(). I think it
> > > > > needs to be exposed, but as a property instead. Is there a use case
> > > > > for setting this per message? I think majority of current phones
> > > > > either provide a global setting for this, or set it on by default.
> > > >
> > > > our idea is actually that every new SMS has its own object path for its
> > > > lifetime. So we can have then properties easily on them.
> > >
> > > Sure, but there should still be a property in SmsManager to control
> > > whether srr is to be set on outgoing messages.
> > >
> > > Another property in the actual SmsMessage (residing on its own object
> > > path) could then indicate whether srr *was* set for that particular
> > > message when it was sent.
> > 
> > I tend to disagree with that; by making it an SmsManager property, you
> > are creating an API that is not reentrant. If more than one application
> > is sending SMS's at the same time and they have different requirements
> > wrt to status-reports, they would be trumping each other:
> 
> While you're 100% correct about a possible race condition, the reality is that 
> nobody actually uses it this way.  It is just a setting buried somewhere in 
> the Settings UI that the user maybe changes once in his lifetime.

If you are confident this will never be a problem then *shrug*; I am
just not comfortable with generic unconstrained APIs that operate like
that.

Re: [PATCH 06/11] Made it possible to ask for status report via SendMessage method parameters. True=status report on, false=off.

[Marcel Holtmann]

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

Hi Inaky,

> > > > > > I don't like this being an argument to SendMessage(). I think it
> > > > > > needs to be exposed, but as a property instead. Is there a use case
> > > > > > for setting this per message? I think majority of current phones
> > > > > > either provide a global setting for this, or set it on by default.
> > > > >
> > > > > our idea is actually that every new SMS has its own object path for its
> > > > > lifetime. So we can have then properties easily on them.
> > > >
> > > > Sure, but there should still be a property in SmsManager to control
> > > > whether srr is to be set on outgoing messages.
> > > >
> > > > Another property in the actual SmsMessage (residing on its own object
> > > > path) could then indicate whether srr *was* set for that particular
> > > > message when it was sent.
> > > 
> > > I tend to disagree with that; by making it an SmsManager property, you
> > > are creating an API that is not reentrant. If more than one application
> > > is sending SMS's at the same time and they have different requirements
> > > wrt to status-reports, they would be trumping each other:
> > 
> > While you're 100% correct about a possible race condition, the reality is that 
> > nobody actually uses it this way.  It is just a setting buried somewhere in 
> > the Settings UI that the user maybe changes once in his lifetime.
> 
> If you are confident this will never be a problem then *shrug*; I am
> just not comfortable with generic unconstrained APIs that operate like
> that.

please keep in mind that we are on purpose not going to expose anything
just for the sake of having an API for it. The API must make sense from
an UI use case point of view. oFono is suppose to do all the heavy
lifting and this also means that we are not exposing every single detail
or possibility of GSM/UMTS.

I would even go one step further in this case and always request status
reports and if the user doesn't want them, then it can throw this kind
of information away.

So there might be cases where we are wrong in the first place, but that
is fine as well. This is a learning experience that we have to go
through to create proper APIs. This API design by committee is not
working out anyway. It just creates clutter and extra shim layers since
everything gets way too complicated. The idea is to have a really simple
API first and then only if needed introduce complexity.

Regards

Marcel