Re: [PATCH 02/13] stk: Handle the Send DTMF proactive command.

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9061 bytes --]

Hi Andrew,

On 10/13/2010 08:54 AM, Andrzej Zaborowski wrote:
> In this version 'p' is used as the pause character in DTMF strings as
> it seems in more widespread use than comma.  ISImodem already
> understands 'p'.  atmodem support comes in the next patch.
> ---

I cannibalized this patch a little bit, see comments below:

> @@ -70,7 +70,7 @@ struct ofono_stk {
>  	gboolean respond_on_exit;
>  	ofono_bool_t immediate_response;
>  	guint remove_agent_source;
> -	struct sms_submit_req *sms_submit_req;
> +	struct extern_req *extern_req;
>  	char *idle_mode_text;
>  	struct timeval get_inkey_start_ts;
>  };

This chunk was applied

> @@ -83,7 +83,7 @@ struct envelope_op {
>  			const unsigned char *data, int length);
>  };
>  
> -struct sms_submit_req {
> +struct extern_req {
>  	struct ofono_stk *stk;
>  	gboolean cancelled;
>  };

This chunk was applied

> @@ -435,8 +435,12 @@ static void default_agent_notify(gpointer user_data)
>  {
>  	struct ofono_stk *stk = user_data;
>  
> -	if (stk->current_agent == stk->default_agent && stk->respond_on_exit)
> +	if (stk->current_agent == stk->default_agent && stk->respond_on_exit) {
> +		if (stk->pending_cmd)
> +			stk->cancel_cmd(stk);
> +
>  		send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED);
> +	}
>  
>  	stk->default_agent = NULL;
>  	stk->current_agent = stk->session_agent;
> @@ -450,6 +454,9 @@ static void session_agent_notify(gpointer user_data)
>  	DBG("Session Agent removed");
>  
>  	if (stk->current_agent == stk->session_agent && stk->respond_on_exit) {
> +		if (stk->pending_cmd)
> +			stk->cancel_cmd(stk);
> +
>  		DBG("Sending Terminate response for session agent");
>  		send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED);
>  	}

Can you explain a bit more what these two chunks are doing?

> @@ -660,7 +667,7 @@ static gboolean handle_command_more_time(const struct stk_command *cmd,
>  
>  static void send_sms_cancel(struct ofono_stk *stk)
>  {
> -	stk->sms_submit_req->cancelled = TRUE;
> +	stk->extern_req->cancelled = TRUE;
>  
>  	if (!stk->pending_cmd->send_sms.alpha_id ||
>  			!stk->pending_cmd->send_sms.alpha_id[0])

This chunk has been applied

> @@ -671,7 +678,7 @@ static void send_sms_cancel(struct ofono_stk *stk)
>  
>  static void send_sms_submit_cb(gboolean ok, void *data)
>  {
> -	struct sms_submit_req *req = data;
> +	struct extern_req *req = data;
>  	struct ofono_stk *stk = req->stk;
>  	struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE };
>  	struct stk_response rsp;

This chunk has been applied

> @@ -697,6 +704,12 @@ static void send_sms_submit_cb(gboolean ok, void *data)
>  		stk_command_cb(&failure, stk);
>  }
>  
> +static void extern_req_start(struct ofono_stk *stk)
> +{
> +	stk->extern_req = g_new0(struct extern_req, 1);
> +	stk->extern_req->stk = stk;
> +}
> +
>  static gboolean handle_command_send_sms(const struct stk_command *cmd,
>  					struct stk_response *rsp,
>  					struct ofono_stk *stk)

This chunk has been applied

> @@ -715,15 +728,14 @@ static gboolean handle_command_send_sms(const struct stk_command *cmd,
>  
>  	sms = __ofono_atom_get_data(sms_atom);
>  
> -	stk->sms_submit_req = g_new0(struct sms_submit_req, 1);
> -	stk->sms_submit_req->stk = stk;
> +	extern_req_start(stk);
>  
>  	msg_list.data = (void *) &cmd->send_sms.gsm_sms;
>  	msg_list.next = NULL;
>  
>  	if (__ofono_sms_txq_submit(sms, &msg_list, 0, NULL, send_sms_submit_cb,
> -				stk->sms_submit_req, g_free) < 0) {
> -		g_free(stk->sms_submit_req);
> +				stk->extern_req, g_free) < 0) {
> +		g_free(stk->extern_req);
>  		rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
>  		return TRUE;
>  	}

This chunk has been applied

> @@ -1824,6 +1836,148 @@ static gboolean handle_command_refresh(const struct stk_command *cmd,
>  	return TRUE;
>  }
>  
> +static void send_dtmf_cancel(struct ofono_stk *stk)
> +{
> +	stk->respond_on_exit = FALSE;
> +	stk->extern_req->cancelled = TRUE;
> +
> +	if (stk->pending_cmd->send_dtmf.alpha_id &&
> +			stk->pending_cmd->send_dtmf.alpha_id[0])
> +		stk_alpha_id_unset(stk);
> +}
> +
> +static void dtmf_sent_cb(const struct ofono_error *error, void *user_data)
> +{
> +	struct extern_req *req = user_data;
> +	struct ofono_stk *stk = req->stk;
> +	gboolean cancelled = req->cancelled;
> +
> +	g_free(req);
> +
> +	if (cancelled) {
> +		DBG("Received a DTMF Sent callback after the "
> +				"proactive command was cancelled");
> +		return;
> +	}
> +
> +	stk->respond_on_exit = FALSE;
> +
> +	if (stk->pending_cmd->send_dtmf.alpha_id &&
> +			stk->pending_cmd->send_dtmf.alpha_id[0])
> +		stk_alpha_id_unset(stk);
> +
> +	if (error->type == OFONO_ERROR_TYPE_FAILURE &&
> +			error->error == ENOENT) {

I don't see how this if can be triggered from reviewing the previous
patch.  Should the error be -ENOENT?

> +		struct stk_response rsp;
> +		static unsigned char not_in_speech_call_result[] = { 0x07 };
> +		static struct ofono_error failure =
> +			{ .type = OFONO_ERROR_TYPE_FAILURE };
> +
> +		memset(&rsp, 0, sizeof(rsp));
> +
> +		rsp.result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
> +		rsp.result.additional_len = sizeof(not_in_speech_call_result);
> +		rsp.result.additional = not_in_speech_call_result;
> +
> +		if (stk_respond(stk, &rsp, stk_command_cb))
> +			stk_command_cb(&failure, stk);
> +
> +		return;
> +	}
> +
> +	if (error->type == OFONO_ERROR_TYPE_FAILURE &&
> +			error->error == ENOENT) {

And now we're repeating the if condition above? Have you tested this part?

> +		send_simple_response(stk, STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD);
> +		return;
> +	}
> +
> +	if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> +		send_simple_response(stk, STK_RESULT_TYPE_SUCCESS);
> +	else
> +		send_simple_response(stk, STK_RESULT_TYPE_NOT_CAPABLE);
> +}
> +
> +static gboolean handle_command_send_dtmf(const struct stk_command *cmd,
> +						struct stk_response *rsp,
> +						struct ofono_stk *stk)
> +{
> +	static unsigned char not_in_speech_call_result[] = { 0x07 };
> +	struct ofono_voicecall *vc = NULL;
> +	struct ofono_atom *vc_atom;
> +	char dtmf[256], *digit;
> +	char *dtmf_from = "01234567890abcABC";
> +	char *dtmf_to = "01234567890*#p*#p";
> +	int err, pos;
> +
> +	vc_atom = __ofono_modem_find_atom(__ofono_atom_get_modem(stk->atom),
> +						OFONO_ATOM_TYPE_VOICECALL);
> +	if (vc_atom)
> +		vc = __ofono_atom_get_data(vc_atom);
> +
> +	if (!vc) {
> +		rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE;
> +		return TRUE;
> +	}
> +
> +	/* Convert the DTMF string to phone number format */
> +	for (pos = 0; cmd->send_dtmf.dtmf[pos] != 0; pos++) {
> +		digit = strchr(dtmf_from, cmd->send_dtmf.dtmf[pos]);
> +		if (!digit) {
> +			rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD;
> +			return TRUE;
> +		}
> +
> +		dtmf[pos] = dtmf_to[digit - dtmf_from];
> +	}
> +	dtmf[pos] = 0;
> +
> +	extern_req_start(stk);

I'm not sure that modeling this after the Send SMS implementation is a
good idea.  Send SMS uses the SMS submit queue, which does not support
cancellation.  Wouldn't the Setup Call implementation be a better
template? E.g. a voicecall_send_tone and voicecall_send_tone_cancel?

> +
> +	err = __ofono_voicecall_send_tone(vc, dtmf,
> +						dtmf_sent_cb, stk->extern_req);
> +	if (err < 0)
> +		g_free(stk->extern_req);
> +
> +	if (err == -EBUSY) {
> +		rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
> +		return TRUE;
> +	}
> +
> +	if (err == -ENOSYS) {
> +		rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE;
> +		return TRUE;
> +	}
> +
> +	if (err == -ENOENT) {
> +		rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
> +		rsp->result.additional_len = sizeof(not_in_speech_call_result);
> +		rsp->result.additional = not_in_speech_call_result;
> +		return TRUE;
> +	}
> +
> +	if (err < 0) {
> +		/*
> +		 * We most likely got an out of memory error, tell SIM
> +		 * to retry
> +		 */
> +		rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY;
> +		return TRUE;
> +	}
> +
> +	if (cmd->send_dtmf.alpha_id && cmd->send_dtmf.alpha_id[0])
> +		stk_alpha_id_set(stk, cmd->send_dtmf.alpha_id);
> +
> +	/*
> +	 * Note that we don't strictly require an agent to be connected,
> +	 * but to comply with 6.4.24 we need to send a End Session when
> +	 * the user decides so.
> +	 */
> +	stk->respond_on_exit = TRUE;
> +	stk->cancel_cmd = send_dtmf_cancel;
> +
> +	return FALSE;
> +}
> +
>  static void stk_proactive_command_cancel(struct ofono_stk *stk)
>  {
>  	if (stk->immediate_response)
> @@ -1996,6 +2150,11 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
>  							&rsp, stk);
>  		break;
>  
> +	case STK_COMMAND_TYPE_SEND_DTMF:
> +		respond = handle_command_send_dtmf(stk->pending_cmd,
> +							&rsp, stk);
> +		break;
> +
>  	default:
>  		rsp.result.type = STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD;
>  		break;

Regards,
-Denis