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

[Adam Pigg <piggz1@gmail.com>]

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