Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
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
Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
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 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
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 000..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
Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
Hi Rajesh,
On 12/03/2010 03:34 PM, rajesh.naga...@elektrobit.com wrote:
Hi Dara,
-Original Message-
From: ofono-boun...@ofono.org
[mailto:ofono-boun...@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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
Re: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
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.naga...@elektrobit.com wrote:
Hi Dara,
-Original Message-
From: ofono-boun...@ofono.org
[mailto:ofono-boun...@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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
Hi Rajesh
On Fri, 2010-12-03 at 01:30 +0100, ext rajesh.naga...@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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
Hi Dara,
-Original Message-
From: ofono-boun...@ofono.org
[mailto:ofono-boun...@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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
[PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
---
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 000..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;
+
+
RE: [PATCH 2/5] cdma-voicecall: Add CDMA MO Call Support
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
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
