[PATCH v3 1/4] qmimodem: voicecall: Implement call dialing

[PATCH v3 1/4] qmimodem: voicecall: Implement call dialing

[Adam Pigg]

Add voicecall dialling to the qmimodem driver
Includes required infratructure and setup of the QMI services

Call State Handling
===================
On initialisation, register the all_call_status_ind callback to be
called for QMI_VOICE_IND_ALL_STATUS.  This will handle notificatiof of
call status to the rest of the system

Dial Handling
=============
The dial function sets up the parameters for the QMI_VOICE_DIAL_CALL
service.  The parameters are the number to be called and the call type,
which is currently hard coded to be QMI_VOICE_CALL_TYPE_VOICE.  The
dial_cb callback wi then be called and will receive the call-id.
---
 drivers/qmimodem/voice.h     |  17 ++
 drivers/qmimodem/voicecall.c | 479 ++++++++++++++++++++++++++++++++++-
 2 files changed, 495 insertions(+), 1 deletion(-)

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index 917e72f7..2344fd50 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -48,6 +48,9 @@ enum qmi_ussd_user_required {
 
 /* QMI service voice. Using an enum to prevent doublicated entries */
 enum voice_commands {
+	QMI_VOICE_DIAL_CALL =			0x20,
+	QMI_VOICE_ALL_CALL_STATUS_IND =		0x2e,
+	QMI_VOICE_GET_ALL_CALL_INFO =		0x2f,
 	QMI_VOICE_SUPS_NOTIFICATION_IND =	0x32,
 	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
 	QMI_VOICE_GET_CALL_WAITING =		0x34,
@@ -66,6 +69,20 @@ enum voice_commands {
 	QMI_VOICE_GET_CNAP =			0x4d
 };
 
+enum qmi_voice_call_state {
+	QMI_VOICE_CALL_STATE_IDLE = 0x0,
+	QMI_VOICE_CALL_STATE_ORIG,
+	QMI_VOICE_CALL_STATE_INCOMING,
+	QMI_VOICE_CALL_STATE_CONV,
+	QMI_VOICE_CALL_STATE_CC_IN_PROG,
+	QMI_VOICE_CALL_STATE_ALERTING,
+	QMI_VOICE_CALL_STATE_HOLD,
+	QMI_VOICE_CALL_STATE_WAITING,
+	QMI_VOICE_CALL_STATE_DISCONNECTING,
+	QMI_VOICE_CALL_STATE_END,
+	QMI_VOICE_CALL_STATE_SETUP
+};
+
 struct qmi_ussd_data {
 	uint8_t dcs;
 	uint8_t length;
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index aa34fc25..cadb5adf 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -3,6 +3,7 @@
  *  oFono - Open Source Telephony
  *
  *  Copyright (C) 2011-2012  Intel Corporation. All rights reserved.
+ *  Copyright (C) 2024 Adam Pigg <adam@piggz.co.uk>
  *
  *  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
@@ -26,6 +27,10 @@
 #include <ofono/log.h>
 #include <ofono/modem.h>
 #include <ofono/voicecall.h>
+#include <src/common.h>
+#include <ell/ell.h>
+
+#include "voice.h"
 
 #include "qmi.h"
 
@@ -35,8 +40,474 @@ struct voicecall_data {
 	struct qmi_service *voice;
 	uint16_t major;
 	uint16_t minor;
+	struct l_queue *call_list;
+	struct ofono_phone_number dialed;
+};
+
+struct qmi_voice_all_call_status_ind {
+	bool call_information_set;
+	const struct qmi_voice_call_information *call_information;
+	bool remote_party_number_set;
+	uint8_t remote_party_number_size;
+	const struct qmi_voice_remote_party_number_instance
+		*remote_party_number[16];
 };
 
+struct qmi_voice_call_information_instance {
+	uint8_t id;
+	uint8_t state;
+	uint8_t type;
+	uint8_t direction;
+	uint8_t mode;
+	uint8_t multipart_indicator;
+	uint8_t als;
+} __attribute__((__packed__));
+
+struct qmi_voice_call_information {
+	uint8_t size;
+	struct qmi_voice_call_information_instance instance[0];
+} __attribute__((__packed__));
+
+struct qmi_voice_remote_party_number_instance {
+	uint8_t call_id;
+	uint8_t presentation_indicator;
+	uint8_t number_size;
+	char number[0];
+} __attribute__((__packed__));
+
+struct qmi_voice_remote_party_number {
+	uint8_t size;
+	struct qmi_voice_remote_party_number_instance instance[0];
+} __attribute__((__packed__));
+
+static int ofono_call_compare(const void *a, const void *b, void *data)
+{
+	const struct ofono_call *ca = a;
+	const struct ofono_call *cb = b;
+
+	if (ca->id < cb->id)
+		return -1;
+
+	if (ca->id > cb->id)
+		return 1;
+
+	return 0;
+}
+
+static bool ofono_call_compare_by_status(const void *a, const void *b)
+{
+	const struct ofono_call *call = a;
+	int status = L_PTR_TO_INT(b);
+
+	return status == call->status;
+}
+
+static bool ofono_call_compare_by_id(const void *a, const void *b)
+{
+	const struct ofono_call *call = a;
+	unsigned int id = L_PTR_TO_UINT(b);
+
+	return (call->id == id);
+}
+
+static void ofono_call_list_dial_callback(struct ofono_voicecall *vc,
+						int call_id)
+{
+	struct ofono_call *call;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct l_queue *call_list = vd->call_list;
+	const struct ofono_phone_number *ph = &vd->dialed;
+
+	/* check if call_id already present */
+	call = l_queue_find(call_list, ofono_call_compare_by_id,
+				L_UINT_TO_PTR(call_id));
+
+	if (call)
+		return;
+
+	call = l_new(struct ofono_call, 1);
+	call->id = call_id;
+
+	memcpy(&call->called_number, ph, sizeof(*ph));
+	call->direction = CALL_DIRECTION_MOBILE_ORIGINATED;
+	call->status = CALL_STATUS_DIALING;
+	call->type = 0; /* voice */
+
+	l_queue_insert(call_list, call, ofono_call_compare, NULL);
+
+	ofono_voicecall_notify(vc, call);
+}
+
+static void ofono_call_list_notify(struct ofono_voicecall *vc,
+					struct l_queue *calls)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct l_queue *old_calls = vd->call_list;
+	struct l_queue *new_calls = calls;
+	struct ofono_call *new_call, *old_call;
+	const struct l_queue_entry *old_entry, *new_entry;
+
+	uint loop_length =
+		MAX(l_queue_length(old_calls), l_queue_length(new_calls));
+
+	old_entry = l_queue_get_entries(old_calls);
+	new_entry = l_queue_get_entries(new_calls);
+
+	for (uint i = 0; i < loop_length; ++i) {
+		old_call = old_entry ? old_entry->data : NULL;
+		new_call = new_entry ? new_entry->data : NULL;
+
+		if (new_call && new_call->status == CALL_STATUS_DISCONNECTED) {
+			ofono_voicecall_disconnected(
+				vc, new_call->id,
+				OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL);
+
+			l_queue_remove(calls, new_call);
+			l_free(new_call);
+			continue;
+		}
+
+		if (old_call &&
+		    (new_call == NULL || (new_call->id > old_call->id)))
+			ofono_voicecall_disconnected(
+				vc, old_call->id,
+				OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL);
+		else if (new_call &&
+			 (old_call == NULL || (new_call->id < old_call->id))) {
+			DBG("Notify new call %d", new_call->id);
+			/* new call, signal it */
+			if (new_call->type == 0)
+				ofono_voicecall_notify(vc, new_call);
+		} else if (memcmp(new_call, old_call, sizeof(*new_call)) &&
+				new_call->type == 0)
+			ofono_voicecall_notify(vc, new_call);
+
+		if (old_entry)
+			old_entry = old_entry->next;
+		if (new_entry)
+			new_entry = new_entry->next;
+	}
+
+	l_queue_destroy(old_calls, l_free);
+	vd->call_list = calls;
+}
+
+static const char *qmi_voice_call_state_name(enum qmi_voice_call_state value)
+{
+	switch (value) {
+	case QMI_VOICE_CALL_STATE_IDLE:
+		return "QMI_VOICE_CALL_STATE_IDLE";
+	case QMI_VOICE_CALL_STATE_ORIG:
+		return "QMI_VOICE_CALL_STATE_ORIG";
+	case QMI_VOICE_CALL_STATE_INCOMING:
+		return "QMI_VOICE_CALL_STATE_INCOMING";
+	case QMI_VOICE_CALL_STATE_CONV:
+		return "QMI_VOICE_CALL_STATE_CONV";
+	case QMI_VOICE_CALL_STATE_CC_IN_PROG:
+		return "QMI_VOICE_CALL_STATE_CC_IN_PROG";
+	case QMI_VOICE_CALL_STATE_ALERTING:
+		return "QMI_VOICE_CALL_STATE_ALERTING";
+	case QMI_VOICE_CALL_STATE_HOLD:
+		return "QMI_VOICE_CALL_STATE_HOLD";
+	case QMI_VOICE_CALL_STATE_WAITING:
+		return "QMI_VOICE_CALL_STATE_WAITING";
+	case QMI_VOICE_CALL_STATE_DISCONNECTING:
+		return "QMI_VOICE_CALL_STATE_DISCONNECTING";
+	case QMI_VOICE_CALL_STATE_END:
+		return "QMI_VOICE_CALL_STATE_END";
+	case QMI_VOICE_CALL_STATE_SETUP:
+		return "QMI_VOICE_CALL_STATE_SETUP";
+	}
+	return "QMI_CALL_STATE_<UNKNOWN>";
+}
+
+static bool qmi_to_ofono_status(uint8_t status, int *ret)
+{
+	int err = false;
+
+	switch (status) {
+	case QMI_VOICE_CALL_STATE_IDLE:
+	case QMI_VOICE_CALL_STATE_END:
+	case QMI_VOICE_CALL_STATE_DISCONNECTING:
+		*ret = CALL_STATUS_DISCONNECTED;
+		break;
+	case QMI_VOICE_CALL_STATE_HOLD:
+		*ret = CALL_STATUS_HELD;
+		break;
+	case QMI_VOICE_CALL_STATE_WAITING:
+		*ret = CALL_STATUS_WAITING;
+		break;
+	case QMI_VOICE_CALL_STATE_ORIG:
+		*ret = CALL_STATUS_DIALING;
+		break;
+	case QMI_VOICE_CALL_STATE_SETUP:
+	case QMI_VOICE_CALL_STATE_INCOMING:
+		*ret = CALL_STATUS_INCOMING;
+		break;
+	case QMI_VOICE_CALL_STATE_CONV:
+		*ret = CALL_STATUS_ACTIVE;
+		break;
+	case QMI_VOICE_CALL_STATE_CC_IN_PROG:
+		*ret = CALL_STATUS_DIALING;
+		break;
+	case QMI_VOICE_CALL_STATE_ALERTING:
+		*ret = CALL_STATUS_ALERTING;
+		break;
+	default:
+		err = true;
+	}
+	return err;
+}
+
+static uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
+{
+	return ofono_direction + 1;
+}
+
+static enum call_direction qmi_to_ofono_direction(uint8_t qmi_direction)
+{
+	return qmi_direction - 1;
+}
+
+static int qmi_voice_call_status(struct qmi_result *qmi_result,
+					struct qmi_voice_all_call_status_ind *result)
+{
+	int offset;
+	uint16_t len;
+	bool status = true;
+	const struct qmi_voice_remote_party_number *remote_party_number;
+	const struct qmi_voice_call_information *call_information;
+
+	static const uint8_t QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01;
+	static const uint8_t QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10;
+	static const uint8_t QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10;
+	static const uint8_t QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11;
+
+	/* mandatory */
+	call_information = qmi_result_get(
+		qmi_result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len);
+
+	if (!call_information) {
+		call_information = qmi_result_get(
+			qmi_result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION,
+			&len);
+		status = false;
+	}
+
+	if (call_information) {
+		/* verify the length */
+		if (len < sizeof(call_information->size))
+			return -EMSGSIZE;
+
+		if (len !=
+			call_information->size *
+			sizeof(struct qmi_voice_call_information_instance) +
+			sizeof(call_information->size))
+			return -EMSGSIZE;
+
+		result->call_information_set = 1;
+		result->call_information = call_information;
+	} else
+		return -EBADMSG;
+
+	/* mandatory */
+	remote_party_number = qmi_result_get(
+		qmi_result,
+		status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER :
+			 QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER,
+		&len);
+
+	if (remote_party_number) {
+		const struct qmi_voice_remote_party_number_instance *instance;
+		int instance_size =
+			sizeof(struct qmi_voice_remote_party_number_instance);
+		int i;
+
+		/* verify the length */
+		if (len < sizeof(remote_party_number->size))
+			return -EMSGSIZE;
+
+		for (i = 0, offset = sizeof(remote_party_number->size);
+			offset <= len && i < 16 && i < remote_party_number->size;
+			i++) {
+			if (offset == len)
+				break;
+			else if (offset + instance_size > len)
+				return -EMSGSIZE;
+
+			instance = (void *)remote_party_number + offset;
+			result->remote_party_number[i] = instance;
+			offset +=
+				sizeof(struct qmi_voice_remote_party_number_instance) +
+				instance->number_size;
+		}
+		result->remote_party_number_set = 1;
+		result->remote_party_number_size = remote_party_number->size;
+	} else
+		return -EBADMSG;
+
+	return 0;
+}
+
+static void all_call_status_ind(struct qmi_result *result, void *user_data)
+{
+	struct ofono_voicecall *vc = user_data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct l_queue *calls = l_queue_new();
+	int i;
+	int size = 0;
+	struct qmi_voice_all_call_status_ind status_ind;
+
+	DBG("");
+	if (qmi_voice_call_status(result, &status_ind) != 0) {
+		DBG("Parsing of all call status indication failed");
+		goto error;
+	}
+
+	if (!status_ind.remote_party_number_set ||
+	    !status_ind.call_information_set) {
+		DBG("Some required fields are not set");
+		goto error;
+	}
+
+	size = status_ind.call_information->size;
+	if (!size) {
+		DBG("No call informations received!");
+		goto error;
+	}
+
+	/* expect we have valid fields for every call */
+	if (size != status_ind.remote_party_number_size) {
+		DBG("Not all fields have the same size");
+		goto error;
+	}
+
+	for (i = 0; i < size; i++) {
+		struct qmi_voice_call_information_instance call_info;
+		struct ofono_call *call;
+		const struct qmi_voice_remote_party_number_instance
+			*remote_party = status_ind.remote_party_number[i];
+		int number_size;
+
+		call_info = status_ind.call_information->instance[i];
+		call = l_new(struct ofono_call, 1);
+		call->id = call_info.id;
+		call->direction = qmi_to_ofono_direction(call_info.direction);
+
+		if (qmi_to_ofono_status(call_info.state, &call->status)) {
+			DBG("Ignore call id %d, because can not convert QMI state 0x%x to ofono.",
+				call_info.id, call_info.state);
+			l_free(call);
+			continue;
+		}
+		DBG("Call %d in state %s(%d)", call_info.id,
+			qmi_voice_call_state_name(call_info.state),
+			call_info.state);
+
+		call->type = 0; /* always voice */
+		number_size = remote_party->number_size + 1;
+		if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH)
+			number_size = OFONO_MAX_PHONE_NUMBER_LENGTH;
+		l_strlcpy(call->phone_number.number, remote_party->number,
+				number_size);
+
+		if (strlen(call->phone_number.number) > 0)
+			call->clip_validity = 0;
+		else
+			call->clip_validity = 2;
+
+		l_queue_push_tail(calls, call);
+		DBG("%d", l_queue_length(calls));
+	}
+
+	ofono_call_list_notify(vc, calls);
+
+	return;
+error:
+	l_queue_destroy(calls, l_free);
+}
+
+static void dial_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	struct ofono_voicecall *vc = cbd->user;
+	ofono_voicecall_cb_t cb = cbd->cb;
+	uint16_t error;
+	uint8_t call_id;
+	bool call_id_set = false;
+
+	static const uint8_t QMI_VOICE_DIAL_RESULT_CALL_ID = 0x10;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	if (qmi_result_get_uint8(result, QMI_VOICE_DIAL_RESULT_CALL_ID,
+					&call_id))
+		call_id_set = true;
+	else {
+		DBG("Received invalid Result");
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	if (!call_id_set) {
+		DBG("Didn't receive a call id");
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	DBG("New call QMI id %d", call_id);
+	ofono_call_list_dial_callback(vc, call_id);
+
+	CALLBACK_WITH_SUCCESS(cb, cbd->data);
+}
+
+static void dial(struct ofono_voicecall *vc,
+			const struct ofono_phone_number *ph,
+			enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
+			void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	struct qmi_param *param;
+	const char *calling_number = phone_number_to_string(ph);
+
+	static const uint8_t QMI_VOICE_DIAL_CALL_NUMBER = 0x01;
+	static const uint8_t QMI_VOICE_DIAL_CALL_TYPE = 0x10;
+	static const uint8_t QMI_VOICE_CALL_TYPE_VOICE = 0x00;
+
+	DBG("");
+
+	cbd->user = vc;
+	memcpy(&vd->dialed, ph, sizeof(*ph));
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER,
+				strlen(calling_number), calling_number)) {
+		goto error;
+	}
+
+	qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE,
+				QMI_VOICE_CALL_TYPE_VOICE);
+
+	if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb,
+				cbd, l_free) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, data);
+	l_free(cbd);
+	l_free(param);
+}
+
 static void create_voice_cb(struct qmi_service *service, void *user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -58,6 +529,9 @@ static void create_voice_cb(struct qmi_service *service, void *user_data)
 
 	data->voice = qmi_service_ref(service);
 
+	qmi_service_register(data->voice, QMI_VOICE_ALL_CALL_STATUS_IND,
+			     all_call_status_ind, vc, NULL);
+
 	ofono_voicecall_register(vc);
 }
 
@@ -70,6 +544,7 @@ static int qmi_voicecall_probe(struct ofono_voicecall *vc,
 	DBG("");
 
 	data = l_new(struct voicecall_data, 1);
+	data->call_list = l_queue_new();
 
 	ofono_voicecall_set_data(vc, data);
 
@@ -77,7 +552,6 @@ static int qmi_voicecall_probe(struct ofono_voicecall *vc,
 					create_voice_cb, vc, NULL);
 
 	return 0;
-
 }
 
 static void qmi_voicecall_remove(struct ofono_voicecall *vc)
@@ -92,13 +566,16 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc)
 
 	qmi_service_unref(data->voice);
 
+	l_queue_destroy(data->call_list, l_free);
 	l_free(data);
 }
 
 static const struct ofono_voicecall_driver driver = {
 	.probe		= qmi_voicecall_probe,
 	.remove		= qmi_voicecall_remove,
+	.dial		= dial,
 };
 
 OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
 
+
-- 
2.44.0

[PATCH v3 2/4] qmimodem: voicecall: Implement call answer

[Adam Pigg]

The answer function set up the parameters for a call to the service
function QMI_VOICE_ANSWER_CALL.  The only parameter is the call-id.
anser_cb will then be called which retrieves the call-id and checks
the status of the result.
---
 drivers/qmimodem/voice.h     |  1 +
 drivers/qmimodem/voicecall.c | 65 ++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index 2344fd50..a524cf98 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -51,6 +51,7 @@ enum voice_commands {
 	QMI_VOICE_DIAL_CALL =			0x20,
 	QMI_VOICE_ALL_CALL_STATUS_IND =		0x2e,
 	QMI_VOICE_GET_ALL_CALL_INFO =		0x2f,
+	QMI_VOICE_ANSWER_CALL =			0x22,
 	QMI_VOICE_SUPS_NOTIFICATION_IND =	0x32,
 	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
 	QMI_VOICE_GET_CALL_WAITING =		0x34,
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index cadb5adf..8ee49acf 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -508,6 +508,70 @@ error:
 	l_free(param);
 }
 
+static void answer_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_voicecall_cb_t cb = cbd->cb;
+	uint16_t error;
+	uint8_t call_id;
+
+	static const uint8_t QMI_VOICE_ANSWER_RETURN_CALL_ID = 0x10;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	if (qmi_result_get_uint8(result, QMI_VOICE_ANSWER_RETURN_CALL_ID, &call_id))
+		DBG("Received answer result with call id %d", call_id);
+
+	CALLBACK_WITH_SUCCESS(cb, cbd->data);
+}
+
+static void answer(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+	struct ofono_call *call;
+	struct qmi_param *param = NULL;
+
+	static const uint8_t QMI_VOICE_ANSWER_CALL_ID = 0x01;
+
+	DBG("");
+
+	cbd->user = vc;
+
+	call = l_queue_find(vd->call_list,
+					ofono_call_compare_by_status,
+					L_UINT_TO_PTR(CALL_STATUS_INCOMING));
+
+	if (call == NULL) {
+		DBG("Can not find a call to pick up");
+		goto error;
+	}
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	if (!qmi_param_append_uint8(param, QMI_VOICE_ANSWER_CALL_ID,
+		call->id))
+		goto error;
+
+
+	if (qmi_service_send(vd->voice, QMI_VOICE_ANSWER_CALL, param,
+		answer_cb, cbd, l_free) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, data);
+	l_free(cbd);
+	l_free(param);
+}
+
 static void create_voice_cb(struct qmi_service *service, void *user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -574,6 +638,7 @@ static const struct ofono_voicecall_driver driver = {
 	.probe		= qmi_voicecall_probe,
 	.remove		= qmi_voicecall_remove,
 	.dial		= dial,
+	.answer		= answer,
 };
 
 OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
-- 
2.44.0

[PATCH v3 3/4] qmimodem: voicecall: Implement active call hangup

[Adam Pigg]

hangup_active iterates the current list of calls, looking for the first
active call and then calls release_specific.  This then sets up the
parameters for a call to QMI_VOICE_END_CALL, with the only paramters
being the call-id.

end_call_cb will then be called and will parse out the call-id and check
for success.
---
 drivers/qmimodem/voice.h     |  1 +
 drivers/qmimodem/voicecall.c | 84 +++++++++++++++++++++++++++++++++++-
 2 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index a524cf98..caedb079 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -51,6 +51,7 @@ enum voice_commands {
 	QMI_VOICE_DIAL_CALL =			0x20,
 	QMI_VOICE_ALL_CALL_STATUS_IND =		0x2e,
 	QMI_VOICE_GET_ALL_CALL_INFO =		0x2f,
+	QMI_VOICE_END_CALL =			0x21,
 	QMI_VOICE_ANSWER_CALL =			0x22,
 	QMI_VOICE_SUPS_NOTIFICATION_IND =	0x32,
 	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 8ee49acf..627719a8 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -572,6 +572,86 @@ error:
 	l_free(param);
 }
 
+static void end_call_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_voicecall_cb_t cb = cbd->cb;
+	uint16_t error;
+	uint8_t call_id;
+
+	static const uint8_t QMI_VOICE_END_RETURN_CALL_ID = 0x10;
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	if (qmi_result_get_uint8(result, QMI_VOICE_END_RETURN_CALL_ID, &call_id))
+		DBG("Received end call result with call id %d", call_id);
+
+	CALLBACK_WITH_SUCCESS(cb, cbd->data);
+}
+
+static void 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 cb_data *cbd = cb_data_new(cb, data);
+	struct qmi_param *param = NULL;
+
+	static const uint8_t QMI_VOICE_END_CALL_ID = 0x01;
+
+	DBG("");
+	cbd->user = vc;
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	if (!qmi_param_append_uint8(param, QMI_VOICE_END_CALL_ID, id))
+		goto error;
+
+	if (qmi_service_send(vd->voice, QMI_VOICE_END_CALL, param, end_call_cb,
+		cbd, l_free) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, data);
+	l_free(cbd);
+	l_free(param);
+}
+
+static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
+				void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct ofono_call *call;
+	enum call_status active[] = {
+		CALL_STATUS_ACTIVE,
+		CALL_STATUS_DIALING,
+		CALL_STATUS_ALERTING,
+		CALL_STATUS_INCOMING,
+	};
+
+	DBG("");
+	for (uint32_t i = 0; i < L_ARRAY_SIZE(active); i++) {
+		call = l_queue_find(vd->call_list, ofono_call_compare_by_status,
+			L_INT_TO_PTR(active[i]));
+
+		if (call)
+			break;
+	}
+
+	if (call == NULL) {
+		DBG("Can not find a call to hang up");
+		CALLBACK_WITH_FAILURE(cb, data);
+		return;
+	}
+
+	release_specific(vc, call->id, cb, data);
+}
+
 static void create_voice_cb(struct qmi_service *service, void *user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -594,7 +674,7 @@ static void create_voice_cb(struct qmi_service *service, void *user_data)
 	data->voice = qmi_service_ref(service);
 
 	qmi_service_register(data->voice, QMI_VOICE_ALL_CALL_STATUS_IND,
-			     all_call_status_ind, vc, NULL);
+				all_call_status_ind, vc, NULL);
 
 	ofono_voicecall_register(vc);
 }
@@ -639,6 +719,8 @@ static const struct ofono_voicecall_driver driver = {
 	.remove		= qmi_voicecall_remove,
 	.dial		= dial,
 	.answer		= answer,
+	.hangup_active  = hangup_active,
+	.release_specific  = release_specific,
 };
 
 OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
-- 
2.44.0

[PATCH v3 4/4] qmimodem: voicecall: Implement DTMF tones

[Adam Pigg]

The send_dtmf function sets up a call to send_one_dtmf, which will call
the QMI_VOICE_START_CONTINUOUS_DTMF service function.  The paramters to
this call are a hard coded call-id and the DTMF character to send.
start_cont_dtmf_cb will then be called which will set up a call to
QMI_VOICE_STOP_CONTINUOUS_DTMF to stop the tone.  Finally,
stop_cont_dtmf_cb will check the final status.
---
 drivers/qmimodem/voice.h     |  12 ++++
 drivers/qmimodem/voicecall.c | 131 +++++++++++++++++++++++++++++++++++
 2 files changed, 143 insertions(+)

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index caedb079..5b777af8 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -53,6 +53,8 @@ enum voice_commands {
 	QMI_VOICE_GET_ALL_CALL_INFO =		0x2f,
 	QMI_VOICE_END_CALL =			0x21,
 	QMI_VOICE_ANSWER_CALL =			0x22,
+	QMI_VOICE_START_CONTINUOUS_DTMF =	0x29,
+	QMI_VOICE_STOP_CONTINUOUS_DTMF =	0x2A,
 	QMI_VOICE_SUPS_NOTIFICATION_IND =	0x32,
 	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
 	QMI_VOICE_GET_CALL_WAITING =		0x34,
@@ -85,6 +87,16 @@ enum qmi_voice_call_state {
 	QMI_VOICE_CALL_STATE_SETUP
 };
 
+enum qmi_voice_call_dtmf_param {
+	QMI_VOICE_DTMF_DATA = 0x01,
+};
+
+enum parse_error {
+	NONE = 0,
+	MISSING_MANDATORY = 1,
+	INVALID_LENGTH = 2,
+};
+
 struct qmi_ussd_data {
 	uint8_t dcs;
 	uint8_t length;
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 627719a8..4bdb64b7 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -42,6 +42,8 @@ struct voicecall_data {
 	uint16_t minor;
 	struct l_queue *call_list;
 	struct ofono_phone_number dialed;
+	char *full_dtmf;
+	char *next_dtmf;
 };
 
 struct qmi_voice_all_call_status_ind {
@@ -652,6 +654,134 @@ static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
 	release_specific(vc, call->id, cb, data);
 }
 
+static void stop_cont_dtmf_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_voicecall_cb_t cb = cbd->cb;
+
+	uint16_t error;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	CALLBACK_WITH_SUCCESS(cb, cbd->data);
+	l_free(cbd);
+}
+
+static void start_cont_dtmf_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	ofono_voicecall_cb_t cb = cbd->cb;
+	struct ofono_voicecall *vc = cbd->user;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	uint16_t error;
+	struct qmi_param *param = NULL;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	if (!qmi_param_append_uint8(param, QMI_VOICE_DTMF_DATA, 0xff))
+		goto error;
+
+	if (qmi_service_send(vd->voice, QMI_VOICE_STOP_CONTINUOUS_DTMF, param,
+			     stop_cont_dtmf_cb, cbd, NULL) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, cbd->data);
+	l_free(cbd);
+	l_free(param);
+}
+
+static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf,
+				ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct qmi_param *param = NULL;
+	uint8_t param_body[2];
+	struct cb_data *cbd = cb_data_new(cb, data);
+
+	DBG("");
+
+	cbd->user = vc;
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	param_body[0] = 0xff;
+	param_body[1] = (uint8_t)dtmf;
+
+	if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, sizeof(param_body),
+			      param_body)) {
+		goto error;
+	}
+
+	if (qmi_service_send(vd->voice, QMI_VOICE_START_CONTINUOUS_DTMF, param,
+			     start_cont_dtmf_cb, cbd, NULL) > 0) {
+		return;
+	}
+
+error:
+	CALLBACK_WITH_FAILURE(cb, data);
+	l_free(cbd);
+	l_free(param);
+}
+
+static void send_one_dtmf_cb(const struct ofono_error *error, void *data)
+{
+	struct cb_data *cbd = data;
+	struct voicecall_data *vd = ofono_voicecall_get_data(cbd->user);
+	ofono_voicecall_cb_t cb = cbd->cb;
+
+	DBG("");
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR ||
+	    *vd->next_dtmf == 0) {
+		if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
+			CALLBACK_WITH_SUCCESS(cb, cbd->data);
+		else
+			CALLBACK_WITH_FAILURE(cb, cbd->data);
+
+		l_free(vd->full_dtmf);
+		l_free(cbd);
+	} else {
+		send_one_dtmf(cbd->user,
+				*(vd->next_dtmf++),
+				send_one_dtmf_cb, data);
+	}
+}
+
+static void send_dtmf(struct ofono_voicecall *vc, const char *dtmf,
+		      ofono_voicecall_cb_t cb, void *data)
+{
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	struct cb_data *cbd = cb_data_new(cb, data);
+
+	DBG("");
+
+	vd->full_dtmf = l_strdup(dtmf);
+	vd->next_dtmf = &vd->full_dtmf[1];
+
+	cbd->user = vc;
+
+	send_one_dtmf(vc, *dtmf, send_one_dtmf_cb, cbd);
+}
+
 static void create_voice_cb(struct qmi_service *service, void *user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -721,6 +851,7 @@ static const struct ofono_voicecall_driver driver = {
 	.answer		= answer,
 	.hangup_active  = hangup_active,
 	.release_specific  = release_specific,
+	.send_tones	= send_dtmf,
 };
 
 OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
-- 
2.44.0

Re: [PATCH v3 1/4] qmimodem: voicecall: Implement call dialing

[Adam Pigg]

Id just like to add some context about the work ive done for this revision.  I 
didnt figure out how to get git send-email to add a cover note, so this is it 
:)

I -believe- ive implemented all of Dennis' review comments, with the following 
notes:
Patch1
The suggestion to add
_auto_(queue_desroy_free) struct l_queue *calls = l_queue_new() instead.
wasnt nescessary and resulted in a double-free, as the call-list was being 
free'd correctly later on.

Patch4
I moved the cpd params into voicecall as suggested, and added an l_free for 
cbd in the success path which was missing.

I have performed tests of incoming and outgoing call, answering and hanging 
up, doing dtms tones, all while running in valgrind, and did not pick up any 
leaks in this code (there was a leak in the ofono sim code, and one of my 
other patches which I wont upstream)

I have also just noticed that I forgot to do the last comment from Patch 3, so 
feel free to mention that again!

Thanks

Adam

On Tuesday 2 April 2024 22:25:15 BST Adam Pigg wrote:
> Add voicecall dialling to the qmimodem driver
> Includes required infratructure and setup of the QMI services
> 
> Call State Handling
> ===================
> On initialisation, register the all_call_status_ind callback to be
> called for QMI_VOICE_IND_ALL_STATUS.  This will handle notificatiof of
> call status to the rest of the system
> 
> Dial Handling
> =============
> The dial function sets up the parameters for the QMI_VOICE_DIAL_CALL
> service.  The parameters are the number to be called and the call type,
> which is currently hard coded to be QMI_VOICE_CALL_TYPE_VOICE.  The
> dial_cb callback wi then be called and will receive the call-id.
> ---
>  drivers/qmimodem/voice.h     |  17 ++
>  drivers/qmimodem/voicecall.c | 479 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 495 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
> index 917e72f7..2344fd50 100644
> --- a/drivers/qmimodem/voice.h
> +++ b/drivers/qmimodem/voice.h
> @@ -48,6 +48,9 @@ enum qmi_ussd_user_required {
> 
>  /* QMI service voice. Using an enum to prevent doublicated entries */
>  enum voice_commands {
> +	QMI_VOICE_DIAL_CALL =			0x20,
> +	QMI_VOICE_ALL_CALL_STATUS_IND =		0x2e,
> +	QMI_VOICE_GET_ALL_CALL_INFO =		0x2f,
>  	QMI_VOICE_SUPS_NOTIFICATION_IND =	0x32,
>  	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
>  	QMI_VOICE_GET_CALL_WAITING =		0x34,
> @@ -66,6 +69,20 @@ enum voice_commands {
>  	QMI_VOICE_GET_CNAP =			0x4d
>  };
> 
> +enum qmi_voice_call_state {
> +	QMI_VOICE_CALL_STATE_IDLE = 0x0,
> +	QMI_VOICE_CALL_STATE_ORIG,
> +	QMI_VOICE_CALL_STATE_INCOMING,
> +	QMI_VOICE_CALL_STATE_CONV,
> +	QMI_VOICE_CALL_STATE_CC_IN_PROG,
> +	QMI_VOICE_CALL_STATE_ALERTING,
> +	QMI_VOICE_CALL_STATE_HOLD,
> +	QMI_VOICE_CALL_STATE_WAITING,
> +	QMI_VOICE_CALL_STATE_DISCONNECTING,
> +	QMI_VOICE_CALL_STATE_END,
> +	QMI_VOICE_CALL_STATE_SETUP
> +};
> +
>  struct qmi_ussd_data {
>  	uint8_t dcs;
>  	uint8_t length;
> diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
> index aa34fc25..cadb5adf 100644
> --- a/drivers/qmimodem/voicecall.c
> +++ b/drivers/qmimodem/voicecall.c
> @@ -3,6 +3,7 @@
>   *  oFono - Open Source Telephony
>   *
>   *  Copyright (C) 2011-2012  Intel Corporation. All rights reserved.
> + *  Copyright (C) 2024 Adam Pigg <adam@piggz.co.uk>
>   *
>   *  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
> @@ -26,6 +27,10 @@
>  #include <ofono/log.h>
>  #include <ofono/modem.h>
>  #include <ofono/voicecall.h>
> +#include <src/common.h>
> +#include <ell/ell.h>
> +
> +#include "voice.h"
> 
>  #include "qmi.h"
> 
> @@ -35,8 +40,474 @@ struct voicecall_data {
>  	struct qmi_service *voice;
>  	uint16_t major;
>  	uint16_t minor;
> +	struct l_queue *call_list;
> +	struct ofono_phone_number dialed;
> +};
> +
> +struct qmi_voice_all_call_status_ind {
> +	bool call_information_set;
> +	const struct qmi_voice_call_information *call_information;
> +	bool remote_party_number_set;
> +	uint8_t remote_party_number_size;
> +	const struct qmi_voice_remote_party_number_instance
> +		*remote_party_number[16];
>  };
> 
> +struct qmi_voice_call_information_instance {
> +	uint8_t id;
> +	uint8_t state;
> +	uint8_t type;
> +	uint8_t direction;
> +	uint8_t mode;
> +	uint8_t multipart_indicator;
> +	uint8_t als;
> +} __attribute__((__packed__));
> +
> +struct qmi_voice_call_information {
> +	uint8_t size;
> +	struct qmi_voice_call_information_instance instance[0];
> +} __attribute__((__packed__));
> +
> +struct qmi_voice_remote_party_number_instance {
> +	uint8_t call_id;
> +	uint8_t presentation_indicator;
> +	uint8_t number_size;
> +	char number[0];
> +} __attribute__((__packed__));
> +
> +struct qmi_voice_remote_party_number {
> +	uint8_t size;
> +	struct qmi_voice_remote_party_number_instance instance[0];
> +} __attribute__((__packed__));
> +
> +static int ofono_call_compare(const void *a, const void *b, void *data)
> +{
> +	const struct ofono_call *ca = a;
> +	const struct ofono_call *cb = b;
> +
> +	if (ca->id < cb->id)
> +		return -1;
> +
> +	if (ca->id > cb->id)
> +		return 1;
> +
> +	return 0;
> +}
> +
> +static bool ofono_call_compare_by_status(const void *a, const void *b)
> +{
> +	const struct ofono_call *call = a;
> +	int status = L_PTR_TO_INT(b);
> +
> +	return status == call->status;
> +}
> +
> +static bool ofono_call_compare_by_id(const void *a, const void *b)
> +{
> +	const struct ofono_call *call = a;
> +	unsigned int id = L_PTR_TO_UINT(b);
> +
> +	return (call->id == id);
> +}
> +
> +static void ofono_call_list_dial_callback(struct ofono_voicecall *vc,
> +						int call_id)
> +{
> +	struct ofono_call *call;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct l_queue *call_list = vd->call_list;
> +	const struct ofono_phone_number *ph = &vd->dialed;
> +
> +	/* check if call_id already present */
> +	call = l_queue_find(call_list, ofono_call_compare_by_id,
> +				L_UINT_TO_PTR(call_id));
> +
> +	if (call)
> +		return;
> +
> +	call = l_new(struct ofono_call, 1);
> +	call->id = call_id;
> +
> +	memcpy(&call->called_number, ph, sizeof(*ph));
> +	call->direction = CALL_DIRECTION_MOBILE_ORIGINATED;
> +	call->status = CALL_STATUS_DIALING;
> +	call->type = 0; /* voice */
> +
> +	l_queue_insert(call_list, call, ofono_call_compare, NULL);
> +
> +	ofono_voicecall_notify(vc, call);
> +}
> +
> +static void ofono_call_list_notify(struct ofono_voicecall *vc,
> +					struct l_queue *calls)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct l_queue *old_calls = vd->call_list;
> +	struct l_queue *new_calls = calls;
> +	struct ofono_call *new_call, *old_call;
> +	const struct l_queue_entry *old_entry, *new_entry;
> +
> +	uint loop_length =
> +		MAX(l_queue_length(old_calls), 
l_queue_length(new_calls));
> +
> +	old_entry = l_queue_get_entries(old_calls);
> +	new_entry = l_queue_get_entries(new_calls);
> +
> +	for (uint i = 0; i < loop_length; ++i) {
> +		old_call = old_entry ? old_entry->data : NULL;
> +		new_call = new_entry ? new_entry->data : NULL;
> +
> +		if (new_call && new_call->status == 
CALL_STATUS_DISCONNECTED) {
> +			ofono_voicecall_disconnected(
> +				vc, new_call->id,
> +				
OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL);
> +
> +			l_queue_remove(calls, new_call);
> +			l_free(new_call);
> +			continue;
> +		}
> +
> +		if (old_call &&
> +		    (new_call == NULL || (new_call->id > old_call->id)))
> +			ofono_voicecall_disconnected(
> +				vc, old_call->id,
> +				
OFONO_DISCONNECT_REASON_REMOTE_HANGUP, NULL);
> +		else if (new_call &&
> +			 (old_call == NULL || (new_call->id < 
old_call->id))) {
> +			DBG("Notify new call %d", new_call->id);
> +			/* new call, signal it */
> +			if (new_call->type == 0)
> +				ofono_voicecall_notify(vc, 
new_call);
> +		} else if (memcmp(new_call, old_call, sizeof(*new_call)) 
&&
> +				new_call->type == 0)
> +			ofono_voicecall_notify(vc, new_call);
> +
> +		if (old_entry)
> +			old_entry = old_entry->next;
> +		if (new_entry)
> +			new_entry = new_entry->next;
> +	}
> +
> +	l_queue_destroy(old_calls, l_free);
> +	vd->call_list = calls;
> +}
> +
> +static const char *qmi_voice_call_state_name(enum qmi_voice_call_state
> value) +{
> +	switch (value) {
> +	case QMI_VOICE_CALL_STATE_IDLE:
> +		return "QMI_VOICE_CALL_STATE_IDLE";
> +	case QMI_VOICE_CALL_STATE_ORIG:
> +		return "QMI_VOICE_CALL_STATE_ORIG";
> +	case QMI_VOICE_CALL_STATE_INCOMING:
> +		return "QMI_VOICE_CALL_STATE_INCOMING";
> +	case QMI_VOICE_CALL_STATE_CONV:
> +		return "QMI_VOICE_CALL_STATE_CONV";
> +	case QMI_VOICE_CALL_STATE_CC_IN_PROG:
> +		return "QMI_VOICE_CALL_STATE_CC_IN_PROG";
> +	case QMI_VOICE_CALL_STATE_ALERTING:
> +		return "QMI_VOICE_CALL_STATE_ALERTING";
> +	case QMI_VOICE_CALL_STATE_HOLD:
> +		return "QMI_VOICE_CALL_STATE_HOLD";
> +	case QMI_VOICE_CALL_STATE_WAITING:
> +		return "QMI_VOICE_CALL_STATE_WAITING";
> +	case QMI_VOICE_CALL_STATE_DISCONNECTING:
> +		return "QMI_VOICE_CALL_STATE_DISCONNECTING";
> +	case QMI_VOICE_CALL_STATE_END:
> +		return "QMI_VOICE_CALL_STATE_END";
> +	case QMI_VOICE_CALL_STATE_SETUP:
> +		return "QMI_VOICE_CALL_STATE_SETUP";
> +	}
> +	return "QMI_CALL_STATE_<UNKNOWN>";
> +}
> +
> +static bool qmi_to_ofono_status(uint8_t status, int *ret)
> +{
> +	int err = false;
> +
> +	switch (status) {
> +	case QMI_VOICE_CALL_STATE_IDLE:
> +	case QMI_VOICE_CALL_STATE_END:
> +	case QMI_VOICE_CALL_STATE_DISCONNECTING:
> +		*ret = CALL_STATUS_DISCONNECTED;
> +		break;
> +	case QMI_VOICE_CALL_STATE_HOLD:
> +		*ret = CALL_STATUS_HELD;
> +		break;
> +	case QMI_VOICE_CALL_STATE_WAITING:
> +		*ret = CALL_STATUS_WAITING;
> +		break;
> +	case QMI_VOICE_CALL_STATE_ORIG:
> +		*ret = CALL_STATUS_DIALING;
> +		break;
> +	case QMI_VOICE_CALL_STATE_SETUP:
> +	case QMI_VOICE_CALL_STATE_INCOMING:
> +		*ret = CALL_STATUS_INCOMING;
> +		break;
> +	case QMI_VOICE_CALL_STATE_CONV:
> +		*ret = CALL_STATUS_ACTIVE;
> +		break;
> +	case QMI_VOICE_CALL_STATE_CC_IN_PROG:
> +		*ret = CALL_STATUS_DIALING;
> +		break;
> +	case QMI_VOICE_CALL_STATE_ALERTING:
> +		*ret = CALL_STATUS_ALERTING;
> +		break;
> +	default:
> +		err = true;
> +	}
> +	return err;
> +}
> +
> +static uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
> +{
> +	return ofono_direction + 1;
> +}
> +
> +static enum call_direction qmi_to_ofono_direction(uint8_t qmi_direction)
> +{
> +	return qmi_direction - 1;
> +}
> +
> +static int qmi_voice_call_status(struct qmi_result *qmi_result,
> +					struct 
qmi_voice_all_call_status_ind *result)
> +{
> +	int offset;
> +	uint16_t len;
> +	bool status = true;
> +	const struct qmi_voice_remote_party_number *remote_party_number;
> +	const struct qmi_voice_call_information *call_information;
> +
> +	static const uint8_t QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 
0x01;
> +	static const uint8_t QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 
0x10;
> +	static const uint8_t QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 
0x10;
> +	static const uint8_t QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11;
> +
> +	/* mandatory */
> +	call_information = qmi_result_get(
> +		qmi_result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, 
&len);
> +
> +	if (!call_information) {
> +		call_information = qmi_result_get(
> +			qmi_result, 
QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION,
> +			&len);
> +		status = false;
> +	}
> +
> +	if (call_information) {
> +		/* verify the length */
> +		if (len < sizeof(call_information->size))
> +			return -EMSGSIZE;
> +
> +		if (len !=
> +			call_information->size *
> +			sizeof(struct 
qmi_voice_call_information_instance) +
> +			sizeof(call_information->size))
> +			return -EMSGSIZE;
> +
> +		result->call_information_set = 1;
> +		result->call_information = call_information;
> +	} else
> +		return -EBADMSG;
> +
> +	/* mandatory */
> +	remote_party_number = qmi_result_get(
> +		qmi_result,
> +		status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER :
> +			 QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER,
> +		&len);
> +
> +	if (remote_party_number) {
> +		const struct qmi_voice_remote_party_number_instance 
*instance;
> +		int instance_size =
> +			sizeof(struct 
qmi_voice_remote_party_number_instance);
> +		int i;
> +
> +		/* verify the length */
> +		if (len < sizeof(remote_party_number->size))
> +			return -EMSGSIZE;
> +
> +		for (i = 0, offset = sizeof(remote_party_number->size);
> +			offset <= len && i < 16 && i < 
remote_party_number->size;
> +			i++) {
> +			if (offset == len)
> +				break;
> +			else if (offset + instance_size > len)
> +				return -EMSGSIZE;
> +
> +			instance = (void *)remote_party_number + 
offset;
> +			result->remote_party_number[i] = instance;
> +			offset +=
> +				sizeof(struct 
qmi_voice_remote_party_number_instance) +
> +				instance->number_size;
> +		}
> +		result->remote_party_number_set = 1;
> +		result->remote_party_number_size = remote_party_number-
>size;
> +	} else
> +		return -EBADMSG;
> +
> +	return 0;
> +}
> +
> +static void all_call_status_ind(struct qmi_result *result, void *user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct l_queue *calls = l_queue_new();
> +	int i;
> +	int size = 0;
> +	struct qmi_voice_all_call_status_ind status_ind;
> +
> +	DBG("");
> +	if (qmi_voice_call_status(result, &status_ind) != 0) {
> +		DBG("Parsing of all call status indication failed");
> +		goto error;
> +	}
> +
> +	if (!status_ind.remote_party_number_set ||
> +	    !status_ind.call_information_set) {
> +		DBG("Some required fields are not set");
> +		goto error;
> +	}
> +
> +	size = status_ind.call_information->size;
> +	if (!size) {
> +		DBG("No call informations received!");
> +		goto error;
> +	}
> +
> +	/* expect we have valid fields for every call */
> +	if (size != status_ind.remote_party_number_size) {
> +		DBG("Not all fields have the same size");
> +		goto error;
> +	}
> +
> +	for (i = 0; i < size; i++) {
> +		struct qmi_voice_call_information_instance call_info;
> +		struct ofono_call *call;
> +		const struct qmi_voice_remote_party_number_instance
> +			*remote_party = 
status_ind.remote_party_number[i];
> +		int number_size;
> +
> +		call_info = status_ind.call_information->instance[i];
> +		call = l_new(struct ofono_call, 1);
> +		call->id = call_info.id;
> +		call->direction = 
qmi_to_ofono_direction(call_info.direction);
> +
> +		if (qmi_to_ofono_status(call_info.state, &call->status)) 
{
> +			DBG("Ignore call id %d, because can not 
convert QMI state 0x%x to
> ofono.", +				call_info.id, call_info.state);
> +			l_free(call);
> +			continue;
> +		}
> +		DBG("Call %d in state %s(%d)", call_info.id,
> +			qmi_voice_call_state_name(call_info.state),
> +			call_info.state);
> +
> +		call->type = 0; /* always voice */
> +		number_size = remote_party->number_size + 1;
> +		if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH)
> +			number_size = OFONO_MAX_PHONE_NUMBER_LENGTH;
> +		l_strlcpy(call->phone_number.number, remote_party-
>number,
> +				number_size);
> +
> +		if (strlen(call->phone_number.number) > 0)
> +			call->clip_validity = 0;
> +		else
> +			call->clip_validity = 2;
> +
> +		l_queue_push_tail(calls, call);
> +		DBG("%d", l_queue_length(calls));
> +	}
> +
> +	ofono_call_list_notify(vc, calls);
> +
> +	return;
> +error:
> +	l_queue_destroy(calls, l_free);
> +}
> +
> +static void dial_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	struct ofono_voicecall *vc = cbd->user;
> +	ofono_voicecall_cb_t cb = cbd->cb;
> +	uint16_t error;
> +	uint8_t call_id;
> +	bool call_id_set = false;
> +
> +	static const uint8_t QMI_VOICE_DIAL_RESULT_CALL_ID = 0x10;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (qmi_result_get_uint8(result, QMI_VOICE_DIAL_RESULT_CALL_ID,
> +					&call_id))
> +		call_id_set = true;
> +	else {
> +		DBG("Received invalid Result");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (!call_id_set) {
> +		DBG("Didn't receive a call id");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	DBG("New call QMI id %d", call_id);
> +	ofono_call_list_dial_callback(vc, call_id);
> +
> +	CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +}
> +
> +static void dial(struct ofono_voicecall *vc,
> +			const struct ofono_phone_number *ph,
> +			enum ofono_clir_option clir, 
ofono_voicecall_cb_t cb,
> +			void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	struct qmi_param *param;
> +	const char *calling_number = phone_number_to_string(ph);
> +
> +	static const uint8_t QMI_VOICE_DIAL_CALL_NUMBER = 0x01;
> +	static const uint8_t QMI_VOICE_DIAL_CALL_TYPE = 0x10;
> +	static const uint8_t QMI_VOICE_CALL_TYPE_VOICE = 0x00;
> +
> +	DBG("");
> +
> +	cbd->user = vc;
> +	memcpy(&vd->dialed, ph, sizeof(*ph));
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;
> +
> +	if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER,
> +				strlen(calling_number), 
calling_number)) {
> +		goto error;
> +	}
> +
> +	qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE,
> +				QMI_VOICE_CALL_TYPE_VOICE);
> +
> +	if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, 
dial_cb,
> +				cbd, l_free) > 0)
> +		return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, data);
> +	l_free(cbd);
> +	l_free(param);
> +}
> +
>  static void create_voice_cb(struct qmi_service *service, void *user_data)
>  {
>  	struct ofono_voicecall *vc = user_data;
> @@ -58,6 +529,9 @@ static void create_voice_cb(struct qmi_service *service,
> void *user_data)
> 
>  	data->voice = qmi_service_ref(service);
> 
> +	qmi_service_register(data->voice, QMI_VOICE_ALL_CALL_STATUS_IND,
> +			     all_call_status_ind, vc, NULL);
> +
>  	ofono_voicecall_register(vc);
>  }
> 
> @@ -70,6 +544,7 @@ static int qmi_voicecall_probe(struct ofono_voicecall
> *vc, DBG("");
> 
>  	data = l_new(struct voicecall_data, 1);
> +	data->call_list = l_queue_new();
> 
>  	ofono_voicecall_set_data(vc, data);
> 
> @@ -77,7 +552,6 @@ static int qmi_voicecall_probe(struct ofono_voicecall
> *vc, create_voice_cb, vc, NULL);
> 
>  	return 0;
> -
>  }
> 
>  static void qmi_voicecall_remove(struct ofono_voicecall *vc)
> @@ -92,13 +566,16 @@ static void qmi_voicecall_remove(struct ofono_voicecall
> *vc)
> 
>  	qmi_service_unref(data->voice);
> 
> +	l_queue_destroy(data->call_list, l_free);
>  	l_free(data);
>  }
> 
>  static const struct ofono_voicecall_driver driver = {
>  	.probe		= qmi_voicecall_probe,
>  	.remove		= qmi_voicecall_remove,
> +	.dial		= dial,
>  };
> 
>  OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
> 
> +

Re: [PATCH v3 1/4] qmimodem: voicecall: Implement call dialing

[Denis Kenzior]

Hi Adam,

On 4/2/24 16:25, Adam Pigg wrote:
> Add voicecall dialling to the qmimodem driver
> Includes required infratructure and setup of the QMI services
> 
> Call State Handling
> ===================
> On initialisation, register the all_call_status_ind callback to be
> called for QMI_VOICE_IND_ALL_STATUS.  This will handle notificatiof of
> call status to the rest of the system
> 
> Dial Handling
> =============
> The dial function sets up the parameters for the QMI_VOICE_DIAL_CALL
> service.  The parameters are the number to be called and the call type,
> which is currently hard coded to be QMI_VOICE_CALL_TYPE_VOICE.  The
> dial_cb callback wi then be called and will receive the call-id.
> ---
>   drivers/qmimodem/voice.h     |  17 ++
>   drivers/qmimodem/voicecall.c | 479 ++++++++++++++++++++++++++++++++++-
>   2 files changed, 495 insertions(+), 1 deletion(-)
> 

<snip>

Couple of general comments:

- There's still an empty line @ EOF in this commit, please fix that.
- After applying just this patch, compilation fails:

[denkenz@archdev ofono]$ make
make --no-print-directory all-am
   CC       drivers/qmimodem/voicecall.o
drivers/qmimodem/voicecall.c: In function ‘all_call_status_ind’:
drivers/qmimodem/voicecall.c:355:32: error: unused variable ‘vd’ 
[-Werror=unused-variable]
   355 |         struct voicecall_data *vd = ofono_voicecall_get_data(vc);
       |                                ^~
drivers/qmimodem/voicecall.c: At top level:
drivers/qmimodem/voicecall.c:262:16: error: ‘ofono_to_qmi_direction’ defined but 
not used [-Werror=unused-function]
   262 | static uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
       |                ^~~~~~~~~~~~~~~~~~~~~~
drivers/qmimodem/voicecall.c:97:13: error: ‘ofono_call_compare_by_status’ 
defined but not used [-Werror=unused-function]
    97 | static bool ofono_call_compare_by_status(const void *a, const void *b)
       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:4094: drivers/qmimodem/voicecall.o] Error 1
make: *** [Makefile:2402: all] Error 2

Ideally, each patch should compile and run cleanly to help with 'git bisect'.

<snip>

> +
> +static bool ofono_call_compare_by_status(const void *a, const void *b)
> +{
> +	const struct ofono_call *call = a;
> +	int status = L_PTR_TO_INT(b);
> +
> +	return status == call->status;
> +}
> +

This function belongs in patch 2.  It isn't used in this commit and is causing a 
compiler warning / error.

<snip>

> +static uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
> +{
> +	return ofono_direction + 1;
> +}

This function isn't used, get rid of it

<snip>

> +static int qmi_voice_call_status(struct qmi_result *qmi_result,
> +					struct qmi_voice_all_call_status_ind *result)

Right now you have this function producing an intermediate structure which 
points to memory owned by struct qmi_result and is only valid as long as 
qmi_result is valid.  all_call_status_ind() then converts this intermediate 
structure into an l_queue with ofono_call objects.  The intermediate structure 
is never needed after or used elsewhere.  No other caller uses this intermediate 
structure.

Have you thought about having this function return an allocated l_queue with 
ofono_call objects as contents instead and getting rid of struct 
qmi_voice_all_call_status_ind completely?

> +{
> +	int offset;
> +	uint16_t len;
> +	bool status = true;
> +	const struct qmi_voice_remote_party_number *remote_party_number;
> +	const struct qmi_voice_call_information *call_information;
> +
> +	static const uint8_t QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01;
> +	static const uint8_t QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10;
> +	static const uint8_t QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10;
> +	static const uint8_t QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11;
> +
> +	/* mandatory */
> +	call_information = qmi_result_get(
> +		qmi_result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len);
> +
> +	if (!call_information) {
> +		call_information = qmi_result_get(
> +			qmi_result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION,
> +			&len);
> +		status = false;
> +	}
> +
> +	if (call_information) {
> +		/* verify the length */
> +		if (len < sizeof(call_information->size))
> +			return -EMSGSIZE;
> +
> +		if (len !=
> +			call_information->size *
> +			sizeof(struct qmi_voice_call_information_instance) +
> +			sizeof(call_information->size))
> +			return -EMSGSIZE;

doc/coding-style.txt item M4

> +
> +		result->call_information_set = 1;
> +		result->call_information = call_information;
> +	} else
> +		return -EBADMSG;

Easier to bail early and not nest or need the if condition:

if (!call_information)
	return -EBADMSG;

if (len < sizeof(call_information->size))
	return -EMSGSIZE;

...

> +
> +	/* mandatory */
> +	remote_party_number = qmi_result_get(
> +		qmi_result,
> +		status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER :
> +			 QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER,
> +		&len);
> +
> +	if (remote_party_number) {
> +		const struct qmi_voice_remote_party_number_instance *instance;
> +		int instance_size =
> +			sizeof(struct qmi_voice_remote_party_number_instance);
> +		int i;
> +
> +		/* verify the length */
> +		if (len < sizeof(remote_party_number->size))
> +			return -EMSGSIZE;
> +
> +		for (i = 0, offset = sizeof(remote_party_number->size);
> +			offset <= len && i < 16 && i < remote_party_number->size;
> +			i++) {
> +			if (offset == len)
> +				break;
> +			else if (offset + instance_size > len)
> +				return -EMSGSIZE;
> +
> +			instance = (void *)remote_party_number + offset;
> +			result->remote_party_number[i] = instance;
> +			offset +=
> +				sizeof(struct qmi_voice_remote_party_number_instance) +
> +				instance->number_size;
> +		}
> +		result->remote_party_number_set = 1;
> +		result->remote_party_number_size = remote_party_number->size;
> +	} else
> +		return -EBADMSG;

Same here:

if (!remote_party_number)
	return -EBADMSG;

if (len < ...)

for (...)

See doc/coding-style.txt item O2

> +
> +	return 0;
> +}
> +
> +static void all_call_status_ind(struct qmi_result *result, void *user_data)
> +{
> +	struct ofono_voicecall *vc = user_data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct l_queue *calls = l_queue_new();
> +	int i;
> +	int size = 0;
> +	struct qmi_voice_all_call_status_ind status_ind;
> +
> +	DBG("");

doc/coding-style.txt Item M1

> +	if (qmi_voice_call_status(result, &status_ind) != 0) {

nit: Prefer < 0 check for functions that return -errno.

> +		DBG("Parsing of all call status indication failed");
> +		goto error;
> +	}
> +
> +	if (!status_ind.remote_party_number_set ||
> +	    !status_ind.call_information_set) {

Incorrect indentation

> +		DBG("Some required fields are not set");
> +		goto error;
> +	}
> +
> +	size = status_ind.call_information->size;
> +	if (!size) {
> +		DBG("No call informations received!");
> +		goto error;
> +	}
> +
> +	/* expect we have valid fields for every call */
> +	if (size != status_ind.remote_party_number_size) {
> +		DBG("Not all fields have the same size");
> +		goto error;
> +	}
> +
> +	for (i = 0; i < size; i++) {
> +		struct qmi_voice_call_information_instance call_info;
> +		struct ofono_call *call;
> +		const struct qmi_voice_remote_party_number_instance
> +			*remote_party = status_ind.remote_party_number[i];
> +		int number_size;
> +
> +		call_info = status_ind.call_information->instance[i];
> +		call = l_new(struct ofono_call, 1);
> +		call->id = call_info.id;
> +		call->direction = qmi_to_ofono_direction(call_info.direction);
> +
> +		if (qmi_to_ofono_status(call_info.state, &call->status)) {
> +			DBG("Ignore call id %d, because can not convert QMI state 0x%x to ofono.",
> +				call_info.id, call_info.state);
> +			l_free(call);
> +			continue;
> +		}
> +		DBG("Call %d in state %s(%d)", call_info.id,
> +			qmi_voice_call_state_name(call_info.state),
> +			call_info.state);
> +
> +		call->type = 0; /* always voice */
> +		number_size = remote_party->number_size + 1;
> +		if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH)
> +			number_size = OFONO_MAX_PHONE_NUMBER_LENGTH;
> +		l_strlcpy(call->phone_number.number, remote_party->number,
> +				number_size);
> +
> +		if (strlen(call->phone_number.number) > 0)
> +			call->clip_validity = 0;
> +		else
> +			call->clip_validity = 2;
> +
> +		l_queue_push_tail(calls, call);
> +		DBG("%d", l_queue_length(calls));
> +	}
> +
> +	ofono_call_list_notify(vc, calls);
> +
> +	return;
> +error:
> +	l_queue_destroy(calls, l_free);
> +}
> +
> +static void dial_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	struct ofono_voicecall *vc = cbd->user;
> +	ofono_voicecall_cb_t cb = cbd->cb;
> +	uint16_t error;
> +	uint8_t call_id;
> +	bool call_id_set = false;
> +
> +	static const uint8_t QMI_VOICE_DIAL_RESULT_CALL_ID = 0x10;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (qmi_result_get_uint8(result, QMI_VOICE_DIAL_RESULT_CALL_ID,
> +					&call_id))
> +		call_id_set = true;
> +	else {
> +		DBG("Received invalid Result");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (!call_id_set) {
> +		DBG("Didn't receive a call id");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}

This if looks like a dead branch.  The only way call_id_set is false here is if 
qmi_result_get_uint8 earlier failed.  In fact, the above section can be 
rewritten as:

if (!qmi_result_get_uint8(...)) {
	ofono_error("No call id in dial result");
	CALLBACK_WITH_FAILURE(...);
	return;
}

No need for call_id_set variable either.

> +
> +	DBG("New call QMI id %d", call_id);
> +	ofono_call_list_dial_callback(vc, call_id);
> +
> +	CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +}
> +

<snip>

> +static void dial(struct ofono_voicecall *vc,
> +			const struct ofono_phone_number *ph,
> +			enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
> +			void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	struct qmi_param *param;
> +	const char *calling_number = phone_number_to_string(ph);
> +
> +	static const uint8_t QMI_VOICE_DIAL_CALL_NUMBER = 0x01;
> +	static const uint8_t QMI_VOICE_DIAL_CALL_TYPE = 0x10;
> +	static const uint8_t QMI_VOICE_CALL_TYPE_VOICE = 0x00;
> +
> +	DBG("");
> +
> +	cbd->user = vc;
> +	memcpy(&vd->dialed, ph, sizeof(*ph));
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;
> +
> +	if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER,
> +				strlen(calling_number), calling_number)) {
> +		goto error;
> +	}

No need for {}

> +
> +	qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE,
> +				QMI_VOICE_CALL_TYPE_VOICE);
> +
> +	if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb,
> +				cbd, l_free) > 0)
> +		return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, data);
> +	l_free(cbd);
> +	l_free(param);
> +}
> +
>   static void create_voice_cb(struct qmi_service *service, void *user_data)
>   {
>   	struct ofono_voicecall *vc = user_data;

<snip>

> @@ -92,13 +566,16 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc)
>   
>   	qmi_service_unref(data->voice);
>   
> +	l_queue_destroy(data->call_list, l_free);
>   	l_free(data);
>   }
>   
>   static const struct ofono_voicecall_driver driver = {
>   	.probe		= qmi_voicecall_probe,
>   	.remove		= qmi_voicecall_remove,
> +	.dial		= dial,
>   };
>   
>   OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
>   
> +

Empty line at EOF I mentioned earlier is here

Regards,
-Denis

Re: [PATCH v3 2/4] qmimodem: voicecall: Implement call answer

[Denis Kenzior]

Hi Adam,

On 4/2/24 16:25, Adam Pigg wrote:
> The answer function set up the parameters for a call to the service
> function QMI_VOICE_ANSWER_CALL.  The only parameter is the call-id.
> anser_cb will then be called which retrieves the call-id and checks

typo, 'anser_cb'

> the status of the result.
> ---
>   drivers/qmimodem/voice.h     |  1 +
>   drivers/qmimodem/voicecall.c | 65 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 66 insertions(+)
> 

<snip>

> +static void answer(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +	struct ofono_call *call;
> +	struct qmi_param *param = NULL;

I would delay initialization of cbd until after qmi_param_new()

> +
> +	static const uint8_t QMI_VOICE_ANSWER_CALL_ID = 0x01;
> +
> +	DBG("");
> +
> +	cbd->user = vc;
> +
> +	call = l_queue_find(vd->call_list,
> +					ofono_call_compare_by_status,
> +					L_UINT_TO_PTR(CALL_STATUS_INCOMING));
> +
> +	if (call == NULL) {
> +		DBG("Can not find a call to pick up");
> +		goto error;

That way you can just return here.

> +	}
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;

qmi_param_new can't fail (l_new aborts on failure), so this if check isn't needed.

> +
> +	if (!qmi_param_append_uint8(param, QMI_VOICE_ANSWER_CALL_ID,
> +		call->id))

doc/coding-style.txt item M4

> +		goto error;
> +
> +
> +	if (qmi_service_send(vd->voice, QMI_VOICE_ANSWER_CALL, param,
> +		answer_cb, cbd, l_free) > 0)

as above

> +		return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, data);
> +	l_free(cbd);
> +	l_free(param);
> +}
> +

<snip>

Regards,
-Denis

Re: [PATCH v3 3/4] qmimodem: voicecall: Implement active call hangup

[Denis Kenzior]

Hi Adam,

On 4/2/24 16:25, Adam Pigg wrote:
> hangup_active iterates the current list of calls, looking for the first
> active call and then calls release_specific.  This then sets up the
> parameters for a call to QMI_VOICE_END_CALL, with the only paramters

typo: 'paramters'

> being the call-id.
> 
> end_call_cb will then be called and will parse out the call-id and check
> for success.
> ---
>   drivers/qmimodem/voice.h     |  1 +
>   drivers/qmimodem/voicecall.c | 84 +++++++++++++++++++++++++++++++++++-
>   2 files changed, 84 insertions(+), 1 deletion(-)
> 

<snip>

> +static void 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 cb_data *cbd = cb_data_new(cb, data);
> +	struct qmi_param *param = NULL;
> +
> +	static const uint8_t QMI_VOICE_END_CALL_ID = 0x01;
> +
> +	DBG("");
> +	cbd->user = vc;
> +
> +	param = qmi_param_new();

As mentioned before, qmi_param_new() can't fail, so feel free to drop !param check

> +	if (!param)
> +		goto error;
> +
> +	if (!qmi_param_append_uint8(param, QMI_VOICE_END_CALL_ID, id))
> +		goto error;
> +
> +	if (qmi_service_send(vd->voice, QMI_VOICE_END_CALL, param, end_call_cb,
> +		cbd, l_free) > 0)

item M4

> +		return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, data);
> +	l_free(cbd);
> +	l_free(param);
> +}
> +
> +static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
> +				void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct ofono_call *call;
> +	enum call_status active[] = {
> +		CALL_STATUS_ACTIVE,
> +		CALL_STATUS_DIALING,
> +		CALL_STATUS_ALERTING,
> +		CALL_STATUS_INCOMING,
> +	};
> +
> +	DBG("");

item M1

> +	for (uint32_t i = 0; i < L_ARRAY_SIZE(active); i++) {
> +		call = l_queue_find(vd->call_list, ofono_call_compare_by_status,
> +			L_INT_TO_PTR(active[i]));
> +
> +		if (call)
> +			break;
> +	}
> +
> +	if (call == NULL) {
> +		DBG("Can not find a call to hang up");
> +		CALLBACK_WITH_FAILURE(cb, data);
> +		return;
> +	}
> +
> +	release_specific(vc, call->id, cb, data);
> +}
> +
>   static void create_voice_cb(struct qmi_service *service, void *user_data)
>   {
>   	struct ofono_voicecall *vc = user_data;
> @@ -594,7 +674,7 @@ static void create_voice_cb(struct qmi_service *service, void *user_data)
>   	data->voice = qmi_service_ref(service);
>   
>   	qmi_service_register(data->voice, QMI_VOICE_ALL_CALL_STATUS_IND,
> -			     all_call_status_ind, vc, NULL);
> +				all_call_status_ind, vc, NULL);

This belongs in patch 1.

>   
>   	ofono_voicecall_register(vc);
>   }

Regards,
-Denis

Re: [PATCH v3 1/4] qmimodem: voicecall: Implement call dialing

[piggz1]

On Thursday, 4 April 2024, Denis Kenzior wrote:
> Hi Adam,
> 
> On 4/2/24 16:25, Adam Pigg wrote:
> > Add voicecall dialling to the qmimodem driver
> > Includes required infratructure and setup of the QMI services
> > 
> > Call State Handling
> > ===================
> > On initialisation, register the all_call_status_ind callback to be
> > called for QMI_VOICE_IND_ALL_STATUS.  This will handle notificatiof of
> > call status to the rest of the system
> > 
> > Dial Handling
> > =============
> > The dial function sets up the parameters for the QMI_VOICE_DIAL_CALL
> > service.  The parameters are the number to be called and the call type,
> > which is currently hard coded to be QMI_VOICE_CALL_TYPE_VOICE.  The
> > dial_cb callback wi then be called and will receive the call-id.
> > ---
> >   drivers/qmimodem/voice.h     |  17 ++
> >   drivers/qmimodem/voicecall.c | 479 ++++++++++++++++++++++++++++++++++-
> >   2 files changed, 495 insertions(+), 1 deletion(-)
> > 
> 
> <snip>
> 
> Couple of general comments:
> 
> - There's still an empty line @ EOF in this commit, please fix that.
> - After applying just this patch, compilation fails:
> 
> [denkenz@archdev ofono]$ make
> make --no-print-directory all-am
>    CC       drivers/qmimodem/voicecall.o
> drivers/qmimodem/voicecall.c: In function ‘all_call_status_ind’:
> drivers/qmimodem/voicecall.c:355:32: error: unused variable ‘vd’ 
> [-Werror=unused-variable]
>    355 |         struct voicecall_data *vd = ofono_voicecall_get_data(vc);
>        |                                ^~
> drivers/qmimodem/voicecall.c: At top level:
> drivers/qmimodem/voicecall.c:262:16: error: ‘ofono_to_qmi_direction’ defined but 
> not used [-Werror=unused-function]
>    262 | static uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
>        |                ^~~~~~~~~~~~~~~~~~~~~~
> drivers/qmimodem/voicecall.c:97:13: error: ‘ofono_call_compare_by_status’ 
> defined but not used [-Werror=unused-function]
>     97 | static bool ofono_call_compare_by_status(const void *a, const void *b)
>        |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
> cc1: all warnings being treated as errors
> make[1]: *** [Makefile:4094: drivers/qmimodem/voicecall.o] Error 1
> make: *** [Makefile:2402: all] Error 2
> 
> Ideally, each patch should compile and run cleanly to help with 'git bisect'.
> 
> <snip>

Thanks for this, i think the issue might be compiler differences. It all builds ok for me, but sailfish is using gcc 8 atm, so i guess defaults have changed.


> 
> > +
> > +static bool ofono_call_compare_by_status(const void *a, const void *b)
> > +{
> > +	const struct ofono_call *call = a;
> > +	int status = L_PTR_TO_INT(b);
> > +
> > +	return status == call->status;
> > +}
> > +
> 
> This function belongs in patch 2.  It isn't used in this commit and is causing a 
> compiler warning / error.
> 
> <snip>
> 
> > +static uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
> > +{
> > +	return ofono_direction + 1;
> > +}
> 
> This function isn't used, get rid of it
> 
> <snip>
> 
> > +static int qmi_voice_call_status(struct qmi_result *qmi_result,
> > +					struct qmi_voice_all_call_status_ind *result)
> 
> Right now you have this function producing an intermediate structure which 
> points to memory owned by struct qmi_result and is only valid as long as 
> qmi_result is valid.  all_call_status_ind() then converts this intermediate 
> structure into an l_queue with ofono_call objects.  The intermediate structure 
> is never needed after or used elsewhere.  No other caller uses this intermediate 
> structure.
> 
> Have you thought about having this function return an allocated l_queue with 
> ofono_call objects as contents instead and getting rid of struct 
> qmi_voice_all_call_status_ind completely?
> 
> > +{
> > +	int offset;
> > +	uint16_t len;
> > +	bool status = true;
> > +	const struct qmi_voice_remote_party_number *remote_party_number;
> > +	const struct qmi_voice_call_information *call_information;
> > +
> > +	static const uint8_t QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01;
> > +	static const uint8_t QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10;
> > +	static const uint8_t QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10;
> > +	static const uint8_t QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11;
> > +
> > +	/* mandatory */
> > +	call_information = qmi_result_get(
> > +		qmi_result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len);
> > +
> > +	if (!call_information) {
> > +		call_information = qmi_result_get(
> > +			qmi_result, QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION,
> > +			&len);
> > +		status = false;
> > +	}
> > +
> > +	if (call_information) {
> > +		/* verify the length */
> > +		if (len < sizeof(call_information->size))
> > +			return -EMSGSIZE;
> > +
> > +		if (len !=
> > +			call_information->size *
> > +			sizeof(struct qmi_voice_call_information_instance) +
> > +			sizeof(call_information->size))
> > +			return -EMSGSIZE;
> 
> doc/coding-style.txt item M4
> 
> > +
> > +		result->call_information_set = 1;
> > +		result->call_information = call_information;
> > +	} else
> > +		return -EBADMSG;
> 
> Easier to bail early and not nest or need the if condition:
> 
> if (!call_information)
> 	return -EBADMSG;
> 
> if (len < sizeof(call_information->size))
> 	return -EMSGSIZE;
> 
> ...
> 
> > +
> > +	/* mandatory */
> > +	remote_party_number = qmi_result_get(
> > +		qmi_result,
> > +		status ? QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER :
> > +			 QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER,
> > +		&len);
> > +
> > +	if (remote_party_number) {
> > +		const struct qmi_voice_remote_party_number_instance *instance;
> > +		int instance_size =
> > +			sizeof(struct qmi_voice_remote_party_number_instance);
> > +		int i;
> > +
> > +		/* verify the length */
> > +		if (len < sizeof(remote_party_number->size))
> > +			return -EMSGSIZE;
> > +
> > +		for (i = 0, offset = sizeof(remote_party_number->size);
> > +			offset <= len && i < 16 && i < remote_party_number->size;
> > +			i++) {
> > +			if (offset == len)
> > +				break;
> > +			else if (offset + instance_size > len)
> > +				return -EMSGSIZE;
> > +
> > +			instance = (void *)remote_party_number + offset;
> > +			result->remote_party_number[i] = instance;
> > +			offset +=
> > +				sizeof(struct qmi_voice_remote_party_number_instance) +
> > +				instance->number_size;
> > +		}
> > +		result->remote_party_number_set = 1;
> > +		result->remote_party_number_size = remote_party_number->size;
> > +	} else
> > +		return -EBADMSG;
> 
> Same here:
> 
> if (!remote_party_number)
> 	return -EBADMSG;
> 
> if (len < ...)
> 
> for (...)
> 
> See doc/coding-style.txt item O2
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static void all_call_status_ind(struct qmi_result *result, void *user_data)
> > +{
> > +	struct ofono_voicecall *vc = user_data;
> > +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> > +	struct l_queue *calls = l_queue_new();
> > +	int i;
> > +	int size = 0;
> > +	struct qmi_voice_all_call_status_ind status_ind;
> > +
> > +	DBG("");
> 
> doc/coding-style.txt Item M1
> 
> > +	if (qmi_voice_call_status(result, &status_ind) != 0) {
> 
> nit: Prefer < 0 check for functions that return -errno.
> 
> > +		DBG("Parsing of all call status indication failed");
> > +		goto error;
> > +	}
> > +
> > +	if (!status_ind.remote_party_number_set ||
> > +	    !status_ind.call_information_set) {
> 
> Incorrect indentation
> 
> > +		DBG("Some required fields are not set");
> > +		goto error;
> > +	}
> > +
> > +	size = status_ind.call_information->size;
> > +	if (!size) {
> > +		DBG("No call informations received!");
> > +		goto error;
> > +	}
> > +
> > +	/* expect we have valid fields for every call */
> > +	if (size != status_ind.remote_party_number_size) {
> > +		DBG("Not all fields have the same size");
> > +		goto error;
> > +	}
> > +
> > +	for (i = 0; i < size; i++) {
> > +		struct qmi_voice_call_information_instance call_info;
> > +		struct ofono_call *call;
> > +		const struct qmi_voice_remote_party_number_instance
> > +			*remote_party = status_ind.remote_party_number[i];
> > +		int number_size;
> > +
> > +		call_info = status_ind.call_information->instance[i];
> > +		call = l_new(struct ofono_call, 1);
> > +		call->id = call_info.id;
> > +		call->direction = qmi_to_ofono_direction(call_info.direction);
> > +
> > +		if (qmi_to_ofono_status(call_info.state, &call->status)) {
> > +			DBG("Ignore call id %d, because can not convert QMI state 0x%x to ofono.",
> > +				call_info.id, call_info.state);
> > +			l_free(call);
> > +			continue;
> > +		}
> > +		DBG("Call %d in state %s(%d)", call_info.id,
> > +			qmi_voice_call_state_name(call_info.state),
> > +			call_info.state);
> > +
> > +		call->type = 0; /* always voice */
> > +		number_size = remote_party->number_size + 1;
> > +		if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH)
> > +			number_size = OFONO_MAX_PHONE_NUMBER_LENGTH;
> > +		l_strlcpy(call->phone_number.number, remote_party->number,
> > +				number_size);
> > +
> > +		if (strlen(call->phone_number.number) > 0)
> > +			call->clip_validity = 0;
> > +		else
> > +			call->clip_validity = 2;
> > +
> > +		l_queue_push_tail(calls, call);
> > +		DBG("%d", l_queue_length(calls));
> > +	}
> > +
> > +	ofono_call_list_notify(vc, calls);
> > +
> > +	return;
> > +error:
> > +	l_queue_destroy(calls, l_free);
> > +}
> > +
> > +static void dial_cb(struct qmi_result *result, void *user_data)
> > +{
> > +	struct cb_data *cbd = user_data;
> > +	struct ofono_voicecall *vc = cbd->user;
> > +	ofono_voicecall_cb_t cb = cbd->cb;
> > +	uint16_t error;
> > +	uint8_t call_id;
> > +	bool call_id_set = false;
> > +
> > +	static const uint8_t QMI_VOICE_DIAL_RESULT_CALL_ID = 0x10;
> > +
> > +	DBG("");
> > +
> > +	if (qmi_result_set_error(result, &error)) {
> > +		DBG("QMI Error %d", error);
> > +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> > +		return;
> > +	}
> > +
> > +	if (qmi_result_get_uint8(result, QMI_VOICE_DIAL_RESULT_CALL_ID,
> > +					&call_id))
> > +		call_id_set = true;
> > +	else {
> > +		DBG("Received invalid Result");
> > +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> > +		return;
> > +	}
> > +
> > +	if (!call_id_set) {
> > +		DBG("Didn't receive a call id");
> > +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> > +		return;
> > +	}
> 
> This if looks like a dead branch.  The only way call_id_set is false here is if 
> qmi_result_get_uint8 earlier failed.  In fact, the above section can be 
> rewritten as:
> 
> if (!qmi_result_get_uint8(...)) {
> 	ofono_error("No call id in dial result");
> 	CALLBACK_WITH_FAILURE(...);
> 	return;
> }
> 
> No need for call_id_set variable either.
> 
> > +
> > +	DBG("New call QMI id %d", call_id);
> > +	ofono_call_list_dial_callback(vc, call_id);
> > +
> > +	CALLBACK_WITH_SUCCESS(cb, cbd->data);
> > +}
> > +
> 
> <snip>
> 
> > +static void dial(struct ofono_voicecall *vc,
> > +			const struct ofono_phone_number *ph,
> > +			enum ofono_clir_option clir, ofono_voicecall_cb_t cb,
> > +			void *data)
> > +{
> > +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> > +	struct cb_data *cbd = cb_data_new(cb, data);
> > +	struct qmi_param *param;
> > +	const char *calling_number = phone_number_to_string(ph);
> > +
> > +	static const uint8_t QMI_VOICE_DIAL_CALL_NUMBER = 0x01;
> > +	static const uint8_t QMI_VOICE_DIAL_CALL_TYPE = 0x10;
> > +	static const uint8_t QMI_VOICE_CALL_TYPE_VOICE = 0x00;
> > +
> > +	DBG("");
> > +
> > +	cbd->user = vc;
> > +	memcpy(&vd->dialed, ph, sizeof(*ph));
> > +
> > +	param = qmi_param_new();
> > +	if (!param)
> > +		goto error;
> > +
> > +	if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER,
> > +				strlen(calling_number), calling_number)) {
> > +		goto error;
> > +	}
> 
> No need for {}
> 
> > +
> > +	qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE,
> > +				QMI_VOICE_CALL_TYPE_VOICE);
> > +
> > +	if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb,
> > +				cbd, l_free) > 0)
> > +		return;
> > +
> > +error:
> > +	CALLBACK_WITH_FAILURE(cb, data);
> > +	l_free(cbd);
> > +	l_free(param);
> > +}
> > +
> >   static void create_voice_cb(struct qmi_service *service, void *user_data)
> >   {
> >   	struct ofono_voicecall *vc = user_data;
> 
> <snip>
> 
> > @@ -92,13 +566,16 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc)
> >   
> >   	qmi_service_unref(data->voice);
> >   
> > +	l_queue_destroy(data->call_list, l_free);
> >   	l_free(data);
> >   }
> >   
> >   static const struct ofono_voicecall_driver driver = {
> >   	.probe		= qmi_voicecall_probe,
> >   	.remove		= qmi_voicecall_remove,
> > +	.dial		= dial,
> >   };
> >   
> >   OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
> >   
> > +
> 
> Empty line at EOF I mentioned earlier is here
> 
> Regards,
> -Denis
>

-- 
Sent from my Sailfish device

Re: [PATCH v3 4/4] qmimodem: voicecall: Implement DTMF tones

[Denis Kenzior]

Hi Adam,

On 4/2/24 16:25, Adam Pigg wrote:
> The send_dtmf function sets up a call to send_one_dtmf, which will call
> the QMI_VOICE_START_CONTINUOUS_DTMF service function.  The paramters to

typo 'paramters'

> this call are a hard coded call-id and the DTMF character to send.
> start_cont_dtmf_cb will then be called which will set up a call to
> QMI_VOICE_STOP_CONTINUOUS_DTMF to stop the tone.  Finally,
> stop_cont_dtmf_cb will check the final status.
> ---
>   drivers/qmimodem/voice.h     |  12 ++++
>   drivers/qmimodem/voicecall.c | 131 +++++++++++++++++++++++++++++++++++
>   2 files changed, 143 insertions(+)
> 

<snip>

> +
> +enum parse_error {
> +	NONE = 0,
> +	MISSING_MANDATORY = 1,
> +	INVALID_LENGTH = 2,
> +};
> +

This enum isn't used?

>   struct qmi_ussd_data {
>   	uint8_t dcs;
>   	uint8_t length; > diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
> index 627719a8..4bdb64b7 100644
> --- a/drivers/qmimodem/voicecall.c
> +++ b/drivers/qmimodem/voicecall.c
> @@ -42,6 +42,8 @@ struct voicecall_data {
>   	uint16_t minor;
>   	struct l_queue *call_list;
>   	struct ofono_phone_number dialed;
> +	char *full_dtmf;
> +	char *next_dtmf;

next_dtmf can probably be a const char *.

>   };
>   
>   struct qmi_voice_all_call_status_ind {
> @@ -652,6 +654,134 @@ static void hangup_active(struct ofono_voicecall *vc, ofono_voicecall_cb_t cb,
>   	release_specific(vc, call->id, cb, data);
>   }
>   
> +static void stop_cont_dtmf_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_voicecall_cb_t cb = cbd->cb;
> +
> +	uint16_t error;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +	l_free(cbd);

Why is cbd freed only on the success path?  It looks like you're trying to reuse 
the cbd object between START and STOP commands.  Something like this, right?

cbd = l_new(..);
START_DTMF(cbd, start_callback);
start_callback: STOP_DTMF(cbd, stop_callback)
stop_callback: l_free(cbd)

The problem is that start_callback or stop_callback might never be invoked due 
to modem being un-plugged for example.  This is what the destroy callback is 
for, it makes sure to free the user data even if the callback is never called. 
Destroy callback is also invoked after invocation of the actual callback.

So you have two ways of carrying cbd between multiple qmi_send() instances:

1. cb_data_ref/cb_data_unref
2. Storing the 'send_dtmf' callback and callback data in struct voicecall_data

For 1, see drivers/qmimodem/sim.c query_passwd_state_retry()

For 2, you'd store the send_dtmf 'cb' and userdata inside struct voicecall_data 
and use 'vc' as userdata to qmi_send instead of cbd.

> +}
> +
> +static void start_cont_dtmf_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	ofono_voicecall_cb_t cb = cbd->cb;
> +	struct ofono_voicecall *vc = cbd->user;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	uint16_t error;
> +	struct qmi_param *param = NULL;

Do not initialize unnecessarily.  The compiler is pretty good about warnings 
when an uninitialized variable is being used.

> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;
> +
> +	if (!qmi_param_append_uint8(param, QMI_VOICE_DTMF_DATA, 0xff))
> +		goto error;
> +
> +	if (qmi_service_send(vd->voice, QMI_VOICE_STOP_CONTINUOUS_DTMF, param,
> +			     stop_cont_dtmf_cb, cbd, NULL) > 0)

See above about the use of cbd and destroy function

> +		return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, cbd->data);
> +	l_free(cbd);
> +	l_free(param);
> +}
> +
> +static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf,
> +				ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct qmi_param *param = NULL;
> +	uint8_t param_body[2];
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +
> +	DBG("");
> +
> +	cbd->user = vc;
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;

No need for this check

> +
> +	param_body[0] = 0xff;
> +	param_body[1] = (uint8_t)dtmf;
> +
> +	if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, sizeof(param_body),
> +			      param_body)) {
> +		goto error;
> +	}

No need for {}

> +
> +	if (qmi_service_send(vd->voice, QMI_VOICE_START_CONTINUOUS_DTMF, param,
> +			     start_cont_dtmf_cb, cbd, NULL) > 0) {

See above for discussion of cbd and destroy functions

> +		return;
> +	}

No need for {}

> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, data);
> +	l_free(cbd);
> +	l_free(param);
> +}
> +
> +static void send_one_dtmf_cb(const struct ofono_error *error, void *data)
> +{
> +	struct cb_data *cbd = data;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(cbd->user);
> +	ofono_voicecall_cb_t cb = cbd->cb;
> +
> +	DBG("");
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR ||
> +	    *vd->next_dtmf == 0) {
> +		if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> +			CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +		else
> +			CALLBACK_WITH_FAILURE(cb, cbd->data);
> +
> +		l_free(vd->full_dtmf);

vd->full_dtmf = NULL;

> +		l_free(cbd);
> +	} else {
> +		send_one_dtmf(cbd->user,
> +				*(vd->next_dtmf++),
> +				send_one_dtmf_cb, data);
> +	}
> +}
> +
> +static void send_dtmf(struct ofono_voicecall *vc, const char *dtmf,
> +		      ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct cb_data *cbd = cb_data_new(cb, data);
> +
> +	DBG("");
> +
> +	vd->full_dtmf = l_strdup(dtmf);

Should you be freeing full_dtmf first?  Just in case the previous dtmf request 
failed mid-way?

Make sure to free full_dtmf in .remove(), just in case the modem is pulled when 
the DTMF chars are being processed.

> +	vd->next_dtmf = &vd->full_dtmf[1];
> +
> +	cbd->user = vc;
> +
> +	send_one_dtmf(vc, *dtmf, send_one_dtmf_cb, cbd);
> +}
> +
>   static void create_voice_cb(struct qmi_service *service, void *user_data)
>   {
>   	struct ofono_voicecall *vc = user_data;

<snip>

Regards,
-Denis

Re: [PATCH v3 1/4] qmimodem: voicecall: Implement call dialing

[Denis Kenzior]

Hi Adam,

> 
> Thanks for this, i think the issue might be compiler differences. It all builds ok for me, but sailfish is using gcc 8 atm, so i guess defaults have changed.
> 
> 

No worries.  There are indeed differences between compiler warnings.  Also, 
non-maintainer builds do not treat warnings as errors.

The CI will do a good job about pointing out how the project compiles on 
different compilers.  Right now it runs clang and gcc in several configurations.

Still, unused variable and function warnings should be pretty ubiquitous.  What 
you can do pro-actively on your side is to 'git clone' oFono on a regular 
desktop/laptop, run ./bootstrap-configure and try compiling after applying your 
patches.

Regards,
-Denis