Add text telephony support

Add text telephony support

[Lucas De Marchi]

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

Second version of this patch series adds support for text telephony.
Difference from previous patch:

	o As requested, now this patch is split in a per-directory basis;
	o Fixed documentation about the Errors that might be returned
	o Add documentation to build system
	o Add CACHED flag in text telephony implementation

[PATCH v2 1/7] text-telephony: add public header

[Lucas De Marchi]

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

---
 Makefile.am              |    3 +-
 include/text-telephony.h |   71 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 73 insertions(+), 1 deletions(-)
 create mode 100644 include/text-telephony.h

diff --git a/Makefile.am b/Makefile.am
index f841b4c..fb075a0 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -13,7 +13,8 @@ include_HEADERS = include/log.h include/plugin.h include/history.h \
 			include/cbs.h include/call-volume.h \
 			include/gprs.h include/gprs-context.h \
 			include/radio-settings.h include/stk.h \
-			include/audio-settings.h include/nettime.h
+			include/audio-settings.h include/nettime.h \
+			include/text-telephony.h
 
 nodist_include_HEADERS = include/version.h
 
diff --git a/include/text-telephony.h b/include/text-telephony.h
new file mode 100644
index 0000000..fafa7dd
--- /dev/null
+++ b/include/text-telephony.h
@@ -0,0 +1,71 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
+ *  Copyright (C) 2010  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __OFONO_TEXT_TELEPHONY_H
+#define __OFONO_TEXT_TELEPHONY_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <ofono/types.h>
+
+struct ofono_text_telephony;
+
+typedef void (*ofono_text_telephony_set_cb_t)(const struct ofono_error *error,
+						void *data);
+typedef void (*ofono_text_telephony_query_cb_t)(const struct ofono_error *error,
+						ofono_bool_t enable, void *data);
+
+struct ofono_text_telephony_driver {
+	const char *name;
+	int (*probe)(struct ofono_text_telephony *tt, unsigned int vendor,
+			void *data);
+	void (*remove)(struct ofono_text_telephony *tt);
+	void (*query_tty)(struct ofono_text_telephony *tt,
+				ofono_text_telephony_query_cb_t cb,
+				void *data);
+	void (*set_tty)(struct ofono_text_telephony *tt,
+				int enable,
+				ofono_text_telephony_set_cb_t cb,
+				void *data);
+};
+
+int ofono_text_telephony_driver_register(const struct ofono_text_telephony_driver *d);
+void ofono_text_telephony_driver_unregister(const struct ofono_text_telephony_driver *d);
+
+struct ofono_text_telephony *ofono_text_telephony_create(struct ofono_modem *modem,
+								unsigned int vendor,
+								const char *driver,
+								void *data);
+
+void ofono_text_telephony_register(struct ofono_text_telephony *tt);
+void ofono_text_telephony_remove(struct ofono_text_telephony *tt);
+
+void ofono_text_telephony_set_data(struct ofono_text_telephony *tt, void *data);
+void *ofono_text_telephony_get_data(struct ofono_text_telephony *tt);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __OFONO_TEXT_TELEPHONY_H */
-- 
1.7.3.2

[PATCH v2 2/7] text-telephony: define new dbus interface

[Lucas De Marchi]

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

---
 include/dbus.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/dbus.h b/include/dbus.h
index 59b2aae..9e29afb 100644
--- a/include/dbus.h
+++ b/include/dbus.h
@@ -48,6 +48,7 @@ extern "C" {
 #define OFONO_PHONEBOOK_INTERFACE "org.ofono.Phonebook"
 #define OFONO_RADIO_SETTINGS_INTERFACE "org.ofono.RadioSettings"
 #define OFONO_AUDIO_SETTINGS_INTERFACE "org.ofono.AudioSettings"
+#define OFONO_TEXT_TELEPHONY_INTERFACE "org.ofono.TextTelephony"
 #define OFONO_SIM_MANAGER_INTERFACE "org.ofono.SimManager"
 #define OFONO_VOICECALL_INTERFACE "org.ofono.VoiceCall"
 #define OFONO_VOICECALL_MANAGER_INTERFACE "org.ofono.VoiceCallManager"
-- 
1.7.3.2

[PATCH v2 3/7] text-telephony: add new interface to feature map

[Lucas De Marchi]

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

---
 src/modem.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/src/modem.c b/src/modem.c
index cfc767e..f032d93 100644
--- a/src/modem.c
+++ b/src/modem.c
@@ -904,6 +904,7 @@ static const struct {
 	{ OFONO_SIM_MANAGER_INTERFACE,			"sim"	},
 	{ OFONO_STK_INTERFACE,				"stk"	},
 	{ OFONO_CONNECTION_MANAGER_INTERFACE,		"gprs"	},
+	{ OFONO_TEXT_TELEPHONY_INTERFACE,		"tty"	},
 	{ },
 };
 
-- 
1.7.3.2

[PATCH v2 4/7] text-telephony: implement interface/atom

[Lucas De Marchi]

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

---
 Makefile.am          |    2 +-
 src/ofono.h          |    2 +
 src/text-telephony.c |  341 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 344 insertions(+), 1 deletions(-)
 create mode 100644 src/text-telephony.c

diff --git a/Makefile.am b/Makefile.am
index fb075a0..ee1313d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -319,7 +319,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) src/ofono.ver \
 			src/radio-settings.c src/stkutil.h src/stkutil.c \
 			src/nettime.c src/stkagent.c src/stkagent.h \
 			src/simfs.c src/simfs.h src/audio-settings.c \
-			src/smsagent.c src/smsagent.h
+			src/smsagent.c src/smsagent.h src/text-telephony.c
 
 src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl
 
diff --git a/src/ofono.h b/src/ofono.h
index d1a4bdc..42736bb 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -125,6 +125,7 @@ enum ofono_atom_type {
 	OFONO_ATOM_TYPE_AUDIO_SETTINGS = 19,
 	OFONO_ATOM_TYPE_STK = 20,
 	OFONO_ATOM_TYPE_NETTIME = 21,
+	OFONO_ATOM_TYPE_TEXT_TELEPHONY = 22,
 };
 
 enum ofono_atom_watch_condition {
@@ -205,6 +206,7 @@ gboolean __ofono_call_settings_is_busy(struct ofono_call_settings *cs);
 #include <ofono/gprs-context.h>
 #include <ofono/radio-settings.h>
 #include <ofono/audio-settings.h>
+#include <ofono/text-telephony.h>
 
 #include <ofono/voicecall.h>
 
diff --git a/src/text-telephony.c b/src/text-telephony.c
new file mode 100644
index 0000000..df87378
--- /dev/null
+++ b/src/text-telephony.c
@@ -0,0 +1,341 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
+ *  Copyright (C) 2010  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <string.h>
+#include <stdio.h>
+#include <errno.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#include "ofono.h"
+#include "common.h"
+
+#define TEXT_TELEPHONY_FLAG_CACHED 0x1
+
+static GSList *g_drivers = NULL;
+
+struct ofono_text_telephony {
+	DBusMessage *pending;
+	int flags;
+	ofono_bool_t powered;
+	ofono_bool_t powered_pending;
+	const struct ofono_text_telephony_driver *driver;
+	void *driver_data;
+	struct ofono_atom *atom;
+};
+
+static DBusMessage *tt_get_properties_reply(DBusMessage *msg,
+						struct ofono_text_telephony *tt)
+{
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+	dbus_bool_t value;
+
+	reply = dbus_message_new_method_return(msg);
+	if (!reply)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					&dict);
+
+	value = tt->powered;
+	ofono_dbus_dict_append(&dict, "Powered", DBUS_TYPE_BOOLEAN, &value);
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static void tt_set_powered(struct ofono_text_telephony *tt,
+					ofono_bool_t enable)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path = __ofono_atom_get_path(tt->atom);
+	dbus_bool_t value = enable;
+
+	if (tt->powered == enable)
+		return;
+
+	ofono_dbus_signal_property_changed(conn, path,
+						OFONO_TEXT_TELEPHONY_INTERFACE,
+						"Powered",
+						DBUS_TYPE_BOOLEAN, &value);
+	tt->powered = enable;
+}
+
+static void tt_set_powered_callback(const struct ofono_error *error,
+						void *data)
+{
+	struct ofono_text_telephony *tt = data;
+	DBusMessage *reply;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		DBG("Error setting powered property");
+
+		tt->powered_pending = tt->powered;
+
+		reply = __ofono_error_failed(tt->pending);
+		__ofono_dbus_pending_reply(&tt->pending, reply);
+
+		return;
+	}
+
+	reply = dbus_message_new_method_return(tt->pending);
+	__ofono_dbus_pending_reply(&tt->pending, reply);
+
+	tt_set_powered(tt, tt->powered_pending);
+}
+
+static void tt_send_properties_reply(struct ofono_text_telephony *tt)
+{
+	DBusMessage *reply;
+
+	tt->flags |= TEXT_TELEPHONY_FLAG_CACHED;
+
+	reply = tt_get_properties_reply(tt->pending, tt);
+	__ofono_dbus_pending_reply(&tt->pending, reply);
+}
+
+static void tt_query_powered_callback(const struct ofono_error *error,
+						ofono_bool_t enable, void *data)
+{
+	struct ofono_text_telephony *tt = data;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		DBusMessage *reply;
+
+		ofono_debug("Error during powered query");
+
+		reply = __ofono_error_failed(tt->pending);
+		__ofono_dbus_pending_reply(&tt->pending, reply);
+
+		return;
+	}
+
+	tt_set_powered(tt, enable);
+	tt_send_properties_reply(tt);
+}
+
+static DBusMessage *tt_get_properties(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	struct ofono_text_telephony *tt = data;
+
+	if (tt->flags & TEXT_TELEPHONY_FLAG_CACHED)
+		return tt_get_properties_reply(msg, tt);
+
+	if (tt->pending)
+		return __ofono_error_busy(msg);
+
+	tt->pending = dbus_message_ref(msg);
+	tt->driver->query_tty(tt, tt_query_powered_callback, tt);
+
+	return NULL;
+}
+
+static DBusMessage *tt_set_property(DBusConnection *conn, DBusMessage *msg,
+					void *data)
+{
+	struct ofono_text_telephony *tt = data;
+	DBusMessageIter iter;
+	DBusMessageIter var;
+	const char *property;
+
+	if (tt->pending)
+		return __ofono_error_busy(msg);
+
+	if (!dbus_message_iter_init(msg, &iter))
+		return __ofono_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&iter, &property);
+	dbus_message_iter_next(&iter);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_recurse(&iter, &var);
+
+	if (g_strcmp0(property, "Powered") == 0) {
+		dbus_bool_t value;
+		int target;
+
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &value);
+		target = value;
+
+		if (tt->powered == target)
+			return dbus_message_new_method_return(msg);
+
+		tt->pending = dbus_message_ref(msg);
+		tt->powered_pending = target;
+
+		tt->driver->set_tty(tt, target,
+					tt_set_powered_callback, tt);
+		return NULL;
+	}
+
+	return __ofono_error_invalid_args(msg);
+}
+
+static GDBusMethodTable tt_methods[] = {
+	{ "GetProperties",  "",    "a{sv}",  tt_get_properties,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "SetProperty",    "sv",  "",       tt_set_property,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ }
+};
+
+static GDBusSignalTable tt_signals[] = {
+	{ "PropertyChanged",	"sv" },
+	{ }
+};
+
+int ofono_text_telephony_driver_register(const struct ofono_text_telephony_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	if (!d || !d->probe)
+		return -EINVAL;
+
+	g_drivers = g_slist_prepend(g_drivers, (void *)d);
+
+	return 0;
+}
+
+void ofono_text_telephony_driver_unregister(const struct ofono_text_telephony_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	if (!d)
+		return;
+
+	g_drivers = g_slist_remove(g_drivers, (void *)d);
+}
+
+static void text_telephony_unregister(struct ofono_atom *atom)
+{
+	struct ofono_text_telephony *tt = __ofono_atom_get_data(atom);
+	const char *path = __ofono_atom_get_path(tt->atom);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(tt->atom);
+
+	ofono_modem_remove_interface(modem, OFONO_TEXT_TELEPHONY_INTERFACE);
+	g_dbus_unregister_interface(conn, path, OFONO_TEXT_TELEPHONY_INTERFACE);
+}
+
+static void text_telephony_remove(struct ofono_atom *atom)
+{
+	struct ofono_text_telephony *tt = __ofono_atom_get_data(atom);
+
+	DBG("atom: %p", atom);
+
+	if (!tt)
+		return;
+
+	if (tt->driver && tt->driver->remove)
+		tt->driver->remove(tt);
+
+	g_free(tt);
+}
+
+struct ofono_text_telephony *ofono_text_telephony_create(struct ofono_modem *modem,
+							unsigned int vendor,
+							const char *driver,
+							void *data)
+{
+	struct ofono_text_telephony *tt;
+	GSList *l;
+	if (!driver)
+		return NULL;
+
+	tt = g_try_new0(struct ofono_text_telephony, 1);
+	if (!tt)
+		return NULL;
+
+	tt->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_TEXT_TELEPHONY,
+						text_telephony_remove, tt);
+
+	tt->powered = -1;
+
+	for (l = g_drivers; l; l = l->next) {
+		const struct ofono_text_telephony_driver *drv = l->data;
+
+		if (g_strcmp0(drv->name, driver) != 0)
+			continue;
+
+		if (drv->probe(tt, vendor, data) < 0)
+			continue;
+
+		tt->driver = drv;
+		break;
+	}
+
+	return tt;
+}
+
+void ofono_text_telephony_register(struct ofono_text_telephony *tt)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(tt->atom);
+	const char *path = __ofono_atom_get_path(tt->atom);
+
+	if (!g_dbus_register_interface(conn, path,
+					OFONO_TEXT_TELEPHONY_INTERFACE,
+						tt_methods, tt_signals,
+					NULL, tt, NULL)) {
+		ofono_error("Could not create %s interface",
+				OFONO_TEXT_TELEPHONY_INTERFACE);
+
+		return;
+	}
+
+	ofono_modem_add_interface(modem, OFONO_TEXT_TELEPHONY_INTERFACE);
+	__ofono_atom_register(tt->atom, text_telephony_unregister);
+}
+
+void ofono_text_telephony_remove(struct ofono_text_telephony *tt)
+{
+	__ofono_atom_free(tt->atom);
+}
+
+void ofono_text_telephony_set_data(struct ofono_text_telephony *tt,
+					void *data)
+{
+	tt->driver_data = data;
+}
+
+void *ofono_text_telephony_get_data(struct ofono_text_telephony *tt)
+{
+	return tt->driver_data;
+}
-- 
1.7.3.2

[PATCH v2 5/7] text-telephony: add documentation

[Lucas De Marchi]

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

---
 Makefile.am                |    2 +-
 doc/text-telephony-api.txt |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletions(-)
 create mode 100644 doc/text-telephony-api.txt

diff --git a/Makefile.am b/Makefile.am
index ee1313d..e231457 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -356,7 +356,7 @@ doc_files = doc/overview.txt doc/ofono-paper.txt \
 			doc/message-api.txt doc/message-waiting-api.txt \
 			doc/phonebook-api.txt doc/radio-settings-api.txt \
 			doc/sim-api.txt doc/stk-api.txt \
-			doc/audio-settings-api.txt
+			doc/audio-settings-api.txt doc/text-telephony-api.txt
 
 
 test_scripts = test/backtrace \
diff --git a/doc/text-telephony-api.txt b/doc/text-telephony-api.txt
new file mode 100644
index 0000000..e7c898f
--- /dev/null
+++ b/doc/text-telephony-api.txt
@@ -0,0 +1,39 @@
+Text Telephony hierarchy
+========================
+
+Service		org.ofono
+Interface	org.ofono.TextTelephony
+Object path	[variable prefix]/{modem0,modem1,...}
+
+Methods		dict GetProperties()
+
+			Returns all Text Telephony properties. See the
+			properties section for available properties.
+
+			Possible Errors: [service].Error.InProgress
+					 [service].Error.Failed
+
+		void SetProperty(string name, variant value)
+
+			Changes the value of the specified property. Only
+			properties that are listed as read-write are
+			changeable. On success a PropertyChanged signal
+			will be emitted.
+
+			Possible Errors: [service].Error.InvalidArguments
+					 [service].Error.InProgress
+					 [service].Error.Failed
+
+Signals		PropertyChanged(string property, variant value)
+
+			This signal indicates a changed value of the given
+			property.
+
+Properties	boolean	Powered [readwrite]
+
+			This property will enable or disable the text
+			telephony feature in the modem.
+
+			Text telephony (TTY), also known as TDD, is a feature
+			present in some modems that allow them to be used by
+			hearing impaired people.
-- 
1.7.3.2

[PATCH v2 6/7] phonesim: implement text-telephony atom

[Lucas De Marchi]

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

---
 plugins/phonesim.c |  148 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 144 insertions(+), 4 deletions(-)

diff --git a/plugins/phonesim.c b/plugins/phonesim.c
index d2faf42..8c31df4 100644
--- a/plugins/phonesim.c
+++ b/plugins/phonesim.c
@@ -54,6 +54,7 @@
 #include <ofono/stk.h>
 #include <ofono/sms.h>
 #include <ofono/ssn.h>
+#include <ofono/text-telephony.h>
 #include <ofono/ussd.h>
 #include <ofono/voicecall.h>
 #include <ofono/gprs.h>
@@ -64,6 +65,7 @@
 #include <drivers/atmodem/atutil.h>
 
 static const char *none_prefix[] = { NULL };
+static const char *ptty_prefix[] = { "+PTTY:", NULL };
 static int next_iface = 0;
 
 struct phonesim_data {
@@ -78,6 +80,11 @@ struct gprs_context_data {
 	char *interface;
 };
 
+struct text_telephony_data {
+	GAtChat *chat;
+	char *interface;
+};
+
 static void at_cgact_up_cb(gboolean ok, GAtResult *result, gpointer user_data)
 {
 	struct cb_data *cbd = user_data;
@@ -190,12 +197,140 @@ static void phonesim_context_remove(struct ofono_gprs_context *gc)
 	g_free(gcd);
 }
 
+static int phonesim_tty_probe(struct ofono_text_telephony *tt,
+				unsigned int vendor, void *data)
+{
+	GAtChat *chat = data;
+	struct text_telephony_data *ttd;
+
+	ttd = g_try_new0(struct text_telephony_data, 1);
+	if (!ttd)
+		return -ENOMEM;
+
+	ttd->chat = g_at_chat_clone(chat);
+	ttd->interface = g_strdup_printf("dummy%d", next_iface++);
+
+	ofono_text_telephony_set_data(tt, ttd);
+	ofono_text_telephony_register(tt);
+
+	return 0;
+}
+
+static void phonesim_tty_remove(struct ofono_text_telephony *tt)
+{
+	struct text_telephony_data *ttd = ofono_text_telephony_get_data(tt);
+
+	DBG("");
+
+	ofono_text_telephony_set_data(tt, NULL);
+
+	g_at_chat_unref(ttd->chat);
+	g_free(ttd->interface);
+
+	g_free(ttd);
+}
+
+static void tty_query_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	struct ofono_error error;
+	GAtResultIter iter;
+	ofono_text_telephony_query_cb_t cb = cbd->cb;
+	int value;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok) {
+		cb(&error, -1, cbd->data);
+		return;
+	}
+
+	g_at_result_iter_init(&iter, result);
+
+	if (g_at_result_iter_next(&iter, "+PTTY:") == FALSE)
+		goto error;
+
+	if (g_at_result_iter_next_number(&iter, &value) == FALSE)
+		goto error;
+
+	cb(&error, value, cbd->data);
+
+	return;
+
+error:
+
+	CALLBACK_WITH_FAILURE(cb, -1, cbd->data);
+}
+
+static void phonesim_tty_query(struct ofono_text_telephony *tt,
+				ofono_text_telephony_query_cb_t cb, void *data)
+{
+	struct text_telephony_data *ttd = ofono_text_telephony_get_data(tt);
+	struct cb_data *cbd = cb_data_new(cb, data);
+
+	DBG("");
+
+	if (!cbd)
+		goto error;
+
+	if (g_at_chat_send(ttd->chat, "AT+PTTY?", ptty_prefix,
+				tty_query_cb, cbd, g_free) > 0)
+		return;
+
+error:
+	g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, 0, data);
+}
+
+static void tty_set_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_text_telephony_set_cb_t cb = cbd->cb;
+	struct ofono_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+	cb(&error, cbd->data);
+}
+
+static void phonesim_tty_set(struct ofono_text_telephony *tt, int enable,
+				ofono_text_telephony_set_cb_t cb, void *data)
+{
+	struct text_telephony_data *ttd = ofono_text_telephony_get_data(tt);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[12];
+
+	DBG("");
+
+	if (!cbd)
+		goto error;
+
+	enable = !!enable;
+	snprintf(buf, sizeof(buf), "AT+PTTY=%d", enable);
+
+	if (g_at_chat_send(ttd->chat, buf, none_prefix,
+					tty_set_cb, cbd, g_free) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, data);
+	g_free(cbd);
+}
+
 static struct ofono_gprs_context_driver context_driver = {
+	.name                   = "phonesim",
+	.probe                  = phonesim_context_probe,
+	.remove                 = phonesim_context_remove,
+	.activate_primary       = phonesim_activate_primary,
+	.deactivate_primary     = phonesim_deactivate_primary,
+};
+
+static struct ofono_text_telephony_driver tty_driver = {
 	.name			= "phonesim",
-	.probe			= phonesim_context_probe,
-	.remove			= phonesim_context_remove,
-	.activate_primary	= phonesim_activate_primary,
-	.deactivate_primary	= phonesim_deactivate_primary,
+	.probe			= phonesim_tty_probe,
+	.remove			= phonesim_tty_remove,
+	.query_tty		= phonesim_tty_query,
+	.set_tty		= phonesim_tty_set,
 };
 
 static int phonesim_probe(struct ofono_modem *modem)
@@ -465,6 +600,7 @@ static void phonesim_post_sim(struct ofono_modem *modem)
 
 	DBG("%p", modem);
 
+	ofono_text_telephony_create(modem, 0, "phonesim", data->chat);
 	ofono_phonebook_create(modem, 0, "atmodem", data->chat);
 
 	if (!data->calypso)
@@ -630,6 +766,8 @@ static int phonesim_init(void)
 
 	ofono_gprs_context_driver_register(&context_driver);
 
+	ofono_text_telephony_driver_register(&tty_driver);
+
 	parse_config(CONFIGDIR "/phonesim.conf");
 
 	return 0;
@@ -648,6 +786,8 @@ static void phonesim_exit(void)
 	g_slist_free(modem_list);
 	modem_list = NULL;
 
+	ofono_text_telephony_driver_unregister(&tty_driver);
+
 	ofono_gprs_context_driver_unregister(&context_driver);
 
 	ofono_modem_driver_unregister(&phonesim_driver);
-- 
1.7.3.2

[PATCH v2 7/7] Add script to enable/disable text-telephony support

[Lucas De Marchi]

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

---
 test/set-tty |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)
 create mode 100755 test/set-tty

diff --git a/test/set-tty b/test/set-tty
new file mode 100755
index 0000000..cbbb043
--- /dev/null
+++ b/test/set-tty
@@ -0,0 +1,25 @@
+#!/usr/bin/python
+
+import dbus
+import sys
+
+bus = dbus.SystemBus()
+
+if len(sys.argv) == 3:
+	path = sys.argv[1]
+	enable = int(sys.argv[2])
+elif len(sys.argv) == 2:
+	manager = dbus.Interface(bus.get_object('org.ofono', '/'),
+						'org.ofono.Manager')
+	modems = manager.GetModems()
+	path = modems[0][0]
+	enable = int(sys.argv[1])
+else:
+	print "%s [PATH] {0|1}" % (sys.argv[0])
+	exit(1)
+
+print "Setting TTY for modem %s..." % path
+texttelephony = dbus.Interface(bus.get_object('org.ofono', path),
+						'org.ofono.TextTelephony')
+
+texttelephony.SetProperty("Powered", dbus.Boolean(enable));
-- 
1.7.3.2

Re: [PATCH v2 5/7] text-telephony: add documentation

[Denis Kenzior]

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

Hi Lucas,

> +Properties	boolean	Powered [readwrite]
> +
> +			This property will enable or disable the text
> +			telephony feature in the modem.
> +
> +			Text telephony (TTY), also known as TDD, is a feature
> +			present in some modems that allow them to be used by
> +			hearing impaired people.

Marcel and I discussed this, and we decided we prefer 'Enabled' for the
property name.  You might also want to mention that this feature is also
known as 'CTM', Cellular Text Modem.

Regards,
-Denis

Re: [PATCH v2 2/7] text-telephony: define new dbus interface

[Denis Kenzior]

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

Hi Lucas,

On 11/23/2010 12:04 PM, Lucas De Marchi wrote:
> ---
>  include/dbus.h |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH v2 3/7] text-telephony: add new interface to feature map

[Denis Kenzior]

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

Hi Lucas,

On 11/23/2010 12:04 PM, Lucas De Marchi wrote:
> ---
>  src/modem.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 

Patch has been applied, thanks.

Regards,
-Denis

Re: [PATCH v2 1/7] text-telephony: add public header

[Denis Kenzior]

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

Hi Lucas,

On 11/23/2010 12:04 PM, Lucas De Marchi wrote:
> ---
>  Makefile.am              |    3 +-
>  include/text-telephony.h |   71 ++++++++++++++++++++++++++++++++++++++++++++++

So I'd like to name this file include/ctm.h, looks nicer and is shorter
to type.

>  2 files changed, 73 insertions(+), 1 deletions(-)
>  create mode 100644 include/text-telephony.h
> 
> diff --git a/Makefile.am b/Makefile.am
> index f841b4c..fb075a0 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -13,7 +13,8 @@ include_HEADERS = include/log.h include/plugin.h include/history.h \
>  			include/cbs.h include/call-volume.h \
>  			include/gprs.h include/gprs-context.h \
>  			include/radio-settings.h include/stk.h \
> -			include/audio-settings.h include/nettime.h
> +			include/audio-settings.h include/nettime.h \
> +			include/text-telephony.h
>  
>  nodist_include_HEADERS = include/version.h
>  
> diff --git a/include/text-telephony.h b/include/text-telephony.h
> new file mode 100644
> index 0000000..fafa7dd
> --- /dev/null
> +++ b/include/text-telephony.h
> @@ -0,0 +1,71 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
> + *  Copyright (C) 2010  Intel Corporation. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifndef __OFONO_TEXT_TELEPHONY_H
> +#define __OFONO_TEXT_TELEPHONY_H

OFONO_CTM here please

> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <ofono/types.h>
> +
> +struct ofono_text_telephony;

Can you do me a favor and rename all occurrences of ofono_text_telephony
into ofono_ctm?

> +
> +typedef void (*ofono_text_telephony_set_cb_t)(const struct ofono_error *error,
> +						void *data);
> +typedef void (*ofono_text_telephony_query_cb_t)(const struct ofono_error *error,
> +						ofono_bool_t enable, void *data);
> +
> +struct ofono_text_telephony_driver {
> +	const char *name;
> +	int (*probe)(struct ofono_text_telephony *tt, unsigned int vendor,
> +			void *data);
> +	void (*remove)(struct ofono_text_telephony *tt);
> +	void (*query_tty)(struct ofono_text_telephony *tt,
> +				ofono_text_telephony_query_cb_t cb,
> +				void *data);
> +	void (*set_tty)(struct ofono_text_telephony *tt,
> +				int enable,
> +				ofono_text_telephony_set_cb_t cb,
> +				void *data);
> +};
> +
> +int ofono_text_telephony_driver_register(const struct ofono_text_telephony_driver *d);
> +void ofono_text_telephony_driver_unregister(const struct ofono_text_telephony_driver *d);
> +
> +struct ofono_text_telephony *ofono_text_telephony_create(struct ofono_modem *modem,
> +								unsigned int vendor,
> +								const char *driver,
> +								void *data);
> +
> +void ofono_text_telephony_register(struct ofono_text_telephony *tt);
> +void ofono_text_telephony_remove(struct ofono_text_telephony *tt);
> +
> +void ofono_text_telephony_set_data(struct ofono_text_telephony *tt, void *data);
> +void *ofono_text_telephony_get_data(struct ofono_text_telephony *tt);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __OFONO_TEXT_TELEPHONY_H */

Otherwise, looks good to me.

Regards,
-Denis

Re: [PATCH v2 4/7] text-telephony: implement interface/atom

[Denis Kenzior]

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

Hi Lucas,

On 11/23/2010 12:04 PM, Lucas De Marchi wrote:
> ---
>  Makefile.am          |    2 +-
>  src/ofono.h          |    2 +
>  src/text-telephony.c |  341 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 344 insertions(+), 1 deletions(-)
>  create mode 100644 src/text-telephony.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index fb075a0..ee1313d 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -319,7 +319,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) src/ofono.ver \
>  			src/radio-settings.c src/stkutil.h src/stkutil.c \
>  			src/nettime.c src/stkagent.c src/stkagent.h \
>  			src/simfs.c src/simfs.h src/audio-settings.c \
> -			src/smsagent.c src/smsagent.h
> +			src/smsagent.c src/smsagent.h src/text-telephony.c
>  
>  src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl
>  
> diff --git a/src/ofono.h b/src/ofono.h
> index d1a4bdc..42736bb 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -125,6 +125,7 @@ enum ofono_atom_type {
>  	OFONO_ATOM_TYPE_AUDIO_SETTINGS = 19,
>  	OFONO_ATOM_TYPE_STK = 20,
>  	OFONO_ATOM_TYPE_NETTIME = 21,
> +	OFONO_ATOM_TYPE_TEXT_TELEPHONY = 22,
>  };
>  
>  enum ofono_atom_watch_condition {
> @@ -205,6 +206,7 @@ gboolean __ofono_call_settings_is_busy(struct ofono_call_settings *cs);
>  #include <ofono/gprs-context.h>
>  #include <ofono/radio-settings.h>
>  #include <ofono/audio-settings.h>
> +#include <ofono/text-telephony.h>
>  
>  #include <ofono/voicecall.h>
>  
> diff --git a/src/text-telephony.c b/src/text-telephony.c
> new file mode 100644
> index 0000000..df87378
> --- /dev/null
> +++ b/src/text-telephony.c
> @@ -0,0 +1,341 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies).
> + *  Copyright (C) 2010  Intel Corporation. All rights reserved.
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License version 2 as
> + *  published by the Free Software Foundation.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <string.h>
> +#include <stdio.h>
> +#include <errno.h>
> +
> +#include <glib.h>
> +#include <gdbus.h>
> +
> +#include "ofono.h"
> +#include "common.h"
> +
> +#define TEXT_TELEPHONY_FLAG_CACHED 0x1
> +
> +static GSList *g_drivers = NULL;
> +
> +struct ofono_text_telephony {
> +	DBusMessage *pending;
> +	int flags;
> +	ofono_bool_t powered;
> +	ofono_bool_t powered_pending;
> +	const struct ofono_text_telephony_driver *driver;
> +	void *driver_data;
> +	struct ofono_atom *atom;
> +};
> +
> +static DBusMessage *tt_get_properties_reply(DBusMessage *msg,
> +						struct ofono_text_telephony *tt)
> +{
> +	DBusMessage *reply;
> +	DBusMessageIter iter;
> +	DBusMessageIter dict;
> +	dbus_bool_t value;
> +
> +	reply = dbus_message_new_method_return(msg);
> +	if (!reply)
> +		return NULL;
> +
> +	dbus_message_iter_init_append(reply, &iter);
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					&dict);
> +
> +	value = tt->powered;
> +	ofono_dbus_dict_append(&dict, "Powered", DBUS_TYPE_BOOLEAN, &value);

As mentioned previously, please use "Enabled" here.

> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	return reply;
> +}
> +
> +static void tt_set_powered(struct ofono_text_telephony *tt,
> +					ofono_bool_t enable)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path = __ofono_atom_get_path(tt->atom);
> +	dbus_bool_t value = enable;
> +
> +	if (tt->powered == enable)
> +		return;
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_TEXT_TELEPHONY_INTERFACE,
> +						"Powered",
> +						DBUS_TYPE_BOOLEAN, &value);
> +	tt->powered = enable;
> +}
> +
> +static void tt_set_powered_callback(const struct ofono_error *error,
> +						void *data)
> +{
> +	struct ofono_text_telephony *tt = data;
> +	DBusMessage *reply;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		DBG("Error setting powered property");
> +
> +		tt->powered_pending = tt->powered;
> +
> +		reply = __ofono_error_failed(tt->pending);
> +		__ofono_dbus_pending_reply(&tt->pending, reply);
> +
> +		return;
> +	}
> +
> +	reply = dbus_message_new_method_return(tt->pending);
> +	__ofono_dbus_pending_reply(&tt->pending, reply);
> +
> +	tt_set_powered(tt, tt->powered_pending);
> +}
> +
> +static void tt_send_properties_reply(struct ofono_text_telephony *tt)
> +{
> +	DBusMessage *reply;
> +
> +	tt->flags |= TEXT_TELEPHONY_FLAG_CACHED;
> +
> +	reply = tt_get_properties_reply(tt->pending, tt);
> +	__ofono_dbus_pending_reply(&tt->pending, reply);

I actually prefer to fold this function into powered_callback

> +}
> +
> +static void tt_query_powered_callback(const struct ofono_error *error,
> +						ofono_bool_t enable, void *data)
> +{
> +	struct ofono_text_telephony *tt = data;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		DBusMessage *reply;
> +
> +		ofono_debug("Error during powered query");
> +
> +		reply = __ofono_error_failed(tt->pending);
> +		__ofono_dbus_pending_reply(&tt->pending, reply);
> +
> +		return;
> +	}
> +
> +	tt_set_powered(tt, enable);
> +	tt_send_properties_reply(tt);

The oFono convention is to reply to the pending D-Bus method call and
then signal the property changed signals.

> +}
> +
> +static DBusMessage *tt_get_properties(DBusConnection *conn,
> +						DBusMessage *msg, void *data)
> +{
> +	struct ofono_text_telephony *tt = data;
> +
> +	if (tt->flags & TEXT_TELEPHONY_FLAG_CACHED)
> +		return tt_get_properties_reply(msg, tt);
> +
> +	if (tt->pending)
> +		return __ofono_error_busy(msg);
> +
> +	tt->pending = dbus_message_ref(msg);
> +	tt->driver->query_tty(tt, tt_query_powered_callback, tt);
> +
> +	return NULL;
> +}
> +
> +static DBusMessage *tt_set_property(DBusConnection *conn, DBusMessage *msg,
> +					void *data)
> +{
> +	struct ofono_text_telephony *tt = data;
> +	DBusMessageIter iter;
> +	DBusMessageIter var;
> +	const char *property;
> +
> +	if (tt->pending)
> +		return __ofono_error_busy(msg);
> +
> +	if (!dbus_message_iter_init(msg, &iter))
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_get_basic(&iter, &property);
> +	dbus_message_iter_next(&iter);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_recurse(&iter, &var);
> +
> +	if (g_strcmp0(property, "Powered") == 0) {
> +		dbus_bool_t value;
> +		int target;
> +
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_BOOLEAN)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &value);
> +		target = value;
> +
> +		if (tt->powered == target)
> +			return dbus_message_new_method_return(msg);
> +
> +		tt->pending = dbus_message_ref(msg);
> +		tt->powered_pending = target;
> +
> +		tt->driver->set_tty(tt, target,
> +					tt_set_powered_callback, tt);
> +		return NULL;
> +	}
> +
> +	return __ofono_error_invalid_args(msg);
> +}

<snip>

> +int ofono_text_telephony_driver_register(const struct ofono_text_telephony_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	if (!d || !d->probe)
> +		return -EINVAL;
> +
> +	g_drivers = g_slist_prepend(g_drivers, (void *)d);
> +
> +	return 0;
> +}
> +
> +void ofono_text_telephony_driver_unregister(const struct ofono_text_telephony_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	if (!d)

The preferred style is to compare to NULL, e.g. if (d == NULL)

> +		return;
> +
> +	g_drivers = g_slist_remove(g_drivers, (void *)d);
> +}
> +

<snip>

> +struct ofono_text_telephony *ofono_text_telephony_create(struct ofono_modem *modem,
> +							unsigned int vendor,
> +							const char *driver,
> +							void *data)
> +{
> +	struct ofono_text_telephony *tt;
> +	GSList *l;

Please add a newline here

> +	if (!driver)
> +		return NULL;
> +
> +	tt = g_try_new0(struct ofono_text_telephony, 1);
> +	if (!tt)
> +		return NULL;
> +
> +	tt->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_TEXT_TELEPHONY,
> +						text_telephony_remove, tt);
> +
> +	tt->powered = -1;

I think it is safe to default this to 0 (off).

> +
> +	for (l = g_drivers; l; l = l->next) {
> +		const struct ofono_text_telephony_driver *drv = l->data;
> +
> +		if (g_strcmp0(drv->name, driver) != 0)
> +			continue;
> +
> +		if (drv->probe(tt, vendor, data) < 0)
> +			continue;
> +
> +		tt->driver = drv;
> +		break;
> +	}
> +
> +	return tt;
> +}
> +

Otherwise, looks good.

Regards,
-Denis

Re: [PATCH v2 4/7] text-telephony: implement interface/atom

[Lucas De Marchi]

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

Hi Denis,

On Wed, Nov 24, 2010 at 1:58 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Lucas,
>
> On 11/23/2010 12:04 PM, Lucas De Marchi wrote:
>> ---
>>  Makefile.am          |    2 +-
>>  src/ofono.h          |    2 +
>>  src/text-telephony.c |  341 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 344 insertions(+), 1 deletions(-)
>>  create mode 100644 src/text-telephony.c
>>
>> diff --git a/Makefile.am b/Makefile.am
>> index fb075a0..ee1313d 100644
>> --- a/Makefile.am
>> +++ b/Makefile.am
>> +static DBusMessage *tt_get_properties_reply(DBusMessage *msg,
>> +                                             struct ofono_text_telephony *tt)
>> +{
>> +     DBusMessage *reply;
>> +     DBusMessageIter iter;
>> +     DBusMessageIter dict;
>> +     dbus_bool_t value;
>> +
>> +     reply = dbus_message_new_method_return(msg);
>> +     if (!reply)
>> +             return NULL;
>> +
>> +     dbus_message_iter_init_append(reply, &iter);
>> +     dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
>> +                                     OFONO_PROPERTIES_ARRAY_SIGNATURE,
>> +                                     &dict);
>> +
>> +     value = tt->powered;
>> +     ofono_dbus_dict_append(&dict, "Powered", DBUS_TYPE_BOOLEAN, &value);
>
> As mentioned previously, please use "Enabled" here.

ok

>> +static void tt_send_properties_reply(struct ofono_text_telephony *tt)
>> +{
>> +     DBusMessage *reply;
>> +
>> +     tt->flags |= TEXT_TELEPHONY_FLAG_CACHED;
>> +
>> +     reply = tt_get_properties_reply(tt->pending, tt);
>> +     __ofono_dbus_pending_reply(&tt->pending, reply);
>
> I actually prefer to fold this function into powered_callback

ok

>
>> +}
>> +
>> +static void tt_query_powered_callback(const struct ofono_error *error,
>> +                                             ofono_bool_t enable, void *data)
>> +{
>> +     struct ofono_text_telephony *tt = data;
>> +
>> +     if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>> +             DBusMessage *reply;
>> +
>> +             ofono_debug("Error during powered query");
>> +
>> +             reply = __ofono_error_failed(tt->pending);
>> +             __ofono_dbus_pending_reply(&tt->pending, reply);
>> +
>> +             return;
>> +     }
>> +
>> +     tt_set_powered(tt, enable);
>> +     tt_send_properties_reply(tt);
>
> The oFono convention is to reply to the pending D-Bus method call and
> then signal the property changed signals.
>

The examples I found for query methods use exactly the opposite. See,
cw_ss_query_callback() in call-settings.c and
radio_fast_dormancy_query_callback() in radio-settings.c.

This is because when calling the "tt_get_properties_reply(tt->pending,
tt)" we need the new value in tt->powered in order to generate the
reply correctly. Otherwise, we would need to save the value elsewhere,
set with the new value, generate the message, restore the previous
value and then call tt_set_powered(tt, enable).

If we do this, maybe we have to change the other places too.

>
>> +int ofono_text_telephony_driver_register(const struct ofono_text_telephony_driver *d)
>> +{
>> +     DBG("driver: %p, name: %s", d, d->name);
>> +
>> +     if (!d || !d->probe)
>> +             return -EINVAL;
>> +
>> +     g_drivers = g_slist_prepend(g_drivers, (void *)d);
>> +
>> +     return 0;
>> +}
>> +
>> +void ofono_text_telephony_driver_unregister(const struct ofono_text_telephony_driver *d)
>> +{
>> +     DBG("driver: %p, name: %s", d, d->name);
>> +
>> +     if (!d)
>
> The preferred style is to compare to NULL, e.g. if (d == NULL)

Ok. I changed all the other places as well.

>
>> +             return;
>> +
>> +     g_drivers = g_slist_remove(g_drivers, (void *)d);
>> +}
>> +
>
> <snip>
>
>> +struct ofono_text_telephony *ofono_text_telephony_create(struct ofono_modem *modem,
>> +                                                     unsigned int vendor,
>> +                                                     const char *driver,
>> +                                                     void *data)
>> +{
>> +     struct ofono_text_telephony *tt;
>> +     GSList *l;
>
> Please add a newline here

ok

>
>> +     if (!driver)
>> +             return NULL;
>> +
>> +     tt = g_try_new0(struct ofono_text_telephony, 1);
>> +     if (!tt)
>> +             return NULL;
>> +
>> +     tt->atom = __ofono_modem_add_atom(modem, OFONO_ATOM_TYPE_TEXT_TELEPHONY,
>> +                                             text_telephony_remove, tt);
>> +
>> +     tt->powered = -1;
>
> I think it is safe to default this to 0 (off).

ok

>
>> +
>> +     for (l = g_drivers; l; l = l->next) {
>> +             const struct ofono_text_telephony_driver *drv = l->data;
>> +
>> +             if (g_strcmp0(drv->name, driver) != 0)
>> +                     continue;
>> +
>> +             if (drv->probe(tt, vendor, data) < 0)
>> +                     continue;
>> +
>> +             tt->driver = drv;
>> +             break;
>> +     }
>> +
>> +     return tt;
>> +}
>> +
>
> Otherwise, looks good.

regards,

Lucas De Marchi

Re: [PATCH v2 4/7] text-telephony: implement interface/atom

[Denis Kenzior]

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

Hi Lucas,

>>> +     tt_set_powered(tt, enable);
>>> +     tt_send_properties_reply(tt);
>>
>> The oFono convention is to reply to the pending D-Bus method call and
>> then signal the property changed signals.
>>
> 
> The examples I found for query methods use exactly the opposite. See,
> cw_ss_query_callback() in call-settings.c and
> radio_fast_dormancy_query_callback() in radio-settings.c.

So we do prefer the reply before the signal.  However, we're still a bit
inconsistent in this area because multi-property queries make this very
hard.  In the single property case it is doable, but I'm fine accepting
this part as is.  So feel free to ignore this feedback.

Regards,
-Denis

RE: [PATCH v2 5/7] text-telephony: add documentation

[Kai.Vehmanen]

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

Hi,

On 23 Nov 2010, Lucas De Marchi wrote:
> +Interface	org.ofono.TextTelephony
[...]
> +Properties	boolean	Powered [readwrite]
> +
> +			This property will enable or disable the text
> +			telephony feature in the modem.
> +
> +			Text telephony (TTY), also known as TDD, is a feature
> +			present in some modems that allow them to be used by
> +			hearing impaired people.

just to clarify, I guess this covers all aspects of TTY
support handled by the modem? 

E.g. for modems, TTY/CTM support can mean:
- signaling TTY/CTM support as a capability (e.g. in Bearer Capability 
  of 24.008 at the start of calls) ->all modems that support TTY
  do this
- above plus: performing CTM coding on modem ->some modems
- above plus: handling audio input/output  ->some modems

So depending on the modem, enabling TTY with this new oFono 
interface enables all of the above (those the apply), right? 

This makes sense as AFAIK there is no need to control the different
aspects separately (and is analogous to handling voice calls
in general with oFono), but just making sure I understand 
the semantics correctly.

If in the future ofono clients need more information about the speech 
data path processing (w.r.t. TTY), that can be then added to
org.ofono.AudioSettings interface (e.g. whether modem does 
CTM coding, whether it supports HCO/VCO (does voice go through
at all when TTY mode is enabled). But until we have a modem (and
use case) that needs this, it's too early to add these. E.g. Nokia 
N900 does not support TTY, but if the support was added, in theory 
it could technically do CTM coding both on modem and/or on Linux 
side.

br,
-- 
Kai Vehmanen

Re: [PATCH v2 4/7] text-telephony: implement interface/atom

[Lucas De Marchi]

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

Hi Denis

On Wed, Nov 24, 2010 at 7:29 PM, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Lucas,
>
>>>> +     tt_set_powered(tt, enable);
>>>> +     tt_send_properties_reply(tt);
>>>
>>> The oFono convention is to reply to the pending D-Bus method call and
>>> then signal the property changed signals.
>>>
>>
>> The examples I found for query methods use exactly the opposite. See,
>> cw_ss_query_callback() in call-settings.c and
>> radio_fast_dormancy_query_callback() in radio-settings.c.
>
> So we do prefer the reply before the signal.  However, we're still a bit
> inconsistent in this area because multi-property queries make this very
> hard.  In the single property case it is doable, but I'm fine accepting
> this part as is.  So feel free to ignore this feedback.

HUmn... If this is the correct approach, I'm ok with doing it.

regards,
Lucas De Marchi