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

[Denis Kenzior <denkenz@gmail.com>]

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