[PATCH v4 0/2] Gemalto: support for voicecall

[PATCH v4 0/2] Gemalto: support for voicecall

[Giacinto Cifelli]

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

This series of patches adds a vendor-specific voicecall atom
for gemaltomodem, and then adds it to the plugin, along with all
other call-related atoms.

related to the previous versions submitted for RFC, it integrates the
comments from Denis and Jonas, uses the required URCs (leaving
out, for example, RING), and doesn't check the memory allocation
anymore: g_new0 instead of g_try_new0.
This version further simplify the treatment of ATD, because even if some old
models support the blocking ATD, these modules are mostly obsolete, and can
anyway be configured to be non-blocking. These old modules are currently
not supported in the Gemalto plugin.

From the second version, it fixes also a problem with the initial
version, where the SLCC URC was parsed exactly like the AT+CLCC command:
now the code takes into account that the URC function is called for each
line, and not for all together (unlike CLCC parsing, where all lines are
available at once).

Giacinto Cifelli (2):
  Gemalto: voicecall atom
  plugins/gemalto: added voice support

 Makefile.am                         |   3 +-
 drivers/gemaltomodem/gemaltomodem.c |   3 +
 drivers/gemaltomodem/gemaltomodem.h |   6 +
 drivers/gemaltomodem/voicecall.c    | 581 ++++++++++++++++++++++++++++
 plugins/gemalto.c                   |  12 +
 5 files changed, 604 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gemaltomodem/voicecall.c

-- 
2.17.1

[PATCH v4 1/2] Gemalto: voicecall atom

[Giacinto Cifelli]

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

Added voicecall atom specific for Gemalto modems.
This atom uses the URC ^SLCC to monitor the call status, as well as
incoming calls.
Note the use in the atom of the variable GemaltoVtsQuotes: this is
needed to support future modules, as of today not yet available in the
plugin.
---
 Makefile.am                         |   3 +-
 drivers/gemaltomodem/gemaltomodem.c |   3 +
 drivers/gemaltomodem/gemaltomodem.h |   6 +
 drivers/gemaltomodem/voicecall.c    | 581 ++++++++++++++++++++++++++++
 4 files changed, 592 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gemaltomodem/voicecall.c

diff --git a/Makefile.am b/Makefile.am
index 6667524f..e8e4ed95 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -397,7 +397,8 @@ builtin_modules += gemaltomodem
 builtin_sources += drivers/atmodem/atutil.h \
 			drivers/gemaltomodem/gemaltomodem.h \
 			drivers/gemaltomodem/gemaltomodem.c \
-			drivers/gemaltomodem/location-reporting.c
+			drivers/gemaltomodem/location-reporting.c \
+			drivers/gemaltomodem/voicecall.c
 
 builtin_modules += xmm7modem
 builtin_sources += drivers/atmodem/atutil.h \
diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
index 91cf238a..4818ac66 100644
--- a/drivers/gemaltomodem/gemaltomodem.c
+++ b/drivers/gemaltomodem/gemaltomodem.c
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2017 Vincent Cesson. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
  *
  *  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
@@ -35,12 +36,14 @@
 static int gemaltomodem_init(void)
 {
 	gemalto_location_reporting_init();
+	gemalto_voicecall_init();
 
 	return 0;
 }
 
 static void gemaltomodem_exit(void)
 {
+	gemalto_voicecall_exit();
 	gemalto_location_reporting_exit();
 }
 
diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
index 7ea1e8fb..c6667411 100644
--- a/drivers/gemaltomodem/gemaltomodem.h
+++ b/drivers/gemaltomodem/gemaltomodem.h
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2017 Vincent Cesson. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
  *
  *  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
@@ -20,6 +21,11 @@
  */
 
 #include <drivers/atmodem/atutil.h>
+#include "gemaltoutil.h"
 
 extern void gemalto_location_reporting_init();
 extern void gemalto_location_reporting_exit();
+
+extern void gemalto_voicecall_init();
+extern void gemalto_voicecall_exit();
+
diff --git a/drivers/gemaltomodem/voicecall.c b/drivers/gemaltomodem/voicecall.c
new file mode 100644
index 00000000..8e2c7e10
--- /dev/null
+++ b/drivers/gemaltomodem/voicecall.c
@@ -0,0 +1,581 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
+ *
+ *  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/voicecall.h>
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "common.h"
+
+#include "gemaltomodem.h"
+
+static const char *clcc_prefix[] = { "+CLCC:", NULL };
+static const char *none_prefix[] = { NULL };
+
+struct voicecall_data {
+	GAtChat *chat;
+	GSList *calls;
+	unsigned int local_release;
+	GSList *new_calls;
+};
+
+struct release_id_req {
+	struct ofono_voicecall *vc;
+	ofono_voicecall_cb_t cb;
+	void *data;
+	int id;
+};
+
+struct change_state_req {
+	struct ofono_voicecall *vc;
+	ofono_voicecall_cb_t cb;
+	void *data;
+	int affected_types;
+};
+
+static void generic_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct change_state_req *req = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+	struct ofono_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (ok && req->affected_types) {
+		GSList *l;
+		struct ofono_call *call;
+
+		for (l = vd->calls; l; l = l->next) {
+			call = l->data;
+
+			if (req->affected_types & (1 << call->status))
+				vd->local_release |= (1 << call->id);
+		}
+	}
+
+	req->cb(&error, req->data);
+}
+
+static void gemalto_call_common(const char *cmd, struct ofono_voicecall *vc,
+			GAtResultFunc result_cb, unsigned int affected_types,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct change_state_req *req = g_new0(struct change_state_req, 1);
+
+	req->vc = vc;
+	req->cb = cb;
+	req->data = data;
+	req->affected_types = affected_types;
+
+	if (g_at_chat_send(vd->chat, cmd, none_prefix,
+				result_cb, req, g_free) > 0)
+		return;
+
+	g_free(req);
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void gemalto_answer(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	gemalto_call_common("ATA", vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_hangup_all(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
+				(1 << CALL_STATUS_DIALING) |
+				(1 << CALL_STATUS_ALERTING) |
+				(1 << CALL_STATUS_WAITING) |
+				(1 << CALL_STATUS_HELD) |
+				(1 << CALL_STATUS_ACTIVE);
+
+	/* Hangup all calls */
+	gemalto_call_common("AT+CHUP", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_hangup(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
+
+	/* Hangup current active call */
+	gemalto_call_common("AT+CHLD=1", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_hold_all_active(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
+	gemalto_call_common("AT+CHLD=2", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_release_all_held(struct ofono_voicecall *vc,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
+				(1 << CALL_STATUS_WAITING);
+
+	gemalto_call_common("AT+CHLD=0", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_set_udub(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
+				(1 << CALL_STATUS_WAITING);
+
+	gemalto_call_common("AT+CHLD=0", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_release_all_active(struct ofono_voicecall *vc,
+					ofono_voicecall_cb_t cb, void *data)
+{
+	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
+
+	gemalto_call_common("AT+CHLD=1", vc, generic_cb, affected, cb, data);
+}
+
+static void release_id_cb(gboolean ok, GAtResult *result,
+				gpointer user_data)
+{
+	struct release_id_req *req = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+	struct ofono_error error;
+
+	decode_at_error(&error, g_at_result_final_response(result));
+
+	if (ok)
+		vd->local_release = 1 << req->id;
+
+	req->cb(&error, req->data);
+}
+
+static void gemalto_release_specific(struct ofono_voicecall *vc, int id,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct release_id_req *req = g_new0(struct release_id_req, 1);
+	char buf[32];
+
+	req->vc = vc;
+	req->cb = cb;
+	req->data = data;
+	req->id = id;
+
+	snprintf(buf, sizeof(buf), "AT+CHLD=1%d", id);
+
+	if (g_at_chat_send(vd->chat, buf, none_prefix,
+				release_id_cb, req, g_free) > 0)
+		return;
+
+	g_free(req);
+	CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void gemalto_private_chat(struct ofono_voicecall *vc, int id,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	char buf[32];
+
+	snprintf(buf, sizeof(buf), "AT+CHLD=2%d", id);
+	gemalto_call_common(buf, vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_create_multiparty(struct ofono_voicecall *vc,
+					ofono_voicecall_cb_t cb, void *data)
+{
+	gemalto_call_common("AT+CHLD=3", vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_transfer(struct ofono_voicecall *vc,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	/* Held & Active */
+	unsigned int affected = (1 << CALL_STATUS_ACTIVE) |
+				(1 << CALL_STATUS_HELD);
+
+	/* Transfer can puts held & active calls together and disconnects
+	 * from both.  However, some networks support transferring of
+	 * dialing/ringing calls as well.
+	 */
+	affected |= (1 << CALL_STATUS_DIALING) |
+				(1 << CALL_STATUS_ALERTING);
+
+	gemalto_call_common("AT+CHLD=4", vc, generic_cb, affected, cb, data);
+}
+
+static void gemalto_send_dtmf(struct ofono_voicecall *vc, const char *dtmf,
+			ofono_voicecall_cb_t cb, void *data)
+{
+	int len = strlen(dtmf);
+	int s;
+	int i;
+	char *buf;
+	struct ofono_modem *modem = ofono_voicecall_get_modem(vc);
+	int use_quotes = ofono_modem_get_integer(modem, "GemaltoVtsQuotes");
+
+	/* strlen("+VTS=\"T\";") = 9 + initial AT + null */
+	buf = g_new(char, len * 9 + 3);
+
+	if (use_quotes)
+		s = sprintf(buf, "AT+VTS=\"%c\"", dtmf[0]);
+	else
+		s = sprintf(buf, "AT+VTS=%c", dtmf[0]);
+
+	for (i = 1; i < len; i++) {
+
+		if (use_quotes)
+			s += sprintf(buf + s, ";+VTS=\"%c\"", dtmf[i]);
+		else
+			s += sprintf(buf + s, ";+VTS=%c", dtmf[i]);
+	}
+
+	gemalto_call_common(buf, vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_dial(struct ofono_voicecall *vc,
+			const struct ofono_phone_number *ph,
+			enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
+			void *data)
+{
+	struct cb_data *cbd = cb_data_new(cb, data);
+	char buf[256];
+	size_t len;
+
+	cbd->user = vc;
+
+	if (ph->type == 145)
+		len = snprintf(buf, sizeof(buf), "ATD+%s", ph->number);
+	else
+		len = snprintf(buf, sizeof(buf), "ATD%s", ph->number);
+
+	switch (clir) {
+	case OFONO_CLIR_OPTION_INVOCATION:
+		len += snprintf(buf+len, sizeof(buf)-len, "I");
+		break;
+	case OFONO_CLIR_OPTION_SUPPRESSION:
+		len += snprintf(buf+len, sizeof(buf)-len, "i");
+		break;
+	default:
+		break;
+	}
+
+	snprintf(buf+len, sizeof(buf)-len, ";");
+
+	gemalto_call_common(buf, vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_parse_slcc(GAtResult *result, GSList **l,
+				ofono_bool_t *ret_mpty, gboolean *last)
+{
+	GAtResultIter iter;
+	int id, dir, status, type;
+	ofono_bool_t mpty;
+	struct ofono_call *call;
+	const char *str = "";
+	int number_type = 129;
+
+	if (last)
+		*last = TRUE;
+
+	g_at_result_iter_init(&iter, result);
+
+	g_at_result_iter_next(&iter, "^SLCC:");
+
+	if (!g_at_result_iter_next_number(&iter, &id))
+		return;
+
+	if (last)
+		*last = FALSE;
+
+	if (id == 0)
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &dir))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &status))
+		return;
+
+	if (status > 5)
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &type))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &mpty))
+		return;
+
+	/* skip 'Reserved=0' parameter, only difference from CLCC */
+	if (!g_at_result_iter_skip_next(&iter))
+		return;
+
+	if (g_at_result_iter_next_string(&iter, &str))
+		g_at_result_iter_next_number(&iter, &number_type);
+
+	call = g_new0(struct ofono_call, 1);
+	ofono_call_init(call);
+	call->id = id;
+	call->direction = dir;
+	call->status = status;
+	call->type = type;
+	strncpy(call->phone_number.number, str,
+			OFONO_MAX_PHONE_NUMBER_LENGTH);
+	call->phone_number.type = number_type;
+
+	if (strlen(str) > 0)
+		call->clip_validity = 2;
+	else
+		call->clip_validity = 0;
+
+	*l = g_slist_insert_sorted(*l, call, at_util_call_compare);
+
+	if (ret_mpty)
+		*ret_mpty = mpty;
+
+}
+
+static void clcc_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GSList *l;
+
+	if (!ok)
+		return;
+
+	vd->calls = at_util_parse_clcc(result, NULL);
+
+	for (l = vd->calls; l; l = l->next)
+		ofono_voicecall_notify(vc, l->data);
+}
+
+/*
+ * ^SLCC, except for one RFU parameter (see above in the parsing), is identical
+ * to +CLCC, but as URC it is parsed line by line, and the last line is
+ * indicated by an empty "^SLCC:" (equivalent to the "OK" for CLCC).
+ */
+static void slcc_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	GSList *n, *o;
+	struct ofono_call *nc, *oc;
+	gboolean last;
+
+	gemalto_parse_slcc(result, &vd->new_calls, NULL, &last);
+
+	if (!last)
+		return;
+
+	n = vd->new_calls;
+	o = vd->calls;
+
+	while (n || o) {
+		nc = n ? n->data : NULL;
+		oc = o ? o->data : NULL;
+
+		if (oc && (nc == NULL || (nc->id > oc->id))) {
+			enum ofono_disconnect_reason reason;
+
+			if (vd->local_release & (1 << oc->id))
+				reason = OFONO_DISCONNECT_REASON_LOCAL_HANGUP;
+			else
+				reason = OFONO_DISCONNECT_REASON_REMOTE_HANGUP;
+
+			if (!oc->type)
+				ofono_voicecall_disconnected(vc, oc->id,
+								reason, NULL);
+
+			o = o->next;
+		} else if (nc && (oc == NULL || (nc->id < oc->id))) {
+
+			if (nc->type == 0) /* new call, signal it */
+				ofono_voicecall_notify(vc, nc);
+
+			n = n->next;
+		} else {
+
+			DBG("modify call part");
+
+			/* notify in case of changes */
+			if (memcmp(nc, oc, sizeof(*nc)))
+				ofono_voicecall_notify(vc, nc);
+
+			n = n->next;
+			o = o->next;
+		}
+	}
+
+	g_slist_free_full(vd->calls, g_free);
+	vd->calls = vd->new_calls;
+	vd->new_calls = NULL;
+	vd->local_release = 0;
+}
+
+static void cssi_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	GAtResultIter iter;
+	int code, index;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CSSI:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &code))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &index))
+		index = 0;
+
+	ofono_voicecall_ssn_mo_notify(vc, 0, code, index);
+}
+
+static void cssu_notify(GAtResult *result, gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	GAtResultIter iter;
+	int code;
+	int index;
+	const char *num;
+	struct ofono_phone_number ph;
+
+	ph.number[0] = '\0';
+	ph.type = 129;
+
+	g_at_result_iter_init(&iter, result);
+
+	if (!g_at_result_iter_next(&iter, "+CSSU:"))
+		return;
+
+	if (!g_at_result_iter_next_number(&iter, &code))
+		return;
+
+	if (!g_at_result_iter_next_number_default(&iter, -1, &index))
+		goto out;
+
+	if (!g_at_result_iter_next_string(&iter, &num))
+		goto out;
+
+	strncpy(ph.number, num, OFONO_MAX_PHONE_NUMBER_LENGTH);
+
+	if (!g_at_result_iter_next_number(&iter, &ph.type))
+		return;
+
+out:
+	ofono_voicecall_ssn_mt_notify(vc, 0, code, index, &ph);
+}
+
+static void gemalto_voicecall_initialized(gboolean ok, GAtResult *result,
+					gpointer user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	DBG("voicecall_init: registering to notifications");
+
+	/* NO CARRIER, NO ANSWER, BUSY, NO DIALTONE are handled through SLCC */
+	g_at_chat_register(vd->chat, "^SLCC:", slcc_notify, FALSE, vc, NULL);
+	g_at_chat_register(vd->chat, "+CSSI:", cssi_notify, FALSE, vc, NULL);
+	g_at_chat_register(vd->chat, "+CSSU:", cssu_notify, FALSE, vc, NULL);
+
+	ofono_voicecall_register(vc);
+
+	/* Populate the call list */
+	g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix, clcc_cb, vc, NULL);
+}
+
+static int gemalto_voicecall_probe(struct ofono_voicecall *vc,
+					unsigned int vendor, void *data)
+{
+	GAtChat *chat = data;
+	struct voicecall_data *vd;
+
+	vd = g_new0(struct voicecall_data, 1);
+	vd->chat = g_at_chat_clone(chat);
+	ofono_voicecall_set_data(vc, vd);
+	g_at_chat_send(vd->chat, "AT+CSSN=1,1", NULL, NULL, NULL, NULL);
+	g_at_chat_send(vd->chat, "AT^SLCC=1", NULL,
+			gemalto_voicecall_initialized, vc, NULL);
+	return 0;
+}
+
+static void gemalto_voicecall_remove(struct ofono_voicecall *vc)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+
+	ofono_voicecall_set_data(vc, NULL);
+
+	g_at_chat_unref(vd->chat);
+	g_free(vd);
+}
+
+static struct ofono_voicecall_driver driver = {
+	.name			= "gemaltomodem",
+	.probe			= gemalto_voicecall_probe,
+	.remove			= gemalto_voicecall_remove,
+	.dial			= gemalto_dial,
+	.answer			= gemalto_answer,
+	.hangup_all		= gemalto_hangup_all,
+	.hangup_active		= gemalto_hangup,
+	.hold_all_active	= gemalto_hold_all_active,
+	.release_all_held	= gemalto_release_all_held,
+	.set_udub		= gemalto_set_udub,
+	.release_all_active	= gemalto_release_all_active,
+	.release_specific	= gemalto_release_specific,
+	.private_chat		= gemalto_private_chat,
+	.create_multiparty	= gemalto_create_multiparty,
+	.transfer		= gemalto_transfer,
+	.deflect		= NULL,
+	.swap_without_accept	= NULL,
+	.send_tones		= gemalto_send_dtmf
+};
+
+void gemalto_voicecall_init(void)
+{
+	ofono_voicecall_driver_register(&driver);
+}
+
+void gemalto_voicecall_exit(void)
+{
+	ofono_voicecall_driver_unregister(&driver);
+}
-- 
2.17.1

[PATCH v4 2/2] plugins/gemalto: added voice support

[Giacinto Cifelli]

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

The plugin for Gemalto modems is enriched with all voice-related atoms,
as well as USSD.
All except the voicecall itself are from the atmodem, while the
voicecall is from gemaltomodem.
---
 plugins/gemalto.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 0fcf30d8..5d3c77a9 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2017 Vincent Cesson. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
  *
  *  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
@@ -571,6 +572,10 @@ static void gemalto_pre_sim(struct ofono_modem *modem)
 
 	ofono_devinfo_create(modem, 0, "atmodem", data->app);
 	ofono_location_reporting_create(modem, 0, "gemaltomodem", data->app);
+
+	ofono_modem_set_integer(modem, "GemaltoVtsQuotes", 1);
+	ofono_voicecall_create(modem, 0, "gemaltomodem", data->app);
+
 	data->sim = ofono_sim_create(modem, OFONO_VENDOR_GEMALTO, "atmodem",
 						data->app);
 
@@ -597,6 +602,13 @@ static void gemalto_post_sim(struct ofono_modem *modem)
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
 
+	ofono_ussd_create(modem, 0, "atmodem", data->app);
+
+	ofono_call_forwarding_create(modem, 0, "atmodem", data->app);
+	ofono_call_settings_create(modem, 0, "atmodem", data->app);
+	ofono_call_meter_create(modem, 0, "atmodem", data->app);
+	ofono_call_barring_create(modem, 0, "atmodem", data->app);
+
 	if (!g_strcmp0(model, GEMALTO_MODEL_ALS3_PLS8x))
 		ofono_lte_create(modem, OFONO_VENDOR_CINTERION,
 						"atmodem", data->app);
-- 
2.17.1

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Jonas Bonn]

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

Hi,


On 17/10/18 06:52, Giacinto Cifelli wrote:
> Added voicecall atom specific for Gemalto modems.
> This atom uses the URC ^SLCC to monitor the call status, as well as
> incoming calls.
> Note the use in the atom of the variable GemaltoVtsQuotes: this is
> needed to support future modules, as of today not yet available in the
> plugin.
> ---
>   Makefile.am                         |   3 +-
>   drivers/gemaltomodem/gemaltomodem.c |   3 +
>   drivers/gemaltomodem/gemaltomodem.h |   6 +
>   drivers/gemaltomodem/voicecall.c    | 581 ++++++++++++++++++++++++++++
>   4 files changed, 592 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gemaltomodem/voicecall.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 6667524f..e8e4ed95 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -397,7 +397,8 @@ builtin_modules += gemaltomodem
>   builtin_sources += drivers/atmodem/atutil.h \
>   			drivers/gemaltomodem/gemaltomodem.h \
>   			drivers/gemaltomodem/gemaltomodem.c \
> -			drivers/gemaltomodem/location-reporting.c
> +			drivers/gemaltomodem/location-reporting.c \
> +			drivers/gemaltomodem/voicecall.c
>   
>   builtin_modules += xmm7modem
>   builtin_sources += drivers/atmodem/atutil.h \
> diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
> index 91cf238a..4818ac66 100644
> --- a/drivers/gemaltomodem/gemaltomodem.c
> +++ b/drivers/gemaltomodem/gemaltomodem.c
> @@ -3,6 +3,7 @@
>    *  oFono - Open Source Telephony
>    *
>    *  Copyright (C) 2017 Vincent Cesson. All rights reserved.
> + *  Copyright (C) 2018 Gemalto M2M

OK, this bugs me enough that I need to comment on it.  I find updating 
the copyright header for patches such as this to be _inappropriate_.

i)  The patch is trivial
ii)  If everyone who sends a patch updates the copyright header then we 
end up with 300 lines of copyright header in every source file
iii)  The copyright header barely serves any purpose as it is; as long 
as there is _one_ copyright line in the source then the international 
conventions are applicable in all jurisdictions... i.e. there is never 
any legal reason to add a second copyright line and even the first one 
is mostly not necessary
iv)  Authorship and ownership are tracked in git in case there are ever 
any issues
v)  The one thing that the line brings is a bit of credit to an actor 
who does _major_ work on the file in question, so it's ok to add a line 
if you want visible credit for a _significant_ contribution... a bit of 
free advertising for yourself.  But advertising comes at a cost, so the 
contribution should be significant.

Anyway, that's my two bits worth...

>    *
>    *  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
> @@ -35,12 +36,14 @@
>   static int gemaltomodem_init(void)
>   {
>   	gemalto_location_reporting_init();
> +	gemalto_voicecall_init();
>   
>   	return 0;
>   }
>   
>   static void gemaltomodem_exit(void)
>   {
> +	gemalto_voicecall_exit();
>   	gemalto_location_reporting_exit();
>   }
>   
> diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
> index 7ea1e8fb..c6667411 100644
> --- a/drivers/gemaltomodem/gemaltomodem.h
> +++ b/drivers/gemaltomodem/gemaltomodem.h
> @@ -3,6 +3,7 @@
>    *  oFono - Open Source Telephony
>    *
>    *  Copyright (C) 2017 Vincent Cesson. All rights reserved.
> + *  Copyright (C) 2018 Gemalto M2M

Ick... consider what you are providing here:  function declarations.  
What does copyright on this mean?

>    *
>    *  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
> @@ -20,6 +21,11 @@
>    */
>   
>   #include <drivers/atmodem/atutil.h>
> +#include "gemaltoutil.h"
>   
>   extern void gemalto_location_reporting_init();
>   extern void gemalto_location_reporting_exit();
> +
> +extern void gemalto_voicecall_init();
> +extern void gemalto_voicecall_exit();
> +

Why are these "extern"?

> diff --git a/drivers/gemaltomodem/voicecall.c b/drivers/gemaltomodem/voicecall.c
> new file mode 100644
> index 00000000..8e2c7e10
> --- /dev/null
> +++ b/drivers/gemaltomodem/voicecall.c
> @@ -0,0 +1,581 @@
> +/*
> + *
> + *  oFono - Open Source Telephony
> + *
> + *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
> + *  Copyright (C) 2018 Gemalto M2M

OK... here the copyright line feels appropriate.

> + *
> + *  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

What requires _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/voicecall.h>
> +
> +#include "gatchat.h"
> +#include "gatresult.h"
> +
> +#include "common.h"
> +
> +#include "gemaltomodem.h"
> +
> +static const char *clcc_prefix[] = { "+CLCC:", NULL };
> +static const char *none_prefix[] = { NULL };
> +
> +struct voicecall_data {
> +	GAtChat *chat;
> +	GSList *calls;
> +	unsigned int local_release;
> +	GSList *new_calls;
> +};
> +
> +struct release_id_req {
> +	struct ofono_voicecall *vc;
> +	ofono_voicecall_cb_t cb;
> +	void *data;
> +	int id;
> +};
> +
> +struct change_state_req {
> +	struct ofono_voicecall *vc;
> +	ofono_voicecall_cb_t cb;
> +	void *data;
> +	int affected_types;
> +};
> +
> +static void generic_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct change_state_req *req = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
> +	struct ofono_error error;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (ok && req->affected_types) {
> +		GSList *l;
> +		struct ofono_call *call;
> +
> +		for (l = vd->calls; l; l = l->next) {
> +			call = l->data;
> +
> +			if (req->affected_types & (1 << call->status))
> +				vd->local_release |= (1 << call->id);
> +		}
> +	}
> +
> +	req->cb(&error, req->data);
> +}
> +
> +static void gemalto_call_common(const char *cmd, struct ofono_voicecall *vc,
> +			GAtResultFunc result_cb, unsigned int affected_types,
> +			ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct change_state_req *req = g_new0(struct change_state_req, 1);
> +
> +	req->vc = vc;
> +	req->cb = cb;
> +	req->data = data;
> +	req->affected_types = affected_types;
> +
> +	if (g_at_chat_send(vd->chat, cmd, none_prefix,
> +				result_cb, req, g_free) > 0)
> +		return;
> +
> +	g_free(req);
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void gemalto_answer(struct ofono_voicecall *vc,
> +			ofono_voicecall_cb_t cb, void *data)
> +{
> +	gemalto_call_common("ATA", vc, generic_cb, 0, cb, data);
> +}
> +
> +static void gemalto_hangup_all(struct ofono_voicecall *vc,
> +			ofono_voicecall_cb_t cb, void *data)
> +{
> +	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
> +				(1 << CALL_STATUS_DIALING) |
> +				(1 << CALL_STATUS_ALERTING) |
> +				(1 << CALL_STATUS_WAITING) |
> +				(1 << CALL_STATUS_HELD) |
> +				(1 << CALL_STATUS_ACTIVE);
> +
> +	/* Hangup all calls */
> +	gemalto_call_common("AT+CHUP", vc, generic_cb, affected, cb, data);
> +}
> +
> +static void gemalto_hangup(struct ofono_voicecall *vc,
> +			ofono_voicecall_cb_t cb, void *data)
> +{
> +	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
> +
> +	/* Hangup current active call */
> +	gemalto_call_common("AT+CHLD=1", vc, generic_cb, affected, cb, data);
> +}
> +
> +static void gemalto_hold_all_active(struct ofono_voicecall *vc,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
> +	gemalto_call_common("AT+CHLD=2", vc, generic_cb, affected, cb, data);
> +}
> +
> +static void gemalto_release_all_held(struct ofono_voicecall *vc,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
> +				(1 << CALL_STATUS_WAITING);
> +
> +	gemalto_call_common("AT+CHLD=0", vc, generic_cb, affected, cb, data);
> +}
> +
> +static void gemalto_set_udub(struct ofono_voicecall *vc,
> +			ofono_voicecall_cb_t cb, void *data)
> +{
> +	unsigned int affected = (1 << CALL_STATUS_INCOMING) |
> +				(1 << CALL_STATUS_WAITING);
> +
> +	gemalto_call_common("AT+CHLD=0", vc, generic_cb, affected, cb, data);
> +}
> +
> +static void gemalto_release_all_active(struct ofono_voicecall *vc,
> +					ofono_voicecall_cb_t cb, void *data)
> +{
> +	unsigned int affected = (1 << CALL_STATUS_ACTIVE);
> +
> +	gemalto_call_common("AT+CHLD=1", vc, generic_cb, affected, cb, data);
> +}
> +
> +static void release_id_cb(gboolean ok, GAtResult *result,
> +				gpointer user_data)
> +{
> +	struct release_id_req *req = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
> +	struct ofono_error error;
> +
> +	decode_at_error(&error, g_at_result_final_response(result));
> +
> +	if (ok)
> +		vd->local_release = 1 << req->id;
> +
> +	req->cb(&error, req->data);
> +}
> +
> +static void gemalto_release_specific(struct ofono_voicecall *vc, int id,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct release_id_req *req = g_new0(struct release_id_req, 1);
> +	char buf[32];
> +
> +	req->vc = vc;
> +	req->cb = cb;
> +	req->data = data;
> +	req->id = id;
> +
> +	snprintf(buf, sizeof(buf), "AT+CHLD=1%d", id);
> +
> +	if (g_at_chat_send(vd->chat, buf, none_prefix,
> +				release_id_cb, req, g_free) > 0)
> +		return;
> +
> +	g_free(req);
> +	CALLBACK_WITH_FAILURE(cb, data);
> +}
> +
> +static void gemalto_private_chat(struct ofono_voicecall *vc, int id,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	char buf[32];
> +
> +	snprintf(buf, sizeof(buf), "AT+CHLD=2%d", id);
> +	gemalto_call_common(buf, vc, generic_cb, 0, cb, data);
> +}
> +
> +static void gemalto_create_multiparty(struct ofono_voicecall *vc,
> +					ofono_voicecall_cb_t cb, void *data)
> +{
> +	gemalto_call_common("AT+CHLD=3", vc, generic_cb, 0, cb, data);
> +}
> +
> +static void gemalto_transfer(struct ofono_voicecall *vc,
> +			ofono_voicecall_cb_t cb, void *data)
> +{
> +	/* Held & Active */
> +	unsigned int affected = (1 << CALL_STATUS_ACTIVE) |
> +				(1 << CALL_STATUS_HELD);
> +
> +	/* Transfer can puts held & active calls together and disconnects
> +	 * from both.  However, some networks support transferring of
> +	 * dialing/ringing calls as well.
> +	 */
> +	affected |= (1 << CALL_STATUS_DIALING) |
> +				(1 << CALL_STATUS_ALERTING);
> +
> +	gemalto_call_common("AT+CHLD=4", vc, generic_cb, affected, cb, data);
> +}
> +
> +static void gemalto_send_dtmf(struct ofono_voicecall *vc, const char *dtmf,
> +			ofono_voicecall_cb_t cb, void *data)
> +{
> +	int len = strlen(dtmf);
> +	int s;
> +	int i;
> +	char *buf;
> +	struct ofono_modem *modem = ofono_voicecall_get_modem(vc);
> +	int use_quotes = ofono_modem_get_integer(modem, "GemaltoVtsQuotes");
> +
> +	/* strlen("+VTS=\"T\";") = 9 + initial AT + null */
> +	buf = g_new(char, len * 9 + 3);

Use alloca() instead... currently you are leaking buf.
> +
> +	if (use_quotes)
> +		s = sprintf(buf, "AT+VTS=\"%c\"", dtmf[0]);
> +	else
> +		s = sprintf(buf, "AT+VTS=%c", dtmf[0]);
> +
> +	for (i = 1; i < len; i++) {
> +
> +		if (use_quotes)
> +			s += sprintf(buf + s, ";+VTS=\"%c\"", dtmf[i]);
> +		else
> +			s += sprintf(buf + s, ";+VTS=%c", dtmf[i]);
> +	}
> +
> +	gemalto_call_common(buf, vc, generic_cb, 0, cb, data);
> +}
> +
> +static void gemalto_dial(struct ofono_voicecall *vc,
> +			const struct ofono_phone_number *ph,
> +			enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
> +			void *data)
> +{
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	char buf[256];
> +	size_t len;
> +
> +	cbd->user = vc;
> +
> +	if (ph->type == 145)
> +		len = snprintf(buf, sizeof(buf), "ATD+%s", ph->number);
> +	else
> +		len = snprintf(buf, sizeof(buf), "ATD%s", ph->number);
> +
> +	switch (clir) {
> +	case OFONO_CLIR_OPTION_INVOCATION:
> +		len += snprintf(buf+len, sizeof(buf)-len, "I");
> +		break;
> +	case OFONO_CLIR_OPTION_SUPPRESSION:
> +		len += snprintf(buf+len, sizeof(buf)-len, "i");
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	snprintf(buf+len, sizeof(buf)-len, ";");
> +
> +	gemalto_call_common(buf, vc, generic_cb, 0, cb, data);
> +}
> +
> +static void gemalto_parse_slcc(GAtResult *result, GSList **l,
> +				ofono_bool_t *ret_mpty, gboolean *last)
> +{
> +	GAtResultIter iter;
> +	int id, dir, status, type;
> +	ofono_bool_t mpty;
> +	struct ofono_call *call;
> +	const char *str = "";
> +	int number_type = 129;
> +
> +	if (last)
> +		*last = TRUE;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	g_at_result_iter_next(&iter, "^SLCC:");
> +
> +	if (!g_at_result_iter_next_number(&iter, &id))
> +		return;
> +
> +	if (last)
> +		*last = FALSE;
> +
> +	if (id == 0)
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &dir))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &status))
> +		return;
> +
> +	if (status > 5)
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &type))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &mpty))
> +		return;
> +
> +	/* skip 'Reserved=0' parameter, only difference from CLCC */
> +	if (!g_at_result_iter_skip_next(&iter))
> +		return;
> +
> +	if (g_at_result_iter_next_string(&iter, &str))
> +		g_at_result_iter_next_number(&iter, &number_type);
> +
> +	call = g_new0(struct ofono_call, 1);
> +	ofono_call_init(call);
> +	call->id = id;
> +	call->direction = dir;
> +	call->status = status;
> +	call->type = type;
> +	strncpy(call->phone_number.number, str,
> +			OFONO_MAX_PHONE_NUMBER_LENGTH);
> +	call->phone_number.type = number_type;
> +
> +	if (strlen(str) > 0)
> +		call->clip_validity = 2;
> +	else
> +		call->clip_validity = 0;
> +
> +	*l = g_slist_insert_sorted(*l, call, at_util_call_compare);
> +
> +	if (ret_mpty)
> +		*ret_mpty = mpty;
> +
> +}
> +
> +static void clcc_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	GSList *l;
> +
> +	if (!ok)
> +		return;
> +
> +	vd->calls = at_util_parse_clcc(result, NULL);
> +
> +	for (l = vd->calls; l; l = l->next)
> +		ofono_voicecall_notify(vc, l->data);
> +}
> +
> +/*
> + * ^SLCC, except for one RFU parameter (see above in the parsing), is identical
> + * to +CLCC, but as URC it is parsed line by line, and the last line is
> + * indicated by an empty "^SLCC:" (equivalent to the "OK" for CLCC).
> + */
> +static void slcc_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	GSList *n, *o;
> +	struct ofono_call *nc, *oc;
> +	gboolean last;
> +
> +	gemalto_parse_slcc(result, &vd->new_calls, NULL, &last);
> +
> +	if (!last)
> +		return;
> +
> +	n = vd->new_calls;
> +	o = vd->calls;
> +
> +	while (n || o) {
> +		nc = n ? n->data : NULL;
> +		oc = o ? o->data : NULL;
> +
> +		if (oc && (nc == NULL || (nc->id > oc->id))) {
> +			enum ofono_disconnect_reason reason;
> +
> +			if (vd->local_release & (1 << oc->id))
> +				reason = OFONO_DISCONNECT_REASON_LOCAL_HANGUP;
> +			else
> +				reason = OFONO_DISCONNECT_REASON_REMOTE_HANGUP;
> +
> +			if (!oc->type)
> +				ofono_voicecall_disconnected(vc, oc->id,
> +								reason, NULL);
> +
> +			o = o->next;
> +		} else if (nc && (oc == NULL || (nc->id < oc->id))) {
> +
> +			if (nc->type == 0) /* new call, signal it */
> +				ofono_voicecall_notify(vc, nc);
> +
> +			n = n->next;
> +		} else {
> +
> +			DBG("modify call part");
> +
> +			/* notify in case of changes */
> +			if (memcmp(nc, oc, sizeof(*nc)))
> +				ofono_voicecall_notify(vc, nc);
> +
> +			n = n->next;
> +			o = o->next;
> +		}
> +	}
> +
> +	g_slist_free_full(vd->calls, g_free);
> +	vd->calls = vd->new_calls;
> +	vd->new_calls = NULL;
> +	vd->local_release = 0;
> +}
> +
> +static void cssi_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	GAtResultIter iter;
> +	int code, index;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CSSI:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &code))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &index))
> +		index = 0;
> +
> +	ofono_voicecall_ssn_mo_notify(vc, 0, code, index);
> +}
> +
> +static void cssu_notify(GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	GAtResultIter iter;
> +	int code;
> +	int index;
> +	const char *num;
> +	struct ofono_phone_number ph;
> +
> +	ph.number[0] = '\0';
> +	ph.type = 129;
> +
> +	g_at_result_iter_init(&iter, result);
> +
> +	if (!g_at_result_iter_next(&iter, "+CSSU:"))
> +		return;
> +
> +	if (!g_at_result_iter_next_number(&iter, &code))
> +		return;
> +
> +	if (!g_at_result_iter_next_number_default(&iter, -1, &index))
> +		goto out;
> +
> +	if (!g_at_result_iter_next_string(&iter, &num))
> +		goto out;
> +
> +	strncpy(ph.number, num, OFONO_MAX_PHONE_NUMBER_LENGTH);
> +
> +	if (!g_at_result_iter_next_number(&iter, &ph.type))
> +		return;
> +
> +out:
> +	ofono_voicecall_ssn_mt_notify(vc, 0, code, index, &ph);
> +}
> +
> +static void gemalto_voicecall_initialized(gboolean ok, GAtResult *result,
> +					gpointer user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +
> +	DBG("voicecall_init: registering to notifications");
> +
> +	/* NO CARRIER, NO ANSWER, BUSY, NO DIALTONE are handled through SLCC */
> +	g_at_chat_register(vd->chat, "^SLCC:", slcc_notify, FALSE, vc, NULL);
> +	g_at_chat_register(vd->chat, "+CSSI:", cssi_notify, FALSE, vc, NULL);
> +	g_at_chat_register(vd->chat, "+CSSU:", cssu_notify, FALSE, vc, NULL);
> +
> +	ofono_voicecall_register(vc);
> +
> +	/* Populate the call list */
> +	g_at_chat_send(vd->chat, "AT+CLCC", clcc_prefix, clcc_cb, vc, NULL);
> +}
> +
> +static int gemalto_voicecall_probe(struct ofono_voicecall *vc,
> +					unsigned int vendor, void *data)
> +{
> +	GAtChat *chat = data;
> +	struct voicecall_data *vd;
> +
> +	vd = g_new0(struct voicecall_data, 1);
> +	vd->chat = g_at_chat_clone(chat);
> +	ofono_voicecall_set_data(vc, vd);
> +	g_at_chat_send(vd->chat, "AT+CSSN=1,1", NULL, NULL, NULL, NULL);
> +	g_at_chat_send(vd->chat, "AT^SLCC=1", NULL,
> +			gemalto_voicecall_initialized, vc, NULL);
> +	return 0;
> +}
> +
> +static void gemalto_voicecall_remove(struct ofono_voicecall *vc)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +
> +	ofono_voicecall_set_data(vc, NULL);
> +
> +	g_at_chat_unref(vd->chat);
> +	g_free(vd);
> +}
> +
> +static struct ofono_voicecall_driver driver = {
> +	.name			= "gemaltomodem",
> +	.probe			= gemalto_voicecall_probe,
> +	.remove			= gemalto_voicecall_remove,
> +	.dial			= gemalto_dial,
> +	.answer			= gemalto_answer,
> +	.hangup_all		= gemalto_hangup_all,
> +	.hangup_active		= gemalto_hangup,
> +	.hold_all_active	= gemalto_hold_all_active,
> +	.release_all_held	= gemalto_release_all_held,
> +	.set_udub		= gemalto_set_udub,
> +	.release_all_active	= gemalto_release_all_active,
> +	.release_specific	= gemalto_release_specific,
> +	.private_chat		= gemalto_private_chat,
> +	.create_multiparty	= gemalto_create_multiparty,
> +	.transfer		= gemalto_transfer,
> +	.deflect		= NULL,
> +	.swap_without_accept	= NULL,

Are other drivers explicit about _not_ providing implementations? I'd 
drop these otherwise?

> +	.send_tones		= gemalto_send_dtmf
> +};
> +
> +void gemalto_voicecall_init(void)
> +{
> +	ofono_voicecall_driver_register(&driver);
> +}
> +
> +void gemalto_voicecall_exit(void)
> +{
> +	ofono_voicecall_driver_unregister(&driver);
> +}

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Giacinto Cifelli]

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

Hi,

On Wed, Oct 17, 2018 at 8:28 AM Jonas Bonn <jonas@southpole.se> wrote:
>
> Hi,
>
>
> On 17/10/18 06:52, Giacinto Cifelli wrote:
> > Added voicecall atom specific for Gemalto modems.
> > This atom uses the URC ^SLCC to monitor the call status, as well as
> > incoming calls.
> > Note the use in the atom of the variable GemaltoVtsQuotes: this is
> > needed to support future modules, as of today not yet available in the
> > plugin.
> > ---
> >   Makefile.am                         |   3 +-
> >   drivers/gemaltomodem/gemaltomodem.c |   3 +
> >   drivers/gemaltomodem/gemaltomodem.h |   6 +
> >   drivers/gemaltomodem/voicecall.c    | 581 ++++++++++++++++++++++++++++
> >   4 files changed, 592 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/gemaltomodem/voicecall.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 6667524f..e8e4ed95 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -397,7 +397,8 @@ builtin_modules += gemaltomodem
> >   builtin_sources += drivers/atmodem/atutil.h \
> >                       drivers/gemaltomodem/gemaltomodem.h \
> >                       drivers/gemaltomodem/gemaltomodem.c \
> > -                     drivers/gemaltomodem/location-reporting.c
> > +                     drivers/gemaltomodem/location-reporting.c \
> > +                     drivers/gemaltomodem/voicecall.c
> >
> >   builtin_modules += xmm7modem
> >   builtin_sources += drivers/atmodem/atutil.h \
> > diff --git a/drivers/gemaltomodem/gemaltomodem.c b/drivers/gemaltomodem/gemaltomodem.c
> > index 91cf238a..4818ac66 100644
> > --- a/drivers/gemaltomodem/gemaltomodem.c
> > +++ b/drivers/gemaltomodem/gemaltomodem.c
> > @@ -3,6 +3,7 @@
> >    *  oFono - Open Source Telephony
> >    *
> >    *  Copyright (C) 2017 Vincent Cesson. All rights reserved.
> > + *  Copyright (C) 2018 Gemalto M2M
>
> OK, this bugs me enough that I need to comment on it.  I find updating
> the copyright header for patches such as this to be _inappropriate_.
>
> i)  The patch is trivial
> ii)  If everyone who sends a patch updates the copyright header then we
> end up with 300 lines of copyright header in every source file
> iii)  The copyright header barely serves any purpose as it is; as long
> as there is _one_ copyright line in the source then the international
> conventions are applicable in all jurisdictions... i.e. there is never
> any legal reason to add a second copyright line and even the first one
> is mostly not necessary
> iv)  Authorship and ownership are tracked in git in case there are ever
> any issues
> v)  The one thing that the line brings is a bit of credit to an actor
> who does _major_ work on the file in question, so it's ok to add a line
> if you want visible credit for a _significant_ contribution... a bit of
> free advertising for yourself.  But advertising comes at a cost, so the
> contribution should be significant.
>
> Anyway, that's my two bits worth...

According to our licenses experts, the copyright in a file has a
specific meaning:
Whoever is mentioned there has to be consulted before changing the
type of license in a future version of the file.
For the current version is GPLv2, it is given away and won't change.
Should Intel decide to go for a closed license for this file tomorrow,
Gemalto has to be consulted first.

The copyright is something different from the authorship, that can be
consulted, as you point out, in the git log.
Personally I would get rid of the AUTHORS file entirely.

I did not add a copyright note for a 1-line fixes, but I intend to
insert it where the file is specific for gemalto modem or where the
contribution is significant.
At least when done on Gemalto time.

>
> >    *
> >    *  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
> > @@ -35,12 +36,14 @@
> >   static int gemaltomodem_init(void)
> >   {
> >       gemalto_location_reporting_init();
> > +     gemalto_voicecall_init();
> >
> >       return 0;
> >   }
> >
> >   static void gemaltomodem_exit(void)
> >   {
> > +     gemalto_voicecall_exit();
> >       gemalto_location_reporting_exit();
> >   }
> >
> > diff --git a/drivers/gemaltomodem/gemaltomodem.h b/drivers/gemaltomodem/gemaltomodem.h
> > index 7ea1e8fb..c6667411 100644
> > --- a/drivers/gemaltomodem/gemaltomodem.h
> > +++ b/drivers/gemaltomodem/gemaltomodem.h
> > @@ -3,6 +3,7 @@
> >    *  oFono - Open Source Telephony
> >    *
> >    *  Copyright (C) 2017 Vincent Cesson. All rights reserved.
> > + *  Copyright (C) 2018 Gemalto M2M
>
> Ick... consider what you are providing here:  function declarations.
> What does copyright on this mean?

above.

>
> >    *
> >    *  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
> > @@ -20,6 +21,11 @@
> >    */
> >
> >   #include <drivers/atmodem/atutil.h>
> > +#include "gemaltoutil.h"
> >
> >   extern void gemalto_location_reporting_init();
> >   extern void gemalto_location_reporting_exit();
> > +
> > +extern void gemalto_voicecall_init();
> > +extern void gemalto_voicecall_exit();
> > +
>
> Why are these "extern"?

This is a good question, but to the wrong person, I have been wondering myself.
My answer is: it has always been done so. See at the end for the full list.

>
> > diff --git a/drivers/gemaltomodem/voicecall.c b/drivers/gemaltomodem/voicecall.c
> > new file mode 100644
> > index 00000000..8e2c7e10
> > --- /dev/null
> > +++ b/drivers/gemaltomodem/voicecall.c
> > @@ -0,0 +1,581 @@
> > +/*
> > + *
> > + *  oFono - Open Source Telephony
> > + *
> > + *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
> > + *  Copyright (C) 2018 Gemalto M2M
>
> OK... here the copyright line feels appropriate.

above.

>
> > +
> > +#ifdef HAVE_CONFIG_H
> > +#include <config.h>
> > +#endif
> > +
> > +#define _GNU_SOURCE
>
> What requires _GNU_SOURCE?

I don't know. It was part of drivers/atmodem/voicecall.c.
Maybe Denis knows?

>
> > +
> > +static struct ofono_voicecall_driver driver = {
> > +     .name                   = "gemaltomodem",
> > +     .probe                  = gemalto_voicecall_probe,
> > +     .remove                 = gemalto_voicecall_remove,
> > +     .dial                   = gemalto_dial,
> > +     .answer                 = gemalto_answer,
> > +     .hangup_all             = gemalto_hangup_all,
> > +     .hangup_active          = gemalto_hangup,
> > +     .hold_all_active        = gemalto_hold_all_active,
> > +     .release_all_held       = gemalto_release_all_held,
> > +     .set_udub               = gemalto_set_udub,
> > +     .release_all_active     = gemalto_release_all_active,
> > +     .release_specific       = gemalto_release_specific,
> > +     .private_chat           = gemalto_private_chat,
> > +     .create_multiparty      = gemalto_create_multiparty,
> > +     .transfer               = gemalto_transfer,
> > +     .deflect                = NULL,
> > +     .swap_without_accept    = NULL,
>
> Are other drivers explicit about _not_ providing implementations? I'd
> drop these otherwise?
>

again, it is done so in the original drivers/atmodem/voicecall.c.

>

overall, thank you for reviewing and also for your questions, it gave
me the possibility to clarify some points.

regards,
Giacinto
---
full list of suspicious externs:

drivers/mbimmodem/mbimmodem.h:extern void mbim_devinfo_init(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_devinfo_exit(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_sim_init(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_sim_exit(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_netreg_init(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_netreg_exit(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_sms_init(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_sms_exit(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_gprs_init(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_gprs_exit(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_gprs_context_init(void);
drivers/mbimmodem/mbimmodem.h:extern void mbim_gprs_context_exit(void);
drivers/dunmodem/dunmodem.h:extern void dun_netreg_init(void);
drivers/dunmodem/dunmodem.h:extern void dun_netreg_exit(void);
drivers/dunmodem/dunmodem.h:extern void dun_gprs_init(void);
drivers/dunmodem/dunmodem.h:extern void dun_gprs_exit(void);
drivers/hsomodem/hsomodem.h:extern void hso_gprs_context_init(void);
drivers/hsomodem/hsomodem.h:extern void hso_gprs_context_exit(void);
drivers/hsomodem/hsomodem.h:extern void hso_radio_settings_init(void);
drivers/hsomodem/hsomodem.h:extern void hso_radio_settings_exit(void);
drivers/swmodem/swmodem.h:extern void sw_gprs_context_init(void);
drivers/swmodem/swmodem.h:extern void sw_gprs_context_exit(void);
drivers/gemaltomodem/gemaltomodem.h:extern void
gemalto_location_reporting_init();
drivers/gemaltomodem/gemaltomodem.h:extern void
gemalto_location_reporting_exit();
drivers/gemaltomodem/gemaltomodem.h:extern void gemalto_voicecall_init();
drivers/gemaltomodem/gemaltomodem.h:extern void gemalto_voicecall_exit();
drivers/nwmodem/nwmodem.h:extern void nw_radio_settings_init(void);
drivers/nwmodem/nwmodem.h:extern void nw_radio_settings_exit(void);
drivers/cdmamodem/cdmamodem.h:extern void cdma_voicecall_init(void);
drivers/cdmamodem/cdmamodem.h:extern void cdma_voicecall_exit(void);
drivers/cdmamodem/cdmamodem.h:extern void cdma_devinfo_init(void);
drivers/cdmamodem/cdmamodem.h:extern void cdma_devinfo_exit(void);
drivers/cdmamodem/cdmamodem.h:extern void cdma_connman_init(void);
drivers/cdmamodem/cdmamodem.h:extern void cdma_connman_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_devinfo_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_devinfo_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_call_volume_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_call_volume_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_voicecall_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_voicecall_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_sim_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_sim_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_sms_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_sms_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_netreg_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_netreg_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_gprs_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_gprs_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_gprs_context_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_gprs_context_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_ussd_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_ussd_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_call_settings_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_call_settings_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_call_forwarding_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_call_forwarding_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_radio_settings_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_radio_settings_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_call_barring_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_call_barring_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_phonebook_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_phonebook_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_netmon_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_netmon_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_stk_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_stk_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_cbs_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_cbs_exit(void);
drivers/rilmodem/rilmodem.h:extern void ril_lte_init(void);
drivers/rilmodem/rilmodem.h:extern void ril_lte_exit(void);
drivers/iceramodem/iceramodem.h:extern void icera_gprs_context_init(void);
drivers/iceramodem/iceramodem.h:extern void icera_gprs_context_exit(void);
drivers/iceramodem/iceramodem.h:extern void icera_radio_settings_init(void);
drivers/iceramodem/iceramodem.h:extern void icera_radio_settings_exit(void);
drivers/ubloxmodem/ubloxmodem.h:extern void ublox_gprs_context_init(void);
drivers/ubloxmodem/ubloxmodem.h:extern void ublox_gprs_context_exit(void);
drivers/ubloxmodem/ubloxmodem.h:extern void ublox_netmon_init(void);
drivers/ubloxmodem/ubloxmodem.h:extern void ublox_netmon_exit(void);
drivers/ubloxmodem/ubloxmodem.h:extern void ublox_lte_init(void);
drivers/ubloxmodem/ubloxmodem.h:extern void ublox_lte_exit(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_netreg_init(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_netreg_exit(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_call_volume_init(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_call_volume_exit(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_voicecall_init(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_voicecall_exit(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_handsfree_init(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_handsfree_exit(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_siri_init(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_siri_exit(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_devinfo_init(void);
drivers/hfpmodem/hfpmodem.h:extern void hfp_devinfo_exit(void);
drivers/speedupmodem/speedupmodem.h:extern void speedup_ussd_init(void);
drivers/speedupmodem/speedupmodem.h:extern void speedup_ussd_exit(void);
drivers/atmodem/atmodem.h:extern void at_netreg_init(void);
drivers/atmodem/atmodem.h:extern void at_netreg_exit(void);
drivers/atmodem/atmodem.h:extern void at_call_forwarding_init(void);
drivers/atmodem/atmodem.h:extern void at_call_forwarding_exit(void);
drivers/atmodem/atmodem.h:extern void at_call_settings_init(void);
drivers/atmodem/atmodem.h:extern void at_call_settings_exit(void);
drivers/atmodem/atmodem.h:extern void at_ussd_init(void);
drivers/atmodem/atmodem.h:extern void at_ussd_exit(void);
drivers/atmodem/atmodem.h:extern void at_voicecall_init(void);
drivers/atmodem/atmodem.h:extern void at_voicecall_exit(void);
drivers/atmodem/atmodem.h:extern void at_call_meter_init(void);
drivers/atmodem/atmodem.h:extern void at_call_meter_exit(void);
drivers/atmodem/atmodem.h:extern void at_call_barring_init(void);
drivers/atmodem/atmodem.h:extern void at_call_barring_exit(void);
drivers/atmodem/atmodem.h:extern void at_sim_init(void);
drivers/atmodem/atmodem.h:extern void at_sim_exit(void);
drivers/atmodem/atmodem.h:extern void at_stk_init(void);
drivers/atmodem/atmodem.h:extern void at_stk_exit(void);
drivers/atmodem/atmodem.h:extern void at_sms_init(void);
drivers/atmodem/atmodem.h:extern void at_sms_exit(void);
drivers/atmodem/atmodem.h:extern void at_phonebook_init(void);
drivers/atmodem/atmodem.h:extern void at_phonebook_exit(void);
drivers/atmodem/atmodem.h:extern void at_devinfo_init(void);
drivers/atmodem/atmodem.h:extern void at_devinfo_exit(void);
drivers/atmodem/atmodem.h:extern void at_cbs_init(void);
drivers/atmodem/atmodem.h:extern void at_cbs_exit(void);
drivers/atmodem/atmodem.h:extern void at_call_volume_init(void);
drivers/atmodem/atmodem.h:extern void at_call_volume_exit(void);
drivers/atmodem/atmodem.h:extern void at_gprs_init(void);
drivers/atmodem/atmodem.h:extern void at_gprs_exit(void);
drivers/atmodem/atmodem.h:extern void at_gprs_context_init(void);
drivers/atmodem/atmodem.h:extern void at_gprs_context_exit(void);
drivers/atmodem/atmodem.h:extern void at_sim_auth_init(void);
drivers/atmodem/atmodem.h:extern void at_sim_auth_exit(void);
drivers/atmodem/atmodem.h:extern void at_gnss_init(void);
drivers/atmodem/atmodem.h:extern void at_gnss_exit(void);
drivers/atmodem/atmodem.h:extern void at_lte_init(void);
drivers/atmodem/atmodem.h:extern void at_lte_exit(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_ussd_init(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_ussd_exit(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_voicecall_init(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_voicecall_exit(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_audio_settings_init(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_audio_settings_exit(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_radio_settings_init(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_radio_settings_exit(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_gprs_context_init(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_gprs_context_exit(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_cdma_netreg_init(void);
drivers/huaweimodem/huaweimodem.h:extern void huawei_cdma_netreg_exit(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_voicecall_init(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_voicecall_exit(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_audio_settings_init(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_audio_settings_exit(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_radio_settings_init(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_radio_settings_exit(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_gprs_context_init(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_gprs_context_exit(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_stk_init(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_stk_exit(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_ctm_init(void);
drivers/ifxmodem/ifxmodem.h:extern void ifx_ctm_exit(void);
drivers/xmm7modem/xmm7modem.h:extern void xmm_radio_settings_init(void);
drivers/xmm7modem/xmm7modem.h:extern void xmm_radio_settings_exit(void);
drivers/xmm7modem/xmm7modem.h:extern void xmm_ims_init(void);
drivers/xmm7modem/xmm7modem.h:extern void xmm_ims_exit(void);
drivers/xmm7modem/xmm7modem.h:extern void xmm_netmon_init(void);
drivers/xmm7modem/xmm7modem.h:extern void xmm_netmon_exit(void);
drivers/calypsomodem/calypsomodem.h:extern void calypso_voicecall_init(void);
drivers/calypsomodem/calypsomodem.h:extern void calypso_voicecall_exit(void);
drivers/calypsomodem/calypsomodem.h:extern void calypso_stk_init(void);
drivers/calypsomodem/calypsomodem.h:extern void calypso_stk_exit(void);
drivers/mbmmodem/mbmmodem.h:extern void mbm_gprs_context_init(void);
drivers/mbmmodem/mbmmodem.h:extern void mbm_gprs_context_exit(void);
drivers/mbmmodem/mbmmodem.h:extern void mbm_stk_init(void);
drivers/mbmmodem/mbmmodem.h:extern void mbm_stk_exit(void);
drivers/mbmmodem/mbmmodem.h:extern void mbm_location_reporting_init();
drivers/mbmmodem/mbmmodem.h:extern void mbm_location_reporting_exit();
drivers/isimodem/isimodem.h:extern void isi_phonebook_init(void);
drivers/isimodem/isimodem.h:extern void isi_phonebook_exit(void);
drivers/isimodem/isimodem.h:extern void isi_devinfo_init(void);
drivers/isimodem/isimodem.h:extern void isi_devinfo_exit(void);
drivers/isimodem/isimodem.h:extern void isi_netreg_init(void);
drivers/isimodem/isimodem.h:extern void isi_netreg_exit(void);
drivers/isimodem/isimodem.h:extern void isi_voicecall_init(void);
drivers/isimodem/isimodem.h:extern void isi_voicecall_exit(void);
drivers/isimodem/isimodem.h:extern void isi_sms_init(void);
drivers/isimodem/isimodem.h:extern void isi_sms_exit(void);
drivers/isimodem/isimodem.h:extern void isi_cbs_init(void);
drivers/isimodem/isimodem.h:extern void isi_cbs_exit(void);
drivers/isimodem/isimodem.h:extern void isi_sim_init(void);
drivers/isimodem/isimodem.h:extern void isi_sim_exit(void);
drivers/isimodem/isimodem.h:extern void isi_ussd_init(void);
drivers/isimodem/isimodem.h:extern void isi_ussd_exit(void);
drivers/isimodem/isimodem.h:extern void isi_call_forwarding_init(void);
drivers/isimodem/isimodem.h:extern void isi_call_forwarding_exit(void);
drivers/isimodem/isimodem.h:extern void isi_call_settings_init(void);
drivers/isimodem/isimodem.h:extern void isi_call_settings_exit(void);
drivers/isimodem/isimodem.h:extern void isi_call_barring_init(void);
drivers/isimodem/isimodem.h:extern void isi_call_barring_exit(void);
drivers/isimodem/isimodem.h:extern void isi_call_meter_init(void);
drivers/isimodem/isimodem.h:extern void isi_call_meter_exit(void);
drivers/isimodem/isimodem.h:extern void isi_radio_settings_init(void);
drivers/isimodem/isimodem.h:extern void isi_radio_settings_exit(void);
drivers/isimodem/isimodem.h:extern void isi_gprs_init(void);
drivers/isimodem/isimodem.h:extern void isi_gprs_exit(void);
drivers/isimodem/isimodem.h:extern void isi_gprs_context_init(void);
drivers/isimodem/isimodem.h:extern void isi_gprs_context_exit(void);
drivers/isimodem/isimodem.h:extern void isi_audio_settings_init(void);
drivers/isimodem/isimodem.h:extern void isi_audio_settings_exit(void);
drivers/isimodem/isimodem.h:extern void isi_uicc_init(void);
drivers/isimodem/isimodem.h:extern void isi_uicc_exit(void);
drivers/stemodem/stemodem.h:extern void ste_gprs_context_init(void);
drivers/stemodem/stemodem.h:extern void ste_gprs_context_exit(void);
drivers/stemodem/stemodem.h:extern void ste_voicecall_init(void);
drivers/stemodem/stemodem.h:extern void ste_voicecall_exit(void);
drivers/stemodem/stemodem.h:extern void ste_radio_settings_init(void);
drivers/stemodem/stemodem.h:extern void ste_radio_settings_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_devinfo_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_devinfo_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_netreg_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_netreg_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_voicecall_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_voicecall_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_sim_legacy_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_sim_legacy_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_sim_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_sim_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_sms_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_sms_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_ussd_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_ussd_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_gprs_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_gprs_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_gprs_context_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_gprs_context_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_lte_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_lte_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_radio_settings_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_radio_settings_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_location_reporting_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_location_reporting_exit(void);
drivers/qmimodem/qmimodem.h:extern void qmi_netmon_init(void);
drivers/qmimodem/qmimodem.h:extern void qmi_netmon_exit(void);
drivers/ztemodem/ztemodem.h:extern void zte_radio_settings_init(void);
drivers/ztemodem/ztemodem.h:extern void zte_radio_settings_exit(void);
drivers/telitmodem/telitmodem.h:extern void telit_location_reporting_init();
drivers/telitmodem/telitmodem.h:extern void telit_location_reporting_exit();
drivers/telitmodem/telitmodem.h:extern void telitncm_gprs_context_init();
drivers/telitmodem/telitmodem.h:extern void telitncm_gprs_context_exit();

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Giacinto Cifelli]

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

Hi again,

On Wed, Oct 17, 2018 at 8:28 AM Jonas Bonn <jonas@southpole.se> wrote:
>
> Hi,
>
>
> On 17/10/18 06:52, Giacinto Cifelli wrote:
> > Added voicecall atom specific for Gemalto modems.

> > diff --git a/drivers/gemaltomodem/voicecall.c b/drivers/gemaltomodem/voicecall.c
> > new file mode 100644
> > index 00000000..8e2c7e10
> > --- /dev/null
> > +++ b/drivers/gemaltomodem/voicecall.c
> > @@ -0,0 +1,581 @@

> > +static void gemalto_send_dtmf(struct ofono_voicecall *vc, const char *dtmf,
> > +                     ofono_voicecall_cb_t cb, void *data)
> > +{
> > +     int len = strlen(dtmf);
> > +     int s;
> > +     int i;
> > +     char *buf;
> > +     struct ofono_modem *modem = ofono_voicecall_get_modem(vc);
> > +     int use_quotes = ofono_modem_get_integer(modem, "GemaltoVtsQuotes");
> > +
> > +     /* strlen("+VTS=\"T\";") = 9 + initial AT + null */
> > +     buf = g_new(char, len * 9 + 3);
>
> Use alloca() instead... currently you are leaking buf.

Gladly, excellent idea.
Stack allocation is very sparingly used in ofono, while it would be a
better alternative in many cases.

Regards,
Giacinto

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Jonas Bonn]

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

Hi,


On 17/10/18 09:30, Giacinto Cifelli wrote:

>>> +
>>> +#ifdef HAVE_CONFIG_H
>>> +#include <config.h>
>>> +#endif
>>> +
>>> +#define _GNU_SOURCE
>> What requires _GNU_SOURCE?
> I don't know. It was part of drivers/atmodem/voicecall.c.
> Maybe Denis knows?

If you don't know that you need it, drop it.  If the code fails to build 
without it, you need it.  I can't immediately see what requires this in 
your code, but maybe there's something.  Otherwise it would be nice not 
to propagate this further when somebody cut-and-paste's your code. :)

drivers/atmodem/voicecall.c doesn't need it either.  Builds fine without.

>
>>> +
>>> +static struct ofono_voicecall_driver driver = {
>>> +     .name                   = "gemaltomodem",
>>> +     .probe                  = gemalto_voicecall_probe,
>>> +     .remove                 = gemalto_voicecall_remove,
>>> +     .dial                   = gemalto_dial,
>>> +     .answer                 = gemalto_answer,
>>> +     .hangup_all             = gemalto_hangup_all,
>>> +     .hangup_active          = gemalto_hangup,
>>> +     .hold_all_active        = gemalto_hold_all_active,
>>> +     .release_all_held       = gemalto_release_all_held,
>>> +     .set_udub               = gemalto_set_udub,
>>> +     .release_all_active     = gemalto_release_all_active,
>>> +     .release_specific       = gemalto_release_specific,
>>> +     .private_chat           = gemalto_private_chat,
>>> +     .create_multiparty      = gemalto_create_multiparty,
>>> +     .transfer               = gemalto_transfer,
>>> +     .deflect                = NULL,
>>> +     .swap_without_accept    = NULL,
>> Are other drivers explicit about _not_ providing implementations? I'd
>> drop these otherwise?
>>
> again, it is done so in the original drivers/atmodem/voicecall.c.

I would drop them...

>
> overall, thank you for reviewing and also for your questions, it gave
> me the possibility to clarify some points.
>
> regards,
> Giacinto
> ---
> full list of suspicious externs:

Thanks.  Some of those "suspicious externs" are mine, too! :)

/Jonas

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Giacinto Cifelli]

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

Hi,

On Wed, Oct 17, 2018 at 10:33 AM Jonas Bonn <jonas@southpole.se> wrote:
>
> Hi,
>
>
> On 17/10/18 09:30, Giacinto Cifelli wrote:
>
> >>> +
> >>> +#ifdef HAVE_CONFIG_H
> >>> +#include <config.h>
> >>> +#endif
> >>> +
> >>> +#define _GNU_SOURCE
> >> What requires _GNU_SOURCE?
> > I don't know. It was part of drivers/atmodem/voicecall.c.
> > Maybe Denis knows?
>
> If you don't know that you need it, drop it.  If the code fails to build
> without it, you need it.  I can't immediately see what requires this in
> your code, but maybe there's something.  Otherwise it would be nice not
> to propagate this further when somebody cut-and-paste's your code. :)
>
> drivers/atmodem/voicecall.c doesn't need it either.  Builds fine without.

it builds without. I have dropped it.
I will push a patch for atmodem/voicecall too.

>
> >
> >>> +
> >>> +static struct ofono_voicecall_driver driver = {
> >>> +     .name                   = "gemaltomodem",
> >>> +     .probe                  = gemalto_voicecall_probe,
> >>> +     .remove                 = gemalto_voicecall_remove,
> >>> +     .dial                   = gemalto_dial,
> >>> +     .answer                 = gemalto_answer,
> >>> +     .hangup_all             = gemalto_hangup_all,
> >>> +     .hangup_active          = gemalto_hangup,
> >>> +     .hold_all_active        = gemalto_hold_all_active,
> >>> +     .release_all_held       = gemalto_release_all_held,
> >>> +     .set_udub               = gemalto_set_udub,
> >>> +     .release_all_active     = gemalto_release_all_active,
> >>> +     .release_specific       = gemalto_release_specific,
> >>> +     .private_chat           = gemalto_private_chat,
> >>> +     .create_multiparty      = gemalto_create_multiparty,
> >>> +     .transfer               = gemalto_transfer,
> >>> +     .deflect                = NULL,
> >>> +     .swap_without_accept    = NULL,
> >> Are other drivers explicit about _not_ providing implementations? I'd
> >> drop these otherwise?
> >>
> > again, it is done so in the original drivers/atmodem/voicecall.c.
>
> I would drop them...

I would like to know why they weren't dropped in atmodem in the first
place, before dropping them.
Either solution is ok for me, I just want some clarification.

>
> >
> > overall, thank you for reviewing and also for your questions, it gave
> > me the possibility to clarify some points.
> >
> > regards,
> > Giacinto
> > ---
> > full list of suspicious externs:
>
> Thanks.  Some of those "suspicious externs" are mine, too! :)

so we keep them?

>
> /Jonas
>

Giacinto

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Jonas Bonn]

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



On 17/10/18 10:52, Giacinto Cifelli wrote:
>
>>>>> +
>>>>> +static struct ofono_voicecall_driver driver = {
>>>>> +     .name                   = "gemaltomodem",
>>>>> +     .probe                  = gemalto_voicecall_probe,
>>>>> +     .remove                 = gemalto_voicecall_remove,
>>>>> +     .dial                   = gemalto_dial,
>>>>> +     .answer                 = gemalto_answer,
>>>>> +     .hangup_all             = gemalto_hangup_all,
>>>>> +     .hangup_active          = gemalto_hangup,
>>>>> +     .hold_all_active        = gemalto_hold_all_active,
>>>>> +     .release_all_held       = gemalto_release_all_held,
>>>>> +     .set_udub               = gemalto_set_udub,
>>>>> +     .release_all_active     = gemalto_release_all_active,
>>>>> +     .release_specific       = gemalto_release_specific,
>>>>> +     .private_chat           = gemalto_private_chat,
>>>>> +     .create_multiparty      = gemalto_create_multiparty,
>>>>> +     .transfer               = gemalto_transfer,
>>>>> +     .deflect                = NULL,
>>>>> +     .swap_without_accept    = NULL,
>>>> Are other drivers explicit about _not_ providing implementations? I'd
>>>> drop these otherwise?
>>>>
>>> again, it is done so in the original drivers/atmodem/voicecall.c.
>> I would drop them...
> I would like to know why they weren't dropped in atmodem in the first
> place, before dropping them.
> Either solution is ok for me, I just want some clarification.

Make the declaration 'const' while you're at it.

/Jonas

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Denis Kenzior]

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

Hi Jonas,
>> +
>> +extern void gemalto_voicecall_init();
>> +extern void gemalto_voicecall_exit();
>> +
> 
> Why are these "extern"?
> 

Okay, I haven't had my coffee yet, but what is the actual problem?  All 
functions defined in the header are extern anyway, whether they're 
labeled extern or not.  So, dropping extern would have no actual effect 
besides style.

Regards,
-Denis

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Jonas Bonn]

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



On 17/10/18 17:18, Denis Kenzior wrote:
> Hi Jonas,
>>> +
>>> +extern void gemalto_voicecall_init();
>>> +extern void gemalto_voicecall_exit();
>>> +
>>
>> Why are these "extern"?
>>
> 
> Okay, I haven't had my coffee yet, but what is the actual problem?  All 
> functions defined in the header are extern anyway, whether they're 
> labeled extern or not.  So, dropping extern would have no actual effect 
> besides style.

No, of course, it's stylistic... just interesting that this particular 
style is used in these particular header files and nowhere else.  Was 
there a reason for that once?  Something to do with modules/plugins?

Grab a coffee.  The day is long!

/Jonas


> 
> Regards,
> -Denis
> _______________________________________________
> ofono mailing list
> ofono(a)ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Denis Kenzior]

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

Hi Jonas,

On 10/17/2018 11:30 AM, Jonas Bonn wrote:
> 
> 
> On 17/10/18 17:18, Denis Kenzior wrote:
>> Hi Jonas,
>>>> +
>>>> +extern void gemalto_voicecall_init();
>>>> +extern void gemalto_voicecall_exit();
>>>> +
>>>
>>> Why are these "extern"?
>>>
>>
>> Okay, I haven't had my coffee yet, but what is the actual problem?  
>> All functions defined in the header are extern anyway, whether they're 
>> labeled extern or not.  So, dropping extern would have no actual 
>> effect besides style.
> 
> No, of course, it's stylistic... just interesting that this particular 
> style is used in these particular header files and nowhere else.  Was 
> there a reason for that once?  Something to do with modules/plugins?

That is a good question.  I'm not sure there was a reason.  But then 
that code is ~10 years old, so I wouldn't remember the reason anyway.

If it bothers you so much, feel free to remove the externs.

> 
> Grab a coffee.  The day is long!
> 

Yep, having it now.  :)

Regards,
-Denis

Re: [PATCH v4 1/2] Gemalto: voicecall atom

[Giacinto Cifelli]

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

Hi Denis,

>
> Yep, having it now.  :)
>

oh good.

I am not interested in philosophy these days, but I would appreciate
some comments on the code of this patch.
So far, apart from a memory leak that I have fixed, some clarification
about headers and style, I haven't got much.
May I proceed with the next submit?

> Regards,
> -Denis