[PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Dara Spieker-Doyle]

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

---
 Makefile.am          |    3 +-
 src/cdma-voicecall.c |  449 ++++++++++++++++++++++++++++++++++++++++++++++++++
 src/common.c         |   42 +++++
 src/common.h         |   14 ++
 src/ofono.h          |    3 +
 5 files changed, 510 insertions(+), 1 deletions(-)
 create mode 100644 src/cdma-voicecall.c

diff --git a/Makefile.am b/Makefile.am
index aea3fd3..f111f96 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -319,7 +319,8 @@ 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/ctm.c
+			src/smsagent.c src/smsagent.h src/ctm.c \
+			src/cdma-voicecall.c
 
 src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl
 
diff --git a/src/cdma-voicecall.c b/src/cdma-voicecall.c
new file mode 100644
index 0000000..8ba3222
--- /dev/null
+++ b/src/cdma-voicecall.c
@@ -0,0 +1,449 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2010 Nokia 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 <time.h>
+#include <errno.h>
+#include <stdint.h>
+
+#include <glib.h>
+#include <gdbus.h>
+
+#include "ofono.h"
+
+#include "common.h"
+
+static GSList *g_drivers;
+
+struct ofono_cdma_voicecall_manager {
+	struct ofono_cdma_phone_number phone_number;
+	int direction;
+	enum cdma_call_status status;
+	time_t start_time;
+	DBusMessage *pending;
+	const struct ofono_cdma_voicecall_manager_driver *driver;
+	void *driver_data;
+	struct ofono_atom *atom;
+};
+
+static void generic_callback(const struct ofono_error *error, void *data);
+
+static const char *disconnect_reason_to_string(enum ofono_disconnect_reason r)
+{
+	switch (r) {
+	case OFONO_DISCONNECT_REASON_LOCAL_HANGUP:
+		return "local";
+	case OFONO_DISCONNECT_REASON_REMOTE_HANGUP:
+		return "remote";
+	default:
+		return "network";
+	}
+}
+
+static const char *cdma_call_status_to_string(enum cdma_call_status status)
+{
+	switch (status) {
+	case CDMA_CALL_STATUS_ACTIVE:
+		return "active";
+	case CDMA_CALL_STATUS_DIALING:
+		return "dialing";
+	case CDMA_CALL_STATUS_ALERTING:
+		return "alerting";
+	case CDMA_CALL_STATUS_INCOMING:
+		return "incoming";
+	default:
+		return "disconnected";
+	}
+}
+
+static const char *time_to_str(const time_t *t)
+{
+	static char buf[128];
+	struct tm tm;
+
+	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", localtime_r(t, &tm));
+	buf[127] = '\0';
+
+	return buf;
+}
+
+static void append_voicecall_properties(struct ofono_cdma_voicecall_manager *v,
+					DBusMessageIter *dict)
+{
+	const char *status;
+	const char *lineid;
+	const char *timestr;
+
+	status = cdma_call_status_to_string(v->status);
+	lineid = cdma_phone_number_to_string(&v->phone_number);
+
+	ofono_dbus_dict_append(dict, "State", DBUS_TYPE_STRING, &status);
+
+	ofono_dbus_dict_append(dict, "LineIdentification",
+				DBUS_TYPE_STRING, &lineid);
+
+	if (v->status == CDMA_CALL_STATUS_ACTIVE ||
+			(v->status == CDMA_CALL_STATUS_DISCONNECTED &&
+				v->start_time != 0)) {
+		timestr = time_to_str(&v->start_time);
+
+		ofono_dbus_dict_append(dict, "StartTime", DBUS_TYPE_STRING,
+					&timestr);
+	}
+}
+
+static DBusMessage *voicecall_manager_get_properties(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	struct ofono_cdma_voicecall_manager *v = data;
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+
+	reply = dbus_message_new_method_return(msg);
+
+	if (reply == NULL)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					&dict);
+	append_voicecall_properties(v, &dict);
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static const char *voicecall_build_path(struct ofono_cdma_voicecall_manager *vc)
+{
+	static char path[256];
+
+	snprintf(path, sizeof(path), "%s/voicecall",
+			__ofono_atom_get_path(vc->atom));
+
+	return path;
+}
+
+static void voicecall_emit_disconnect_reason(
+		struct ofono_cdma_voicecall_manager *vc,
+		enum ofono_disconnect_reason reason)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path;
+	const char *reason_str;
+
+	reason_str = disconnect_reason_to_string(reason);
+	path = voicecall_build_path(vc);
+
+	g_dbus_emit_signal(conn, path, OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
+				"DisconnectReason",
+				DBUS_TYPE_STRING, &reason_str,
+				DBUS_TYPE_INVALID);
+}
+
+static void voicecall_set_call_status(struct ofono_cdma_voicecall_manager *vc,
+		enum cdma_call_status status)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path;
+	const char *status_str;
+	enum cdma_call_status old_status;
+
+	if (vc->status == status)
+		return;
+
+	old_status = vc->status;
+
+	vc->status = status;
+
+	status_str = cdma_call_status_to_string(status);
+	path = voicecall_build_path(vc);
+
+	ofono_dbus_signal_property_changed(conn, path,
+					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
+					"State", DBUS_TYPE_STRING,
+					&status_str);
+
+	if (status == CDMA_CALL_STATUS_ACTIVE &&
+			old_status == CDMA_CALL_STATUS_DIALING) {
+		const char *timestr;
+
+		vc->start_time = time(NULL);
+		timestr = time_to_str(&vc->start_time);
+
+		ofono_dbus_signal_property_changed(conn, path,
+					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
+					"StartTime", DBUS_TYPE_STRING,
+					&timestr);
+	}
+}
+
+static void voicecall_set_call_lineid(struct ofono_cdma_voicecall_manager *v)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	const char *path;
+	const char *lineid_str;
+
+	path = voicecall_build_path(v);
+
+	/* For MO calls, LineID is the dialed phone number */
+	lineid_str = cdma_phone_number_to_string(&v->phone_number);
+
+	ofono_dbus_signal_property_changed(conn, path,
+					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
+					"LineIdentification",
+					DBUS_TYPE_STRING, &lineid_str);
+}
+
+static void manager_dial_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_cdma_voicecall_manager *vc = data;
+	DBusMessage *reply;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		reply = __ofono_error_failed(vc->pending);
+	} else {
+		const char *path = voicecall_build_path(vc);
+
+		reply = dbus_message_new_method_return(vc->pending);
+
+		dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
+						DBUS_TYPE_INVALID);
+	}
+
+	__ofono_dbus_pending_reply(&vc->pending, reply);
+}
+
+static DBusMessage *voicecall_manager_dial(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_cdma_voicecall_manager *vc = data;
+	const char *number;
+
+	if (vc->pending)
+		return __ofono_error_busy(msg);
+
+	if (vc->status != CDMA_CALL_STATUS_DISCONNECTED)
+		return __ofono_error_failed(msg);
+
+	if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &number,
+					DBUS_TYPE_INVALID) == FALSE)
+		return __ofono_error_invalid_args(msg);
+
+	if (!valid_cdma_phone_number_format(number))
+		return __ofono_error_invalid_format(msg);
+
+	if (vc->driver->dial == NULL)
+		return __ofono_error_not_implemented(msg);
+
+	vc->pending = dbus_message_ref(msg);
+
+	string_to_cdma_phone_number(number, &vc->phone_number);
+	voicecall_set_call_lineid(vc);
+	vc->direction = CALL_DIRECTION_MOBILE_ORIGINATED;
+	voicecall_set_call_status(vc, CDMA_CALL_STATUS_DIALING);
+	vc->driver->dial(vc, &vc->phone_number, manager_dial_callback, vc);
+
+	return NULL;
+}
+
+static DBusMessage *voicecall_manager_hangup(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_cdma_voicecall_manager *vc = data;
+
+	if (vc->pending)
+		return __ofono_error_busy(msg);
+
+	if (vc->driver->hangup == NULL)
+		return __ofono_error_not_implemented(msg);
+
+	if (vc->status == CDMA_CALL_STATUS_DISCONNECTED)
+		return __ofono_error_failed(msg);
+
+	vc->pending = dbus_message_ref(msg);
+
+	vc->driver->hangup(vc, generic_callback, vc);
+
+	return NULL;
+}
+
+static GDBusMethodTable manager_methods[] = {
+	{ "GetProperties",    "",    "a{sv}",
+					voicecall_manager_get_properties },
+	{ "Dial",             "ss",  "o",        voicecall_manager_dial,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ "Hangup",           "",    "",         voicecall_manager_hangup,
+						G_DBUS_METHOD_FLAG_ASYNC },
+	{ }
+};
+
+static GDBusSignalTable manager_signals[] = {
+	{ "PropertyChanged",	"sv" },
+	{ "DisconnectReason",	"s" },
+	{ }
+};
+
+void ofono_cdma_voicecall_manager_disconnected(
+					struct ofono_cdma_voicecall_manager *vc,
+					enum ofono_disconnect_reason reason,
+					const struct ofono_error *error)
+{
+	DBG("Got disconnection event for reason: %d", reason);
+
+	if (reason != OFONO_DISCONNECT_REASON_UNKNOWN)
+		voicecall_emit_disconnect_reason(vc, reason);
+
+	voicecall_set_call_status(vc, CDMA_CALL_STATUS_DISCONNECTED);
+}
+
+static void generic_callback(const struct ofono_error *error, void *data)
+{
+	struct ofono_cdma_voicecall_manager *vc = data;
+	DBusMessage *reply;
+
+	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
+		reply = dbus_message_new_method_return(vc->pending);
+	else
+		reply = __ofono_error_failed(vc->pending);
+
+	__ofono_dbus_pending_reply(&vc->pending, reply);
+}
+
+int ofono_cdma_voicecall_manager_driver_register(
+		const struct ofono_cdma_voicecall_manager_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	if (d->probe == NULL)
+		return -EINVAL;
+
+	g_drivers = g_slist_prepend(g_drivers, (void *)d);
+
+	return 0;
+}
+
+void ofono_cdma_voicecall_manager_driver_unregister(
+		const struct ofono_cdma_voicecall_manager_driver *d)
+{
+	DBG("driver: %p, name: %s", d, d->name);
+
+	g_drivers = g_slist_remove(g_drivers, (void *)d);
+}
+
+
+static void voicecall_manager_remove(struct ofono_atom *atom)
+{
+	struct ofono_cdma_voicecall_manager *vc = __ofono_atom_get_data(atom);
+
+	DBG("atom: %p", atom);
+
+	if (vc == NULL)
+		return;
+
+	if (vc->driver && vc->driver->remove)
+		vc->driver->remove(vc);
+
+	g_free(vc);
+}
+
+struct ofono_cdma_voicecall_manager *ofono_cdma_voicecall_manager_create(
+						struct ofono_modem *modem,
+						unsigned int vendor,
+						const char *driver,
+						void *data)
+{
+	struct ofono_cdma_voicecall_manager *vc;
+	GSList *l;
+
+	if (driver == NULL)
+		return NULL;
+
+	vc = g_try_new0(struct ofono_cdma_voicecall_manager, 1);
+	if (vc == NULL)
+		return NULL;
+
+	vc->status = CDMA_CALL_STATUS_DISCONNECTED;
+
+	vc->atom = __ofono_modem_add_atom(modem,
+			OFONO_ATOM_TYPE_CDMA_VOICECALL_MANAGER,
+			voicecall_manager_remove, vc);
+
+	for (l = g_drivers; l; l = l->next) {
+		const struct ofono_cdma_voicecall_manager_driver *drv = l->data;
+
+		if (g_strcmp0(drv->name, driver))
+			continue;
+
+		if (drv->probe(vc, vendor, data) < 0)
+			continue;
+
+		vc->driver = drv;
+		break;
+	}
+
+	return vc;
+}
+
+void ofono_cdma_voicecall_manager_register(
+				struct ofono_cdma_voicecall_manager *vc)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
+	const char *path = __ofono_atom_get_path(vc->atom);
+
+	if (!g_dbus_register_interface(conn, path,
+					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
+					manager_methods, manager_signals, NULL,
+					vc, NULL)) {
+		ofono_error("Could not create %s interface",
+				OFONO_CDMA_VOICECALL_MANAGER_INTERFACE);
+		return;
+	}
+
+	ofono_modem_add_interface(modem,
+			OFONO_CDMA_VOICECALL_MANAGER_INTERFACE);
+}
+
+void ofono_cdma_voicecall_manager_remove(
+			struct ofono_cdma_voicecall_manager *vc)
+{
+	__ofono_atom_free(vc->atom);
+}
+
+void ofono_cdma_voicecall_manager_set_data(
+			struct ofono_cdma_voicecall_manager *vc, void *data)
+{
+	vc->driver_data = data;
+}
+
+void *ofono_cdma_voicecall_manager_get_data(
+			struct ofono_cdma_voicecall_manager *vc)
+{
+	return vc->driver_data;
+}
diff --git a/src/common.c b/src/common.c
index 5154b8d..7aa63ce 100644
--- a/src/common.c
+++ b/src/common.c
@@ -262,6 +262,31 @@ gboolean valid_phone_number_format(const char *number)
 	return TRUE;
 }
 
+gboolean valid_cdma_phone_number_format(const char *number)
+{
+	int len = strlen(number);
+	int begin = 0;
+	int i;
+
+	if (!len)
+		return FALSE;
+
+	if ((len - begin) > OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH)
+		return FALSE;
+
+	for (i = begin; i < len; i++) {
+		if (number[i] >= '0' && number[i] <= '9')
+			continue;
+
+		if (number[i] == '*' || number[i] == '#')
+			continue;
+
+		return FALSE;
+	}
+
+	return TRUE;
+}
+
 const char *telephony_error_to_str(const struct ofono_error *error)
 {
 	struct error_entry *e;
@@ -405,6 +430,23 @@ void string_to_phone_number(const char *str, struct ofono_phone_number *ph)
 	}
 }
 
+const char *cdma_phone_number_to_string(
+			const struct ofono_cdma_phone_number *ph)
+{
+	static char buffer[OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH + 1];
+
+	strncpy(buffer, ph->number, OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH);
+	buffer[OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH] = '\0';
+
+	return buffer;
+}
+
+void string_to_cdma_phone_number(const char *str,
+			struct ofono_cdma_phone_number *ph)
+{
+	strcpy(ph->number, str);
+}
+
 gboolean valid_ussd_string(const char *str, gboolean call_in_progress)
 {
 	int len = strlen(str);
diff --git a/src/common.h b/src/common.h
index 8b5798a..df0adcd 100644
--- a/src/common.h
+++ b/src/common.h
@@ -59,6 +59,14 @@ enum call_status {
 	CALL_STATUS_DISCONNECTED
 };
 
+enum cdma_call_status {
+	CDMA_CALL_STATUS_ACTIVE = 0,
+	CDMA_CALL_STATUS_DIALING = 1,
+	CDMA_CALL_STATUS_ALERTING = 2,
+	CDMA_CALL_STATUS_INCOMING = 4,
+	CDMA_CALL_STATUS_DISCONNECTED
+};
+
 /* 27.007 Section 7.18 */
 enum call_direction {
 	CALL_DIRECTION_MOBILE_ORIGINATED = 0,
@@ -128,6 +136,12 @@ gboolean valid_phone_number_format(const char *number);
 const char *phone_number_to_string(const struct ofono_phone_number *ph);
 void string_to_phone_number(const char *str, struct ofono_phone_number *ph);
 
+gboolean valid_cdma_phone_number_format(const char *number);
+const char *cdma_phone_number_to_string(
+				const struct ofono_cdma_phone_number *ph);
+void string_to_cdma_phone_number(const char *str,
+				struct ofono_cdma_phone_number *ph);
+
 int mmi_service_code_to_bearer_class(int code);
 
 gboolean valid_ussd_string(const char *str, gboolean call_in_progress);
diff --git a/src/ofono.h b/src/ofono.h
index 792134b..cab70cd 100644
--- a/src/ofono.h
+++ b/src/ofono.h
@@ -126,6 +126,7 @@ enum ofono_atom_type {
 	OFONO_ATOM_TYPE_STK = 20,
 	OFONO_ATOM_TYPE_NETTIME = 21,
 	OFONO_ATOM_TYPE_CTM = 22,
+	OFONO_ATOM_TYPE_CDMA_VOICECALL_MANAGER = 23,
 };
 
 enum ofono_atom_watch_condition {
@@ -415,3 +416,5 @@ void __ofono_nettime_probe_drivers(struct ofono_modem *modem);
 
 void __ofono_nettime_info_received(struct ofono_modem *modem,
 					struct ofono_network_time *info);
+
+#include <ofono/cdma-voicecall.h>
-- 
1.7.0.4

RE: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Rajesh.Nagaiah]

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

Hi Dara, 
  
> +enum cdma_call_status {
> +	CDMA_CALL_STATUS_ACTIVE = 0,
> +	CDMA_CALL_STATUS_DIALING = 1,
> +	CDMA_CALL_STATUS_ALERTING = 2,
> +	CDMA_CALL_STATUS_INCOMING = 4,

Should be 3 ?

> +	CDMA_CALL_STATUS_DISCONNECTED
> +};

We can use the existing gsm call status itself ?
Just that we wont use the HELD and WAITING status
in CDMA.

BR,
Rajesh
 

RE: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Dara Spieker-Doyle]

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

Hi Rajesh

On Fri, 2010-12-03 at 01:30 +0100, ext Rajesh.Nagaiah(a)elektrobit.com
wrote:

> > +enum cdma_call_status {
> > +	CDMA_CALL_STATUS_ACTIVE = 0,
> > +	CDMA_CALL_STATUS_DIALING = 1,
> > +	CDMA_CALL_STATUS_ALERTING = 2,
> > +	CDMA_CALL_STATUS_INCOMING = 4,
> 
> Should be 3 ?

Yes indeed. I will fix this, thank you for catching it.
 
> > +	CDMA_CALL_STATUS_DISCONNECTED
> > +};
> 
> We can use the existing gsm call status itself ?
> Just that we wont use the HELD and WAITING status
> in CDMA.

In this early phase of CDMA support in oFono, we would like to evolve it
in its own right for a while, per the offline conversation from the
MeeGo Conference in Dublin. On a case by case basis, for items of large
architectural impact, we intend to evaluate potential re-use upfront.
The plan is to allow smaller items like this for now, until the related
feature has matured sufficiently that they can be re-factored correctly
if applicable.

Cheers
Dara

RE: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Rajesh.Nagaiah]

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


Hi Dara,

> -----Original Message-----
> From: ofono-bounces(a)ofono.org 
> [mailto:ofono-bounces(a)ofono.org] On Behalf Of Dara Spieker-Doyle
> Sent: 03 December 2010 13:21
 
> > > +enum cdma_call_status {
> > > +	CDMA_CALL_STATUS_ACTIVE = 0,
> > > +	CDMA_CALL_STATUS_DIALING = 1,
> > > +	CDMA_CALL_STATUS_ALERTING = 2,
> > > +	CDMA_CALL_STATUS_INCOMING = 4,
> > 
> > Should be 3 ?
> 
> Yes indeed. I will fix this, thank you for catching it.
>  
> > > +	CDMA_CALL_STATUS_DISCONNECTED
> > > +};
> > 
> > We can use the existing gsm call status itself ?
> > Just that we wont use the HELD and WAITING status in CDMA.
> 
> In this early phase of CDMA support in oFono, we would like 
> to evolve it in its own right for a while, per the offline 
> conversation from the MeeGo Conference in Dublin. On a case 
> by case basis, for items of large architectural impact, we 
> intend to evaluate potential re-use upfront.
> The plan is to allow smaller items like this for now, until 
> the related feature has matured sufficiently that they can be 
> re-factored correctly if applicable.
 
I agree with the fact that we should evolve it in its own right.
But with these kind of straight forward cases, where the GSM values
are a superset of the CDMA values and these value definitions being
internal to ofono, we should try to to reuse the values rather than
creating new ones exclusive for CDMA.
Thats my view, Denis/Marcel any comments ?

BR,
Rajesh

Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Denis Kenzior]

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

Hi Rajesh,

On 12/03/2010 03:34 PM, Rajesh.Nagaiah(a)elektrobit.com wrote:
> 
> Hi Dara,
> 
>> -----Original Message-----
>> From: ofono-bounces(a)ofono.org 
>> [mailto:ofono-bounces(a)ofono.org] On Behalf Of Dara Spieker-Doyle
>> Sent: 03 December 2010 13:21
>  
>>>> +enum cdma_call_status {
>>>> +	CDMA_CALL_STATUS_ACTIVE = 0,
>>>> +	CDMA_CALL_STATUS_DIALING = 1,
>>>> +	CDMA_CALL_STATUS_ALERTING = 2,
>>>> +	CDMA_CALL_STATUS_INCOMING = 4,
>>>
>>> Should be 3 ?
>>
>> Yes indeed. I will fix this, thank you for catching it.
>>  
>>>> +	CDMA_CALL_STATUS_DISCONNECTED
>>>> +};
>>>
>>> We can use the existing gsm call status itself ?
>>> Just that we wont use the HELD and WAITING status in CDMA.
>>
>> In this early phase of CDMA support in oFono, we would like 
>> to evolve it in its own right for a while, per the offline 
>> conversation from the MeeGo Conference in Dublin. On a case 
>> by case basis, for items of large architectural impact, we 
>> intend to evaluate potential re-use upfront.
>> The plan is to allow smaller items like this for now, until 
>> the related feature has matured sufficiently that they can be 
>> re-factored correctly if applicable.
>  
> I agree with the fact that we should evolve it in its own right.
> But with these kind of straight forward cases, where the GSM values
> are a superset of the CDMA values and these value definitions being
> internal to ofono, we should try to to reuse the values rather than
> creating new ones exclusive for CDMA.
> Thats my view, Denis/Marcel any comments ?
> 

So the general rule of thumb has been to use an int when a spec clearly
defines the meaning of the said int.  E.g. call status int values have
very clear meaning from 27.007.  If no such clear definition exists,
then an enum should be used.

So the question here becomes whether the CDMA modems all re-use the GSM
meanings / values for these states or not?  If they do, then re-using
the GSM enum values makes sense.

If not, then using an enum defined in include/cdma-voicecall.h would be
better.  One major benefit of defining a dedicated enum for CDMA is the
compiler checking that all enum values are being handled.  If you re-use
the GSM version you would have to resort to using default statements.
This causes you to lose that extra compiler sanity checking, not to
mention is against rule M12 of the coding style.  And yes I know we're
not always consistent with this one ;)

Regards,
-Denis

Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Dara Spieker-Doyle]

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

Hi Denis

On Tue, 2010-12-07 at 04:16 +0100, ext Denis Kenzior wrote:
> Hi Rajesh,
> 
> On 12/03/2010 03:34 PM, Rajesh.Nagaiah(a)elektrobit.com wrote:
> > 
> > Hi Dara,
> > 
> >> -----Original Message-----
> >> From: ofono-bounces(a)ofono.org 
> >> [mailto:ofono-bounces(a)ofono.org] On Behalf Of Dara Spieker-Doyle
> >> Sent: 03 December 2010 13:21
> >  
> >>>> +enum cdma_call_status {
> >>>> +	CDMA_CALL_STATUS_ACTIVE = 0,
> >>>> +	CDMA_CALL_STATUS_DIALING = 1,
> >>>> +	CDMA_CALL_STATUS_ALERTING = 2,
> >>>> +	CDMA_CALL_STATUS_INCOMING = 4,
> >>>
> >>> Should be 3 ?
> >>
> >> Yes indeed. I will fix this, thank you for catching it.
> >>  
> >>>> +	CDMA_CALL_STATUS_DISCONNECTED
> >>>> +};
> >>>
> >>> We can use the existing gsm call status itself ?
> >>> Just that we wont use the HELD and WAITING status in CDMA.
> >>
> >> In this early phase of CDMA support in oFono, we would like 
> >> to evolve it in its own right for a while, per the offline 
> >> conversation from the MeeGo Conference in Dublin. On a case 
> >> by case basis, for items of large architectural impact, we 
> >> intend to evaluate potential re-use upfront.
> >> The plan is to allow smaller items like this for now, until 
> >> the related feature has matured sufficiently that they can be 
> >> re-factored correctly if applicable.
> >  
> > I agree with the fact that we should evolve it in its own right.
> > But with these kind of straight forward cases, where the GSM values
> > are a superset of the CDMA values and these value definitions being
> > internal to ofono, we should try to to reuse the values rather than
> > creating new ones exclusive for CDMA.
> > Thats my view, Denis/Marcel any comments ?
> > 
> 
> So the general rule of thumb has been to use an int when a spec clearly
> defines the meaning of the said int.  E.g. call status int values have
> very clear meaning from 27.007.  If no such clear definition exists,
> then an enum should be used.

The 3GPP2 does not provide clear definitions, certainly not as thorough
as 27.007
> 
> So the question here becomes whether the CDMA modems all re-use the GSM
> meanings / values for these states or not?  If they do, then re-using
> the GSM enum values makes sense.

As there is no clear 3GPP2 standard for call states, we cannot guarantee
that CDMA modems would all re-use the GSM meanings.
> 
> If not, then using an enum defined in include/cdma-voicecall.h would be
> better.  One major benefit of defining a dedicated enum for CDMA is the
> compiler checking that all enum values are being handled.  If you re-use
> the GSM version you would have to resort to using default statements.
> This causes you to lose that extra compiler sanity checking, not to
> mention is against rule M12 of the coding style.  And yes I know we're
> not always consistent with this one ;)
> 

This is our preference- we can move the cdma enum from /src/common.h
into include/cdma-voicecall.h if you would prefer that location?

Cheers
Dara

Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Denis Kenzior]

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

Hi Dara,

>> If not, then using an enum defined in include/cdma-voicecall.h would be
>> better.  One major benefit of defining a dedicated enum for CDMA is the
>> compiler checking that all enum values are being handled.  If you re-use
>> the GSM version you would have to resort to using default statements.
>> This causes you to lose that extra compiler sanity checking, not to
>> mention is against rule M12 of the coding style.  And yes I know we're
>> not always consistent with this one ;)
>>
> 
> This is our preference- we can move the cdma enum from /src/common.h
> into include/cdma-voicecall.h if you would prefer that location?

I'm fine with doing that.

Regards,
-Denis

Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Denis Kenzior]

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

Hi Dara,

On 12/02/2010 05:31 PM, Dara Spieker-Doyle wrote:
> ---
>  Makefile.am          |    3 +-
>  src/cdma-voicecall.c |  449 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  src/common.c         |   42 +++++
>  src/common.h         |   14 ++
>  src/ofono.h          |    3 +
>  5 files changed, 510 insertions(+), 1 deletions(-)
>  create mode 100644 src/cdma-voicecall.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index aea3fd3..f111f96 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -319,7 +319,8 @@ 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/ctm.c
> +			src/smsagent.c src/smsagent.h src/ctm.c \
> +			src/cdma-voicecall.c
>  
>  src_ofonod_LDADD = $(builtin_libadd) @GLIB_LIBS@ @DBUS_LIBS@ @CAPNG_LIBS@ -ldl
>  
> diff --git a/src/cdma-voicecall.c b/src/cdma-voicecall.c
> new file mode 100644
> index 0000000..8ba3222
> --- /dev/null
> +++ b/src/cdma-voicecall.c
> @@ -0,0 +1,449 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2010 Nokia 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 <time.h>
> +#include <errno.h>
> +#include <stdint.h>
> +
> +#include <glib.h>
> +#include <gdbus.h>
> +
> +#include "ofono.h"
> +
> +#include "common.h"
> +
> +static GSList *g_drivers;
> +
> +struct ofono_cdma_voicecall_manager {
> +	struct ofono_cdma_phone_number phone_number;
> +	int direction;
> +	enum cdma_call_status status;
> +	time_t start_time;
> +	DBusMessage *pending;
> +	const struct ofono_cdma_voicecall_manager_driver *driver;
> +	void *driver_data;
> +	struct ofono_atom *atom;
> +};
> +
> +static void generic_callback(const struct ofono_error *error, void *data);
> +
> +static const char *disconnect_reason_to_string(enum ofono_disconnect_reason r)
> +{
> +	switch (r) {
> +	case OFONO_DISCONNECT_REASON_LOCAL_HANGUP:
> +		return "local";
> +	case OFONO_DISCONNECT_REASON_REMOTE_HANGUP:
> +		return "remote";
> +	default:
> +		return "network";
> +	}
> +}
> +
> +static const char *cdma_call_status_to_string(enum cdma_call_status status)
> +{
> +	switch (status) {
> +	case CDMA_CALL_STATUS_ACTIVE:
> +		return "active";
> +	case CDMA_CALL_STATUS_DIALING:
> +		return "dialing";
> +	case CDMA_CALL_STATUS_ALERTING:
> +		return "alerting";
> +	case CDMA_CALL_STATUS_INCOMING:
> +		return "incoming";
> +	default:
> +		return "disconnected";
> +	}
> +}
> +
> +static const char *time_to_str(const time_t *t)
> +{
> +	static char buf[128];
> +	struct tm tm;
> +
> +	strftime(buf, 127, "%Y-%m-%dT%H:%M:%S%z", localtime_r(t, &tm));
> +	buf[127] = '\0';
> +
> +	return buf;
> +}
> +
> +static void append_voicecall_properties(struct ofono_cdma_voicecall_manager *v,
> +					DBusMessageIter *dict)
> +{
> +	const char *status;
> +	const char *lineid;
> +	const char *timestr;
> +
> +	status = cdma_call_status_to_string(v->status);
> +	lineid = cdma_phone_number_to_string(&v->phone_number);
> +
> +	ofono_dbus_dict_append(dict, "State", DBUS_TYPE_STRING, &status);
> +
> +	ofono_dbus_dict_append(dict, "LineIdentification",
> +				DBUS_TYPE_STRING, &lineid);
> +
> +	if (v->status == CDMA_CALL_STATUS_ACTIVE ||
> +			(v->status == CDMA_CALL_STATUS_DISCONNECTED &&
> +				v->start_time != 0)) {

Honestly I think you can drop the second or condition, otherwise you
will include the StartTime attribute even when no calls are connected.
This condition should probably be removed in src/voicecall.c as well...

> +		timestr = time_to_str(&v->start_time);
> +
> +		ofono_dbus_dict_append(dict, "StartTime", DBUS_TYPE_STRING,
> +					&timestr);
> +	}
> +}
> +
> +static DBusMessage *voicecall_manager_get_properties(DBusConnection *conn,
> +						DBusMessage *msg, void *data)
> +{
> +	struct ofono_cdma_voicecall_manager *v = data;
> +	DBusMessage *reply;
> +	DBusMessageIter iter;
> +	DBusMessageIter dict;
> +
> +	reply = dbus_message_new_method_return(msg);
> +
> +	if (reply == NULL)
> +		return NULL;
> +
> +	dbus_message_iter_init_append(reply, &iter);
> +
> +	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
> +					OFONO_PROPERTIES_ARRAY_SIGNATURE,
> +					&dict);
> +	append_voicecall_properties(v, &dict);
> +	dbus_message_iter_close_container(&iter, &dict);
> +
> +	return reply;
> +}
> +
> +static const char *voicecall_build_path(struct ofono_cdma_voicecall_manager *vc)
> +{
> +	static char path[256];
> +
> +	snprintf(path, sizeof(path), "%s/voicecall",
> +			__ofono_atom_get_path(vc->atom));
> +
> +	return path;
> +}
> +
> +static void voicecall_emit_disconnect_reason(
> +		struct ofono_cdma_voicecall_manager *vc,
> +		enum ofono_disconnect_reason reason)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path;
> +	const char *reason_str;
> +
> +	reason_str = disconnect_reason_to_string(reason);
> +	path = voicecall_build_path(vc);
> +
> +	g_dbus_emit_signal(conn, path, OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> +				"DisconnectReason",
> +				DBUS_TYPE_STRING, &reason_str,
> +				DBUS_TYPE_INVALID);
> +}
> +
> +static void voicecall_set_call_status(struct ofono_cdma_voicecall_manager *vc,
> +		enum cdma_call_status status)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path;
> +	const char *status_str;
> +	enum cdma_call_status old_status;
> +
> +	if (vc->status == status)
> +		return;
> +
> +	old_status = vc->status;
> +
> +	vc->status = status;
> +
> +	status_str = cdma_call_status_to_string(status);
> +	path = voicecall_build_path(vc);
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> +					"State", DBUS_TYPE_STRING,
> +					&status_str);
> +
> +	if (status == CDMA_CALL_STATUS_ACTIVE &&
> +			old_status == CDMA_CALL_STATUS_DIALING) {
> +		const char *timestr;
> +
> +		vc->start_time = time(NULL);
> +		timestr = time_to_str(&vc->start_time);

So in GSM we only timestamp the call once it moves to the active state.
 Do you really want to timestamp it in the dialing stage?

> +
> +		ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> +					"StartTime", DBUS_TYPE_STRING,
> +					&timestr);
> +	}
> +}
> +
> +static void voicecall_set_call_lineid(struct ofono_cdma_voicecall_manager *v)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	const char *path;
> +	const char *lineid_str;
> +
> +	path = voicecall_build_path(v);
> +
> +	/* For MO calls, LineID is the dialed phone number */
> +	lineid_str = cdma_phone_number_to_string(&v->phone_number);
> +
> +	ofono_dbus_signal_property_changed(conn, path,
> +					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> +					"LineIdentification",
> +					DBUS_TYPE_STRING, &lineid_str);
> +}
> +
> +static void manager_dial_callback(const struct ofono_error *error, void *data)
> +{
> +	struct ofono_cdma_voicecall_manager *vc = data;
> +	DBusMessage *reply;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		reply = __ofono_error_failed(vc->pending);
> +	} else {
> +		const char *path = voicecall_build_path(vc);

what path are you returning here?  This is not in the API proposal...

> +
> +		reply = dbus_message_new_method_return(vc->pending);
> +
> +		dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
> +						DBUS_TYPE_INVALID);
> +	}

In general the preferred style is:

if (foo) {
	bar
} else
	blah

> +
> +	__ofono_dbus_pending_reply(&vc->pending, reply);
> +}
> +
> +static DBusMessage *voicecall_manager_dial(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_cdma_voicecall_manager *vc = data;
> +	const char *number;
> +
> +	if (vc->pending)
> +		return __ofono_error_busy(msg);
> +
> +	if (vc->status != CDMA_CALL_STATUS_DISCONNECTED)
> +		return __ofono_error_failed(msg);
> +
> +	if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &number,
> +					DBUS_TYPE_INVALID) == FALSE)
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (!valid_cdma_phone_number_format(number))
> +		return __ofono_error_invalid_format(msg);
> +
> +	if (vc->driver->dial == NULL)
> +		return __ofono_error_not_implemented(msg);
> +
> +	vc->pending = dbus_message_ref(msg);
> +
> +	string_to_cdma_phone_number(number, &vc->phone_number);
> +	voicecall_set_call_lineid(vc);
> +	vc->direction = CALL_DIRECTION_MOBILE_ORIGINATED;
> +	voicecall_set_call_status(vc, CDMA_CALL_STATUS_DIALING);
> +	vc->driver->dial(vc, &vc->phone_number, manager_dial_callback, vc);
> +

What are the expectations on how the dial works?

e.g. in GSM we don't create the call until after the dial returns.
Normally this works this way:

---> ATD
<--- OK
State Dialing
State Alerting
State Connected

Does CDMA work in the same manner?

My other concern here is that you set the call status, line id, etc but
never reset them if the dial fails...

> +	return NULL;
> +}
> +
> +static DBusMessage *voicecall_manager_hangup(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_cdma_voicecall_manager *vc = data;
> +
> +	if (vc->pending)
> +		return __ofono_error_busy(msg);
> +
> +	if (vc->driver->hangup == NULL)
> +		return __ofono_error_not_implemented(msg);
> +
> +	if (vc->status == CDMA_CALL_STATUS_DISCONNECTED)
> +		return __ofono_error_failed(msg);
> +
> +	vc->pending = dbus_message_ref(msg);
> +
> +	vc->driver->hangup(vc, generic_callback, vc);
> +
> +	return NULL;
> +}
> +
> +static GDBusMethodTable manager_methods[] = {
> +	{ "GetProperties",    "",    "a{sv}",
> +					voicecall_manager_get_properties },
> +	{ "Dial",             "ss",  "o",        voicecall_manager_dial,
> +						G_DBUS_METHOD_FLAG_ASYNC },
> +	{ "Hangup",           "",    "",         voicecall_manager_hangup,
> +						G_DBUS_METHOD_FLAG_ASYNC },
> +	{ }
> +};
> +
> +static GDBusSignalTable manager_signals[] = {
> +	{ "PropertyChanged",	"sv" },
> +	{ "DisconnectReason",	"s" },
> +	{ }
> +};
> +
> +void ofono_cdma_voicecall_manager_disconnected(
> +					struct ofono_cdma_voicecall_manager *vc,
> +					enum ofono_disconnect_reason reason,
> +					const struct ofono_error *error)
> +{
> +	DBG("Got disconnection event for reason: %d", reason);
> +
> +	if (reason != OFONO_DISCONNECT_REASON_UNKNOWN)
> +		voicecall_emit_disconnect_reason(vc, reason);
> +
> +	voicecall_set_call_status(vc, CDMA_CALL_STATUS_DISCONNECTED);
> +}
> +
> +static void generic_callback(const struct ofono_error *error, void *data)
> +{
> +	struct ofono_cdma_voicecall_manager *vc = data;
> +	DBusMessage *reply;
> +
> +	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> +		reply = dbus_message_new_method_return(vc->pending);
> +	else
> +		reply = __ofono_error_failed(vc->pending);
> +
> +	__ofono_dbus_pending_reply(&vc->pending, reply);
> +}
> +
> +int ofono_cdma_voicecall_manager_driver_register(
> +		const struct ofono_cdma_voicecall_manager_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	if (d->probe == NULL)
> +		return -EINVAL;
> +
> +	g_drivers = g_slist_prepend(g_drivers, (void *)d);
> +
> +	return 0;
> +}
> +
> +void ofono_cdma_voicecall_manager_driver_unregister(
> +		const struct ofono_cdma_voicecall_manager_driver *d)
> +{
> +	DBG("driver: %p, name: %s", d, d->name);
> +
> +	g_drivers = g_slist_remove(g_drivers, (void *)d);
> +}
> +
> +

Please watch out for double newlines.  Functions should be separated by
1 and only 1 empty line.

> +static void voicecall_manager_remove(struct ofono_atom *atom)
> +{
> +	struct ofono_cdma_voicecall_manager *vc = __ofono_atom_get_data(atom);
> +
> +	DBG("atom: %p", atom);
> +
> +	if (vc == NULL)
> +		return;
> +
> +	if (vc->driver && vc->driver->remove)
> +		vc->driver->remove(vc);
> +
> +	g_free(vc);
> +}
> +
> +struct ofono_cdma_voicecall_manager *ofono_cdma_voicecall_manager_create(
> +						struct ofono_modem *modem,
> +						unsigned int vendor,
> +						const char *driver,
> +						void *data)
> +{
> +	struct ofono_cdma_voicecall_manager *vc;
> +	GSList *l;
> +
> +	if (driver == NULL)
> +		return NULL;
> +
> +	vc = g_try_new0(struct ofono_cdma_voicecall_manager, 1);
> +	if (vc == NULL)
> +		return NULL;
> +
> +	vc->status = CDMA_CALL_STATUS_DISCONNECTED;
> +
> +	vc->atom = __ofono_modem_add_atom(modem,
> +			OFONO_ATOM_TYPE_CDMA_VOICECALL_MANAGER,
> +			voicecall_manager_remove, vc);
> +
> +	for (l = g_drivers; l; l = l->next) {
> +		const struct ofono_cdma_voicecall_manager_driver *drv = l->data;
> +
> +		if (g_strcmp0(drv->name, driver))
> +			continue;
> +
> +		if (drv->probe(vc, vendor, data) < 0)
> +			continue;
> +
> +		vc->driver = drv;
> +		break;
> +	}
> +
> +	return vc;
> +}
> +
> +void ofono_cdma_voicecall_manager_register(
> +				struct ofono_cdma_voicecall_manager *vc)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	struct ofono_modem *modem = __ofono_atom_get_modem(vc->atom);
> +	const char *path = __ofono_atom_get_path(vc->atom);
> +
> +	if (!g_dbus_register_interface(conn, path,
> +					OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> +					manager_methods, manager_signals, NULL,
> +					vc, NULL)) {
> +		ofono_error("Could not create %s interface",
> +				OFONO_CDMA_VOICECALL_MANAGER_INTERFACE);
> +		return;
> +	}
> +
> +	ofono_modem_add_interface(modem,
> +			OFONO_CDMA_VOICECALL_MANAGER_INTERFACE);
> +}
> +
> +void ofono_cdma_voicecall_manager_remove(
> +			struct ofono_cdma_voicecall_manager *vc)
> +{
> +	__ofono_atom_free(vc->atom);
> +}
> +
> +void ofono_cdma_voicecall_manager_set_data(
> +			struct ofono_cdma_voicecall_manager *vc, void *data)
> +{
> +	vc->driver_data = data;
> +}
> +
> +void *ofono_cdma_voicecall_manager_get_data(
> +			struct ofono_cdma_voicecall_manager *vc)
> +{
> +	return vc->driver_data;
> +}
> diff --git a/src/common.c b/src/common.c
> index 5154b8d..7aa63ce 100644
> --- a/src/common.c
> +++ b/src/common.c
> @@ -262,6 +262,31 @@ gboolean valid_phone_number_format(const char *number)
>  	return TRUE;
>  }
>  
> +gboolean valid_cdma_phone_number_format(const char *number)
> +{
> +	int len = strlen(number);
> +	int begin = 0;

This variable seems to be not needed

> +	int i;
> +
> +	if (!len)
> +		return FALSE;
> +
> +	if ((len - begin) > OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH)

This can simply be len > OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH.  This
reminds me, can we stick to prefixing the CDMA defines / enums with
OFONO_CDMA_?  So this would be OFONO_CDMA_MAX_PHONE_NUMBER_LENGTH.

> +		return FALSE;
> +
> +	for (i = begin; i < len; i++) {

This could simply be i = 0...

> +		if (number[i] >= '0' && number[i] <= '9')
> +			continue;
> +
> +		if (number[i] == '*' || number[i] == '#')
> +			continue;
> +
> +		return FALSE;
> +	}
> +
> +	return TRUE;
> +}
> +
>  const char *telephony_error_to_str(const struct ofono_error *error)
>  {
>  	struct error_entry *e;
> @@ -405,6 +430,23 @@ void string_to_phone_number(const char *str, struct ofono_phone_number *ph)
>  	}
>  }
>  
> +const char *cdma_phone_number_to_string(
> +			const struct ofono_cdma_phone_number *ph)
> +{
> +	static char buffer[OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH + 1];
> +
> +	strncpy(buffer, ph->number, OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH);
> +	buffer[OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH] = '\0';
> +
> +	return buffer;
> +}
> +
> +void string_to_cdma_phone_number(const char *str,
> +			struct ofono_cdma_phone_number *ph)
> +{
> +	strcpy(ph->number, str);

Do we have international '+' prefix in CDMA?

> +}
> +
>  gboolean valid_ussd_string(const char *str, gboolean call_in_progress)
>  {
>  	int len = strlen(str);
> diff --git a/src/common.h b/src/common.h
> index 8b5798a..df0adcd 100644
> --- a/src/common.h
> +++ b/src/common.h
> @@ -59,6 +59,14 @@ enum call_status {
>  	CALL_STATUS_DISCONNECTED
>  };
>  
> +enum cdma_call_status {
> +	CDMA_CALL_STATUS_ACTIVE = 0,
> +	CDMA_CALL_STATUS_DIALING = 1,
> +	CDMA_CALL_STATUS_ALERTING = 2,
> +	CDMA_CALL_STATUS_INCOMING = 4,
> +	CDMA_CALL_STATUS_DISCONNECTED
> +};
> +
>  /* 27.007 Section 7.18 */
>  enum call_direction {
>  	CALL_DIRECTION_MOBILE_ORIGINATED = 0,
> @@ -128,6 +136,12 @@ gboolean valid_phone_number_format(const char *number);
>  const char *phone_number_to_string(const struct ofono_phone_number *ph);
>  void string_to_phone_number(const char *str, struct ofono_phone_number *ph);
>  
> +gboolean valid_cdma_phone_number_format(const char *number);
> +const char *cdma_phone_number_to_string(
> +				const struct ofono_cdma_phone_number *ph);
> +void string_to_cdma_phone_number(const char *str,
> +				struct ofono_cdma_phone_number *ph);
> +
>  int mmi_service_code_to_bearer_class(int code);
>  
>  gboolean valid_ussd_string(const char *str, gboolean call_in_progress);
> diff --git a/src/ofono.h b/src/ofono.h
> index 792134b..cab70cd 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -126,6 +126,7 @@ enum ofono_atom_type {
>  	OFONO_ATOM_TYPE_STK = 20,
>  	OFONO_ATOM_TYPE_NETTIME = 21,
>  	OFONO_ATOM_TYPE_CTM = 22,
> +	OFONO_ATOM_TYPE_CDMA_VOICECALL_MANAGER = 23,
>  };
>  
>  enum ofono_atom_watch_condition {
> @@ -415,3 +416,5 @@ void __ofono_nettime_probe_drivers(struct ofono_modem *modem);
>  
>  void __ofono_nettime_info_received(struct ofono_modem *modem,
>  					struct ofono_network_time *info);
> +
> +#include <ofono/cdma-voicecall.h>

Regards,
-Denis

Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support

[Dara Spieker-Doyle]

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

Hi Denis

Thank you for reviewing this series of patches. We shall submit new
versions according to your comments. Please find any other clarifying
points inline.

On Thu, 2010-12-09 at 09:36 +0100, ext Denis Kenzior wrote:

> > +
> > +static void voicecall_set_call_status(struct ofono_cdma_voicecall_manager *vc,
> > +             enum cdma_call_status status)
> > +{
> > +     DBusConnection *conn = ofono_dbus_get_connection();
> > +     const char *path;
> > +     const char *status_str;
> > +     enum cdma_call_status old_status;
> > +
> > +     if (vc->status == status)
> > +             return;
> > +
> > +     old_status = vc->status;
> > +
> > +     vc->status = status;
> > +
> > +     status_str = cdma_call_status_to_string(status);
> > +     path = voicecall_build_path(vc);
> > +
> > +     ofono_dbus_signal_property_changed(conn, path,
> > +                                     OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> > +                                     "State", DBUS_TYPE_STRING,
> > +                                     &status_str);
> > +
> > +     if (status == CDMA_CALL_STATUS_ACTIVE &&
> > +                     old_status == CDMA_CALL_STATUS_DIALING) {
> > +             const char *timestr;
> > +
> > +             vc->start_time = time(NULL);
> > +             timestr = time_to_str(&vc->start_time);
> 
> So in GSM we only timestamp the call once it moves to the active state.
>  Do you really want to timestamp it in the dialing stage?

I'm not sure what you mean here? We are carrying out the timestamp when
the status is active having previously been dialing in a similar way to
GSM. 
In this patch, setting of the state to active is not supported yet due
to our current hardware limitations, so the time implementation has been
provided but is untested. If you would prefer us to remove the
timestamping altogether from this patch, we can do?

> 
> > +
> > +             ofono_dbus_signal_property_changed(conn, path,
> > +                                     OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> > +                                     "StartTime", DBUS_TYPE_STRING,
> > +                                     &timestr);
> > +     }
> > +}
> > +
> > +static void voicecall_set_call_lineid(struct ofono_cdma_voicecall_manager *v)
> > +{
> > +     DBusConnection *conn = ofono_dbus_get_connection();
> > +     const char *path;
> > +     const char *lineid_str;
> > +
> > +     path = voicecall_build_path(v);
> > +
> > +     /* For MO calls, LineID is the dialed phone number */
> > +     lineid_str = cdma_phone_number_to_string(&v->phone_number);
> > +
> > +     ofono_dbus_signal_property_changed(conn, path,
> > +                                     OFONO_CDMA_VOICECALL_MANAGER_INTERFACE,
> > +                                     "LineIdentification",
> > +                                     DBUS_TYPE_STRING, &lineid_str);
> > +}
> > +
> > +static void manager_dial_callback(const struct ofono_error *error, void *data)
> > +{
> > +     struct ofono_cdma_voicecall_manager *vc = data;
> > +     DBusMessage *reply;
> > +
> > +     if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> > +             reply = __ofono_error_failed(vc->pending);
> > +     } else {
> > +             const char *path = voicecall_build_path(vc);
> 
> what path are you returning here?  This is not in the API proposal...

This was an oversight, thank you for catching it- I will remove this.

> 
> > +
> > +             reply = dbus_message_new_method_return(vc->pending);
> > +
> > +             dbus_message_append_args(reply, DBUS_TYPE_OBJECT_PATH, &path,
> > +                                             DBUS_TYPE_INVALID);
> > +     }
> 
> In general the preferred style is:
> 
> if (foo) {
>         bar
> } else
>         blah
> 
> > +
> > +     __ofono_dbus_pending_reply(&vc->pending, reply);
> > +}
> > +
> > +static DBusMessage *voicecall_manager_dial(DBusConnection *conn,
> > +                                     DBusMessage *msg, void *data)
> > +{
> > +     struct ofono_cdma_voicecall_manager *vc = data;
> > +     const char *number;
> > +
> > +     if (vc->pending)
> > +             return __ofono_error_busy(msg);
> > +
> > +     if (vc->status != CDMA_CALL_STATUS_DISCONNECTED)
> > +             return __ofono_error_failed(msg);
> > +
> > +     if (dbus_message_get_args(msg, NULL, DBUS_TYPE_STRING, &number,
> > +                                     DBUS_TYPE_INVALID) == FALSE)
> > +             return __ofono_error_invalid_args(msg);
> > +
> > +     if (!valid_cdma_phone_number_format(number))
> > +             return __ofono_error_invalid_format(msg);
> > +
> > +     if (vc->driver->dial == NULL)
> > +             return __ofono_error_not_implemented(msg);
> > +
> > +     vc->pending = dbus_message_ref(msg);
> > +
> > +     string_to_cdma_phone_number(number, &vc->phone_number);
> > +     voicecall_set_call_lineid(vc);
> > +     vc->direction = CALL_DIRECTION_MOBILE_ORIGINATED;
> > +     voicecall_set_call_status(vc, CDMA_CALL_STATUS_DIALING);
> > +     vc->driver->dial(vc, &vc->phone_number, manager_dial_callback, vc);
> > +
> 
> What are the expectations on how the dial works?
> 
> e.g. in GSM we don't create the call until after the dial returns.
> Normally this works this way:
> 
> ---> ATD
> <--- OK
> State Dialing
> State Alerting
> State Connected
> 
> Does CDMA work in the same manner?
> 
> My other concern here is that you set the call status, line id, etc but
> never reset them if the dial fails...

Thank you, yes CDMA dial does work in the same way as GSM essentially. I
shall move the call creation into the manager_dial_callback method, to
be executed upon a successful return from the dial.
Support for other call state support will come through with the future
work for call state transitions.

> >
> > +const char *cdma_phone_number_to_string(
> > +                     const struct ofono_cdma_phone_number *ph)
> > +{
> > +     static char buffer[OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH + 1];
> > +
> > +     strncpy(buffer, ph->number, OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH);
> > +     buffer[OFONO_MAX_CDMA_PHONE_NUMBER_LENGTH] = '\0';
> > +
> > +     return buffer;
> > +}
> > +
> > +void string_to_cdma_phone_number(const char *str,
> > +                     struct ofono_cdma_phone_number *ph)
> > +{
> > +     strcpy(ph->number, str);
> 
> Do we have international '+' prefix in CDMA?
> 
Sometimes. If the CDMA networks support network based plus code dialing
then '+' can be used to replace the international direct dial prefix. 
Our intention is to support international dialing with future work.

Cheers
Dara