diff mbox series

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

Message ID 20240421194926.13149-4-adam@piggz.co.uk (mailing list archive)
State Changes Requested, archived
Headers show
Series [1/4] qmimodem: voicecall: Implement call dialing | expand

Commit Message

Adam Pigg April 21, 2024, 7:49 p.m. UTC
The send_dtmf function sets up a call to send_one_dtmf, which will call
the QMI_VOICE_START_CONTINUOUS_DTMF service function.  The parameters 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.

---
Changes in V4
-Removed unused enum
-Minor formatting fixes
-Ensure data->full_dtmf is free'd
-Use cb_data_ref/unref between chains of dtmf calls

Changes in V5
-Store the cb and cbd obejects in the voicall_data  struct
---
---
 drivers/qmimodem/voice.h     |   6 ++
 drivers/qmimodem/voicecall.c | 114 +++++++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

Comments

Adam Pigg April 21, 2024, 8:06 p.m. UTC | #1
This one took the longest, but I think I implemented it correctly as you 
suggested.  I have ran this through valgrind and didnt catch any leaks.

On Sunday 21 April 2024 20:49:06 BST you 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 parameters 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.
> 
> ---
> Changes in V4
> -Removed unused enum
> -Minor formatting fixes
> -Ensure data->full_dtmf is free'd
> -Use cb_data_ref/unref between chains of dtmf calls
> 
> Changes in V5
> -Store the cb and cbd obejects in the voicall_data  struct
> ---
> ---
>  drivers/qmimodem/voice.h     |   6 ++
>  drivers/qmimodem/voicecall.c | 114 +++++++++++++++++++++++++++++++++++
>  2 files changed, 120 insertions(+)
> 
> diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
> index caedb079..961fe19f 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,10 @@ enum qmi_voice_call_state {
>  	QMI_VOICE_CALL_STATE_SETUP
>  };
> 
> +enum qmi_voice_call_dtmf_param {
> +	QMI_VOICE_DTMF_DATA = 0x01,
> +};
> +
>  struct qmi_ussd_data {
>  	uint8_t dcs;
>  	uint8_t length;
> diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
> index 6b80f576..f51452e5 100644
> --- a/drivers/qmimodem/voicecall.c
> +++ b/drivers/qmimodem/voicecall.c
> @@ -42,6 +42,10 @@ struct voicecall_data {
>  	uint16_t minor;
>  	struct l_queue *call_list;
>  	struct ofono_phone_number dialed;
> +	char *full_dtmf;
> +	const char *next_dtmf;
> +	ofono_voicecall_cb_t send_dtmf_cb;
> +	struct cb_data *send_dtmf_data;
>  };
> 
>  struct qmi_voice_call_information_instance {
> @@ -605,6 +609,114 @@ 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 voicecall_data *vd = user_data;
> +	uint16_t error;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(vd->send_dtmf_cb, vd-
>send_dtmf_data);
> +		return;
> +	}
> +
> +	CALLBACK_WITH_SUCCESS(vd->send_dtmf_cb, vd->send_dtmf_data);
> +}
> +
> +static void start_cont_dtmf_cb(struct qmi_result *result, void *user_data)
> +{
> +	struct voicecall_data *vd = user_data;
> +	uint16_t error;
> +	struct qmi_param *param;
> +
> +	DBG("");
> +
> +	param = qmi_param_new();
> +
> +	if (qmi_result_set_error(result, &error))
> +		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, vd, NULL) > 0)
> +		return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(vd->send_dtmf_cb, vd->send_dtmf_data);
> +	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];
> +
> +	DBG("");
> +
> +	param = qmi_param_new();
> +
> +	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, vd, NULL) > 0)
> +		return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, data);
> +	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 = vd->send_dtmf_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;
> +	} else
> +		send_one_dtmf(cbd->user,
> +				*(vd->next_dtmf++),
> +				send_one_dtmf_cb, vd-
>send_dtmf_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);
> +
> +	DBG("");
> +
> +	l_free(vd->full_dtmf);
> +	vd->full_dtmf = l_strdup(dtmf);
> +	vd->next_dtmf = &vd->full_dtmf[1];
> +
> +	vd->send_dtmf_data = data;
> +	vd->send_dtmf_cb = cb;
> +
> +	send_one_dtmf(vc, *dtmf, send_one_dtmf_cb, NULL);
> +}
> +
>  static void create_voice_cb(struct qmi_service *service, void *user_data)
>  {
>  	struct ofono_voicecall *vc = user_data;
> @@ -664,6 +776,7 @@ 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->full_dtmf);
>  	l_free(data);
>  }
> 
> @@ -674,6 +787,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)
Denis Kenzior April 22, 2024, 9 p.m. UTC | #2
Hi Adam,

On 4/21/24 14:49, 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 parameters 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.
> 
> ---
> Changes in V4
> -Removed unused enum
> -Minor formatting fixes
> -Ensure data->full_dtmf is free'd
> -Use cb_data_ref/unref between chains of dtmf calls
> 
> Changes in V5
> -Store the cb and cbd obejects in the voicall_data  struct
> ---
> ---
>   drivers/qmimodem/voice.h     |   6 ++
>   drivers/qmimodem/voicecall.c | 114 +++++++++++++++++++++++++++++++++++
>   2 files changed, 120 insertions(+)
> 

<snip>

> @@ -605,6 +609,114 @@ 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 voicecall_data *vd = user_data;
> +	uint16_t error;
> +
> +	DBG("");
> +
> +	if (qmi_result_set_error(result, &error)) {
> +		DBG("QMI Error %d", error);
> +		CALLBACK_WITH_FAILURE(vd->send_dtmf_cb, vd->send_dtmf_data);
> +		return;
> +	}
> +
> +	CALLBACK_WITH_SUCCESS(vd->send_dtmf_cb, vd->send_dtmf_data);

This calls back into oFono core unconditionally.  Doesn't this imply that you're 
handling only a single DTMF character?

> +}
> +

<snip>

> +static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf,
> +				ofono_voicecall_cb_t cb, void *data)

Both cb and data are not saved anywhere and are only used on the error path. 
The initial invocation in send_dtmf() also passes in NULL for data...

So again, I think this implies only a request which uses a single DTMF tone 
character would work?

> +{
> +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> +	struct qmi_param *param = NULL;
> +	uint8_t param_body[2];
> +
> +	DBG("");
> +
> +	param = qmi_param_new();
> +
> +	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, vd, NULL) > 0)
> +		return;
> +
> +error:
> +	CALLBACK_WITH_FAILURE(cb, data);
> +	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 = vd->send_dtmf_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;
> +	} else
> +		send_one_dtmf(cbd->user,
> +				*(vd->next_dtmf++),
> +				send_one_dtmf_cb, vd->send_dtmf_data);

This function doesn't seem to be invoked?

> +}
> +

<snip>

Regards,
-Denis
Adam Pigg April 23, 2024, 10:10 a.m. UTC | #3
Hi Dennis

Thanks for the thorough review and merging the others....

On Monday 22 April 2024 22:00:41 BST Denis Kenzior wrote:
> Hi Adam,
> 
> On 4/21/24 14:49, 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 parameters 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.
> > 
> > ---
> > Changes in V4
> > -Removed unused enum
> > -Minor formatting fixes
> > -Ensure data->full_dtmf is free'd
> > -Use cb_data_ref/unref between chains of dtmf calls
> > 
> > Changes in V5
> > -Store the cb and cbd obejects in the voicall_data  struct
> > ---
> > ---
> > 
> >   drivers/qmimodem/voice.h     |   6 ++
> >   drivers/qmimodem/voicecall.c | 114 +++++++++++++++++++++++++++++++++++
> >   2 files changed, 120 insertions(+)
> 
> <snip>
> 
> > @@ -605,6 +609,114 @@ 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 voicecall_data *vd = user_data;
> > +	uint16_t error;
> > +
> > +	DBG("");
> > +
> > +	if (qmi_result_set_error(result, &error)) {
> > +		DBG("QMI Error %d", error);
> > +		CALLBACK_WITH_FAILURE(vd->send_dtmf_cb, vd-
>send_dtmf_data);
> > +		return;
> > +	}
> > +
> > +	CALLBACK_WITH_SUCCESS(vd->send_dtmf_cb, vd->send_dtmf_data);
> 
> This calls back into oFono core unconditionally.  Doesn't this imply that
> you're handling only a single DTMF character?
> 
Currently, ive only been able to test using single character invocations, 
using the num-pad on the phone app.  Do you have any suggestions on how to 
trigger multiple characters?


> > +}
> > +
> 
> <snip>
> 
> > +static void send_one_dtmf(struct ofono_voicecall *vc, const char dtmf,
> > +				ofono_voicecall_cb_t cb, void 
*data)
> 
> Both cb and data are not saved anywhere and are only used on the error path.
> The initial invocation in send_dtmf() also passes in NULL for data...
> 
> So again, I think this implies only a request which uses a single DTMF tone
> character would work?

Im saving the cb and data params in the initial call, and used them as vd-
>,,,, where needed (i think!)
All the user_data params are being set to the vd object so I can get a handle 
on the params in there.
> 
> > +{
> > +	struct voicecall_data *vd = ofono_voicecall_get_data(vc);
> > +	struct qmi_param *param = NULL;
> > +	uint8_t param_body[2];
> > +
> > +	DBG("");
> > +
> > +	param = qmi_param_new();
> > +
> > +	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, vd, NULL) > 0)
> > +		return;
> > +
> > +error:
> > +	CALLBACK_WITH_FAILURE(cb, data);
> > +	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 = vd->send_dtmf_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;
> > +	} else
> > +		send_one_dtmf(cbd->user,
> > +				*(vd->next_dtmf++),
> > +				send_one_dtmf_cb, vd-
>send_dtmf_data);
> 
> This function doesn't seem to be invoked?
This function is passed into send_one_dtmf but yes, potentially goes unused, 
unless it gets invoked in the macro call?  Ill double-check the original 
implementation to see if i messed anything up, but open to suggestions :)
> 
> > +}
> > +
> 
> <snip>
> 
> Regards,
> -Denis
Denis Kenzior April 23, 2024, 3:01 p.m. UTC | #4
Hi Adam,

On 4/23/24 05:10, Adam Pigg wrote:
> Hi Dennis
> 
> Thanks for the thorough review and merging the others....
> 

And thanks for doing all the hard work :)

<snip>

>>> +
>>> +	CALLBACK_WITH_SUCCESS(vd->send_dtmf_cb, vd->send_dtmf_data);
>>
>> This calls back into oFono core unconditionally.  Doesn't this imply that
>> you're handling only a single DTMF character?
>>
> Currently, ive only been able to test using single character invocations,
> using the num-pad on the phone app.  Do you have any suggestions on how to
> trigger multiple characters?

Take a look at doc/voicecallmanager-api.txt.  The SendTones() method takes a 
string of tones, so there can be multiple tones per request.  If your phone 
dialer UI does not support sending multiple tones, then the easiest way is to 
use a command line utility like 'dbus-send' (from dbus-1) or 'busctl' (from 
systemd), or a graphical one like d-feet or qdbus.

Something like:

dbus-send --print-reply --system --dest=org.ofono /phonesim 
org.ofono.VoicecallManager.SendTones string:"1234"

Replace /phonesim with the path of your modem device.

<snip>

>>
>> This function doesn't seem to be invoked?
> This function is passed into send_one_dtmf but yes, potentially goes unused,
> unless it gets invoked in the macro call?  Ill double-check the original
> implementation to see if i messed anything up, but open to suggestions :)

  I think your v4 implementation handled multiple characters correctly, just 
FYI.  Just the memory free paths were not completely correct.

Regards,
-Denis
Adam Pigg April 23, 2024, 9:54 p.m. UTC | #5
So, I decided to have  a re-think and try the burst dtmf method, which accepts 
a string of chars, and should be simpler....

I created a packet struct for the parameter data:
struct qmi_dtmf_burst_info {
	uint8_t call_id;
	uint8_t length;
	char digits[0];
} __attribute__((__packed__));

and set it up as follows
	struct qmi_dtmf_burst_info *di;
	di = alloca(2 + strlen(dtmf));
	di->call_id = 0xff;
	di->length = strlen(dtmf);
	l_strlcpy(di->digits, dtmf, strlen(dtmf));
	param = qmi_param_new();
	if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, di->length + 2,
			di))
		goto error;
	if (qmi_service_send(vd->voice, QMI_VOICE_BURST_DTMF, param,
			send_dtmf_cb, cbd, l_free) > 0)
		return;
But,  in the callback, in a call to qmi_result_set_error, i get back error 24, 
which is QMI_PROTOCOL_ERROR_NETWORK_UNSUPPORTED right?

Not sure where ivce gone wrong, the struct seems to match the format I found 
online for this function call, but is the likely culprit, or its not possible 
to do a burst dtmf, and its better to go back to the original style of 
individual characters?

Thanks


On Tuesday 23 April 2024 16:01:10 BST Denis Kenzior wrote:
> Hi Adam,
> 
> On 4/23/24 05:10, Adam Pigg wrote:
> > Hi Dennis
> > 
> > Thanks for the thorough review and merging the others....
> 
> And thanks for doing all the hard work :)
> 
> <snip>
> 
> >>> +
> >>> +	CALLBACK_WITH_SUCCESS(vd->send_dtmf_cb, vd->send_dtmf_data);
> >> 
> >> This calls back into oFono core unconditionally.  Doesn't this imply that
> >> you're handling only a single DTMF character?
> > 
> > Currently, ive only been able to test using single character invocations,
> > using the num-pad on the phone app.  Do you have any suggestions on how to
> > trigger multiple characters?
> 
> Take a look at doc/voicecallmanager-api.txt.  The SendTones() method takes a
> string of tones, so there can be multiple tones per request.  If your phone
> dialer UI does not support sending multiple tones, then the easiest way is
> to use a command line utility like 'dbus-send' (from dbus-1) or 'busctl'
> (from systemd), or a graphical one like d-feet or qdbus.
> 
> Something like:
> 
> dbus-send --print-reply --system --dest=org.ofono /phonesim
> org.ofono.VoicecallManager.SendTones string:"1234"
> 
> Replace /phonesim with the path of your modem device.
> 
> <snip>
> 
> >> This function doesn't seem to be invoked?
> > 
> > This function is passed into send_one_dtmf but yes, potentially goes
> > unused, unless it gets invoked in the macro call?  Ill double-check the
> > original implementation to see if i messed anything up, but open to
> > suggestions :)
>   I think your v4 implementation handled multiple characters correctly, just
> FYI.  Just the memory free paths were not completely correct.
> 
> Regards,
> -Denis
Adam Pigg April 25, 2024, 7:07 p.m. UTC | #6
On Tuesday 23 April 2024 22:54:57 BST Adam Pigg wrote:
> So, I decided to have  a re-think and try the burst dtmf method, which
> accepts a string of chars, and should be simpler....
> 
> I created a packet struct for the parameter data:
> struct qmi_dtmf_burst_info {
> 	uint8_t call_id;
> 	uint8_t length;
> 	char digits[0];
> } __attribute__((__packed__));
> 
> and set it up as follows
> 	struct qmi_dtmf_burst_info *di;
> 	di = alloca(2 + strlen(dtmf));
> 	di->call_id = 0xff;
> 	di->length = strlen(dtmf);
> 	l_strlcpy(di->digits, dtmf, strlen(dtmf));
> 	param = qmi_param_new();
> 	if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, di->length + 2,
> 			di))
> 		goto error;
> 	if (qmi_service_send(vd->voice, QMI_VOICE_BURST_DTMF, param,
> 			send_dtmf_cb, cbd, l_free) > 0)
> 		return;
> But,  in the callback, in a call to qmi_result_set_error, i get back error
> 24, which is QMI_PROTOCOL_ERROR_NETWORK_UNSUPPORTED right?
> 
Some further research suggested burst dtmf is only supported on 3gpp2 
(cdma2000?) .. so maybe the original implementation is more suitable.


> Not sure where ivce gone wrong, the struct seems to match the format I found
> online for this function call, but is the likely culprit, or its not
> possible to do a burst dtmf, and its better to go back to the original
> style of individual characters?
> 
> Thanks
> 
> On Tuesday 23 April 2024 16:01:10 BST Denis Kenzior wrote:
> > Hi Adam,
> > 
> > On 4/23/24 05:10, Adam Pigg wrote:
> > > Hi Dennis
> > > 
> > > Thanks for the thorough review and merging the others....
> > 
> > And thanks for doing all the hard work :)
> > 
> > <snip>
> > 
> > >>> +
> > >>> +	CALLBACK_WITH_SUCCESS(vd->send_dtmf_cb, vd->send_dtmf_data);
> > >> 
> > >> This calls back into oFono core unconditionally.  Doesn't this imply
> > >> that
> > >> you're handling only a single DTMF character?
> > > 
> > > Currently, ive only been able to test using single character
> > > invocations,
> > > using the num-pad on the phone app.  Do you have any suggestions on how
> > > to
> > > trigger multiple characters?
> > 
> > Take a look at doc/voicecallmanager-api.txt.  The SendTones() method takes
> > a string of tones, so there can be multiple tones per request.  If your
> > phone dialer UI does not support sending multiple tones, then the easiest
> > way is to use a command line utility like 'dbus-send' (from dbus-1) or
> > 'busctl' (from systemd), or a graphical one like d-feet or qdbus.
> > 
> > Something like:
> > 
> > dbus-send --print-reply --system --dest=org.ofono /phonesim
> > org.ofono.VoicecallManager.SendTones string:"1234"
> > 
> > Replace /phonesim with the path of your modem device.
> > 
> > <snip>
> > 
> > >> This function doesn't seem to be invoked?
> > > 
> > > This function is passed into send_one_dtmf but yes, potentially goes
> > > unused, unless it gets invoked in the macro call?  Ill double-check the
> > > original implementation to see if i messed anything up, but open to
> > > suggestions :)
> > > 
> >   I think your v4 implementation handled multiple characters correctly,
> >   just
> > 
> > FYI.  Just the memory free paths were not completely correct.
> > 
> > Regards,
> > -Denis
Denis Kenzior April 25, 2024, 8:30 p.m. UTC | #7
Hi Adam,

On 4/25/24 2:07 PM, adam@piggz.co.uk wrote:
> On Tuesday 23 April 2024 22:54:57 BST Adam Pigg wrote:
>> So, I decided to have  a re-think and try the burst dtmf method, which
>> accepts a string of chars, and should be simpler....
>>

Yeah, that's what I was wondering as well, whether this Burst DTMF command was 
an easier approach.

>> I created a packet struct for the parameter data:
>> struct qmi_dtmf_burst_info {
>> 	uint8_t call_id;
>> 	uint8_t length;
>> 	char digits[0];
>> } __attribute__((__packed__));
>>
>> and set it up as follows
>> 	struct qmi_dtmf_burst_info *di;
>> 	di = alloca(2 + strlen(dtmf));
>> 	di->call_id = 0xff;
>> 	di->length = strlen(dtmf);
>> 	l_strlcpy(di->digits, dtmf, strlen(dtmf));
>> 	param = qmi_param_new();
>> 	if (!qmi_param_append(param, QMI_VOICE_DTMF_DATA, di->length + 2,
>> 			di))
>> 		goto error;
>> 	if (qmi_service_send(vd->voice, QMI_VOICE_BURST_DTMF, param,
>> 			send_dtmf_cb, cbd, l_free) > 0)
>> 		return;

Looks reasonable.

>> But,  in the callback, in a call to qmi_result_set_error, i get back error
>> 24, which is QMI_PROTOCOL_ERROR_NETWORK_UNSUPPORTED right?
>> > Some further research suggested burst dtmf is only supported on 3gpp2
> (cdma2000?) .. so maybe the original implementation is more suitable.
> 

Yeah, ERROR_NETWORK_UNSUPPORTED would seem to support this conclusion.  I think 
your v4 version was pretty close to being ready to be upstreamed.

Regards,
-Denis
diff mbox series

Patch

diff --git a/drivers/qmimodem/voice.h b/drivers/qmimodem/voice.h
index caedb079..961fe19f 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,10 @@  enum qmi_voice_call_state {
 	QMI_VOICE_CALL_STATE_SETUP
 };
 
+enum qmi_voice_call_dtmf_param {
+	QMI_VOICE_DTMF_DATA = 0x01,
+};
+
 struct qmi_ussd_data {
 	uint8_t dcs;
 	uint8_t length;
diff --git a/drivers/qmimodem/voicecall.c b/drivers/qmimodem/voicecall.c
index 6b80f576..f51452e5 100644
--- a/drivers/qmimodem/voicecall.c
+++ b/drivers/qmimodem/voicecall.c
@@ -42,6 +42,10 @@  struct voicecall_data {
 	uint16_t minor;
 	struct l_queue *call_list;
 	struct ofono_phone_number dialed;
+	char *full_dtmf;
+	const char *next_dtmf;
+	ofono_voicecall_cb_t send_dtmf_cb;
+	struct cb_data *send_dtmf_data;
 };
 
 struct qmi_voice_call_information_instance {
@@ -605,6 +609,114 @@  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 voicecall_data *vd = user_data;
+	uint16_t error;
+
+	DBG("");
+
+	if (qmi_result_set_error(result, &error)) {
+		DBG("QMI Error %d", error);
+		CALLBACK_WITH_FAILURE(vd->send_dtmf_cb, vd->send_dtmf_data);
+		return;
+	}
+
+	CALLBACK_WITH_SUCCESS(vd->send_dtmf_cb, vd->send_dtmf_data);
+}
+
+static void start_cont_dtmf_cb(struct qmi_result *result, void *user_data)
+{
+	struct voicecall_data *vd = user_data;
+	uint16_t error;
+	struct qmi_param *param;
+
+	DBG("");
+
+	param = qmi_param_new();
+
+	if (qmi_result_set_error(result, &error))
+		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, vd, NULL) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(vd->send_dtmf_cb, vd->send_dtmf_data);
+	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];
+
+	DBG("");
+
+	param = qmi_param_new();
+
+	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, vd, NULL) > 0)
+		return;
+
+error:
+	CALLBACK_WITH_FAILURE(cb, data);
+	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 = vd->send_dtmf_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;
+	} else
+		send_one_dtmf(cbd->user,
+				*(vd->next_dtmf++),
+				send_one_dtmf_cb, vd->send_dtmf_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);
+
+	DBG("");
+
+	l_free(vd->full_dtmf);
+	vd->full_dtmf = l_strdup(dtmf);
+	vd->next_dtmf = &vd->full_dtmf[1];
+
+	vd->send_dtmf_data = data;
+	vd->send_dtmf_cb = cb;
+
+	send_one_dtmf(vc, *dtmf, send_one_dtmf_cb, NULL);
+}
+
 static void create_voice_cb(struct qmi_service *service, void *user_data)
 {
 	struct ofono_voicecall *vc = user_data;
@@ -664,6 +776,7 @@  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->full_dtmf);
 	l_free(data);
 }
 
@@ -674,6 +787,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)