From: Denis Kenzior <denkenz@gmail.com>
To: Adam Pigg <adam@piggz.co.uk>, ofono@lists.linux.dev
Subject: Re: [PATCH v2 4/4] [qmimodem][voicecall] Implement DTMF tones
Date: Wed, 27 Mar 2024 14:31:40 -0500 [thread overview]
Message-ID: <14194773-71a3-47e5-b1e8-70e0c77ce304@gmail.com> (raw)
In-Reply-To: <20240326102054.30946-4-adam@piggz.co.uk>
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
next prev parent reply other threads:[~2024-03-27 19:31 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-26 10:19 [PATCH v2 1/4] [qmimodem][voicecall] Implement call dialing Adam Pigg
2024-03-26 10:19 ` [PATCH v2 2/4] [qmimodem][voicecall] Implement call answer Adam Pigg
2024-03-27 18:38 ` Denis Kenzior
2024-03-26 10:19 ` [PATCH v2 3/4] [qmimodem][voicecall] Implement active call hangup Adam Pigg
2024-03-27 18:49 ` Denis Kenzior
2024-03-26 10:19 ` [PATCH v2 4/4] [qmimodem][voicecall] Implement DTMF tones Adam Pigg
2024-03-27 19:31 ` Denis Kenzior [this message]
2024-03-27 18:32 ` [PATCH v2 1/4] [qmimodem][voicecall] Implement call dialing Denis Kenzior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=14194773-71a3-47e5-b1e8-70e0c77ce304@gmail.com \
--to=denkenz@gmail.com \
--cc=adam@piggz.co.uk \
--cc=ofono@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).