[PATCH v2 1/4] [qmimodem][voicecall] Implement call dialing

[PATCH v2 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     |  51 ++++
 drivers/qmimodem/voicecall.c | 503 ++++++++++++++++++++++++++++++++++-
 2 files changed, 553 insertions(+), 1 deletion(-)

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index 917e72f7..1a584f10 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -15,6 +15,8 @@
  *
  */
 
+#include <stdint.h>
+
 #define QMI_VOICE_PARAM_USS_DATA 0x01
 
 #define QMI_VOICE_PARAM_ASYNC_USSD_ERROR 0x10
@@ -25,6 +27,9 @@
 #define QMI_VOICE_PARAM_USSD_IND_DATA 0x10
 #define QMI_VOICE_PARAM_USSD_IND_UCS2 0x11
 
+#define QMI_VOICE_IND_ALL_STATUS 0x2e
+#define QMI_VOICE_GET_ALL_STATUS 0x2f
+
 /* according to GSM TS 23.038 section 5
  * coding group 1111, No message class, 8 bit data
  */
@@ -48,6 +53,7 @@ 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_SUPS_NOTIFICATION_IND =	0x32,
 	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
 	QMI_VOICE_GET_CALL_WAITING =		0x34,
@@ -66,6 +72,50 @@ 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
+};
+
+enum qmi_voice_call_dial_param {
+	QMI_VOICE_DIAL_CALL_NUMBER = 0x01,
+	QMI_VOICE_DIAL_CALL_TYPE = 0x10
+};
+
+enum qmi_voice_call_dial_return {
+	QMI_VOICE_DIAL_RETURN_CALL_ID = 0x10
+};
+
+enum qmi_voice_all_call_status_commands {
+	QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01,
+	QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10
+};
+
+enum qmi_voice_all_call_info_commands {
+	QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10,
+	QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11
+};
+
+enum qmi_voice_call_type {
+	QMI_VOICE_CALL_TYPE_VOICE = 0x0,
+	QMI_VOICE_CALL_TYPE_VOICE_FORCE,
+};
+
+enum parse_error {
+	NONE = 0,
+	MISSING_MANDATORY = 1,
+	INVALID_LENGTH = 2,
+};
+
 struct qmi_ussd_data {
 	uint8_t dcs;
 	uint8_t length;
@@ -98,3 +148,4 @@ enum qmi_ss_reason {
 	QMI_VOICE_SS_RSN_CLIP =			0x10,
 	QMI_VOICE_SS_RSN_CLIR =			0x11
 };
+
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index aa34fc25..55c7009a 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,499 @@ struct voicecall_data {
 	struct qmi_service *voice;
 	uint16_t major;
 	uint16_t minor;
+	struct l_queue *call_list;
+	struct voicecall_static *vs;
+	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];
+};
+
+enum parse_error
+qmi_voice_call_status(struct qmi_result *qmi_result,
+		      struct qmi_voice_all_call_status_ind *result);
+
+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__));
+
+struct qmi_voice_dial_call_arg {
+	bool calling_number_set;
+	const char *calling_number;
+	bool call_type_set;
+	uint8_t call_type;
+};
+
+struct qmi_voice_dial_call_result {
+	bool call_id_set;
+	uint8_t call_id;
 };
 
+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;
+}
+
+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;
+}
+
+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);
+}
+
+void ofono_call_list_dial_callback(struct ofono_voicecall *vc,
+				   struct l_queue **call_list,
+				   const struct ofono_phone_number *ph,
+				   int call_id)
+{
+	struct ofono_call *call;
+
+	/* 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);
+}
+
+void ofono_call_list_notify(struct ofono_voicecall *vc,
+			    struct l_queue **call_list, struct l_queue *calls)
+{
+	struct l_queue *old_calls = *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(*call_list, l_free);
+	*call_list = calls;
+}
+
+#define _(X)    \
+	case X: \
+		return #X
+
+const char *qmi_voice_call_state_name(enum qmi_voice_call_state value)
+{
+	switch (value) {
+		_(QMI_VOICE_CALL_STATE_IDLE);
+		_(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);
+	}
+	return "QMI_CALL_STATE_<UNKNOWN>";
+}
+
+int qmi_to_ofono_status(uint8_t status, int *ret)
+{
+	int err = 0;
+
+	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 = 1;
+	}
+	return err;
+}
+
+uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
+{
+	return ofono_direction + 1;
+}
+enum call_direction qmi_to_ofono_direction(uint8_t qmi_direction)
+{
+	return qmi_direction - 1;
+}
+
+enum parse_error
+qmi_voice_call_status(struct qmi_result *qmi_result,
+		      struct qmi_voice_all_call_status_ind *result)
+{
+	int err = NONE;
+	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;
+
+	/* mandatory */
+	call_information = qmi_result_get(
+		qmi_result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len);
+
+	/* This is so ugly! but TLV for indicator and response is different */
+	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 INVALID_LENGTH;
+
+		if (len != call_information->size *
+			sizeof(struct qmi_voice_call_information_instance) +
+			sizeof(call_information->size)) {
+			return INVALID_LENGTH;
+		}
+		result->call_information_set = 1;
+		result->call_information = call_information;
+	} else
+		return MISSING_MANDATORY;
+
+	/* 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 INVALID_LENGTH;
+
+		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 INVALID_LENGTH;
+			}
+
+			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 MISSING_MANDATORY;
+
+	return err;
+}
+
+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 = NULL;
+	int i;
+	int size = 0;
+	struct qmi_voice_all_call_status_ind status_ind;
+
+	calls = l_queue_new();
+
+	DBG("");
+	if (qmi_voice_call_status(result, &status_ind) != NONE) {
+		DBG("Parsing of all call status indication failed");
+		return;
+	}
+
+	if (!status_ind.remote_party_number_set ||
+	    !status_ind.call_information_set) {
+		DBG("Some required fields are not set");
+		return;
+	}
+
+	size = status_ind.call_information->size;
+	if (!size) {
+		DBG("No call informations received!");
+		return;
+	}
+
+	/* expect we have valid fields for every call */
+	if (size != status_ind.remote_party_number_size) {
+		DBG("Not all fields have the same size");
+		return;
+	}
+
+	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);
+			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;
+		if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH)
+			number_size = OFONO_MAX_PHONE_NUMBER_LENGTH;
+		strncpy(call->phone_number.number, remote_party->number,
+			number_size);
+		/* FIXME: set phone_number_type */
+
+		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, &vd->call_list, calls);
+}
+
+enum parse_error
+qmi_voice_dial_call_parse(struct qmi_result *qmi_result,
+			  struct qmi_voice_dial_call_result *result)
+{
+	int err = NONE;
+
+	/* mandatory */
+	if (qmi_result_get_uint8(qmi_result, QMI_VOICE_DIAL_RETURN_CALL_ID,
+				 &result->call_id))
+		result->call_id_set = 1;
+	else
+		err = MISSING_MANDATORY;
+
+	return err;
+}
+
+static void dial_cb(struct qmi_result *result, void *user_data)
+{
+	struct cb_data *cbd = user_data;
+	struct ofono_voicecall *vc = cbd->user;
+	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+	ofono_voicecall_cb_t cb = cbd->cb;
+	uint16_t error;
+	struct qmi_voice_dial_call_result dial_result;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	if (qmi_voice_dial_call_parse(result, &dial_result) != NONE) {
+		DBG("Received invalid Result");
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	if (!dial_result.call_id_set) {
+		DBG("Didn't receive a call id");
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	DBG("New call QMI id %d", dial_result.call_id);
+	ofono_call_list_dial_callback(vc, &vd->call_list, &vd->dialed,
+				      dial_result.call_id);
+
+	/* FIXME: create a timeout on this 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_voice_dial_call_arg arg;
+	struct qmi_param *param = NULL;
+
+	DBG("");
+
+	cbd->user = vc;
+	arg.calling_number_set = true;
+	arg.calling_number = phone_number_to_string(ph);
+	memcpy(&vd->dialed, ph, sizeof(*ph));
+
+	arg.call_type_set = true;
+	arg.call_type = QMI_VOICE_CALL_TYPE_VOICE;
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	if (arg.calling_number_set) {
+		if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER,
+				      strlen(arg.calling_number),
+				      arg.calling_number)) {
+			goto error;
+		}
+	}
+
+	if (arg.call_type_set)
+		qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE,
+				       arg.call_type);
+
+	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 +554,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_IND_ALL_STATUS,
+			     all_call_status_ind, vc, NULL);
+
 	ofono_voicecall_register(vc);
 }
 
@@ -70,6 +569,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 +577,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)
@@ -98,7 +597,9 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc)
 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 v2 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     |  9 ++++
 drivers/qmimodem/voicecall.c | 99 ++++++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index 1a584f10..9c31297b 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -54,6 +54,7 @@ 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_ANSWER_CALL =			0x22,
 	QMI_VOICE_SUPS_NOTIFICATION_IND =	0x32,
 	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
 	QMI_VOICE_GET_CALL_WAITING =		0x34,
@@ -110,6 +111,14 @@ enum qmi_voice_call_type {
 	QMI_VOICE_CALL_TYPE_VOICE_FORCE,
 };
 
+enum qmi_voice_call_answer_param {
+	QMI_VOICE_ANSWER_CALL_ID = 0x01,
+};
+
+enum qmi_voice_call_answer_return {
+	QMI_VOICE_ANSWER_RETURN_CALL_ID = 0x10,
+};
+
 enum parse_error {
 	NONE = 0,
 	MISSING_MANDATORY = 1,
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 55c7009a..7c6bc113 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -97,6 +97,16 @@ struct qmi_voice_dial_call_result {
 	uint8_t call_id;
 };
 
+struct qmi_voice_answer_call_arg {
+	bool call_id_set;
+	uint8_t call_id;
+};
+
+struct qmi_voice_answer_call_result {
+	bool call_id_set;
+	uint8_t call_id;
+};
+
 int ofono_call_compare(const void *a, const void *b, void *data)
 {
 	const struct ofono_call *ca = a;
@@ -533,6 +543,94 @@ error:
 	l_free(param);
 }
 
+enum parse_error qmi_voice_answer_call_parse(
+	struct qmi_result *qmi_result,
+	struct qmi_voice_answer_call_result *result)
+{
+	int err = NONE;
+
+	/* optional */
+	if (qmi_result_get_uint8(qmi_result, QMI_VOICE_ANSWER_RETURN_CALL_ID, &result->call_id))
+		result->call_id_set = 1;
+
+	return err;
+}
+
+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;
+	struct qmi_voice_answer_call_result answer_result;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	/* TODO: what happens when calling it with no active call or wrong caller id? */
+	if (qmi_voice_answer_call_parse(result, &answer_result) != NONE) {
+		DBG("Received invalid Result");
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	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 qmi_voice_answer_call_arg arg;
+	struct ofono_call *call;
+	struct qmi_param *param = NULL;
+
+	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 answer");
+		goto error;
+	}
+
+	arg.call_id_set = true;
+	arg.call_id = call->id;
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	if (arg.call_id_set) {
+		if (!qmi_param_append_uint8(
+			param,
+			QMI_VOICE_ANSWER_CALL_ID,
+			arg.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;
@@ -598,6 +696,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 v2 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     |   9 +++
 drivers/qmimodem/voicecall.c | 113 +++++++++++++++++++++++++++++++++++
 2 files changed, 122 insertions(+)

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index 9c31297b..244a6f85 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -54,6 +54,7 @@ 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_END_CALL =			0x21,
 	QMI_VOICE_ANSWER_CALL =			0x22,
 	QMI_VOICE_SUPS_NOTIFICATION_IND =	0x32,
 	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
@@ -119,6 +120,14 @@ enum qmi_voice_call_answer_return {
 	QMI_VOICE_ANSWER_RETURN_CALL_ID = 0x10,
 };
 
+enum qmi_voice_call_end_param {
+	QMI_VOICE_END_CALL_ID = 0x01,
+};
+
+enum qmi_voice_call_end_return {
+	QMI_VOICE_END_RETURN_CALL_ID = 0x10,
+};
+
 enum parse_error {
 	NONE = 0,
 	MISSING_MANDATORY = 1,
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 7c6bc113..4ef8adc1 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -107,6 +107,16 @@ struct qmi_voice_answer_call_result {
 	uint8_t call_id;
 };
 
+struct qmi_voice_end_call_arg {
+	bool call_id_set;
+	uint8_t call_id;
+};
+
+struct qmi_voice_end_call_result {
+	bool call_id_set;
+	uint8_t call_id;
+};
+
 int ofono_call_compare(const void *a, const void *b, void *data)
 {
 	const struct ofono_call *ca = a;
@@ -631,6 +641,107 @@ error:
 	l_free(param);
 }
 
+enum parse_error
+qmi_voice_end_call_parse(struct qmi_result *qmi_result,
+			 struct qmi_voice_end_call_result *result)
+{
+	int err = NONE;
+
+	/* optional */
+	if (qmi_result_get_uint8(qmi_result, QMI_VOICE_END_RETURN_CALL_ID, &result->call_id))
+		result->call_id_set = 1;
+
+	return err;
+}
+
+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;
+	struct qmi_voice_end_call_result end_result;
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	if (qmi_voice_end_call_parse(result, &end_result) != NONE) {
+		DBG("Received invalid Result");
+		CALLBACK_WITH_FAILURE(cb, cbd->data);
+		return;
+	}
+
+	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_voice_end_call_arg arg;
+	struct qmi_param *param = NULL;
+
+	DBG("");
+	cbd->user = vc;
+
+	arg.call_id_set = true;
+	arg.call_id = id;
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	if (arg.call_id_set) {
+		if (!qmi_param_append_uint8(param, QMI_VOICE_END_CALL_ID, arg.call_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,
+	};
+	int i;
+
+	DBG("");
+	for (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;
@@ -697,6 +808,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 v2 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     |   6 ++
 drivers/qmimodem/voicecall.c | 154 +++++++++++++++++++++++++++++++++++
 2 files changed, 160 insertions(+)

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index 244a6f85..42b6b9b6 100644
--- a/drivers/qmimodem/voice.h
+++ b/drivers/qmimodem/voice.h
@@ -56,6 +56,8 @@ enum voice_commands {
 	QMI_VOICE_DIAL_CALL =			0x20,
 	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,
@@ -128,6 +130,10 @@ enum qmi_voice_call_end_return {
 	QMI_VOICE_END_RETURN_CALL_ID = 0x10,
 };
 
+enum qmi_voice_call_dtmf_param {
+	QMI_VOICE_DTMF_DATA = 0x01,
+};
+
 enum parse_error {
 	NONE = 0,
 	MISSING_MANDATORY = 1,
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 4ef8adc1..ab4bd655 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -117,6 +117,21 @@ struct qmi_voice_end_call_result {
 	uint8_t call_id;
 };
 
+struct send_one_dtmf_cb_data {
+	const char *full_dtmf;
+	const char *next_dtmf;
+	struct ofono_voicecall *vc;
+};
+
+struct qmi_voice_start_cont_dtmf_arg {
+	uint8_t call_id;
+	uint8_t dtmf_char;
+};
+
+struct qmi_voice_stop_cont_dtmf_arg {
+	uint8_t call_id;
+};
+
 int ofono_call_compare(const void *a, const void *b, void *data)
 {
 	const struct ofono_call *ca = a;
@@ -742,6 +757,144 @@ 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);
+}
+
+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);
+	struct qmi_voice_stop_cont_dtmf_arg arg;
+	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;
+	}
+
+	arg.call_id = 0xff;
+
+	param = qmi_param_new();
+	if (!param) {
+		goto error;
+	}
+
+	if (!qmi_param_append_uint8(param, QMI_VOICE_DTMF_DATA, arg.call_id)) {
+		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(param);
+}
+
+static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf,
+			  ofono_voicecall_cb_t cb, void *data)
+{
+	struct qmi_voice_start_cont_dtmf_arg arg;
+	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("");
+
+	arg.call_id = 0xff;
+	arg.dtmf_char = (uint8_t)dtmf;
+
+	cbd->user = vc;
+
+	param = qmi_param_new();
+	if (!param)
+		goto error;
+
+	param_body[0] = arg.call_id;
+	param_body[1] = arg.dtmf_char;
+
+	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;
+	ofono_voicecall_cb_t cb = cbd->cb;
+	struct send_one_dtmf_cb_data *send_one_dtmf_cb_data = cbd->user;
+
+	DBG("");
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR ||
+	    *send_one_dtmf_cb_data->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((void *)send_one_dtmf_cb_data->full_dtmf);
+		l_free(send_one_dtmf_cb_data);
+		l_free(cbd);
+	} else {
+		send_one_dtmf(send_one_dtmf_cb_data->vc,
+			      *(send_one_dtmf_cb_data->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 cb_data *cbd = cb_data_new(cb, data);
+	struct send_one_dtmf_cb_data *send_one_dtmf_cb_data =
+		l_new(struct send_one_dtmf_cb_data, 1);
+
+	DBG("");
+
+	send_one_dtmf_cb_data->full_dtmf = l_strdup(dtmf);
+	send_one_dtmf_cb_data->next_dtmf = &send_one_dtmf_cb_data->full_dtmf[1];
+	send_one_dtmf_cb_data->vc = vc;
+	cbd->user = send_one_dtmf_cb_data;
+
+	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;
@@ -810,6 +963,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 v2 1/4] [qmimodem][voicecall] Implement call dialing

[Denis Kenzior]

Hi Adam,

On 3/26/24 05:19, 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     |  51 ++++
>   drivers/qmimodem/voicecall.c | 503 ++++++++++++++++++++++++++++++++++-
>   2 files changed, 553 insertions(+), 1 deletion(-)

Looking better! I have a bunch of feedback, but it is not exhaustive, so just be 
prepared for more rounds in the future :)

First minor thing is the commit naming conventions, oFono usually uses the 
following:
<modem driver>:<atom>: <description> or
<modem driver>: <description>

So in your case:

qmimodem: voicecall: Implement call dialing ... or
qmi: voice: Implement call dialing ... or
qmimodem: Implement call dialing

I find all three acceptable.

Another thing, please fix your editor not to indent to opening brace or mix tabs 
and spaces.  We only use tabs for indentation.  I'll point out a few examples later.

Your submission doesn't compile without warnings, so that's another thing to 
address for future submissions:

     drivers/qmimodem/voicecall.c:135:5: error: no previous declaration for 
'ofono_call_compare' [-Werror=missing-declarations]
       135 | int ofono_call_compare(const void *a, const void *b, void *data)
           |     ^~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:149:6: error: no previous declaration for 
'ofono_call_compare_by_status' [-Werror=missing-declarations]
       149 | bool ofono_call_compare_by_status(const void *a, const void *b)
           |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:157:6: error: no previous declaration for 
'ofono_call_compare_by_id' [-Werror=missing-declarations]
       157 | bool ofono_call_compare_by_id(const void *a, const void *b)
           |      ^~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:165:6: error: no previous declaration for 
'ofono_call_list_dial_callback' [-Werror=missing-declarations]
       165 | void ofono_call_list_dial_callback(struct ofono_voicecall *vc,
           |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:193:6: error: no previous declaration for 
'ofono_call_list_notify' [-Werror=missing-declarations]
       193 | void ofono_call_list_notify(struct ofono_voicecall *vc,
           |      ^~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:252:13: error: no previous declaration for 
'qmi_voice_call_state_name' [-Werror=missing-declarations]
       252 | const char *qmi_voice_call_state_name(enum qmi_voice_call_state value)
           |             ^~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:270:5: error: no previous declaration for 
'qmi_to_ofono_status' [-Werror=missing-declarations]
       270 | int qmi_to_ofono_status(uint8_t status, int *ret)
           |     ^~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:308:9: error: no previous declaration for 
'ofono_to_qmi_direction' [-Werror=missing-declarations]
       308 | uint8_t ofono_to_qmi_direction(enum call_direction ofono_direction)
           |         ^~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:312:21: error: no previous declaration for 
'qmi_to_ofono_direction' [-Werror=missing-declarations]
       312 | enum call_direction qmi_to_ofono_direction(uint8_t qmi_direction)
           |                     ^~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:472:1: error: no previous declaration for 
'qmi_voice_dial_call_parse' [-Werror=missing-declarations]
       472 | qmi_voice_dial_call_parse(struct qmi_result *qmi_result,
           | ^~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:571:18: error: no previous declaration for 
'qmi_voice_answer_call_parse' [-Werror=missing-declarations]
       571 | enum parse_error qmi_voice_answer_call_parse(
           |                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c:660:1: error: no previous declaration for 
'qmi_voice_end_call_parse' [-Werror=missing-declarations]
       660 | qmi_voice_end_call_parse(struct qmi_result *qmi_result,
           | ^~~~~~~~~~~~~~~~~~~~~~~~
     drivers/qmimodem/voicecall.c: In function 'hangup_active':
     drivers/qmimodem/voicecall.c:743:23: error: comparison of integer 
expressions of different signedness: 'int' and 'long unsigned int' 
[-Werror=sign-compare]
       743 |         for (i = 0; i < L_ARRAY_SIZE(active); i++) {
           |                       ^

CI also reported some issues:

0001-Implement-call-dialing.patch:154: WARNING: externs should be avoided in .c 
files
0001-Implement-call-dialing.patch:165: WARNING: 'als' may be misspelled - 
perhaps 'also'?
0001-Implement-call-dialing.patch:238: WARNING: braces {} are not necessary for 
single statement blocks
0001-Implement-call-dialing.patch:292: WARNING: braces {} are not necessary for 
single statement blocks
0001-Implement-call-dialing.patch:310: WARNING: Macros with flow control 
statements should be avoided
0001-Implement-call-dialing.patch:437: WARNING: braces {} are not necessary for 
any arm of this statement
0001-Implement-call-dialing.patch:482: WARNING: 'informations' may be misspelled 
- perhaps 'information'?

Now, lets get to the review:

> 
> diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
> index 917e72f7..1a584f10 100644
> --- a/drivers/qmimodem/voice.h
> +++ b/drivers/qmimodem/voice.h
> @@ -15,6 +15,8 @@
>    *
>    */
>   
> +#include <stdint.h>
> +

This probably belongs in drivers/qmimodem/voicecall.c itself.  Rule of thumb is 
that internal headers such as wds.h, wms.h, voice.h etc should not have any 
system includes.

>   #define QMI_VOICE_PARAM_USS_DATA 0x01
>   
>   #define QMI_VOICE_PARAM_ASYNC_USSD_ERROR 0x10
> @@ -25,6 +27,9 @@
>   #define QMI_VOICE_PARAM_USSD_IND_DATA 0x10
>   #define QMI_VOICE_PARAM_USSD_IND_UCS2 0x11
>   
> +#define QMI_VOICE_IND_ALL_STATUS 0x2e
> +#define QMI_VOICE_GET_ALL_STATUS 0x2f

Probably belongs in voice_commands enum below.

> +
>   /* according to GSM TS 23.038 section 5
>    * coding group 1111, No message class, 8 bit data
>    */

<snip>

> +
> +enum qmi_voice_call_dial_param {
> +	QMI_VOICE_DIAL_CALL_NUMBER = 0x01,
> +	QMI_VOICE_DIAL_CALL_TYPE = 0x10

The other files inside qmimodem/ use #defines for this.  For example:

#define QMI_DMS_PARAM_REPORT_PIN_STATUS         0x12    /* bool */

Similarly, all RESULT TLVs have '_RESULT_' in the #define name.

Sticking to that convention would be more preferred.

An even more preferred approach would be to avoid definiting enumerations in the 
header file completely.  Typically the enumeration is only used in a single 
function, i.e. the parser or builder for the reqesult/request.  In that case, 
use a static const variable in the function itself.  See
drivers/qmimodem/network-registration.c, set_event_report_cb() for an example. 
The advantage of the above approach is that you have all relevant definitions 
very close to the actual use location and are not tripping over sentence sized 
enumeration names.

> +};
> +
> +enum qmi_voice_call_dial_return {
> +	QMI_VOICE_DIAL_RETURN_CALL_ID = 0x10
> +};
> +
> +enum qmi_voice_all_call_status_commands {

This enum is for the TLV types included in the result, no?  If so, see above 
about naming.

> +	QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION = 0x01,
> +	QMI_VOICE_ALL_CALL_STATUS_REMOTE_NUMBER = 0x10
> +};
> +
> +enum qmi_voice_all_call_info_commands {
> +	QMI_VOICE_ALL_CALL_INFO_CALL_INFORMATION = 0x10,
> +	QMI_VOICE_ALL_CALL_INFO_REMOTE_NUMBER = 0x11
> +};
> +
> +enum qmi_voice_call_type {
> +	QMI_VOICE_CALL_TYPE_VOICE = 0x0,
> +	QMI_VOICE_CALL_TYPE_VOICE_FORCE,
> +};
> +
> +enum parse_error {
> +	NONE = 0,
> +	MISSING_MANDATORY = 1,
> +	INVALID_LENGTH = 2,
> +};
> +

Use errno.h for this:
err = 0 -> ok
err = -EINVAL or -EBADMSG -> missing mandatory
err = -EMSGSIZE -> invalid length

>   struct qmi_ussd_data {
>   	uint8_t dcs;
>   	uint8_t length;
> @@ -98,3 +148,4 @@ enum qmi_ss_reason {
>   	QMI_VOICE_SS_RSN_CLIP =			0x10,
>   	QMI_VOICE_SS_RSN_CLIR =			0x11
>   };
> +

No empty lines at EOF please, my git settings refuse to apply it.

<snip>

> @@ -35,8 +40,499 @@ struct voicecall_data {
>   	struct qmi_service *voice;
>   	uint16_t major;
>   	uint16_t minor;
> +	struct l_queue *call_list;
> +	struct voicecall_static *vs;

This member isn't used?

> +	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];
> +};
> +
> +enum parse_error
> +qmi_voice_call_status(struct qmi_result *qmi_result,
> +		      struct qmi_voice_all_call_status_ind *result);

This function isn't exported, so this declaration isn't necessary.

> +
> +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__));
> +
> +struct qmi_voice_dial_call_arg {
> +	bool calling_number_set;
> +	const char *calling_number;
> +	bool call_type_set;
> +	uint8_t call_type;
> +};

This structure is not needed, get rid of it.

> +
> +struct qmi_voice_dial_call_result {
> +	bool call_id_set;
> +	uint8_t call_id;
>   };
>   
> +int ofono_call_compare(const void *a, const void *b, void *data)

static?

> +{
> +	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;
> +}
> +
> +bool ofono_call_compare_by_status(const void *a, const void *b)

static ?

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

static?

> +{
> +	const struct ofono_call *call = a;
> +	unsigned int id = L_PTR_TO_UINT(b);
> +
> +	return (call->id == id);
> +}
> +
> +void ofono_call_list_dial_callback(struct ofono_voicecall *vc,

static void..

> +				   struct l_queue **call_list,

A single pointer is enough...

> +				   const struct ofono_phone_number *ph,
> +				   int call_id)
> +{
> +	struct ofono_call *call;
> +
> +	/* 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;
> +	}

See the CI complaining about this.  Single statement blocks don't need {}

> +
> +	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);
> +}
> +
> +void ofono_call_list_notify(struct ofono_voicecall *vc,
> +			    struct l_queue **call_list, struct l_queue *calls) > +{
> +	struct l_queue *old_calls = *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);
> +			}

No need for {}

> +		} else {
> +			if (memcmp(new_call, old_call, sizeof(*new_call)) &&
> +			    new_call->type == 0)

Please fix your editor, oFono uses tabs for indentation

> +				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(*call_list, l_free);
> +	*call_list = calls;
> +}
> +

<snip>

> +int qmi_to_ofono_status(uint8_t status, int *ret)

There are two acceptable ways to return success/error status from functions:

bool -> false on error, true on success
int -> 0 is success, -ERRNO (-EINVAL) on error.

I suspect using bool here would be better

> +{
> +	int err = 0;
> +
> +	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 = 1;
> +	}
> +	return err;
> +}
> +

<snip>

> +enum parse_error
> +qmi_voice_call_status(struct qmi_result *qmi_result,
> +		      struct qmi_voice_all_call_status_ind *result)

Similar to the above comment, use an int return here

> +{
> +	int err = NONE;
> +	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;
> +
> +	/* mandatory */
> +	call_information = qmi_result_get(
> +		qmi_result, QMI_VOICE_ALL_CALL_STATUS_CALL_INFORMATION, &len);
> +
> +	/* This is so ugly! but TLV for indicator and response is different */
> +	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 INVALID_LENGTH;
> +
> +		if (len != call_information->size *
> +			sizeof(struct qmi_voice_call_information_instance) +
> +			sizeof(call_information->size)) {
> +			return INVALID_LENGTH;
> +		}
> +		result->call_information_set = 1;
> +		result->call_information = call_information;
> +	} else
> +		return MISSING_MANDATORY;
> +
> +	/* 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 INVALID_LENGTH;
> +
> +		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 INVALID_LENGTH;
> +			}
> +
> +			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 MISSING_MANDATORY;
> +
> +	return err;
> +}
> +
> +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 = NULL;
> +	int i;
> +	int size = 0;
> +	struct qmi_voice_all_call_status_ind status_ind;
> +
> +	calls = l_queue_new();

Might as well move initialization up to the definition.

> +
> +	DBG("");
> +	if (qmi_voice_call_status(result, &status_ind) != NONE) {
> +		DBG("Parsing of all call status indication failed");
> +		return;

You're leaking memory allocated to calls here.  If the queue contents are to be 
freed with l_free, you can do something like:

static void queue_destroy_free(void *p)
{
	struct l_queue *q = *(void **) p;
	l_queue_destroy(q, l_free);
}

_auto_(queue_desroy_free) struct l_queue *calls = l_queue_new() instead.

> +	}
> +
> +	if (!status_ind.remote_party_number_set ||
> +	    !status_ind.call_information_set) {

Another case of your editor indenting incorrectly

> +		DBG("Some required fields are not set");
> +		return;
> +	}
> +
> +	size = status_ind.call_information->size;
> +	if (!size) {
> +		DBG("No call informations received!");
> +		return;
> +	}
> +
> +	/* expect we have valid fields for every call */
> +	if (size != status_ind.remote_party_number_size) {
> +		DBG("Not all fields have the same size");
> +		return;
> +	}
> +
> +	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);
> +			continue;

Leaking call here?

> +		}
> +		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;
> +		if (number_size > OFONO_MAX_PHONE_NUMBER_LENGTH)
> +			number_size = OFONO_MAX_PHONE_NUMBER_LENGTH;
> +		strncpy(call->phone_number.number, remote_party->number,
> +			number_size);

Use l_strlcpy instead of strncpy.  Otherwise you must take care of null 
terminating.  Refer to man strncpy to understand why.

> +		/* FIXME: set phone_number_type */
> +
> +		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, &vd->call_list, calls);

Leaking calls here even on the success path...

> +}
> +
> +enum parse_errorstatic int qmi_voice_dial..
> +qmi_voice_dial_call_parse(struct qmi_result *qmi_result,
> +			  struct qmi_voice_dial_call_result *result)
This structure and the corresponding definition seem useless if they're only 
used between dial_cb and this parser helper.  Inlining the contents inside 
dial_cb would seem to be cleaner.
> +{
> +	int err = NONE;
> +
> +	/* mandatory */
> +	if (qmi_result_get_uint8(qmi_result, QMI_VOICE_DIAL_RETURN_CALL_ID,
> +				 &result->call_id))
> +		result->call_id_set = 1;
> +	else
> +		err = MISSING_MANDATORY;
> +
> +	return err;
> +}
> +
> +static void dial_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct cb_data *cbd = user_data;
> +	struct ofono_voicecall *vc = cbd->user;
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	ofono_voicecall_cb_t cb = cbd->cb;
> +	uint16_t error;
> +	struct qmi_voice_dial_call_result dial_result;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (qmi_voice_dial_call_parse(result, &dial_result) != NONE) {
> +		DBG("Received invalid Result");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (!dial_result.call_id_set) {
> +		DBG("Didn't receive a call id");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	DBG("New call QMI id %d", dial_result.call_id);
> +	ofono_call_list_dial_callback(vc, &vd->call_list, &vd->dialed,

You're passing vc to this function, it can easily obtain the call list via 
ofono_voicecall_get_data(vc);

> +				      dial_result.call_id); > +
> +	/* FIXME: create a timeout on this call_id */

What's this about?

> +	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_voice_dial_call_arg arg;

This structure doesn't seem to serve any purpose?  You can fill out the 
qmi_param information based solely on the passed in arguments.

> +	struct qmi_param *param = NULL;

Don't initialize variables unnecessarily.  The compiler will warn if you're 
using an uninitialized variable.

> +
> +	DBG("");
> +
> +	cbd->user = vc;
> +	arg.calling_number_set = true;
> +	arg.calling_number = phone_number_to_string(ph);
> +	memcpy(&vd->dialed, ph, sizeof(*ph));
> +
> +	arg.call_type_set = true;
> +	arg.call_type = QMI_VOICE_CALL_TYPE_VOICE;
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;
> +
> +	if (arg.calling_number_set) {
> +		if (!qmi_param_append(param, QMI_VOICE_DIAL_CALL_NUMBER,
> +				      strlen(arg.calling_number),
> +				      arg.calling_number)) {
> +			goto error;
> +		}
> +	}
> +
> +	if (arg.call_type_set)
> +		qmi_param_append_uint8(param, QMI_VOICE_DIAL_CALL_TYPE,
> +				       arg.call_type);
> +
> +	if (qmi_service_send(vd->voice, QMI_VOICE_DIAL_CALL, param, dial_cb,
> +			     cbd, l_free) > 0) {
> +		return;
> +	}

No need for {}

> +
> +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 +554,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_IND_ALL_STATUS,
> +			     all_call_status_ind, vc, NULL);
> +
>   	ofono_voicecall_register(vc);
>   }
>   
> @@ -70,6 +569,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 +577,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)
> @@ -98,7 +597,9 @@ static void qmi_voicecall_remove(struct ofono_voicecall *vc)
>   static const struct ofono_voicecall_driver driver = {
>   	.probe		= qmi_voicecall_probe,
>   	.remove		= qmi_voicecall_remove,
> +	.dial		= dial,
>   };
>   
>   OFONO_ATOM_DRIVER_BUILTIN(voicecall, qmimodem, &driver)
>   
> +

No empty lines at EOF please.

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

[Denis Kenzior]

Hi Adam,

On 3/26/24 05:19, 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
> the status of the result.
> ---
>   drivers/qmimodem/voice.h     |  9 ++++
>   drivers/qmimodem/voicecall.c | 99 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 108 insertions(+)
> 

Some CI reported info:

0002-Implement-call-answer.patch:122: WARNING: Prefer using '"%s...", __func__' 
to using 'answer', this function's name, in a string

> diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
> index 1a584f10..9c31297b 100644
> --- a/drivers/qmimodem/voice.h
> +++ b/drivers/qmimodem/voice.h
> @@ -54,6 +54,7 @@ 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_ANSWER_CALL =			0x22,
>   	QMI_VOICE_SUPS_NOTIFICATION_IND =	0x32,
>   	QMI_VOICE_SET_SUPS_SERVICE =		0x33,
>   	QMI_VOICE_GET_CALL_WAITING =		0x34,
> @@ -110,6 +111,14 @@ enum qmi_voice_call_type {
>   	QMI_VOICE_CALL_TYPE_VOICE_FORCE,
>   };
>   
> +enum qmi_voice_call_answer_param {
> +	QMI_VOICE_ANSWER_CALL_ID = 0x01,
> +};
> +
> +enum qmi_voice_call_answer_return {
> +	QMI_VOICE_ANSWER_RETURN_CALL_ID = 0x10,
> +};
> +

See earlier feedback about param / result defines / enums.

>   enum parse_error {
>   	NONE = 0,
>   	MISSING_MANDATORY = 1,
> diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
> index 55c7009a..7c6bc113 100644
> --- a/drivers/qmimodem/voicecall.c
> +++ b/drivers/qmimodem/voicecall.c
> @@ -97,6 +97,16 @@ struct qmi_voice_dial_call_result {
>   	uint8_t call_id;
>   };
>   
> +struct qmi_voice_answer_call_arg {
> +	bool call_id_set;
> +	uint8_t call_id;
> +};
> +
> +struct qmi_voice_answer_call_result {
> +	bool call_id_set;
> +	uint8_t call_id;
> +};
> +

Likely these structures are not needed.

>   int ofono_call_compare(const void *a, const void *b, void *data)
>   {
>   	const struct ofono_call *ca = a;
> @@ -533,6 +543,94 @@ error:
>   	l_free(param);
>   }
>   
> +enum parse_error qmi_voice_answer_call_parse(
> +	struct qmi_result *qmi_result,
> +	struct qmi_voice_answer_call_result *result)
> +{
> +	int err = NONE;
> +
> +	/* optional */
> +	if (qmi_result_get_uint8(qmi_result, QMI_VOICE_ANSWER_RETURN_CALL_ID, &result->call_id))
> +		result->call_id_set = 1;

Just inline this inside answer_cb()

> +
> +	return err;
> +}
> +
> +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;
> +	struct qmi_voice_answer_call_result answer_result;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	/* TODO: what happens when calling it with no active call or wrong caller id? */
> +	if (qmi_voice_answer_call_parse(result, &answer_result) != NONE) {

That function doesn't actually return an error, so is this code block even needed?

> +		DBG("Received invalid Result");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	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 qmi_voice_answer_call_arg arg;

No need for this, fill out qmi_param directly.

> +	struct ofono_call *call;
> +	struct qmi_param *param = NULL;
> +
> +	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 answer");
> +		goto error;
> +	}
> +
> +	arg.call_id_set = true;
> +	arg.call_id = call->id;
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;
> +
> +	if (arg.call_id_set) {
> +		if (!qmi_param_append_uint8(
> +			param,
> +			QMI_VOICE_ANSWER_CALL_ID,
> +			arg.call_id))
> +			goto error;
> +	}
> +
> +	if (qmi_service_send(vd->voice,
> +		QMI_VOICE_ANSWER_CALL,
> +		param,
> +		answer_cb,
> +		cbd,
> +		l_free) > 0)

See doc/coding-style.txt item M4.

> +		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;
> @@ -598,6 +696,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)

Regards,
-Denis

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

[Denis Kenzior]

Hi Adam,

On 3/26/24 05:19, 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
> 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     |   9 +++
>   drivers/qmimodem/voicecall.c | 113 +++++++++++++++++++++++++++++++++++
>   2 files changed, 122 insertions(+)
> 

Reported by CI:
0003-Implement-active-call-hangup.patch:8: WARNING: 'paramters' may be 
misspelled - perhaps 'parameters'?
0003-Implement-active-call-hangup.patch:124: WARNING: braces {} are not 
necessary for single statement blocks

<snip>

> diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
> index 7c6bc113..4ef8adc1 100644
> --- a/drivers/qmimodem/voicecall.c
> +++ b/drivers/qmimodem/voicecall.c
> @@ -107,6 +107,16 @@ struct qmi_voice_answer_call_result {
>   	uint8_t call_id;
>   };
>   
> +struct qmi_voice_end_call_arg {
> +	bool call_id_set;
> +	uint8_t call_id;
> +};
> +
> +struct qmi_voice_end_call_result {
> +	bool call_id_set;
> +	uint8_t call_id;
> +};

Again, totally unneeded structures.

> +
>   int ofono_call_compare(const void *a, const void *b, void *data)
>   {
>   	const struct ofono_call *ca = a;
> @@ -631,6 +641,107 @@ error:
>   	l_free(param);
>   }
>   
> +enum parse_error
> +qmi_voice_end_call_parse(struct qmi_result *qmi_result,
> +			 struct qmi_voice_end_call_result *result)
> +{
> +	int err = NONE;
> +
> +	/* optional */
> +	if (qmi_result_get_uint8(qmi_result, QMI_VOICE_END_RETURN_CALL_ID, &result->call_id))
> +		result->call_id_set = 1;
> +

inline this in the end_call_cb

> +	return err;
> +}
> +
> +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;
> +	struct qmi_voice_end_call_result end_result;
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	if (qmi_voice_end_call_parse(result, &end_result) != NONE) {

This function doesn't actually fail, so again, is this block even needed?

> +		DBG("Received invalid Result");
> +		CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		return;
> +	}
> +
> +	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_voice_end_call_arg arg;
> +	struct qmi_param *param = NULL;
> +
> +	DBG("");
> +	cbd->user = vc;
> +
> +	arg.call_id_set = true;
> +	arg.call_id = id;

Fill out qmi_param directly, no need for this intermediate arg structure.

> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;
> +
> +	if (arg.call_id_set) {
> +		if (!qmi_param_append_uint8(param, QMI_VOICE_END_CALL_ID, arg.call_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,
> +	};
> +	int i;
> +
> +	DBG("");
> +	for (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;
> +	}

Might be easier to do something like:

const struct l_queue_entry *entry;

for (entry = l_queue_get_entries(vd->call_list); entry; entry = entry->next) {
	struct ofono_call *call = entry->data;

	if (L_IN_SET(call->status, CALL_STATUS_ACTIVE,
			CALL_STATUS_DIALING,
			CALL_STATUS_ALERTING,
			CALL_STATUS_INCOMING)) {
		release_specific(vc, call->id, cb, data);
		return;
	}
}

DBG("Can not find...");
CALLBACK_WITH_FAILURE(...);

> +
> +	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;
> @@ -697,6 +808,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)

Regards,
-Denis

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

[Denis Kenzior]

Hi Adam,

On 3/26/24 05:19, 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
> 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     |   6 ++
>   drivers/qmimodem/voicecall.c | 154 +++++++++++++++++++++++++++++++++++
>   2 files changed, 160 insertions(+)
> 

CI says:

0004-Implement-DTMF-tones.patch:7: WARNING: 'paramters' may be misspelled - 
perhaps 'parameters'?
0004-Implement-DTMF-tones.patch:110: WARNING: braces {} are not necessary for 
single statement blocks
0004-Implement-DTMF-tones.patch:114: WARNING: braces {} are not necessary for 
single statement blocks
0004-Implement-DTMF-tones.patch:177: WARNING: braces {} are not necessary for 
any arm of this statement

<snip>

> diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
> index 4ef8adc1..ab4bd655 100644
> --- a/drivers/qmimodem/voicecall.c
> +++ b/drivers/qmimodem/voicecall.c
> @@ -117,6 +117,21 @@ struct qmi_voice_end_call_result {
>   	uint8_t call_id;
>   };
>   
> +struct send_one_dtmf_cb_data {
> +	const char *full_dtmf;
> +	const char *next_dtmf;
> +	struct ofono_voicecall *vc;
> +};
> +
> +struct qmi_voice_start_cont_dtmf_arg {
> +	uint8_t call_id;
> +	uint8_t dtmf_char;
> +};
> +
> +struct qmi_voice_stop_cont_dtmf_arg {
> +	uint8_t call_id;
> +};
> +

The two arg structures can be removed, they don't really serve any purpose.

>   int ofono_call_compare(const void *a, const void *b, void *data)
>   {
>   	const struct ofono_call *ca = a;
> @@ -742,6 +757,144 @@ 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);
> +}
> +
> +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);
> +	struct qmi_voice_stop_cont_dtmf_arg arg;
> +	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;
> +	}
> +
> +	arg.call_id = 0xff;
> +
> +	param = qmi_param_new();
> +	if (!param) {
> +		goto error;
> +	}
> +
> +	if (!qmi_param_append_uint8(param, QMI_VOICE_DTMF_DATA, arg.call_id)) {
> +		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(param);
> +}
> +
> +static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf,
> +			  ofono_voicecall_cb_t cb, void *data)
> +{
> +	struct qmi_voice_start_cont_dtmf_arg arg;
> +	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("");
> +
> +	arg.call_id = 0xff;
> +	arg.dtmf_char = (uint8_t)dtmf;
> +
> +	cbd->user = vc;
> +
> +	param = qmi_param_new();
> +	if (!param)
> +		goto error;
> +
> +	param_body[0] = arg.call_id;
> +	param_body[1] = arg.dtmf_char;
> +
> +	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;
> +	ofono_voicecall_cb_t cb = cbd->cb;
> +	struct send_one_dtmf_cb_data *send_one_dtmf_cb_data = cbd->user;
> +
> +	DBG("");
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR ||
> +	    *send_one_dtmf_cb_data->next_dtmf == 0) {
> +		if (error->type == OFONO_ERROR_TYPE_NO_ERROR) {
> +			CALLBACK_WITH_SUCCESS(cb, cbd->data);
> +		} else {
> +			CALLBACK_WITH_FAILURE(cb, cbd->data);
> +		}

If any DTMF fails, you should return an error right away, cleaning up regardless.

Then, if it is the last entry, callback with success and cleanup.

Otherwise, send the next DTMF.

> +		l_free((void *)send_one_dtmf_cb_data->full_dtmf);
> +		l_free(send_one_dtmf_cb_data);
> +		l_free(cbd);
> +	} else {
> +		send_one_dtmf(send_one_dtmf_cb_data->vc,
> +			      *(send_one_dtmf_cb_data->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 cb_data *cbd = cb_data_new(cb, data);
> +	struct send_one_dtmf_cb_data *send_one_dtmf_cb_data =
> +		l_new(struct send_one_dtmf_cb_data, 1);
> +
> +	DBG("");
> +
> +	send_one_dtmf_cb_data->full_dtmf = l_strdup(dtmf);

full_dtmf is declared const.  How are you freeing the string?

> +	send_one_dtmf_cb_data->next_dtmf = &send_one_dtmf_cb_data->full_dtmf[1];
> +	send_one_dtmf_cb_data->vc = vc;
> +	cbd->user = send_one_dtmf_cb_data;

cb_data is not setup to destroy cbd->user on unref.  So it is likely this 
structure is leaked on some of the code paths.  This is something we can add, 
for example by introducing a destroy member to struct cb_data.  However, it may 
be far easier to just put dtmf information into voicecall_data.

Have you looked into using 'Burst DTMF' instead of Start/Stop Continuous DTMF? 
Looks like that one takes a string.  Might be an easier implementation.

> +
> +	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;
> @@ -810,6 +963,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)

Regards,
-Denis