[PATCH v3, Part1, 4/5] cdmamodem: Add CDMA voicecall driver support for MO Call

[PATCH v3, Part1, 4/5] cdmamodem: Add CDMA voicecall driver support for MO Call

[Dara Spieker-Doyle]

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

---
 Makefile.am                   |    3 +-
 drivers/cdmamodem/cdmamodem.c |    3 +
 drivers/cdmamodem/cdmamodem.h |    3 +
 drivers/cdmamodem/voicecall.c |  190 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 198 insertions(+), 1 deletions(-)
 create mode 100644 drivers/cdmamodem/voicecall.c

diff --git a/Makefile.am b/Makefile.am
index 861237c..50e893f 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -245,7 +245,8 @@ endif
 if CDMA_ATMODEM
 builtin_modules += cdma_atmodem
 builtin_sources += drivers/cdmamodem/cdmamodem.h \
-			drivers/cdmamodem/cdmamodem.c
+			drivers/cdmamodem/cdmamodem.c \
+			drivers/cdmamodem/voicecall.c
 endif
 
 builtin_modules += g1
diff --git a/drivers/cdmamodem/cdmamodem.c b/drivers/cdmamodem/cdmamodem.c
index 25bd0f3..20e6d96 100644
--- a/drivers/cdmamodem/cdmamodem.c
+++ b/drivers/cdmamodem/cdmamodem.c
@@ -34,11 +34,14 @@
 
 static int cdma_atmodem_init(void)
 {
+	cdma_at_voicecall_init();
+
 	return 0;
 }
 
 static void cdma_atmodem_exit(void)
 {
+	cdma_at_voicecall_exit();
 }
 
 OFONO_PLUGIN_DEFINE(cdma_atmodem, "CDMA AT modem driver", VERSION,
diff --git a/drivers/cdmamodem/cdmamodem.h b/drivers/cdmamodem/cdmamodem.h
index 114d1fd..a5991cf 100644
--- a/drivers/cdmamodem/cdmamodem.h
+++ b/drivers/cdmamodem/cdmamodem.h
@@ -20,3 +20,6 @@
  */
 
 #include <drivers/atmodem/atutil.h>
+
+extern void cdma_at_voicecall_init();
+extern void cdma_at_voicecall_exit();
diff --git a/drivers/cdmamodem/voicecall.c b/drivers/cdmamodem/voicecall.c
new file mode 100644
index 0000000..e0a62f5
--- /dev/null
+++ b/drivers/cdmamodem/voicecall.c
@@ -0,0 +1,190 @@
+/*
+ *
+ *  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
+
+#define _GNU_SOURCE
+#include <string.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <errno.h>
+
+#include <glib.h>
+
+#include <ofono/log.h>
+#include <ofono/modem.h>
+#include <ofono/cdma-voicecall.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "cdmamodem.h"
+
+static const char *none_prefix[] = { NULL };
+
+struct voicecall_driver {
+	struct ofono_cdma_voicecall *vc;
+	unsigned int local_release;
+	GAtChat *chat;
+	unsigned int vendor;
+};
+
+struct change_state_req {
+	struct ofono_cdma_voicecall *vc;
+	ofono_cdma_voicecall_cb_t cb;
+	void *data;
+};
+
+static void at_dial_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_cdma_voicecall_cb_t cb = cbd->cb;
+	struct ofono_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (!ok)
+		goto out;
+
+out:
+	cb(&error, cbd->data);
+}
+
+static void at_dial(struct ofono_cdma_voicecall *vc,
+				const struct ofono_cdma_phone_number *ph,
+				ofono_cdma_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[256];
+
+	if (cbd == NULL)
+		goto error;
+
+	cbd->user = vc;
+
+	snprintf(buf, sizeof(buf), "AT+CDV=%s", ph->number);
+
+	if (g_at_chat_send(vd->chat, buf, none_prefix,
+				at_dial_cb, cbd, g_free) > 0)
+		return;
+
+error:
+	g_free(cbd);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void at_template(const char *cmd,
+				struct ofono_cdma_voicecall *vc,
+				GAtResultFunc result_cb,
+				ofono_cdma_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
+	struct change_state_req *req = g_try_new0(struct change_state_req, 1);
+
+	if (req == NULL)
+		goto error;
+
+	req->vc = vc;
+	req->cb = cb;
+	req->data = data;
+
+	if (g_at_chat_send(vd->chat, cmd, none_prefix,
+				result_cb, req, g_free) > 0)
+		return;
+
+error:
+	g_free(req);
+
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void at_hangup_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct change_state_req *req = user_data;
+
+	if (!ok) {
+		ofono_error("hangup failed.");
+		return;
+	}
+
+	ofono_cdma_voicecall_disconnected(req->vc,
+				OFONO_DISCONNECT_REASON_LOCAL_HANGUP, NULL);
+		CALLBACK_WITH_SUCCESS(req->cb, req->data);
+}
+
+static void at_hangup(struct ofono_cdma_voicecall *vc,
+			ofono_cdma_voicecall_cb_t cb, void *data)
+{
+	/* Hangup active call */
+	at_template("AT+CHV", vc, at_hangup_cb, cb, data);
+}
+
+static int at_voicecall_probe(struct ofono_cdma_voicecall *vc,
+		unsigned int vendor, void *data)
+{
+	GAtChat *chat = data;
+	struct voicecall_driver *vd;
+
+	vd = g_try_new0(struct voicecall_driver, 1);
+	if (vd == NULL)
+		return -ENOMEM;
+
+	vd->chat = g_at_chat_clone(chat);
+	vd->vendor = vendor;
+
+	ofono_cdma_voicecall_set_data(vc, vd);
+
+	ofono_cdma_voicecall_register(vc);
+
+	return 0;
+}
+
+static void at_voicecall_remove(struct ofono_cdma_voicecall *vc)
+{
+	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
+
+	ofono_cdma_voicecall_set_data(vc, NULL);
+
+	g_at_chat_unref(vd->chat);
+	g_free(vd);
+}
+
+static struct ofono_cdma_voicecall_driver driver = {
+	.name			= "cdmamodem",
+	.probe			= at_voicecall_probe,
+	.remove			= at_voicecall_remove,
+	.dial			= at_dial,
+	.hangup			= at_hangup,
+};
+
+void cdma_at_voicecall_init()
+{
+	ofono_cdma_voicecall_driver_register(&driver);
+}
+
+void cdma_at_voicecall_exit()
+{
+	ofono_cdma_voicecall_driver_unregister(&driver);
+}
-- 
1.7.0.4

Re: [PATCH v3, Part1, 4/5] cdmamodem: Add CDMA voicecall driver support for MO Call

[Denis Kenzior]

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

Hi Dara,

<snip>

> +
> +struct voicecall_driver {
> +	struct ofono_cdma_voicecall *vc;
> +	unsigned int local_release;

What is the use of these two variables?

> +	GAtChat *chat;
> +	unsigned int vendor;
> +};
> +
> +struct change_state_req {
> +	struct ofono_cdma_voicecall *vc;
> +	ofono_cdma_voicecall_cb_t cb;
> +	void *data;
> +};

is cb_data not good enough for some reason? :)

> +
> +static void at_dial_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_cdma_voicecall_cb_t cb = cbd->cb;
> +	struct ofono_error error;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (!ok)
> +		goto out;

This if statement serves no purpose

> +
> +out:

And neither does the label

> +	cb(&error, cbd->data);
> +}
> +
> +static void at_dial(struct ofono_cdma_voicecall *vc,
> +				const struct ofono_cdma_phone_number *ph,
> +				ofono_cdma_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	char buf[256];
> +
> +	if (cbd == NULL)
> +		goto error;
> +
> +	cbd->user = vc;

Why are you assigning cbd->user and never making use of it?

> +
> +	snprintf(buf, sizeof(buf), "AT+CDV=%s", ph->number);
> +
> +	if (g_at_chat_send(vd->chat, buf, none_prefix,
> +				at_dial_cb, cbd, g_free) > 0)
> +		return;
> +
> +error:
> +	g_free(cbd);
> +
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void at_template(const char *cmd,
> +				struct ofono_cdma_voicecall *vc,
> +				GAtResultFunc result_cb,
> +				ofono_cdma_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
> +	struct change_state_req *req = g_try_new0(struct change_state_req, 1);
> +
> +	if (req == NULL)
> +		goto error;
> +
> +	req->vc = vc;
> +	req->cb = cb;
> +	req->data = data;
> +
> +	if (g_at_chat_send(vd->chat, cmd, none_prefix,
> +				result_cb, req, g_free) > 0)
> +		return;
> +
> +error:
> +	g_free(req);
> +
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void at_hangup_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct change_state_req *req = user_data;
> +
> +	if (!ok) {
> +		ofono_error("hangup failed.");
> +		return;

Please don't do this.  If the hangup fails you never return from the
operation and will cause the cdma-voicecall atom to stall forever.
decoding the error and calling the callback is good enough.

> +	}
> +
> +	ofono_cdma_voicecall_disconnected(req->vc,
> +				OFONO_DISCONNECT_REASON_LOCAL_HANGUP, NULL);

Fair enough, but this should really come via a modem unsolicited
notification.

> +		CALLBACK_WITH_SUCCESS(req->cb, req->data);

This is not necessary if you call the callback up above.  And why the
different indentation?

> +}
> +
> +static void at_hangup(struct ofono_cdma_voicecall *vc,
> +			ofono_cdma_voicecall_cb_t cb, void *data)
> +{
> +	/* Hangup active call */
> +	at_template("AT+CHV", vc, at_hangup_cb, cb, data);
> +}
> +
> +static int at_voicecall_probe(struct ofono_cdma_voicecall *vc,
> +		unsigned int vendor, void *data)
> +{
> +	GAtChat *chat = data;
> +	struct voicecall_driver *vd;
> +
> +	vd = g_try_new0(struct voicecall_driver, 1);
> +	if (vd == NULL)
> +		return -ENOMEM;
> +
> +	vd->chat = g_at_chat_clone(chat);
> +	vd->vendor = vendor;
> +
> +	ofono_cdma_voicecall_set_data(vc, vd);
> +
> +	ofono_cdma_voicecall_register(vc);
> +
> +	return 0;
> +}
> +
> +static void at_voicecall_remove(struct ofono_cdma_voicecall *vc)
> +{
> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
> +
> +	ofono_cdma_voicecall_set_data(vc, NULL);
> +
> +	g_at_chat_unref(vd->chat);
> +	g_free(vd);
> +}
> +
> +static struct ofono_cdma_voicecall_driver driver = {
> +	.name			= "cdmamodem",
> +	.probe			= at_voicecall_probe,
> +	.remove			= at_voicecall_remove,
> +	.dial			= at_dial,
> +	.hangup			= at_hangup,
> +};
> +
> +void cdma_at_voicecall_init()
> +{
> +	ofono_cdma_voicecall_driver_register(&driver);
> +}
> +
> +void cdma_at_voicecall_exit()
> +{
> +	ofono_cdma_voicecall_driver_unregister(&driver);
> +}

Regards,
-Denis

Re: [PATCH v3, Part1, 4/5] cdmamodem: Add CDMA voicecall driver support for MO Call

[Dara Spieker-Doyle]

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

Hi Denis

Thank you for reviewing these.

On 12/22/2010 05:26 PM, ext Denis Kenzior wrote:
> Hi Dara,
>
> <snip>
>
>> +
>> +struct voicecall_driver {
>> +	struct ofono_cdma_voicecall *vc;
>> +	unsigned int local_release;
>
> What is the use of these two variables?
>
>> +	GAtChat *chat;
>> +	unsigned int vendor;
>> +};
>> +
>> +struct change_state_req {
>> +	struct ofono_cdma_voicecall *vc;
>> +	ofono_cdma_voicecall_cb_t cb;
>> +	void *data;
>> +};
>
> is cb_data not good enough for some reason? :)
>
>> +
>> +static void at_dial_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +	struct cb_data *cbd = user_data;
>> +	ofono_cdma_voicecall_cb_t cb = cbd->cb;
>> +	struct ofono_error error;
>> +
>> +	decode_at_error(&error, g_at_result_final_response(result));
>> +
>> +	if (!ok)
>> +		goto out;
>
> This if statement serves no purpose
>
>> +
>> +out:
>
> And neither does the label
>
>> +	cb(&error, cbd->data);
>> +}
>> +
>> +static void at_dial(struct ofono_cdma_voicecall *vc,
>> +				const struct ofono_cdma_phone_number *ph,
>> +				ofono_cdma_voicecall_cb_t cb, void *data)
>> +{
>> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
>> +	struct cb_data *cbd = cb_data_new(cb, data);
>> +	char buf[256];
>> +
>> +	if (cbd == NULL)
>> +		goto error;
>> +
>> +	cbd->user = vc;
>
> Why are you assigning cbd->user and never making use of it?

For all of your above comments, yes they shouldn't be there - I will 
clean these up.
>
>> +
>> +	snprintf(buf, sizeof(buf), "AT+CDV=%s", ph->number);
>> +
>> +	if (g_at_chat_send(vd->chat, buf, none_prefix,
>> +				at_dial_cb, cbd, g_free)>  0)
>> +		return;
>> +
>> +error:
>> +	g_free(cbd);
>> +
>> +	CALLBACK_WITH_FAILURE(cb, data);
>> +}
>> +
>> +static void at_template(const char *cmd,
>> +				struct ofono_cdma_voicecall *vc,
>> +				GAtResultFunc result_cb,
>> +				ofono_cdma_voicecall_cb_t cb, void *data)
>> +{
>> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
>> +	struct change_state_req *req = g_try_new0(struct change_state_req, 1);
>> +
>> +	if (req == NULL)
>> +		goto error;
>> +
>> +	req->vc = vc;
>> +	req->cb = cb;
>> +	req->data = data;
>> +
>> +	if (g_at_chat_send(vd->chat, cmd, none_prefix,
>> +				result_cb, req, g_free)>  0)
>> +		return;
>> +
>> +error:
>> +	g_free(req);
>> +
>> +	CALLBACK_WITH_FAILURE(cb, data);
>> +}
>> +
>> +static void at_hangup_cb(gboolean ok, GAtResult *result, gpointer user_data)
>> +{
>> +	struct change_state_req *req = user_data;
>> +
>> +	if (!ok) {
>> +		ofono_error("hangup failed.");
>> +		return;
>
> Please don't do this.  If the hangup fails you never return from the
> operation and will cause the cdma-voicecall atom to stall forever.
> decoding the error and calling the callback is good enough.

Thanks for spotting this, I will fix it.
>
>> +	}
>> +
>> +	ofono_cdma_voicecall_disconnected(req->vc,
>> +				OFONO_DISCONNECT_REASON_LOCAL_HANGUP, NULL);
>
> Fair enough, but this should really come via a modem unsolicited
> notification.

Agreed, I can either mark it with a TODO to make this clear or remove 
the disconnect reason support altogether.
>
>> +		CALLBACK_WITH_SUCCESS(req->cb, req->data);
>
> This is not necessary if you call the callback up above.  And why the
> different indentation?
>
>> +}
>> +
>> +static void at_hangup(struct ofono_cdma_voicecall *vc,
>> +			ofono_cdma_voicecall_cb_t cb, void *data)
>> +{
>> +	/* Hangup active call */
>> +	at_template("AT+CHV", vc, at_hangup_cb, cb, data);
>> +}
>> +
>> +static int at_voicecall_probe(struct ofono_cdma_voicecall *vc,
>> +		unsigned int vendor, void *data)
>> +{
>> +	GAtChat *chat = data;
>> +	struct voicecall_driver *vd;
>> +
>> +	vd = g_try_new0(struct voicecall_driver, 1);
>> +	if (vd == NULL)
>> +		return -ENOMEM;
>> +
>> +	vd->chat = g_at_chat_clone(chat);
>> +	vd->vendor = vendor;
>> +
>> +	ofono_cdma_voicecall_set_data(vc, vd);
>> +
>> +	ofono_cdma_voicecall_register(vc);
>> +
>> +	return 0;
>> +}
>> +
>> +static void at_voicecall_remove(struct ofono_cdma_voicecall *vc)
>> +{
>> +	struct voicecall_driver *vd = ofono_cdma_voicecall_get_data(vc);
>> +
>> +	ofono_cdma_voicecall_set_data(vc, NULL);
>> +
>> +	g_at_chat_unref(vd->chat);
>> +	g_free(vd);
>> +}
>> +
>> +static struct ofono_cdma_voicecall_driver driver = {
>> +	.name			= "cdmamodem",
>> +	.probe			= at_voicecall_probe,
>> +	.remove			= at_voicecall_remove,
>> +	.dial			= at_dial,
>> +	.hangup			= at_hangup,
>> +};
>> +
>> +void cdma_at_voicecall_init()
>> +{
>> +	ofono_cdma_voicecall_driver_register(&driver);
>> +}
>> +
>> +void cdma_at_voicecall_exit()
>> +{
>> +	ofono_cdma_voicecall_driver_unregister(&driver);
>> +}
>
> Regards,
> -Denis

Thanks
Dara