[PATCH 1/9] stk: Utilities for proactive command/envelope handling

[PATCH 1/9] stk: Utilities for proactive command/envelope handling

[Andrzej Zaborowski]

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

---
 include/stk.h |    2 +
 src/stk.c     |  144 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 129 insertions(+), 17 deletions(-)

diff --git a/include/stk.h b/include/stk.h
index ad3f6c5..4e5d01e 100644
--- a/include/stk.h
+++ b/include/stk.h
@@ -65,6 +65,8 @@ void *ofono_stk_get_data(struct ofono_stk *stk);
 void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 					int length, const unsigned char *pdu);
 
+void ofono_stk_proactive_command_cancel(struct ofono_stk *stk);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/src/stk.c b/src/stk.c
index b5c6919..e513b06 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -43,10 +43,61 @@ struct ofono_stk {
 	const struct ofono_stk_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
+	struct stk_command *pending_cmd;
+	void (*cancel_cmd)(struct ofono_stk *stk);
 };
 
+static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
+			void (*cb)(const struct ofono_error *error,
+					struct ofono_stk *stk))
+{
+	const guint8 *tlv;
+	unsigned int tlv_len;
+
+	rsp->src = STK_DEVICE_IDENTITY_TYPE_TERMINAL;
+	rsp->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
+	rsp->number = stk->pending_cmd->number;
+	rsp->type = stk->pending_cmd->type;
+	rsp->qualifier = stk->pending_cmd->qualifier;
+
+	if (stk->driver->terminal_response == NULL)
+		return -ENOSYS;
+
+	tlv = stk_pdu_from_response(rsp, &tlv_len);
+	if (!tlv)
+		return -EINVAL;
+
+	stk->driver->terminal_response(stk, tlv_len, tlv,
+					(ofono_stk_generic_cb_t) cb, stk);
+	return 0;
+}
+
+static int stk_send_envelope(struct ofono_stk *stk, struct stk_envelope *e,
+				void (*cb)(const struct ofono_error *error,
+						const unsigned char *data,
+						int length,
+						struct ofono_stk *stk))
+{
+	const guint8 *tlv;
+	unsigned int tlv_len;
+
+	e->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
+
+	if (stk->driver->envelope == NULL)
+		return -ENOSYS;
+
+	tlv = stk_pdu_from_envelope(e, &tlv_len);
+	if (!tlv)
+		return -EINVAL;
+
+	stk->driver->envelope(stk, tlv_len, tlv,
+				(ofono_stk_envelope_cb_t) cb, stk);
+	return 0;
+}
+
 static void stk_cbs_download_cb(const struct ofono_error *error,
-				const unsigned char *data, int len, void *user)
+				const unsigned char *data, int len,
+				struct ofono_stk *stk)
 {
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
 		ofono_error("CellBroadcast download to UICC failed");
@@ -55,36 +106,62 @@ static void stk_cbs_download_cb(const struct ofono_error *error,
 		return;
 	}
 
+	if (len)
+		ofono_error("CellBroadcast download returned %i bytes of data",
+				len);
+
 	DBG("CellBroadcast download to UICC reported no error");
 }
 
 void __ofono_cbs_sim_download(struct ofono_stk *stk, const struct cbs *msg)
 {
-	const guint8 *tlv;
-	unsigned int tlv_len;
+	struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
 	struct stk_envelope e;
+	int err;
 
-	if (stk->driver->envelope == NULL)
-		return;
+	memset(&e, 0, sizeof(e));
 
 	e.type = STK_ENVELOPE_TYPE_CBS_PP_DOWNLOAD;
 	e.src = STK_DEVICE_IDENTITY_TYPE_NETWORK;
-	e.dst = STK_DEVICE_IDENTITY_TYPE_UICC;
 	memcpy(&e.cbs_pp_download.page, msg, sizeof(msg));
 
-	tlv = stk_pdu_from_envelope(&e, &tlv_len);
-	if (!tlv)
+	err = stk_send_envelope(stk, &e, stk_cbs_download_cb);
+	if (err)
+		stk_cbs_download_cb(&error, NULL, -1, stk);
+}
+
+static void stk_command_cb(const struct ofono_error *error,
+				struct ofono_stk *stk)
+{
+	stk_command_free(stk->pending_cmd);
+	stk->pending_cmd = NULL;
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+		ofono_error("TERMINAL RESPONSE to a UICC command failed");
+		/* "The ME may retry to deliver the same Cell Broadcast
+		 * page." */
 		return;
+	}
 
-	stk->driver->envelope(stk, tlv_len, tlv, stk_cbs_download_cb, stk);
+	DBG("TERMINAL RESPONSE to a command reported no errors");
+}
+
+void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
+{
+	if (!stk->pending_cmd)
+		return;
+
+	stk->cancel_cmd(stk);
 }
 
 void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 					int length, const unsigned char *pdu)
 {
-	struct stk_command *cmd;
+	struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
+	struct stk_response rsp;
 	char *buf;
-	int i;
+	int i, err;
+	gboolean respond = TRUE;
 
 	buf = g_try_malloc(length * 2 + 1);
 	if (!buf)
@@ -93,18 +170,51 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 	for (i = 0; i < length; i ++)
 		sprintf(buf + i * 2, "%02hhx", pdu[i]);
 
-	cmd = stk_command_new_from_pdu(pdu, length);
-	if (!cmd) {
+	stk->pending_cmd = stk_command_new_from_pdu(pdu, length);
+	if (!stk->pending_cmd) {
 		ofono_error("Can't parse proactive command: %s", buf);
 
-		/* TODO: return TERMINAL RESPONSE with permanent error */
+		/*
+		 * Nothing we can do, we'd need at least Command Details
+		 * to be able to respond with an error.
+		 */
 		goto done;
 	}
 
-	/* TODO: execute */
-	ofono_info("Proactive command PDU: %s", buf);
+	ofono_debug("Proactive command PDU: %s", buf);
+
+	memset(&rsp, 0, sizeof(rsp));
+
+	switch (stk->pending_cmd->status) {
+	case STK_PARSE_RESULT_OK:
+		switch (stk->pending_cmd->type) {
+		default:
+			rsp.result.type =
+				STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD;
+			break;
+		}
+
+		if (respond)
+			break;
+		return;
+
+	case STK_PARSE_RESULT_TYPE_NOT_UNDERSTOOD:
+	default:
+		rsp.result.type = STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD;
+		break;
+
+	case STK_PARSE_RESULT_MISSING_VALUE:
+		rsp.result.type = STK_RESULT_TYPE_MINIMUM_NOT_MET;
+		break;
+
+	case STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD:
+		rsp.result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD;
+		break;
+	}
 
-	stk_command_free(cmd);
+	err = stk_respond(stk, &rsp, stk_command_cb);
+	if (err)
+		stk_command_cb(&error, stk);
 
 done:
 	g_free(buf);
-- 
1.7.1.86.g0e460.dirty

[PATCH 2/9] mbmmodem: Cancel running command on *STKEND.

[Andrzej Zaborowski]

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

---
 drivers/mbmmodem/stk.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/mbmmodem/stk.c b/drivers/mbmmodem/stk.c
index 77bd7b5..53b5adf 100644
--- a/drivers/mbmmodem/stk.c
+++ b/drivers/mbmmodem/stk.c
@@ -179,6 +179,14 @@ static void stkn_notify(GAtResult *result, gpointer user_data)
 
 static void stkend_notify(GAtResult *result, gpointer user_data)
 {
+	struct ofono_stk *stk = user_data;
+
+	/* Seems to be used by the modem and/or cards to indicate
+	 * that we have taken too long to respond to a command, but
+	 * does not invalidate current application state, such as
+	 * main menu (?)
+	 */
+	ofono_stk_proactive_command_cancel(stk);
 }
 
 static void mbm_stkc_cb(gboolean ok, GAtResult *result, gpointer user_data)
-- 
1.7.1.86.g0e460.dirty

[PATCH 3/9] stk: Handle the More Time command as a nop.

[Andrzej Zaborowski]

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

---
 src/stk.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/src/stk.c b/src/stk.c
index e513b06..4edf05b 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -154,6 +154,15 @@ void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
 	stk->cancel_cmd(stk);
 }
 
+static gboolean handle_command_more_time(const struct stk_command *cmd,
+						struct stk_response *rsp,
+						struct ofono_stk *stk)
+{
+	/* Do nothing */
+
+	return TRUE;
+}
+
 void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 					int length, const unsigned char *pdu)
 {
@@ -192,6 +201,10 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 			rsp.result.type =
 				STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD;
 			break;
+		case STK_COMMAND_TYPE_MORE_TIME:
+			respond = handle_command_more_time(stk->pending_cmd,
+								&rsp, stk);
+			break;
 		}
 
 		if (respond)
-- 
1.7.1.86.g0e460.dirty

[PATCH 4/9] Make sim operations return sim error codes to core.

[Andrzej Zaborowski]

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

---
 drivers/atmodem/stk.c |   20 ++++++++++++--------
 include/types.h       |    1 +
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/atmodem/stk.c b/drivers/atmodem/stk.c
index 8cff4a2..aede668 100644
--- a/drivers/atmodem/stk.c
+++ b/drivers/atmodem/stk.c
@@ -74,11 +74,13 @@ static void at_csim_envelope_cb(gboolean ok, GAtResult *result,
 	if (rlen != len * 2 || len < 2)
 		goto error;
 
-	if (response[len - 2] != 0x90 && response[len - 2] != 0x91)
-		goto error;
+	if ((response[len - 2] != 0x90 && response[len - 2] != 0x91) ||
+			(response[len - 2] == 0x90 && response[len - 1] != 0)) {
+		memset(&error, 0, sizeof(error));
 
-	if (response[len - 2] == 0x90 && response[len - 1] != 0)
-		goto error;
+		error.type = OFONO_ERROR_TYPE_SIM;
+		error.error = (response[len - 2] << 8) | response[len - 1];
+	}
 
 	DBG("csim_envelope_cb: %i", len);
 
@@ -157,11 +159,13 @@ static void at_csim_terminal_response_cb(gboolean ok, GAtResult *result,
 	if (rlen != len * 2 || len < 2)
 		goto error;
 
-	if (response[len - 2] != 0x90 && response[len - 2] != 0x91)
-		goto error;
+	if ((response[len - 2] != 0x90 && response[len - 2] != 0x91) ||
+			(response[len - 2] == 0x90 && response[len - 1] != 0)) {
+		memset(&error, 0, sizeof(error));
 
-	if (response[len - 2] == 0x90 && response[len - 1] != 0)
-		goto error;
+		error.type = OFONO_ERROR_TYPE_SIM;
+		error.error = (response[len - 2] << 8) | response[len - 1];
+	}
 
 	DBG("csim_terminal_response_cb: %i", len);
 
diff --git a/include/types.h b/include/types.h
index 2b154f0..6098cba 100644
--- a/include/types.h
+++ b/include/types.h
@@ -60,6 +60,7 @@ enum ofono_error_type {
 	OFONO_ERROR_TYPE_CME,
 	OFONO_ERROR_TYPE_CMS,
 	OFONO_ERROR_TYPE_CEER,
+	OFONO_ERROR_TYPE_SIM,
 	OFONO_ERROR_TYPE_FAILURE
 };
 
-- 
1.7.1.86.g0e460.dirty

Re: [PATCH 4/9] Make sim operations return sim error codes to core.

[Denis Kenzior]

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

Hi Andrew,

On 06/29/2010 05:14 AM, Andrzej Zaborowski wrote:
> ---
>  drivers/atmodem/stk.c |   20 ++++++++++++--------
>  include/types.h       |    1 +

When you're touching more than two directories, always break the patches
up / directory.  In this case resubmit two patches, one for types.h and
separate patch for stk.c

>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/atmodem/stk.c b/drivers/atmodem/stk.c
> index 8cff4a2..aede668 100644
> --- a/drivers/atmodem/stk.c
> +++ b/drivers/atmodem/stk.c
> @@ -74,11 +74,13 @@ static void at_csim_envelope_cb(gboolean ok, GAtResult *result,
>  	if (rlen != len * 2 || len < 2)
>  		goto error;
>  
> -	if (response[len - 2] != 0x90 && response[len - 2] != 0x91)
> -		goto error;
> +	if ((response[len - 2] != 0x90 && response[len - 2] != 0x91) ||
> +			(response[len - 2] == 0x90 && response[len - 1] != 0)) {
> +		memset(&error, 0, sizeof(error));
>  
> -	if (response[len - 2] == 0x90 && response[len - 1] != 0)
> -		goto error;
> +		error.type = OFONO_ERROR_TYPE_SIM;
> +		error.error = (response[len - 2] << 8) | response[len - 1];
> +	}
>  
>  	DBG("csim_envelope_cb: %i", len);
>  
> @@ -157,11 +159,13 @@ static void at_csim_terminal_response_cb(gboolean ok, GAtResult *result,
>  	if (rlen != len * 2 || len < 2)
>  		goto error;
>  
> -	if (response[len - 2] != 0x90 && response[len - 2] != 0x91)
> -		goto error;
> +	if ((response[len - 2] != 0x90 && response[len - 2] != 0x91) ||
> +			(response[len - 2] == 0x90 && response[len - 1] != 0)) {
> +		memset(&error, 0, sizeof(error));
>  
> -	if (response[len - 2] == 0x90 && response[len - 1] != 0)
> -		goto error;
> +		error.type = OFONO_ERROR_TYPE_SIM;
> +		error.error = (response[len - 2] << 8) | response[len - 1];
> +	}
>  
>  	DBG("csim_terminal_response_cb: %i", len);
>  

Do you think it is a good idea to do this for all SIM elementary file
operations as well?

Regards,
-Denis

[PATCH 5/9] stk: Handle ENVELOPEs in a queue, retry on sim busy.

[Andrzej Zaborowski]

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

Some envelope types should be retried when sim reports busy status.
Envelopes such as Event Download need to be returned in the order
of the event occurences, so need to be handled in a queue.
---
 src/stk.c |  110 +++++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 90 insertions(+), 20 deletions(-)

diff --git a/src/stk.c b/src/stk.c
index 4edf05b..4fea62b 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -45,8 +45,22 @@ struct ofono_stk {
 	struct ofono_atom *atom;
 	struct stk_command *pending_cmd;
 	void (*cancel_cmd)(struct ofono_stk *stk);
+
+	gboolean envelope_q_busy;
+	GQueue *envelope_q;
+};
+
+struct envelope_op {
+	struct stk_envelope e;
+	int retries;
+	void (*cb)(struct ofono_stk *stk, gboolean ok,
+			const unsigned char *data, int length);
 };
 
+#define ENVELOPE_RETRIES_DEFAULT 5
+
+static void envelope_queue_run(struct ofono_stk *stk);
+
 static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
 			void (*cb)(const struct ofono_error *error,
 					struct ofono_stk *stk))
@@ -72,37 +86,87 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
 	return 0;
 }
 
-static int stk_send_envelope(struct ofono_stk *stk, struct stk_envelope *e,
-				void (*cb)(const struct ofono_error *error,
-						const unsigned char *data,
-						int length,
-						struct ofono_stk *stk))
+static void envelope_cb(const struct ofono_error *error, const uint8_t *data,
+			int length, void *user_data)
+{
+	struct ofono_stk *stk = user_data;
+	struct envelope_op *op = g_queue_peek_head(stk->envelope_q);
+	gboolean result = TRUE;
+
+	stk->envelope_q_busy = FALSE;
+
+	if (op->retries > 0 && error->type == OFONO_ERROR_TYPE_SIM &&
+			error->error == 0x9300) {
+		op->retries--;
+		goto out;
+	}
+
+	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
+		result = FALSE;
+
+	g_queue_pop_head(stk->envelope_q);
+
+	if (op->cb)
+		op->cb(stk, result, data, length);
+	g_free(op);
+
+out:
+	envelope_queue_run(stk);
+}
+
+static void envelope_queue_run(struct ofono_stk *stk)
 {
 	const guint8 *tlv;
 	unsigned int tlv_len;
 
-	e->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
+	while (stk->envelope_q_busy == FALSE &&
+			g_queue_get_length(stk->envelope_q) > 0) {
+		struct envelope_op *op = g_queue_peek_head(stk->envelope_q);
+
+		tlv = stk_pdu_from_envelope(&op->e, &tlv_len);
+		if (!tlv) {
+			g_queue_pop_head(stk->envelope_q);
+
+			op->cb(stk, FALSE, NULL, -1);
+			g_free(op);
+
+			continue;
+		}
+
+		stk->envelope_q_busy = TRUE;
+		stk->driver->envelope(stk, tlv_len, tlv, envelope_cb, stk);
+	}
+}
+
+static int stk_send_envelope(struct ofono_stk *stk, struct stk_envelope *e,
+				void (*cb)(struct ofono_stk *stk, gboolean ok,
+						const uint8_t *data,
+						int length), int retries)
+{
+	struct envelope_op *op;
 
 	if (stk->driver->envelope == NULL)
 		return -ENOSYS;
 
-	tlv = stk_pdu_from_envelope(e, &tlv_len);
-	if (!tlv)
-		return -EINVAL;
+	op = g_new0(struct envelope_op, 1);
+
+	memcpy(&op->e, e, sizeof(op->e));
+	op->e.dst = STK_DEVICE_IDENTITY_TYPE_UICC;
+	op->cb = cb;
+	op->retries = retries;
+
+	g_queue_push_tail(stk->envelope_q, op);
+
+	envelope_queue_run(stk);
 
-	stk->driver->envelope(stk, tlv_len, tlv,
-				(ofono_stk_envelope_cb_t) cb, stk);
 	return 0;
 }
 
-static void stk_cbs_download_cb(const struct ofono_error *error,
-				const unsigned char *data, int len,
-				struct ofono_stk *stk)
+static void stk_cbs_download_cb(struct ofono_stk *stk, gboolean ok,
+				const unsigned char *data, int len)
 {
-	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
+	if (!ok) {
 		ofono_error("CellBroadcast download to UICC failed");
-		/* "The ME may retry to deliver the same Cell Broadcast
-		 * page." */
 		return;
 	}
 
@@ -115,7 +179,6 @@ static void stk_cbs_download_cb(const struct ofono_error *error,
 
 void __ofono_cbs_sim_download(struct ofono_stk *stk, const struct cbs *msg)
 {
-	struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
 	struct stk_envelope e;
 	int err;
 
@@ -125,9 +188,10 @@ void __ofono_cbs_sim_download(struct ofono_stk *stk, const struct cbs *msg)
 	e.src = STK_DEVICE_IDENTITY_TYPE_NETWORK;
 	memcpy(&e.cbs_pp_download.page, msg, sizeof(msg));
 
-	err = stk_send_envelope(stk, &e, stk_cbs_download_cb);
+	err = stk_send_envelope(stk, &e, stk_cbs_download_cb,
+				ENVELOPE_RETRIES_DEFAULT);
 	if (err)
-		stk_cbs_download_cb(&error, NULL, -1, stk);
+		stk_cbs_download_cb(stk, FALSE, NULL, -1);
 }
 
 static void stk_command_cb(const struct ofono_error *error,
@@ -254,6 +318,10 @@ void ofono_stk_driver_unregister(const struct ofono_stk_driver *d)
 
 static void stk_unregister(struct ofono_atom *atom)
 {
+	struct ofono_stk *stk = __ofono_atom_get_data(atom);
+
+	g_queue_foreach(stk->envelope_q, (GFunc) g_free, NULL);
+	g_queue_free(stk->envelope_q);
 }
 
 static void stk_remove(struct ofono_atom *atom)
@@ -309,6 +377,8 @@ struct ofono_stk *ofono_stk_create(struct ofono_modem *modem,
 void ofono_stk_register(struct ofono_stk *stk)
 {
 	__ofono_atom_register(stk->atom, stk_unregister);
+
+	stk->envelope_q = g_queue_new();
 }
 
 void ofono_stk_remove(struct ofono_stk *stk)
-- 
1.7.1.86.g0e460.dirty

Re: [PATCH 5/9] stk: Handle ENVELOPEs in a queue, retry on sim busy.

[Denis Kenzior]

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

Hi Andrew,

> +
> +	gboolean envelope_q_busy;

In my opinion we can get rid of this variable.  The SMS tx_queue does
almost the same thing without requiring such a variable.

> +	GQueue *envelope_q;
> +};
> +
> +struct envelope_op {
> +	struct stk_envelope e;
> +	int retries;
> +	void (*cb)(struct ofono_stk *stk, gboolean ok,
> +			const unsigned char *data, int length);

Is the callback really needed?  What can we intelligently do besides
printing an error to the log?

>  };
>  
> +#define ENVELOPE_RETRIES_DEFAULT 5
> +
> +static void envelope_queue_run(struct ofono_stk *stk);
> +
>  static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
>  			void (*cb)(const struct ofono_error *error,
>  					struct ofono_stk *stk))
> @@ -72,37 +86,87 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
>  	return 0;
>  }
>  
> -static int stk_send_envelope(struct ofono_stk *stk, struct stk_envelope *e,
> -				void (*cb)(const struct ofono_error *error,
> -						const unsigned char *data,
> -						int length,
> -						struct ofono_stk *stk))
> +static void envelope_cb(const struct ofono_error *error, const uint8_t *data,
> +			int length, void *user_data)
> +{
> +	struct ofono_stk *stk = user_data;
> +	struct envelope_op *op = g_queue_peek_head(stk->envelope_q);
> +	gboolean result = TRUE;
> +
> +	stk->envelope_q_busy = FALSE;
> +
> +	if (op->retries > 0 && error->type == OFONO_ERROR_TYPE_SIM &&
> +			error->error == 0x9300) {
> +		op->retries--;
> +		goto out;

You might really want to use an increasing retry timeout here.

> +	}
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
> +		result = FALSE;
> +
> +	g_queue_pop_head(stk->envelope_q);
> +
> +	if (op->cb)
> +		op->cb(stk, result, data, length);
> +	g_free(op);
> +
> +out:
> +	envelope_queue_run(stk);
> +}
> +
> +static void envelope_queue_run(struct ofono_stk *stk)
>  {
>  	const guint8 *tlv;
>  	unsigned int tlv_len;
>  
> -	e->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
> +	while (stk->envelope_q_busy == FALSE &&
> +			g_queue_get_length(stk->envelope_q) > 0) {
> +		struct envelope_op *op = g_queue_peek_head(stk->envelope_q);
> +
> +		tlv = stk_pdu_from_envelope(&op->e, &tlv_len);

Do you think it is efficient for us to re-encode the envelope every time
the queue is retried?  What about encoding once during send (and
erroring out right away if that fails) and then simply storing the PDU?
 Might make the code a bit simpler too.

> +		if (!tlv) {
> +			g_queue_pop_head(stk->envelope_q);
> +
> +			op->cb(stk, FALSE, NULL, -1);
> +			g_free(op);
> +
> +			continue;
> +		}
> +
> +		stk->envelope_q_busy = TRUE;
> +		stk->driver->envelope(stk, tlv_len, tlv, envelope_cb, stk);
> +	}
> +}
> +
> +static int stk_send_envelope(struct ofono_stk *stk, struct stk_envelope *e,
> +				void (*cb)(struct ofono_stk *stk, gboolean ok,
> +						const uint8_t *data,
> +						int length), int retries)
> +{
> +	struct envelope_op *op;
>  
>  	if (stk->driver->envelope == NULL)
>  		return -ENOSYS;
>  
> -	tlv = stk_pdu_from_envelope(e, &tlv_len);
> -	if (!tlv)
> -		return -EINVAL;
> +	op = g_new0(struct envelope_op, 1);
> +
> +	memcpy(&op->e, e, sizeof(op->e));
> +	op->e.dst = STK_DEVICE_IDENTITY_TYPE_UICC;
> +	op->cb = cb;
> +	op->retries = retries;
> +
> +	g_queue_push_tail(stk->envelope_q, op);
> +
> +	envelope_queue_run(stk);
>  
> -	stk->driver->envelope(stk, tlv_len, tlv,
> -				(ofono_stk_envelope_cb_t) cb, stk);
>  	return 0;
>  }
>  
> -static void stk_cbs_download_cb(const struct ofono_error *error,
> -				const unsigned char *data, int len,
> -				struct ofono_stk *stk)
> +static void stk_cbs_download_cb(struct ofono_stk *stk, gboolean ok,
> +				const unsigned char *data, int len)
>  {
> -	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +	if (!ok) {
>  		ofono_error("CellBroadcast download to UICC failed");
> -		/* "The ME may retry to deliver the same Cell Broadcast
> -		 * page." */
>  		return;
>  	}
>  
> @@ -115,7 +179,6 @@ static void stk_cbs_download_cb(const struct ofono_error *error,
>  
>  void __ofono_cbs_sim_download(struct ofono_stk *stk, const struct cbs *msg)
>  {
> -	struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
>  	struct stk_envelope e;
>  	int err;
>  
> @@ -125,9 +188,10 @@ void __ofono_cbs_sim_download(struct ofono_stk *stk, const struct cbs *msg)
>  	e.src = STK_DEVICE_IDENTITY_TYPE_NETWORK;
>  	memcpy(&e.cbs_pp_download.page, msg, sizeof(msg));
>  
> -	err = stk_send_envelope(stk, &e, stk_cbs_download_cb);
> +	err = stk_send_envelope(stk, &e, stk_cbs_download_cb,
> +				ENVELOPE_RETRIES_DEFAULT);
>  	if (err)
> -		stk_cbs_download_cb(&error, NULL, -1, stk);
> +		stk_cbs_download_cb(stk, FALSE, NULL, -1);
>  }
>  
>  static void stk_command_cb(const struct ofono_error *error,
> @@ -254,6 +318,10 @@ void ofono_stk_driver_unregister(const struct ofono_stk_driver *d)
>  
>  static void stk_unregister(struct ofono_atom *atom)
>  {
> +	struct ofono_stk *stk = __ofono_atom_get_data(atom);
> +
> +	g_queue_foreach(stk->envelope_q, (GFunc) g_free, NULL);
> +	g_queue_free(stk->envelope_q);
>  }
>  
>  static void stk_remove(struct ofono_atom *atom)
> @@ -309,6 +377,8 @@ struct ofono_stk *ofono_stk_create(struct ofono_modem *modem,
>  void ofono_stk_register(struct ofono_stk *stk)
>  {
>  	__ofono_atom_register(stk->atom, stk_unregister);
> +
> +	stk->envelope_q = g_queue_new();
>  }
>  
>  void ofono_stk_remove(struct ofono_stk *stk)

Regards,
-Denis

Re: [PATCH 5/9] stk: Handle ENVELOPEs in a queue, retry on sim busy.

[Andrzej Zaborowski]

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

Hi,

On 1 July 2010 21:57, Denis Kenzior <denkenz@gmail.com> wrote:
> Hi Andrew,
>
>> +
>> +     gboolean envelope_q_busy;
>
> In my opinion we can get rid of this variable.  The SMS tx_queue does
> almost the same thing without requiring such a variable.

True, I didn't think of it.

>
>> +     GQueue *envelope_q;
>> +};
>> +
>> +struct envelope_op {
>> +     struct stk_envelope e;
>> +     int retries;
>> +     void (*cb)(struct ofono_stk *stk, gboolean ok,
>> +                     const unsigned char *data, int length);
>
> Is the callback really needed?  What can we intelligently do besides
> printing an error to the log?

In the generic case we should inform whoever asked us to submit the
information to the UICC.  We don't currently have any such case (in
case of Cell Broadcast there's no one to inform.   In case of the
SimAppAgent our d-bus api doesn't let us do it.  In case of SMS-PP
download it's a technical difficulty).  But, for example the Timer
Expiration event is more complicated because it needs to be retried
until it succeeds, and every time we retry sending the envelope it
will be different because it contains current time.  So the Timer
Expiration retrying has to be imlemented separately.

>
>>  };
>>
>> +#define ENVELOPE_RETRIES_DEFAULT 5
>> +
>> +static void envelope_queue_run(struct ofono_stk *stk);
>> +
>>  static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
>>                       void (*cb)(const struct ofono_error *error,
>>                                       struct ofono_stk *stk))
>> @@ -72,37 +86,87 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
>>       return 0;
>>  }
>>
>> -static int stk_send_envelope(struct ofono_stk *stk, struct stk_envelope *e,
>> -                             void (*cb)(const struct ofono_error *error,
>> -                                             const unsigned char *data,
>> -                                             int length,
>> -                                             struct ofono_stk *stk))
>> +static void envelope_cb(const struct ofono_error *error, const uint8_t *data,
>> +                     int length, void *user_data)
>> +{
>> +     struct ofono_stk *stk = user_data;
>> +     struct envelope_op *op = g_queue_peek_head(stk->envelope_q);
>> +     gboolean result = TRUE;
>> +
>> +     stk->envelope_q_busy = FALSE;
>> +
>> +     if (op->retries > 0 && error->type == OFONO_ERROR_TYPE_SIM &&
>> +                     error->error == 0x9300) {
>> +             op->retries--;
>> +             goto out;
>
> You might really want to use an increasing retry timeout here.

For now I'm hoping that this retrying is purely theoretical, and it
never happens in practice.  The problem with increasing timeouts is
that there's a period where we're not doing anything.  And if we have
an envelope like Menu Selection or Timer Expiration later in the
queue, we don't want to delay it.

>
>> +     }
>> +
>> +     if (error->type != OFONO_ERROR_TYPE_NO_ERROR)
>> +             result = FALSE;
>> +
>> +     g_queue_pop_head(stk->envelope_q);
>> +
>> +     if (op->cb)
>> +             op->cb(stk, result, data, length);
>> +     g_free(op);
>> +
>> +out:
>> +     envelope_queue_run(stk);
>> +}
>> +
>> +static void envelope_queue_run(struct ofono_stk *stk)
>>  {
>>       const guint8 *tlv;
>>       unsigned int tlv_len;
>>
>> -     e->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
>> +     while (stk->envelope_q_busy == FALSE &&
>> +                     g_queue_get_length(stk->envelope_q) > 0) {
>> +             struct envelope_op *op = g_queue_peek_head(stk->envelope_q);
>> +
>> +             tlv = stk_pdu_from_envelope(&op->e, &tlv_len);
>
> Do you think it is efficient for us to re-encode the envelope every time
> the queue is retried?  What about encoding once during send (and
> erroring out right away if that fails) and then simply storing the PDU?
>  Might make the code a bit simpler too.

Yes, good idea.

Regards

Re: [PATCH 5/9] stk: Handle ENVELOPEs in a queue, retry on sim busy.

[Denis Kenzior]

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

Hi Andrew,

>>> +     GQueue *envelope_q;
>>> +};
>>> +
>>> +struct envelope_op {
>>> +     struct stk_envelope e;
>>> +     int retries;
>>> +     void (*cb)(struct ofono_stk *stk, gboolean ok,
>>> +                     const unsigned char *data, int length);
>>
>> Is the callback really needed?  What can we intelligently do besides
>> printing an error to the log?
> 
> In the generic case we should inform whoever asked us to submit the
> information to the UICC.  We don't currently have any such case (in
> case of Cell Broadcast there's no one to inform.   In case of the
> SimAppAgent our d-bus api doesn't let us do it.  In case of SMS-PP
> download it's a technical difficulty).  But, for example the Timer
> Expiration event is more complicated because it needs to be retried
> until it succeeds, and every time we retry sending the envelope it
> will be different because it contains current time.  So the Timer
> Expiration retrying has to be imlemented separately.
> 

So I suggest keeping it simple for now and not having the callback.  We
can always add it if really needed.

>>> +static void envelope_cb(const struct ofono_error *error, const uint8_t *data,
>>> +                     int length, void *user_data)
>>> +{
>>> +     struct ofono_stk *stk = user_data;
>>> +     struct envelope_op *op = g_queue_peek_head(stk->envelope_q);
>>> +     gboolean result = TRUE;
>>> +
>>> +     stk->envelope_q_busy = FALSE;
>>> +
>>> +     if (op->retries > 0 && error->type == OFONO_ERROR_TYPE_SIM &&
>>> +                     error->error == 0x9300) {
>>> +             op->retries--;
>>> +             goto out;
>>
>> You might really want to use an increasing retry timeout here.
> 
> For now I'm hoping that this retrying is purely theoretical, and it
> never happens in practice.  The problem with increasing timeouts is
> that there's a period where we're not doing anything.  And if we have
> an envelope like Menu Selection or Timer Expiration later in the
> queue, we don't want to delay it.
> 

Ok fair enough.

Regards,
-Denis

Re: [PATCH 5/9] stk: Handle ENVELOPEs in a queue, retry on sim busy.

[Andrzej Zaborowski]

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

Hi Denis,

On 2 July 2010 04:28, Denis Kenzior <denkenz@gmail.com> wrote:
>>>> +     GQueue *envelope_q;
>>>> +};
>>>> +
>>>> +struct envelope_op {
>>>> +     struct stk_envelope e;
>>>> +     int retries;
>>>> +     void (*cb)(struct ofono_stk *stk, gboolean ok,
>>>> +                     const unsigned char *data, int length);
>>>
>>> Is the callback really needed?  What can we intelligently do besides
>>> printing an error to the log?
>>
>> In the generic case we should inform whoever asked us to submit the
>> information to the UICC.  We don't currently have any such case (in
>> case of Cell Broadcast there's no one to inform.   In case of the
>> SimAppAgent our d-bus api doesn't let us do it.  In case of SMS-PP
>> download it's a technical difficulty).  But, for example the Timer
>> Expiration event is more complicated because it needs to be retried
>> until it succeeds, and every time we retry sending the envelope it
>> will be different because it contains current time.  So the Timer
>> Expiration retrying has to be imlemented separately.
>>
>
> So I suggest keeping it simple for now and not having the callback.  We
> can always add it if really needed.

I tried to remove it (I use the success callback in the menu patch..
but I planned to get rid of that usage anyway), but now I think it
would be conceptually wrong.  The ENVELOPEs send information both
ways, it's a request and a response.  For example the Call Control
envelopes return a response and we will definitely need to know what
the response was.  So I think it's easier to do correctly from the
beginning, instead of remove it and then in a couple of weeks come
back to it and look for what needs to be changed and what other code
may be affected.

It's just about two lines of code anyway.

Best regards

[PATCH 6/9] Add SimToolkit and SimApplicationAgent interfaces.

[Andrzej Zaborowski]

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

The interface is as outlined in
http://lists.ofono.org/pipermail/ofono/2010-June/003040.html
This patch adds a skeleton that command implementations will use.
---
 include/dbus.h |    2 +
 src/stk.c      |  468 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 465 insertions(+), 5 deletions(-)

diff --git a/include/dbus.h b/include/dbus.h
index d988760..d959754 100644
--- a/include/dbus.h
+++ b/include/dbus.h
@@ -49,6 +49,8 @@ extern "C" {
 #define OFONO_VOICECALL_MANAGER_INTERFACE "org.ofono.VoiceCallManager"
 #define OFONO_DATA_CONNECTION_MANAGER_INTERFACE "org.ofono.DataConnectionManager"
 #define OFONO_DATA_CONTEXT_INTERFACE "org.ofono.PrimaryDataContext"
+#define OFONO_STK_INTERFACE OFONO_SERVICE ".SimToolkit"
+#define OFONO_SIM_APP_INTERFACE OFONO_SERVICE ".SimApplicationAgent"
 
 /* Essentially a{sv} */
 #define OFONO_PROPERTIES_ARRAY_SIGNATURE DBUS_DICT_ENTRY_BEGIN_CHAR_AS_STRING \
diff --git a/src/stk.c b/src/stk.c
index 4fea62b..e24897f 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -34,17 +34,37 @@
 
 #include "ofono.h"
 
+#include "common.h"
 #include "smsutil.h"
 #include "stkutil.h"
 
 static GSList *g_drivers = NULL;
 
+struct stk_app_agent {
+	char *path;
+	char *bus;
+	guint watch;
+	DBusMessage *msg;
+	DBusPendingCall *call;
+};
+
+enum stk_agent_state {
+	STK_AGENT_IDLE = 0,
+};
+
 struct ofono_stk {
 	const struct ofono_stk_driver *driver;
 	void *driver_data;
 	struct ofono_atom *atom;
 	struct stk_command *pending_cmd;
-	void (*cancel_cmd)(struct ofono_stk *stk);
+	struct stk_app_agent *app_agent;
+	enum stk_agent_state app_agent_state;
+	int timeout; /* Manufacturer defined timeout */
+	int custom_timeout; /* Command defined, overrides default */
+	guint cmd_timeout;
+
+	void (*cmd_send)(struct ofono_stk *stk, DBusMessage *call);
+	void (*cmd_cb)(struct ofono_stk *stk, DBusMessage *reply);
 
 	gboolean envelope_q_busy;
 	GQueue *envelope_q;
@@ -59,6 +79,10 @@ struct envelope_op {
 
 #define ENVELOPE_RETRIES_DEFAULT 5
 
+#define OFONO_NAVIGATION_PREFIX OFONO_SERVICE ".Navigation."
+#define OFONO_NAVIGATION_GOBACK OFONO_NAVIGATION_PREFIX "Back"
+#define OFONO_NAVIGATION_TERMINATED OFONO_NAVIGATION_PREFIX "Terminated"
+
 static void envelope_queue_run(struct ofono_stk *stk);
 
 static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
@@ -201,7 +225,8 @@ static void stk_command_cb(const struct ofono_error *error,
 	stk->pending_cmd = NULL;
 
 	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
-		ofono_error("TERMINAL RESPONSE to a UICC command failed");
+		ofono_error("TERMINAL RESPONSE to a UICC command failed: %s",
+				telephony_error_to_str(error));
 		/* "The ME may retry to deliver the same Cell Broadcast
 		 * page." */
 		return;
@@ -210,14 +235,392 @@ static void stk_command_cb(const struct ofono_error *error,
 	DBG("TERMINAL RESPONSE to a command reported no errors");
 }
 
-void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
+static void app_agent_request_end(struct ofono_stk *stk)
 {
-	if (!stk->pending_cmd)
+	stk->cmd_send = NULL;
+	stk->cmd_cb = NULL;
+
+	if (stk->cmd_timeout) {
+		g_source_remove(stk->cmd_timeout);
+		stk->cmd_timeout = 0;
+	}
+}
+
+static void app_agent_request_send_cancel(struct stk_app_agent *agent)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+	DBusMessage *message;
+
+	if (!agent->call)
+		return;
+
+	dbus_message_unref(agent->msg);
+	agent->msg = NULL;
+
+	dbus_pending_call_cancel(agent->call);
+	dbus_pending_call_unref(agent->call);
+	agent->call = NULL;
+
+	message = dbus_message_new_method_call(agent->bus, agent->path,
+						OFONO_SIM_APP_INTERFACE,
+						"Cancel");
+	if (message == NULL)
+		return;
+
+	dbus_message_set_no_reply(message, TRUE);
+
+	g_dbus_send_message(conn, message);
+}
+
+static void app_agent_request_cancel(struct ofono_stk *stk)
+{
+	struct stk_app_agent *agent = stk->app_agent;
+	void (*cb)(struct ofono_stk *stk, DBusMessage *reply) = stk->cmd_cb;
+
+	app_agent_request_end(stk);
+	if (cb)
+		cb(stk, NULL);
+
+	stk->app_agent_state = STK_AGENT_IDLE;
+
+	if (!agent)
+		return;
+
+	app_agent_request_send_cancel(agent);
+}
+
+static gboolean app_agent_request_timeout(gpointer user_data)
+{
+	struct ofono_stk *stk = user_data;
+
+	stk->cmd_timeout = 0;
+	app_agent_request_cancel(stk);
+
+	return FALSE;
+}
+
+static void app_agent_request_reply_handle(DBusPendingCall *call, void *data)
+{
+	struct ofono_stk *stk = data;
+	void (*cb)(struct ofono_stk *stk, DBusMessage *reply) = stk->cmd_cb;
+	DBusError err;
+	DBusMessage *reply = dbus_pending_call_steal_reply(call);
+
+	dbus_error_init(&err);
+	if (dbus_set_error_from_message(&err, reply)) {
+		ofono_error("SimAppAgent %s replied with error %s, %s",
+				stk->app_agent ? stk->app_agent->path :
+				"(destroyed)", err.name, err.message);
+
+		if (g_str_equal(DBUS_ERROR_UNKNOWN_METHOD, err.name) ||
+				g_str_equal(DBUS_ERROR_NO_REPLY, err.name))
+			goto err_out;
+
+		if (g_str_has_prefix(err.name, OFONO_NAVIGATION_PREFIX)) {
+			app_agent_request_end(stk);
+
+			cb(stk, reply);
+			stk->app_agent_state = STK_AGENT_IDLE;
+
+			goto err_out;
+		}
+
+		app_agent_request_end(stk);
+
+		cb(stk, NULL);
+		stk->app_agent_state = STK_AGENT_IDLE;
+
+err_out:
+		dbus_error_free(&err);
+		goto out;
+	}
+
+	app_agent_request_end(stk);
+
+	cb(stk, reply);
+	stk->app_agent_state = STK_AGENT_IDLE;
+
+out:
+	dbus_message_unref(reply);
+
+	if (!stk->app_agent)
 		return;
 
-	stk->cancel_cmd(stk);
+	dbus_message_unref(stk->app_agent->msg);
+	stk->app_agent->msg = NULL;
+
+	dbus_pending_call_cancel(stk->app_agent->call);
+	dbus_pending_call_unref(stk->app_agent->call);
+	stk->app_agent->call = NULL;
 }
 
+static gboolean app_agent_request_send(struct ofono_stk *stk,
+					struct stk_app_agent *agent)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+
+	agent->msg = dbus_message_new_method_call(agent->bus, agent->path,
+							OFONO_SIM_APP_INTERFACE,
+							"Cancel");
+	if (agent->msg == NULL) {
+		ofono_error("Couldn't make a DBusMessage");
+		return FALSE;
+	}
+
+	stk->cmd_send(stk, agent->msg);
+
+	if (dbus_connection_send_with_reply(conn,
+				agent->msg, &agent->call, INT_MAX) == FALSE ||
+			agent->call == NULL) {
+		dbus_message_unref(agent->msg);
+		agent->msg = NULL;
+
+		ofono_error("Couldn't send a method call");
+		return FALSE;
+	}
+
+	dbus_pending_call_set_notify(agent->call,
+					app_agent_request_reply_handle,
+					stk, NULL);
+
+	return TRUE;
+}
+
+static void app_agent_request_start(struct ofono_stk *stk,
+					void (*send)(struct ofono_stk *stk,
+							DBusMessage *call),
+					void (*cb)(struct ofono_stk *stk,
+							DBusMessage *reply),
+					enum stk_agent_state state)
+{
+	int timeout = 0;
+
+	if (stk->app_agent_state != STK_AGENT_IDLE)
+		app_agent_request_cancel(stk);
+
+	stk->cmd_send = send;
+	stk->cmd_cb = cb;
+
+	if (stk->app_agent)
+		app_agent_request_send(stk, stk->app_agent);
+
+	stk->app_agent_state = state;
+
+	/* Use the timeout value specified in the command, if any.  */
+	if (stk->custom_timeout > 0)
+		timeout = stk->custom_timeout;
+	else if (stk->custom_timeout == 0)
+		timeout = stk->timeout * 1000;
+
+	if (!timeout)
+		return;
+
+	stk->cmd_timeout = g_timeout_add(timeout, app_agent_request_timeout,
+						stk);
+}
+
+static void app_agent_remove(struct stk_app_agent *agent)
+{
+	DBusConnection *conn = ofono_dbus_get_connection();
+
+	if (agent->watch) {
+		DBusMessage *message;
+
+		g_dbus_remove_watch(conn, agent->watch);
+		agent->watch = 0;
+
+		app_agent_request_send_cancel(agent);
+
+		message = dbus_message_new_method_call(agent->bus, agent->path,
+							OFONO_SIM_APP_INTERFACE,
+							"Release");
+		if (message) {
+			dbus_message_set_no_reply(message, TRUE);
+
+			g_dbus_send_message(conn, message);
+		}
+	} else {
+		if (agent->msg)
+			dbus_message_unref(agent->msg);
+		if (agent->call)
+			dbus_pending_call_unref(agent->call);
+	}
+
+	g_free(agent->path);
+	g_free(agent);
+}
+
+static void app_agent_disconnect_cb(DBusConnection *conn, void *user_data)
+{
+	struct ofono_stk *stk = user_data;
+
+	ofono_debug("Agent exited without calling Unregister");
+
+	stk->app_agent->watch = 0;
+
+	app_agent_remove(stk->app_agent);
+	stk->app_agent = NULL;
+}
+
+static struct stk_app_agent *app_agent_create(struct ofono_stk *stk,
+						const char *path,
+						const char *sender)
+{
+	struct stk_app_agent *agent = g_try_new0(struct stk_app_agent, 1);
+	DBusConnection *conn = ofono_dbus_get_connection();
+
+	if (!agent)
+		return NULL;
+
+	agent->path = g_strdup(path);
+	agent->bus = g_strdup(sender);
+
+	agent->watch = g_dbus_add_disconnect_watch(conn, sender,
+							app_agent_disconnect_cb,
+							stk, NULL);
+
+	if (stk->app_agent_state != STK_AGENT_IDLE)
+		app_agent_request_send(stk, agent);
+
+	return agent;
+}
+
+static DBusMessage *stk_get_properties(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_stk *stk = data;
+	DBusMessage *reply;
+	DBusMessageIter iter;
+	DBusMessageIter dict;
+	dbus_uint16_t timeout = stk->timeout;
+
+	reply = dbus_message_new_method_return(msg);
+	if (!reply)
+		return NULL;
+
+	dbus_message_iter_init_append(reply, &iter);
+
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
+					OFONO_PROPERTIES_ARRAY_SIGNATURE,
+					&dict);
+
+	ofono_dbus_dict_append(&dict, "Timeout",
+				DBUS_TYPE_UINT16, &timeout);
+
+	dbus_message_iter_close_container(&iter, &dict);
+
+	return reply;
+}
+
+static DBusMessage *stk_set_property(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_stk *stk = data;
+	const char *path = __ofono_atom_get_path(stk->atom);
+	DBusMessageIter iter;
+	DBusMessageIter var;
+	const char *property;
+	dbus_uint16_t timeout;
+
+	if (!dbus_message_iter_init(msg, &iter))
+		return __ofono_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_get_basic(&iter, &property);
+	if (!dbus_message_iter_next(&iter))
+		return __ofono_error_invalid_args(msg);
+
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
+		return __ofono_error_invalid_args(msg);
+
+	dbus_message_iter_recurse(&iter, &var);
+
+	if (!strcmp(property, "Timeout")) {
+		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT16)
+			return __ofono_error_invalid_args(msg);
+
+		dbus_message_iter_get_basic(&var, &timeout);
+		if (dbus_message_iter_next(&iter))
+			return __ofono_error_invalid_args(msg);
+
+		stk->timeout = timeout;
+
+		if (stk->cmd_timeout && stk->custom_timeout == 0) {
+			g_source_remove(stk->cmd_timeout);
+			stk->cmd_timeout = g_timeout_add_seconds(stk->timeout,
+						app_agent_request_timeout, stk);
+		}
+
+		ofono_dbus_signal_property_changed(conn, path,
+						OFONO_SIM_APP_INTERFACE,
+						"Timeout",
+						DBUS_TYPE_UINT16, &timeout);
+
+		return dbus_message_new_method_return(msg);
+	}
+
+	return __ofono_error_invalid_args(msg);
+}
+
+static DBusMessage *stk_register_agent(DBusConnection *conn,
+					DBusMessage *msg, void *data)
+{
+	struct ofono_stk *stk = data;
+	const char *agent_path;
+
+	if (stk->app_agent)
+		return __ofono_error_busy(msg);
+
+	if (dbus_message_get_args(msg, NULL,
+					DBUS_TYPE_OBJECT_PATH, &agent_path,
+					DBUS_TYPE_INVALID) == FALSE)
+		return __ofono_error_invalid_args(msg);
+
+	stk->app_agent = app_agent_create(stk, agent_path,
+						dbus_message_get_sender(msg));
+	if (!stk->app_agent)
+		return __ofono_error_failed(msg);
+
+	return dbus_message_new_method_return(msg);
+}
+
+static DBusMessage *stk_unregister_agent(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	struct ofono_stk *stk = data;
+	const char *agent_path;
+
+	if (dbus_message_get_args(msg, NULL,
+					DBUS_TYPE_OBJECT_PATH, &agent_path,
+					DBUS_TYPE_INVALID) == FALSE)
+		return __ofono_error_invalid_args(msg);
+
+	if (!stk->app_agent || strcmp(stk->app_agent->path, agent_path))
+		return __ofono_error_failed(msg);
+
+	app_agent_remove(stk->app_agent);
+	stk->app_agent = NULL;
+
+	return dbus_message_new_method_return(msg);
+}
+
+static GDBusMethodTable stk_methods[] = {
+	{ "GetProperties",		"",	"a{sv}",stk_get_properties },
+	{ "SetProperty",		"sv",	"",	stk_set_property },
+	{ "RegisterSimAppAgent",	"o",	"",	stk_register_agent },
+	{ "UnregisterSimAppAgent",	"o",	"",	stk_unregister_agent },
+
+	{ }
+};
+
+static GDBusSignalTable stk_signals[] = {
+	{ "PropertyChanged",	"sv" },
+
+	{ }
+};
+
 static gboolean handle_command_more_time(const struct stk_command *cmd,
 						struct stk_response *rsp,
 						struct ofono_stk *stk)
@@ -227,6 +630,14 @@ static gboolean handle_command_more_time(const struct stk_command *cmd,
 	return TRUE;
 }
 
+void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
+{
+	if (!stk->pending_cmd)
+		return;
+
+	stk->cmd_cb(stk, NULL);
+}
+
 void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 					int length, const unsigned char *pdu)
 {
@@ -236,6 +647,16 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 	int i, err;
 	gboolean respond = TRUE;
 
+	/*
+	 * Depending on the hardware we may have received a new
+	 * command before we managed to send a TERMINAL RESPONSE to
+	 * the previous one.  3GPP says in the current revision only
+	 * one command can be executing at any time, so assume that
+	 * the previous one is being cancelled and the card just
+	 * expects a response to the new one.
+	 */
+	ofono_stk_proactive_command_cancel(stk);
+
 	buf = g_try_malloc(length * 2 + 1);
 	if (!buf)
 		return;
@@ -254,6 +675,8 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 		goto done;
 	}
 
+	stk->custom_timeout = 0; /* Use the default timeout value */
+
 	ofono_debug("Proactive command PDU: %s", buf);
 
 	memset(&rsp, 0, sizeof(rsp));
@@ -319,9 +742,28 @@ void ofono_stk_driver_unregister(const struct ofono_stk_driver *d)
 static void stk_unregister(struct ofono_atom *atom)
 {
 	struct ofono_stk *stk = __ofono_atom_get_data(atom);
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(atom);
+	const char *path = __ofono_atom_get_path(atom);
+
+	if (stk->app_agent)
+		app_agent_remove(stk->app_agent);
+
+	if (stk->pending_cmd) {
+		stk_command_free(stk->pending_cmd);
+		stk->pending_cmd = NULL;
+
+		if (stk->cmd_timeout) {
+			g_source_remove(stk->cmd_timeout);
+			stk->cmd_timeout = 0;
+		}
+	}
 
 	g_queue_foreach(stk->envelope_q, (GFunc) g_free, NULL);
 	g_queue_free(stk->envelope_q);
+
+	ofono_modem_remove_interface(modem, OFONO_STK_INTERFACE);
+	g_dbus_unregister_interface(conn, path, OFONO_STK_INTERFACE);
 }
 
 static void stk_remove(struct ofono_atom *atom)
@@ -376,8 +818,24 @@ struct ofono_stk *ofono_stk_create(struct ofono_modem *modem,
 
 void ofono_stk_register(struct ofono_stk *stk)
 {
+	DBusConnection *conn = ofono_dbus_get_connection();
+	struct ofono_modem *modem = __ofono_atom_get_modem(stk->atom);
+	const char *path = __ofono_atom_get_path(stk->atom);
+
+	if (!g_dbus_register_interface(conn, path, OFONO_STK_INTERFACE,
+					stk_methods, stk_signals, NULL,
+					stk, NULL)) {
+		ofono_error("Could not create %s interface",
+				OFONO_STK_INTERFACE);
+
+		return;
+	}
+
+	ofono_modem_add_interface(modem, OFONO_STK_INTERFACE);
+
 	__ofono_atom_register(stk->atom, stk_unregister);
 
+	stk->timeout = 20;
 	stk->envelope_q = g_queue_new();
 }
 
-- 
1.7.1.86.g0e460.dirty

Re: [PATCH 6/9] Add SimToolkit and SimApplicationAgent interfaces.

[Denis Kenzior]

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

Hi Andrew,

On 06/29/2010 05:14 AM, Andrzej Zaborowski wrote:
> The interface is as outlined in
> http://lists.ofono.org/pipermail/ofono/2010-June/003040.html
> This patch adds a skeleton that command implementations will use.
> ---
>  include/dbus.h |    2 +
>  src/stk.c      |  468 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-

You might still want to include the dbus api document for easier
cross-reference and I'd still prefer the dbus.h change in a separate patch.

> @@ -34,17 +34,37 @@
>  
>  #include "ofono.h"
>  
> +#include "common.h"
>  #include "smsutil.h"
>  #include "stkutil.h"
>  
>  static GSList *g_drivers = NULL;
>  
> +struct stk_app_agent {
> +	char *path;
> +	char *bus;
> +	guint watch;
> +	DBusMessage *msg;
> +	DBusPendingCall *call;
> +};
> +
> +enum stk_agent_state {
> +	STK_AGENT_IDLE = 0,
> +};
> +
>  struct ofono_stk {
>  	const struct ofono_stk_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
>  	struct stk_command *pending_cmd;
> -	void (*cancel_cmd)(struct ofono_stk *stk);
> +	struct stk_app_agent *app_agent;
> +	enum stk_agent_state app_agent_state;
> +	int timeout; /* Manufacturer defined timeout */
> +	int custom_timeout; /* Command defined, overrides default */
> +	guint cmd_timeout;
> +
> +	void (*cmd_send)(struct ofono_stk *stk, DBusMessage *call);
> +	void (*cmd_cb)(struct ofono_stk *stk, DBusMessage *reply);
>  
>  	gboolean envelope_q_busy;
>  	GQueue *envelope_q;
> @@ -59,6 +79,10 @@ struct envelope_op {
>  
>  #define ENVELOPE_RETRIES_DEFAULT 5
>  
> +#define OFONO_NAVIGATION_PREFIX OFONO_SERVICE ".Navigation."
> +#define OFONO_NAVIGATION_GOBACK OFONO_NAVIGATION_PREFIX "Back"
> +#define OFONO_NAVIGATION_TERMINATED OFONO_NAVIGATION_PREFIX "Terminated"
> +
>  static void envelope_queue_run(struct ofono_stk *stk);
>  
>  static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
> @@ -201,7 +225,8 @@ static void stk_command_cb(const struct ofono_error *error,
>  	stk->pending_cmd = NULL;
>  
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> -		ofono_error("TERMINAL RESPONSE to a UICC command failed");
> +		ofono_error("TERMINAL RESPONSE to a UICC command failed: %s",
> +				telephony_error_to_str(error));

This really belongs in a separate patch.

>  		/* "The ME may retry to deliver the same Cell Broadcast
>  		 * page." */
>  		return;
> @@ -210,14 +235,392 @@ static void stk_command_cb(const struct ofono_error *error,
>  	DBG("TERMINAL RESPONSE to a command reported no errors");
>  }
>  
> -void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
> +static void app_agent_request_end(struct ofono_stk *stk)
>  {
> -	if (!stk->pending_cmd)
> +	stk->cmd_send = NULL;
> +	stk->cmd_cb = NULL;
> +
> +	if (stk->cmd_timeout) {
> +		g_source_remove(stk->cmd_timeout);
> +		stk->cmd_timeout = 0;
> +	}
> +}
> +
> +static void app_agent_request_send_cancel(struct stk_app_agent *agent)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +	DBusMessage *message;
> +
> +	if (!agent->call)
> +		return;
> +
> +	dbus_message_unref(agent->msg);
> +	agent->msg = NULL;
> +
> +	dbus_pending_call_cancel(agent->call);
> +	dbus_pending_call_unref(agent->call);
> +	agent->call = NULL;
> +
> +	message = dbus_message_new_method_call(agent->bus, agent->path,
> +						OFONO_SIM_APP_INTERFACE,
> +						"Cancel");
> +	if (message == NULL)
> +		return;
> +
> +	dbus_message_set_no_reply(message, TRUE);
> +
> +	g_dbus_send_message(conn, message);
> +}
> +
> +static void app_agent_request_cancel(struct ofono_stk *stk)
> +{
> +	struct stk_app_agent *agent = stk->app_agent;
> +	void (*cb)(struct ofono_stk *stk, DBusMessage *reply) = stk->cmd_cb;
> +
> +	app_agent_request_end(stk);
> +	if (cb)
> +		cb(stk, NULL);
> +
> +	stk->app_agent_state = STK_AGENT_IDLE;
> +
> +	if (!agent)
> +		return;
> +
> +	app_agent_request_send_cancel(agent);
> +}
> +
> +static gboolean app_agent_request_timeout(gpointer user_data)
> +{
> +	struct ofono_stk *stk = user_data;
> +
> +	stk->cmd_timeout = 0;
> +	app_agent_request_cancel(stk);
> +
> +	return FALSE;
> +}
> +
> +static void app_agent_request_reply_handle(DBusPendingCall *call, void *data)
> +{
> +	struct ofono_stk *stk = data;
> +	void (*cb)(struct ofono_stk *stk, DBusMessage *reply) = stk->cmd_cb;
> +	DBusError err;
> +	DBusMessage *reply = dbus_pending_call_steal_reply(call);
> +
> +	dbus_error_init(&err);
> +	if (dbus_set_error_from_message(&err, reply)) {
> +		ofono_error("SimAppAgent %s replied with error %s, %s",
> +				stk->app_agent ? stk->app_agent->path :
> +				"(destroyed)", err.name, err.message);
> +
> +		if (g_str_equal(DBUS_ERROR_UNKNOWN_METHOD, err.name) ||
> +				g_str_equal(DBUS_ERROR_NO_REPLY, err.name))
> +			goto err_out;
> +
> +		if (g_str_has_prefix(err.name, OFONO_NAVIGATION_PREFIX)) {
> +			app_agent_request_end(stk);
> +
> +			cb(stk, reply);
> +			stk->app_agent_state = STK_AGENT_IDLE;
> +
> +			goto err_out;
> +		}
> +
> +		app_agent_request_end(stk);
> +
> +		cb(stk, NULL);
> +		stk->app_agent_state = STK_AGENT_IDLE;
> +
> +err_out:
> +		dbus_error_free(&err);
> +		goto out;
> +	}
> +
> +	app_agent_request_end(stk);
> +
> +	cb(stk, reply);
> +	stk->app_agent_state = STK_AGENT_IDLE;
> +
> +out:
> +	dbus_message_unref(reply);
> +
> +	if (!stk->app_agent)
>  		return;
>  
> -	stk->cancel_cmd(stk);
> +	dbus_message_unref(stk->app_agent->msg);
> +	stk->app_agent->msg = NULL;
> +
> +	dbus_pending_call_cancel(stk->app_agent->call);
> +	dbus_pending_call_unref(stk->app_agent->call);
> +	stk->app_agent->call = NULL;
>  }
>  
> +static gboolean app_agent_request_send(struct ofono_stk *stk,
> +					struct stk_app_agent *agent)
> +{
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +
> +	agent->msg = dbus_message_new_method_call(agent->bus, agent->path,
> +							OFONO_SIM_APP_INTERFACE,
> +							"Cancel");
> +	if (agent->msg == NULL) {
> +		ofono_error("Couldn't make a DBusMessage");
> +		return FALSE;
> +	}
> +
> +	stk->cmd_send(stk, agent->msg);
> +
> +	if (dbus_connection_send_with_reply(conn,
> +				agent->msg, &agent->call, INT_MAX) == FALSE ||
> +			agent->call == NULL) {
> +		dbus_message_unref(agent->msg);
> +		agent->msg = NULL;
> +
> +		ofono_error("Couldn't send a method call");
> +		return FALSE;
> +	}
> +
> +	dbus_pending_call_set_notify(agent->call,
> +					app_agent_request_reply_handle,
> +					stk, NULL);
> +
> +	return TRUE;
> +}
> +
> +static void app_agent_request_start(struct ofono_stk *stk,
> +					void (*send)(struct ofono_stk *stk,
> +							DBusMessage *call),
> +					void (*cb)(struct ofono_stk *stk,
> +							DBusMessage *reply),
> +					enum stk_agent_state state)
> +{
> +	int timeout = 0;
> +
> +	if (stk->app_agent_state != STK_AGENT_IDLE)
> +		app_agent_request_cancel(stk);
> +
> +	stk->cmd_send = send;
> +	stk->cmd_cb = cb;
> +
> +	if (stk->app_agent)
> +		app_agent_request_send(stk, stk->app_agent);
> +
> +	stk->app_agent_state = state;
> +
> +	/* Use the timeout value specified in the command, if any.  */
> +	if (stk->custom_timeout > 0)
> +		timeout = stk->custom_timeout;
> +	else if (stk->custom_timeout == 0)
> +		timeout = stk->timeout * 1000;
> +
> +	if (!timeout)
> +		return;
> +
> +	stk->cmd_timeout = g_timeout_add(timeout, app_agent_request_timeout,
> +						stk);
> +}
> +

So to summarize here:
	1. If there's no agent, we start the command anyway
	2. When the agent arrives, we send it the command to handle
	3. If the agent goes away, we don't cancel the command
	4. If the agent is unregistered, we send a cancel but don't cancel the
command

So it seems to me with the exception of Setup Menu and Idle Mode Text we
should simply reject the Proactive Command outright if there is no Agent
registered.

This will prevent weird situations where a proactive command is pending
for Timeout - 1 seconds, an agent is registered, we send the agent the
command and 1 second later the User gets booted out.  It also seems to
make the implementation easier...

I'm also in favor of moving
> +	enum stk_agent_state app_agent_state;
> +	int timeout; /* Manufacturer defined timeout */
> +	int custom_timeout; /* Command defined, overrides default */
> +	guint cmd_timeout;
> +
> +	void (*cmd_send)(struct ofono_stk *stk, DBusMessage *call);
> +	void (*cmd_cb)(struct ofono_stk *stk, DBusMessage *reply);

to the stk_app_agent structure instead of the main structure.  If the
agent isn't registered and we reject everything except idle mode text /
setup menu, then these attributes are not really necessary until the
agent exists.

<snip>

> +
> +static struct stk_app_agent *app_agent_create(struct ofono_stk *stk,
> +						const char *path,
> +						const char *sender)
> +{
> +	struct stk_app_agent *agent = g_try_new0(struct stk_app_agent, 1);
> +	DBusConnection *conn = ofono_dbus_get_connection();
> +
> +	if (!agent)
> +		return NULL;
> +
> +	agent->path = g_strdup(path);
> +	agent->bus = g_strdup(sender);
> +
> +	agent->watch = g_dbus_add_disconnect_watch(conn, sender,
> +							app_agent_disconnect_cb,
> +							stk, NULL);
> +
> +	if (stk->app_agent_state != STK_AGENT_IDLE)
> +		app_agent_request_send(stk, agent);
> +

This looks a bit dangerous, you're sending a request to the agent before
it is registered.  Please be nice and perform the RegisterAgent method
return first, and then send the request.

> +	return agent;
> +}

<snip>

> +
> +static DBusMessage *stk_set_property(DBusConnection *conn,
> +					DBusMessage *msg, void *data)
> +{
> +	struct ofono_stk *stk = data;
> +	const char *path = __ofono_atom_get_path(stk->atom);
> +	DBusMessageIter iter;
> +	DBusMessageIter var;
> +	const char *property;
> +	dbus_uint16_t timeout;
> +
> +	if (!dbus_message_iter_init(msg, &iter))
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_get_basic(&iter, &property);
> +	if (!dbus_message_iter_next(&iter))
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> +		return __ofono_error_invalid_args(msg);
> +
> +	dbus_message_iter_recurse(&iter, &var);
> +
> +	if (!strcmp(property, "Timeout")) {
> +		if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT16)
> +			return __ofono_error_invalid_args(msg);
> +
> +		dbus_message_iter_get_basic(&var, &timeout);
> +		if (dbus_message_iter_next(&iter))
> +			return __ofono_error_invalid_args(msg);
> +
> +		stk->timeout = timeout;
> +
> +		if (stk->cmd_timeout && stk->custom_timeout == 0) {
> +			g_source_remove(stk->cmd_timeout);
> +			stk->cmd_timeout = g_timeout_add_seconds(stk->timeout,
> +						app_agent_request_timeout, stk);
> +		}

This doesn't seem right to me, what if we have just 1 second left on our
timeout and the Timeout property is set?  We end up resetting the
timeout completely.  What do you think of simply using the new value for
all subsequent requests?

I actually question the need for this attribute, who's going to actually
set it?  Let us default to some reasonable value and add a SetProperty
for this later if required...

> +
> +		ofono_dbus_signal_property_changed(conn, path,
> +						OFONO_SIM_APP_INTERFACE,
> +						"Timeout",
> +						DBUS_TYPE_UINT16, &timeout);
> +
> +		return dbus_message_new_method_return(msg);
> +	}
> +
> +	return __ofono_error_invalid_args(msg);
> +}
> +

<snip>

> +static DBusMessage *stk_unregister_agent(DBusConnection *conn,
> +						DBusMessage *msg, void *data)
> +{
> +	struct ofono_stk *stk = data;
> +	const char *agent_path;
> +
> +	if (dbus_message_get_args(msg, NULL,
> +					DBUS_TYPE_OBJECT_PATH, &agent_path,
> +					DBUS_TYPE_INVALID) == FALSE)
> +		return __ofono_error_invalid_args(msg);
> +
> +	if (!stk->app_agent || strcmp(stk->app_agent->path, agent_path))
> +		return __ofono_error_failed(msg);
> +

One other thing to keep in mind here is that only the sender who
registered the agent should be able to unregister it.  This prevents
some random app from messing with the system.  However, this can be
added later if leaving this check out helps in testing in the short term.

> +	app_agent_remove(stk->app_agent);
> +	stk->app_agent = NULL;
> +
> +	return dbus_message_new_method_return(msg);
> +}
> +

<snip>

>  static gboolean handle_command_more_time(const struct stk_command *cmd,
>  						struct stk_response *rsp,
>  						struct ofono_stk *stk)
> @@ -227,6 +630,14 @@ static gboolean handle_command_more_time(const struct stk_command *cmd,
>  	return TRUE;
>  }
>  
> +void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
> +{
> +	if (!stk->pending_cmd)
> +		return;
> +
> +	stk->cmd_cb(stk, NULL);
> +}
> +
>  void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
>  					int length, const unsigned char *pdu)
>  {
> @@ -236,6 +647,16 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
>  	int i, err;
>  	gboolean respond = TRUE;
>  
> +	/*
> +	 * Depending on the hardware we may have received a new
> +	 * command before we managed to send a TERMINAL RESPONSE to
> +	 * the previous one.  3GPP says in the current revision only
> +	 * one command can be executing at any time, so assume that
> +	 * the previous one is being cancelled and the card just
> +	 * expects a response to the new one.
> +	 */
> +	ofono_stk_proactive_command_cancel(stk);
> +

This really belongs in a separate patch

>  	buf = g_try_malloc(length * 2 + 1);
>  	if (!buf)
>  		return;
> @@ -254,6 +675,8 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
>  		goto done;
>  	}
>  
> +	stk->custom_timeout = 0; /* Use the default timeout value */
> +

The custom timeout handling is not used by this patch, I'd really prefer
it to be bundled with the first occurence where it is used or in a
separate patch right before where it is used with a commit description
stating the intent.

>  	ofono_debug("Proactive command PDU: %s", buf);
>  
>  	memset(&rsp, 0, sizeof(rsp));

<snip>

Regards,
-Denis

Re: [PATCH 6/9] Add SimToolkit and SimApplicationAgent interfaces.

[Andrzej Zaborowski]

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

Hi,

On 2 July 2010 19:21, Denis Kenzior <denkenz@gmail.com> wrote:
> So to summarize here:
>        1. If there's no agent, we start the command anyway
>        2. When the agent arrives, we send it the command to handle
>        3. If the agent goes away, we don't cancel the command
>        4. If the agent is unregistered, we send a cancel but don't cancel the
> command

That's about right.

>
> So it seems to me with the exception of Setup Menu and Idle Mode Text we
> should simply reject the Proactive Command outright if there is no Agent
> registered.
>
> This will prevent weird situations where a proactive command is pending
> for Timeout - 1 seconds, an agent is registered, we send the agent the
> command and 1 second later the User gets booted out.  It also seems to
> make the implementation easier...

As I mentioned in the other mail, I believe it's in line with the dbus
design that clients can connect and disconnect any time, and it
shouldn't affect the state.  If we choose to do it differently, I
think it ties the interface very much to dbus implementation and the
single type of users like a Home Screen app on a phone.  This use case
is not affected if we allow agents to connect and disconnect, so
absolutely there's no harm in doing it intelligently.

I agree with moving members related to the agent to a separate struct,
but I think the agent itself should be separate, so it can disconnect
and connect when it feels like it.

>> +
>> +static struct stk_app_agent *app_agent_create(struct ofono_stk *stk,
>> +                                             const char *path,
>> +                                             const char *sender)
>> +{
>> +     struct stk_app_agent *agent = g_try_new0(struct stk_app_agent, 1);
>> +     DBusConnection *conn = ofono_dbus_get_connection();
>> +
>> +     if (!agent)
>> +             return NULL;
>> +
>> +     agent->path = g_strdup(path);
>> +     agent->bus = g_strdup(sender);
>> +
>> +     agent->watch = g_dbus_add_disconnect_watch(conn, sender,
>> +                                                     app_agent_disconnect_cb,
>> +                                                     stk, NULL);
>> +
>> +     if (stk->app_agent_state != STK_AGENT_IDLE)
>> +             app_agent_request_send(stk, agent);
>> +
>
> This looks a bit dangerous, you're sending a request to the agent before
> it is registered.  Please be nice and perform the RegisterAgent method
> return first, and then send the request.

Good point, I'll move it.  (In my tests it always worked though)

>
>> +     return agent;
>> +}
>
> <snip>
>
>> +
>> +static DBusMessage *stk_set_property(DBusConnection *conn,
>> +                                     DBusMessage *msg, void *data)
>> +{
>> +     struct ofono_stk *stk = data;
>> +     const char *path = __ofono_atom_get_path(stk->atom);
>> +     DBusMessageIter iter;
>> +     DBusMessageIter var;
>> +     const char *property;
>> +     dbus_uint16_t timeout;
>> +
>> +     if (!dbus_message_iter_init(msg, &iter))
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     dbus_message_iter_get_basic(&iter, &property);
>> +     if (!dbus_message_iter_next(&iter))
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     dbus_message_iter_recurse(&iter, &var);
>> +
>> +     if (!strcmp(property, "Timeout")) {
>> +             if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT16)
>> +                     return __ofono_error_invalid_args(msg);
>> +
>> +             dbus_message_iter_get_basic(&var, &timeout);
>> +             if (dbus_message_iter_next(&iter))
>> +                     return __ofono_error_invalid_args(msg);
>> +
>> +             stk->timeout = timeout;
>> +
>> +             if (stk->cmd_timeout && stk->custom_timeout == 0) {
>> +                     g_source_remove(stk->cmd_timeout);
>> +                     stk->cmd_timeout = g_timeout_add_seconds(stk->timeout,
>> +                                             app_agent_request_timeout, stk);
>> +             }
>
> This doesn't seem right to me, what if we have just 1 second left on our
> timeout and the Timeout property is set?  We end up resetting the
> timeout completely.  What do you think of simply using the new value for
> all subsequent requests?
>
> I actually question the need for this attribute, who's going to actually
> set it?  Let us default to some reasonable value and add a SetProperty
> for this later if required...

I was thinking in particular of things like writing an e-mail.

When you navigate the menus, 20 seconds seem like a good value for the
timeout, or when you're shown a message like "SMS deliver".  This is
about what my old Nokia dumb-phone menu timeout was.  But the SIM I
tested has a "write an e-mail" application, so it sends a Get Input
proactive command so you can type the contents of the e-mail, and 20
seconds seems very short.  So in my python test client I set the
timeout to 2 minutes for Get Input (should actually reset the timeout
everytime you stop typing).

>> +static DBusMessage *stk_unregister_agent(DBusConnection *conn,
>> +                                             DBusMessage *msg, void *data)
>> +{
>> +     struct ofono_stk *stk = data;
>> +     const char *agent_path;
>> +
>> +     if (dbus_message_get_args(msg, NULL,
>> +                                     DBUS_TYPE_OBJECT_PATH, &agent_path,
>> +                                     DBUS_TYPE_INVALID) == FALSE)
>> +             return __ofono_error_invalid_args(msg);
>> +
>> +     if (!stk->app_agent || strcmp(stk->app_agent->path, agent_path))
>> +             return __ofono_error_failed(msg);
>> +
>
> One other thing to keep in mind here is that only the sender who
> registered the agent should be able to unregister it.  This prevents
> some random app from messing with the system.  However, this can be
> added later if leaving this check out helps in testing in the short term.

I actually forgot the check, we just need to compare the
stk->app_agent->bus value.

Best regards,
Andrew

Re: [PATCH 6/9] Add SimToolkit and SimApplicationAgent interfaces.

[Denis Kenzior]

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

Hi Andrew,

>> So it seems to me with the exception of Setup Menu and Idle Mode Text we
>> should simply reject the Proactive Command outright if there is no Agent
>> registered.
>>
>> This will prevent weird situations where a proactive command is pending
>> for Timeout - 1 seconds, an agent is registered, we send the agent the
>> command and 1 second later the User gets booted out.  It also seems to
>> make the implementation easier...
> 
> As I mentioned in the other mail, I believe it's in line with the dbus
> design that clients can connect and disconnect any time, and it
> shouldn't affect the state.  If we choose to do it differently, I

I have to disagree here.  While what you say is true in the 'classical'
D-Bus usage, it is not true of Agents.  An Agent going away should
affect state, and I really don't want to deal with the 'abrupt user
interruption' caused by doing it this way.

> think it ties the interface very much to dbus implementation and the
> single type of users like a Home Screen app on a phone.  This use case
> is not affected if we allow agents to connect and disconnect, so
> absolutely there's no harm in doing it intelligently.

Actually for the most part that is exactly what we want, a nice and
optimized way for the UI to interact with oFono.  As I pointed out
earlier, we're not making a fully generic stack.  In the case of STK
only a single always-resident Agent is really required.  So I'm against
complicating either Agents or the core unnecessarily.  However, I
welcome anyone else's input here.

> 
> I agree with moving members related to the agent to a separate struct,
> but I think the agent itself should be separate, so it can disconnect
> and connect when it feels like it.

<snip>

>>> +     if (!strcmp(property, "Timeout")) {
>>> +             if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT16)
>>> +                     return __ofono_error_invalid_args(msg);
>>> +
>>> +             dbus_message_iter_get_basic(&var, &timeout);
>>> +             if (dbus_message_iter_next(&iter))
>>> +                     return __ofono_error_invalid_args(msg);
>>> +
>>> +             stk->timeout = timeout;
>>> +
>>> +             if (stk->cmd_timeout && stk->custom_timeout == 0) {
>>> +                     g_source_remove(stk->cmd_timeout);
>>> +                     stk->cmd_timeout = g_timeout_add_seconds(stk->timeout,
>>> +                                             app_agent_request_timeout, stk);
>>> +             }
>>
>> This doesn't seem right to me, what if we have just 1 second left on our
>> timeout and the Timeout property is set?  We end up resetting the
>> timeout completely.  What do you think of simply using the new value for
>> all subsequent requests?
>>
>> I actually question the need for this attribute, who's going to actually
>> set it?  Let us default to some reasonable value and add a SetProperty
>> for this later if required...
> 
> I was thinking in particular of things like writing an e-mail.
> 
> When you navigate the menus, 20 seconds seem like a good value for the
> timeout, or when you're shown a message like "SMS deliver".  This is
> about what my old Nokia dumb-phone menu timeout was.  But the SIM I
> tested has a "write an e-mail" application, so it sends a Get Input
> proactive command so you can type the contents of the e-mail, and 20
> seconds seems very short.  So in my python test client I set the

Get Input can override the duration, is it not happening in this case?

> timeout to 2 minutes for Get Input (should actually reset the timeout
> everytime you stop typing).

Fair enough, but my view is that using this attribute is still the wrong
way to solve this.  I'd simply default to an infinite timeout for Get
Input, Get Inkey unless the SIM overrides the duration.  There's no need
to involve the UI here.

Regards,
-Denis

Re: [PATCH 6/9] Add SimToolkit and SimApplicationAgent interfaces.

[Andrzej Zaborowski]

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

On 2 July 2010 22:20, Denis Kenzior <denkenz@gmail.com> wrote:
>>> So it seems to me with the exception of Setup Menu and Idle Mode Text we
>>> should simply reject the Proactive Command outright if there is no Agent
>>> registered.
>>>
>>> This will prevent weird situations where a proactive command is pending
>>> for Timeout - 1 seconds, an agent is registered, we send the agent the
>>> command and 1 second later the User gets booted out.  It also seems to
>>> make the implementation easier...
>>
>> As I mentioned in the other mail, I believe it's in line with the dbus
>> design that clients can connect and disconnect any time, and it
>> shouldn't affect the state.  If we choose to do it differently, I
>
> I have to disagree here.  While what you say is true in the 'classical'
> D-Bus usage, it is not true of Agents.  An Agent going away should
> affect state, and I really don't want to deal with the 'abrupt user
> interruption' caused by doing it this way.
>
>> think it ties the interface very much to dbus implementation and the
>> single type of users like a Home Screen app on a phone.  This use case
>> is not affected if we allow agents to connect and disconnect, so
>> absolutely there's no harm in doing it intelligently.
>
> Actually for the most part that is exactly what we want, a nice and
> optimized way for the UI to interact with oFono.  As I pointed out
> earlier, we're not making a fully generic stack.  In the case of STK
> only a single always-resident Agent is really required.  So I'm against
> complicating either Agents or the core unnecessarily.  However, I
> welcome anyone else's input here.

I don't mind doing it the way you propose, but I'd really appreciate
if you can comment on api proposal before I implement it instead of
after :)  Basically you read the code, say "oh, this is how it works
(repeating almost word for word the proposal), let's do it
differently".  (Both this request lifetime thing and the Timeout
property)

My personal opinion though (just saying it so I can quote myself) is
that the agent is not a good reason to evade some d-bus principles,
they're there to make life easier for API users.  In this case it also
makes things easier on ourselves.  I also don't think that's a good
excuse to dumb the api down for a particular use case, I myself have
described ofono to people as the linux telephony stack. So I think
this kind of things can come back and bite.

>
>>
>> I agree with moving members related to the agent to a separate struct,
>> but I think the agent itself should be separate, so it can disconnect
>> and connect when it feels like it.
>
> <snip>
>
>>>> +     if (!strcmp(property, "Timeout")) {
>>>> +             if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT16)
>>>> +                     return __ofono_error_invalid_args(msg);
>>>> +
>>>> +             dbus_message_iter_get_basic(&var, &timeout);
>>>> +             if (dbus_message_iter_next(&iter))
>>>> +                     return __ofono_error_invalid_args(msg);
>>>> +
>>>> +             stk->timeout = timeout;
>>>> +
>>>> +             if (stk->cmd_timeout && stk->custom_timeout == 0) {
>>>> +                     g_source_remove(stk->cmd_timeout);
>>>> +                     stk->cmd_timeout = g_timeout_add_seconds(stk->timeout,
>>>> +                                             app_agent_request_timeout, stk);
>>>> +             }
>>>
>>> This doesn't seem right to me, what if we have just 1 second left on our
>>> timeout and the Timeout property is set?  We end up resetting the
>>> timeout completely.  What do you think of simply using the new value for
>>> all subsequent requests?
>>>
>>> I actually question the need for this attribute, who's going to actually
>>> set it?  Let us default to some reasonable value and add a SetProperty
>>> for this later if required...
>>
>> I was thinking in particular of things like writing an e-mail.
>>
>> When you navigate the menus, 20 seconds seem like a good value for the
>> timeout, or when you're shown a message like "SMS deliver".  This is
>> about what my old Nokia dumb-phone menu timeout was.  But the SIM I
>> tested has a "write an e-mail" application, so it sends a Get Input
>> proactive command so you can type the contents of the e-mail, and 20
>> seconds seems very short.  So in my python test client I set the
>
> Get Input can override the duration, is it not happening in this case?

Get InKey can override the timeout, but Get Input can't.  The reason
for the timeout is that while a command is executing other
communications with the card may be blocked, so we may want the ui
programmer to note how much time it is using.

I think it would make sense to get rid of the timeout completely, if
the Agent was registered exactly for the duration of the "UICC
proactive session".  I haven't found a good definition in TS102.223 of
the proactive UICC session, but "6.9 Proactive UICC session and
terminal display interaction" makes it sound like a session is
basically started by choosing an element from the main menu, and
terminates when we go back to the main menu (no other command is
pending).

So when the user launches a SIM Applications app on their device, they
would see the main menu.  Menu would be available as a property of the
D-bus interface, and the user would register an agent and provide a
selection from the menu at the same time.  Then when the session ends,
the agent would be released at the same time.  The question is if a
session can be started by the UICC, there's no indication that it can
or cannot, but I haven't seen the sim use any command like DisplayText
on a different occasion than when navigating the menu.  For any other
command, like SendSMS, call control etc, an Alpha Id is supplied
instead of using DisplayText.

Best regards,
Andrew

Re: [PATCH 6/9] Add SimToolkit and SimApplicationAgent interfaces.

[Andrzej Zaborowski]

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

On 3 July 2010 01:40, Andrzej Zaborowski <andrew.zaborowski@intel.com> wrote:
> So when the user launches a SIM Applications app on their device, they
> would see the main menu.  Menu would be available as a property of the
> D-bus interface, and the user would register an agent and provide a
> selection from the menu at the same time.  Then when the session ends,
> the agent would be released at the same time.  The question is if a
> session can be started by the UICC, there's no indication that it can
> or cannot, but I haven't seen the sim use any command like DisplayText
> on a different occasion than when navigating the menu.  For any other
> command, like SendSMS, call control etc, an Alpha Id is supplied
> instead of using DisplayText.

A data point is which commands allow the "User terminted session" and
"Go back in proactive session" results.  "Go back" is accepted only
for the four commands (DisplayText, GetInkey, GetInput, SelectItem),
and "User terminated session" is allowed for these four plus
DisplayMMS, SetUpCall, PlayTone.  Most other commands provide an
optional Alpha Id, which I believe can be displayed when the screen is
"released", i.e. without the SIM Apps app running.

Best regards,
Andrew

Re: [PATCH 6/9] Add SimToolkit and SimApplicationAgent interfaces.

[Denis Kenzior]

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

Hi Andrew,

> So when the user launches a SIM Applications app on their device, they
> would see the main menu.  Menu would be available as a property of the
> D-bus interface, and the user would register an agent and provide a
> selection from the menu at the same time.  Then when the session ends,
> the agent would be released at the same time.  The question is if a

I've had the same exact thought here independently.  Funny that you're
thinking along the same lines.

> session can be started by the UICC, there's no indication that it can
> or cannot, but I haven't seen the sim use any command like DisplayText
> on a different occasion than when navigating the menu.  For any other
> command, like SendSMS, call control etc, an Alpha Id is supplied
> instead of using DisplayText.

Unfortunately DisplayText / Get Input and Select Item can be started
outside a main menu selection, at least according to GSMA DG.11 Device
Field and Lab Test Guidelines Section 8.9.15.

I need to dig into that spec some more, but looks like any SMS-Data
download can trigger a USAT command, no matter what is currently
happening (unless the SIM is busy of course.)

Regards,
-Denis

[PATCH 7/9] stk: Add menu related utilities.

[Andrzej Zaborowski]

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

---
 src/stk.c |   92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 92 insertions(+), 0 deletions(-)

diff --git a/src/stk.c b/src/stk.c
index e24897f..9136732 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -40,6 +40,17 @@
 
 static GSList *g_drivers = NULL;
 
+struct stk_menu {
+	char *title;
+	GSList *items;
+	struct stk_items_next_action_indicator next_action;
+	int default_item;
+	struct stk_text_attribute title_attr;
+	struct stk_item_text_attribute_list item_attrs;
+	gboolean soft_key;
+	gboolean has_help;
+};
+
 struct stk_app_agent {
 	char *path;
 	char *bus;
@@ -235,6 +246,87 @@ static void stk_command_cb(const struct ofono_error *error,
 	DBG("TERMINAL RESPONSE to a command reported no errors");
 }
 
+static struct stk_menu *stk_menu_create(const char *title,
+		const struct stk_text_attribute *title_attr, GSList *items,
+		const struct stk_item_text_attribute_list *item_attrs,
+		const struct stk_items_next_action_indicator *next_action,
+		int default_id, gboolean soft_key, gboolean has_help)
+{
+	struct stk_menu *ret = g_new(struct stk_menu, 1);
+	GSList *l;
+	int i;
+
+	ret->title = g_strdup(title ?: "");
+	ret->items = g_slist_copy((GSList *) items);
+	ret->default_item = -1;
+	memcpy(&ret->next_action, next_action, sizeof(ret->next_action));
+	memcpy(&ret->title_attr, title_attr, sizeof(ret->title_attr));
+	memcpy(&ret->item_attrs, item_attrs, sizeof(ret->item_attrs));
+	ret->soft_key = soft_key;
+	ret->has_help = has_help;
+
+	for (l = ret->items, i = 0; l; l = l->next, i++) {
+		struct stk_item *j = g_memdup(l->data, sizeof(*j));
+
+		j->text = g_strdup(j->text);
+		l->data = j;
+
+		if (j->id == default_id)
+			ret->default_item = i;
+	}
+
+	return ret;
+}
+
+static void stk_menu_free(struct stk_menu *menu)
+{
+	GSList *l;
+
+	for (l = menu->items; l; l = l->next) {
+		struct stk_item *i = l->data;
+
+		g_free(i->text);
+		g_free(i);
+	}
+
+	g_slist_free(menu->items);
+	g_free(menu->title);
+	g_free(menu);
+}
+
+static char **stk_menu_items_strlist(struct stk_menu *menu)
+{
+	char **items = g_new0(char *, g_slist_length(menu->items) + 1);
+	int i;
+	GSList *l;
+
+	for (i = 0, l = menu->items; l; l = l->next, i++) {
+		struct stk_item *item = l->data;
+		items[i] = item->text;
+	}
+
+	return items;
+}
+
+static void append_menu(DBusMessage *msg, struct stk_menu *menu,
+			dbus_bool_t main_menu)
+{
+	char **items;
+	dbus_bool_t has_help = menu->has_help;
+	dbus_int16_t default_item = menu->default_item;
+
+	items = stk_menu_items_strlist(menu);
+	dbus_message_append_args(msg,
+					DBUS_TYPE_ARRAY, DBUS_TYPE_STRING,
+					&items, g_slist_length(menu->items),
+					DBUS_TYPE_STRING, &menu->title,
+					DBUS_TYPE_BOOLEAN, &has_help,
+					DBUS_TYPE_INT16, &default_item,
+					DBUS_TYPE_BOOLEAN, &main_menu,
+					DBUS_TYPE_INVALID);
+	g_free(items);
+}
+
 static void app_agent_request_end(struct ofono_stk *stk)
 {
 	stk->cmd_send = NULL;
-- 
1.7.1.86.g0e460.dirty

[PATCH 8/9] stk: Handle Select Item and Set Up Menu commands.

[Andrzej Zaborowski]

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

---
 src/stk.c |  261 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 261 insertions(+), 0 deletions(-)

diff --git a/src/stk.c b/src/stk.c
index 9136732..fd6dcb0 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -61,6 +61,8 @@ struct stk_app_agent {
 
 enum stk_agent_state {
 	STK_AGENT_IDLE = 0,
+	STK_AGENT_MAIN_MENU,
+	STK_AGENT_SELECT_ITEM,
 };
 
 struct ofono_stk {
@@ -72,11 +74,15 @@ struct ofono_stk {
 	enum stk_agent_state app_agent_state;
 	int timeout; /* Manufacturer defined timeout */
 	int custom_timeout; /* Command defined, overrides default */
+	guint display_source;
 	guint cmd_timeout;
 
 	void (*cmd_send)(struct ofono_stk *stk, DBusMessage *call);
 	void (*cmd_cb)(struct ofono_stk *stk, DBusMessage *reply);
 
+	struct stk_menu *main_menu;
+	struct stk_menu *select_item_menu;
+
 	gboolean envelope_q_busy;
 	GQueue *envelope_q;
 };
@@ -95,6 +101,7 @@ struct envelope_op {
 #define OFONO_NAVIGATION_TERMINATED OFONO_NAVIGATION_PREFIX "Terminated"
 
 static void envelope_queue_run(struct ofono_stk *stk);
+static void display_idle(struct ofono_stk *stk);
 
 static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
 			void (*cb)(const struct ofono_error *error,
@@ -722,6 +729,239 @@ static gboolean handle_command_more_time(const struct stk_command *cmd,
 	return TRUE;
 }
 
+static void display_command_cb(const struct ofono_error *error,
+				struct ofono_stk *stk)
+{
+	stk_command_cb(error, stk);
+
+	display_idle(stk);
+}
+
+static void display_envelope_cb(struct ofono_stk *stk, gboolean ok,
+				const unsigned char *data, int len)
+{
+	if (!ok) {
+		ofono_error("Sending envelope to UICC failed");
+		return;
+	}
+
+	if (len)
+		ofono_error("Envelope returned %i bytes of unwanted data",
+				len);
+
+	DBG("Envelope submission gave no error");
+
+	display_idle(stk);
+}
+
+static void request_menu_send(struct ofono_stk *stk, DBusMessage *call)
+{
+	struct stk_menu *menu = stk->main_menu;
+	gboolean select_item = stk->pending_cmd != NULL &&
+		stk->pending_cmd->type == STK_COMMAND_TYPE_SELECT_ITEM;
+
+	if (select_item)
+		menu = stk->select_item_menu;
+
+	dbus_message_set_member(call, "SelectItem");
+
+	append_menu(call, menu, !select_item);
+}
+
+static void request_menu_cb(struct ofono_stk *stk, DBusMessage *reply)
+{
+	DBusError err;
+	enum stk_result_type type = STK_RESULT_TYPE_SUCCESS;
+	gboolean select_item = stk->app_agent_state == STK_AGENT_SELECT_ITEM;
+	dbus_int16_t selection;
+	dbus_bool_t help = FALSE;
+	struct stk_menu *menu = select_item ?
+		stk->select_item_menu : stk->main_menu;
+	struct stk_item *item = NULL;
+
+	if (!reply) {
+		if (!select_item) {
+			/* Main menu request is cancelled, nothing to do */
+			display_idle(stk);
+			goto out;
+		}
+
+		if (stk->cmd_timeout) {
+			app_agent_request_cancel(stk);
+
+			display_idle(stk);
+			goto out;
+		}
+
+		type = STK_RESULT_TYPE_NO_RESPONSE;
+		goto send;
+	}
+
+	dbus_error_init(&err);
+	if (dbus_set_error_from_message(&err, reply)) {
+		if (g_str_equal(err.name, OFONO_NAVIGATION_TERMINATED))
+			type = STK_RESULT_TYPE_USER_TERMINATED;
+		else if (g_str_equal(err.name, OFONO_NAVIGATION_GOBACK))
+			type = STK_RESULT_TYPE_GO_BACK;
+		else {
+			type = STK_RESULT_TYPE_USER_TERMINATED;
+
+			ofono_error("Unknown reply %s\n", err.name);
+		}
+
+		dbus_error_free(&err);
+		goto send;
+	} else if (dbus_message_get_args(reply, NULL,
+						DBUS_TYPE_INT16, &selection,
+						DBUS_TYPE_BOOLEAN, &help,
+						DBUS_TYPE_INVALID) != FALSE) {
+		if (help == FALSE)
+			type = STK_RESULT_TYPE_SUCCESS;
+		else
+			type = STK_RESULT_TYPE_HELP_REQUESTED;
+	} else if (dbus_message_get_args(reply, NULL,
+						DBUS_TYPE_INT16, &selection,
+						DBUS_TYPE_INVALID) != FALSE) {
+		type = STK_RESULT_TYPE_SUCCESS;
+	} else {
+		type = STK_RESULT_TYPE_USER_TERMINATED;
+
+		ofono_error("Can't parse the reply to SelectItem()");
+		goto send;
+	}
+
+	if (help != FALSE && !menu->has_help) {
+		type = STK_RESULT_TYPE_USER_TERMINATED;
+		ofono_error("Help requested but not available");
+
+		goto send;
+	}
+
+	item = g_slist_nth_data(menu->items, selection);
+	if (!item) {
+		type = STK_RESULT_TYPE_USER_TERMINATED;
+		ofono_error("Invalid item selected");
+	}
+
+send:
+	if (select_item) {
+		struct stk_response rsp;
+		struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
+
+		memset(&rsp, 0, sizeof(rsp));
+		rsp.result.type = type;
+
+		if (item)
+			rsp.select_item.item_id = item->id;
+
+		if (stk_respond(stk, &rsp, display_command_cb))
+			display_command_cb(&error, stk);
+	} else {
+		struct stk_envelope e;
+
+		if (type != STK_RESULT_TYPE_SUCCESS &&
+				type != STK_RESULT_TYPE_HELP_REQUESTED) {
+			ofono_error("Invalid reply to main menu request");
+
+			display_idle(stk);
+			goto out;
+		}
+
+		memset(&e, 0, sizeof(e));
+
+		e.type = STK_ENVELOPE_TYPE_MENU_SELECTION;
+		e.src = STK_DEVICE_IDENTITY_TYPE_KEYPAD,
+		e.menu_selection.item_id = item->id;
+		e.menu_selection.help_request = help;
+
+		if (stk_send_envelope(stk, &e, display_envelope_cb, 0))
+			display_envelope_cb(stk, FALSE, NULL, -1);
+	}
+
+out:
+	if (select_item) {
+		stk_menu_free(menu);
+		stk->select_item_menu = NULL;
+	}
+}
+
+static gboolean display_idle_cb(gpointer user_data)
+{
+	struct ofono_stk *stk = user_data;
+
+	stk->display_source = 0;
+
+	if (stk->app_agent_state != STK_AGENT_IDLE)
+		return FALSE;
+
+	if (stk->main_menu) {
+		stk->custom_timeout = -1; /* No timeout */
+		app_agent_request_start(stk, request_menu_send,
+					request_menu_cb, STK_AGENT_MAIN_MENU);
+	}
+
+	return FALSE;
+}
+
+static void display_idle(struct ofono_stk *stk)
+{
+	if (stk->display_source)
+		return;
+
+	stk->display_source = g_timeout_add(0, display_idle_cb, stk);
+}
+
+static gboolean handle_command_select_item(const struct stk_command *cmd,
+						struct stk_response *rsp,
+						struct ofono_stk *stk)
+{
+	gboolean soft_key = (cmd->qualifier & (1 << 2)) != 0;
+	gboolean has_help = (cmd->qualifier & (1 << 7)) != 0;
+
+	stk->select_item_menu = stk_menu_create(cmd->select_item.alpha_id,
+					&cmd->select_item.text_attr,
+					cmd->select_item.items,
+					&cmd->select_item.item_text_attr_list,
+					&cmd->select_item.next_act,
+					cmd->select_item.item_id,
+					soft_key, has_help);
+
+	app_agent_request_start(stk, request_menu_send, request_menu_cb,
+				STK_AGENT_SELECT_ITEM);
+
+	return FALSE;
+}
+
+static gboolean handle_command_set_up_menu(const struct stk_command *cmd,
+						struct stk_response *rsp,
+						struct ofono_stk *stk)
+{
+	gboolean soft_key = (cmd->qualifier & (1 << 0)) != 0;
+	gboolean has_help = (cmd->qualifier & (1 << 7)) != 0;
+
+	if (stk->main_menu) {
+		stk_menu_free(stk->main_menu);
+		stk->main_menu = NULL;
+	}
+
+	if (cmd->setup_menu.items == NULL)
+		goto out;
+
+	stk->main_menu = stk_menu_create(cmd->setup_menu.alpha_id,
+					&cmd->setup_menu.text_attr,
+					cmd->setup_menu.items,
+					&cmd->setup_menu.item_text_attr_list,
+					&cmd->setup_menu.next_act,
+					0, soft_key, has_help);
+
+out:
+	stk->custom_timeout = -1; /* No timeout */
+	app_agent_request_start(stk, request_menu_send, request_menu_cb,
+				STK_AGENT_MAIN_MENU);
+
+	return TRUE;
+}
+
 void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
 {
 	if (!stk->pending_cmd)
@@ -784,6 +1024,14 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 			respond = handle_command_more_time(stk->pending_cmd,
 								&rsp, stk);
 			break;
+		case STK_COMMAND_TYPE_SELECT_ITEM:
+			respond = handle_command_select_item(stk->pending_cmd,
+								&rsp, stk);
+			break;
+		case STK_COMMAND_TYPE_SETUP_MENU:
+			respond = handle_command_set_up_menu(stk->pending_cmd,
+								&rsp, stk);
+			break;
 		}
 
 		if (respond)
@@ -851,6 +1099,19 @@ static void stk_unregister(struct ofono_atom *atom)
 		}
 	}
 
+	if (stk->select_item_menu) {
+		stk_menu_free(stk->select_item_menu);
+		stk->select_item_menu = NULL;
+	}
+
+	if (stk->main_menu) {
+		stk_menu_free(stk->main_menu);
+		stk->main_menu = NULL;
+	}
+
+	if (stk->display_source)
+		g_source_remove(stk->display_source);
+
 	g_queue_foreach(stk->envelope_q, (GFunc) g_free, NULL);
 	g_queue_free(stk->envelope_q);
 
-- 
1.7.1.86.g0e460.dirty

[PATCH 9/9] stk: Handle the Display Text command.

[Andrzej Zaborowski]

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

---
 src/stk.c |  108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 108 insertions(+), 0 deletions(-)

diff --git a/src/stk.c b/src/stk.c
index fd6dcb0..ff47649 100644
--- a/src/stk.c
+++ b/src/stk.c
@@ -63,6 +63,7 @@ enum stk_agent_state {
 	STK_AGENT_IDLE = 0,
 	STK_AGENT_MAIN_MENU,
 	STK_AGENT_SELECT_ITEM,
+	STK_AGENT_DISPLAY_TEXT,
 };
 
 struct ofono_stk {
@@ -962,6 +963,110 @@ out:
 	return TRUE;
 }
 
+static void request_text_send(struct ofono_stk *stk, DBusMessage *call)
+{
+	struct stk_command_display_text *txt = &stk->pending_cmd->display_text;
+	uint8_t qualifier = stk->pending_cmd->qualifier;
+	dbus_bool_t confirm = (qualifier & (1 << 7)) != 0;
+	dbus_bool_t priority = (qualifier & (1 << 0)) != 0;
+	dbus_bool_t navigation = !txt->immediate_response;
+
+	dbus_message_set_member(call, "DisplayText");
+
+	dbus_message_append_args(call,
+					DBUS_TYPE_STRING, &txt->text,
+					DBUS_TYPE_BOOLEAN, &confirm,
+					DBUS_TYPE_BOOLEAN, &priority,
+					DBUS_TYPE_BOOLEAN, &navigation,
+					DBUS_TYPE_INVALID);
+}
+
+static void request_text_cb(struct ofono_stk *stk, DBusMessage *reply)
+{
+	struct stk_command_display_text *txt = &stk->pending_cmd->display_text;
+	uint8_t qualifier = stk->pending_cmd->qualifier;
+	gboolean confirm = (qualifier & (1 << 7)) != 0;
+	DBusError err;
+	enum stk_result_type type = STK_RESULT_TYPE_SUCCESS;
+	struct stk_response rsp;
+	struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
+
+	if (!reply) {
+		if (stk->cmd_timeout) {
+			app_agent_request_cancel(stk);
+
+			goto out;
+		}
+
+		if (confirm)
+			type = STK_RESULT_TYPE_NO_RESPONSE;
+
+		goto send;
+	}
+
+	dbus_error_init(&err);
+	if (dbus_set_error_from_message(&err, reply)) {
+		if (g_str_equal(err.name, OFONO_NAVIGATION_TERMINATED))
+			type = STK_RESULT_TYPE_USER_TERMINATED;
+		else if (g_str_equal(err.name, OFONO_NAVIGATION_GOBACK))
+			type = STK_RESULT_TYPE_GO_BACK;
+		else {
+			type = STK_RESULT_TYPE_USER_TERMINATED;
+
+			ofono_error("Unknown reply %s", err.name);
+		}
+
+		dbus_error_free(&err);
+		goto send;
+	}
+
+	if (dbus_message_get_args(reply, NULL, DBUS_TYPE_INVALID) == FALSE) {
+		type = STK_RESULT_TYPE_USER_TERMINATED;
+
+		ofono_error("Reply not understood");
+	}
+
+send:
+	if (txt->immediate_response)
+		goto out;
+
+	memset(&rsp, 0, sizeof(rsp));
+	rsp.result.type = type;
+
+	if (stk_respond(stk, &rsp, display_command_cb))
+		display_command_cb(&error, stk);
+
+	return;
+out:
+	display_idle(stk);
+}
+
+
+static gboolean handle_command_display_text(const struct stk_command *cmd,
+						struct stk_response *rsp,
+						struct ofono_stk *stk)
+{
+	int timeout = 0;
+
+	if (cmd->display_text.duration.interval) {
+		timeout = cmd->display_text.duration.interval;
+		switch (cmd->display_text.duration.unit) {
+		case STK_DURATION_TYPE_MINUTES:
+			timeout *= 60;
+		case STK_DURATION_TYPE_SECONDS:
+			timeout *= 10;
+		case STK_DURATION_TYPE_SECOND_TENTHS:
+			timeout *= 100;
+		}
+	}
+
+	stk->custom_timeout = timeout;
+	app_agent_request_start(stk, request_text_send, request_text_cb,
+				STK_AGENT_DISPLAY_TEXT);
+
+	return cmd->display_text.immediate_response;
+}
+
 void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
 {
 	if (!stk->pending_cmd)
@@ -1032,6 +1137,9 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
 			respond = handle_command_set_up_menu(stk->pending_cmd,
 								&rsp, stk);
 			break;
+		case STK_COMMAND_TYPE_DISPLAY_TEXT:
+			respond = handle_command_display_text(stk->pending_cmd,
+								&rsp, stk);
 		}
 
 		if (respond)
-- 
1.7.1.86.g0e460.dirty

Re: [PATCH 1/9] stk: Utilities for proactive command/envelope handling

[Denis Kenzior]

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

Hi Andrew,

> @@ -43,10 +43,61 @@ struct ofono_stk {
>  	const struct ofono_stk_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> +	struct stk_command *pending_cmd;
> +	void (*cancel_cmd)(struct ofono_stk *stk);

I notice that you remove cancel_cmd in a later patch.  Please don't do
that.  Clean this patch up not to include cancel_cmd and resubmit.

>  };
>  
> +static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp,
> +			void (*cb)(const struct ofono_error *error,
> +					struct ofono_stk *stk))

Please don't do this.  The callback should just take the standard
ofono_stk_generic_cb_t

> +{
> +	const guint8 *tlv;
> +	unsigned int tlv_len;
> +
> +	rsp->src = STK_DEVICE_IDENTITY_TYPE_TERMINAL;
> +	rsp->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
> +	rsp->number = stk->pending_cmd->number;
> +	rsp->type = stk->pending_cmd->type;
> +	rsp->qualifier = stk->pending_cmd->qualifier;
> +
> +	if (stk->driver->terminal_response == NULL)
> +		return -ENOSYS;

Might want to do this check first.

> +
> +	tlv = stk_pdu_from_response(rsp, &tlv_len);
> +	if (!tlv)
> +		return -EINVAL;
> +
> +	stk->driver->terminal_response(stk, tlv_len, tlv,
> +					(ofono_stk_generic_cb_t) cb, stk);

Do not cast please, just bite the bullet and use already defined
callback typedefs and use
ofono_stk *stk = user;

> +	return 0;

Empty line before return 0 please.

> +}
> +
> +static int stk_send_envelope(struct ofono_stk *stk, struct stk_envelope *e,
> +				void (*cb)(const struct ofono_error *error,
> +						const unsigned char *data,
> +						int length,
> +						struct ofono_stk *stk))
> +{
> +	const guint8 *tlv;
> +	unsigned int tlv_len;
> +
> +	e->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
> +
> +	if (stk->driver->envelope == NULL)
> +		return -ENOSYS;
> +
> +	tlv = stk_pdu_from_envelope(e, &tlv_len);
> +	if (!tlv)
> +		return -EINVAL;
> +
> +	stk->driver->envelope(stk, tlv_len, tlv,
> +				(ofono_stk_envelope_cb_t) cb, stk);
> +	return 0;

Same comment as above, and also an empty line before return 0 please.

> +}
> +
>  static void stk_cbs_download_cb(const struct ofono_error *error,
> -				const unsigned char *data, int len, void *user)
> +				const unsigned char *data, int len,
> +				struct ofono_stk *stk)
>  {
>  	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>  		ofono_error("CellBroadcast download to UICC failed");
> @@ -55,36 +106,62 @@ static void stk_cbs_download_cb(const struct ofono_error *error,
>  		return;
>  	}
>  
> +	if (len)
> +		ofono_error("CellBroadcast download returned %i bytes of data",
> +				len);
> +
>  	DBG("CellBroadcast download to UICC reported no error");
>  }
>  
>  void __ofono_cbs_sim_download(struct ofono_stk *stk, const struct cbs *msg)
>  {
> -	const guint8 *tlv;
> -	unsigned int tlv_len;
> +	struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
>  	struct stk_envelope e;
> +	int err;
>  
> -	if (stk->driver->envelope == NULL)
> -		return;
> +	memset(&e, 0, sizeof(e));
>  
>  	e.type = STK_ENVELOPE_TYPE_CBS_PP_DOWNLOAD;
>  	e.src = STK_DEVICE_IDENTITY_TYPE_NETWORK;
> -	e.dst = STK_DEVICE_IDENTITY_TYPE_UICC;
>  	memcpy(&e.cbs_pp_download.page, msg, sizeof(msg));
>  
> -	tlv = stk_pdu_from_envelope(&e, &tlv_len);
> -	if (!tlv)
> +	err = stk_send_envelope(stk, &e, stk_cbs_download_cb);
> +	if (err)
> +		stk_cbs_download_cb(&error, NULL, -1, stk);
> +}
> +
> +static void stk_command_cb(const struct ofono_error *error,
> +				struct ofono_stk *stk)
> +{
> +	stk_command_free(stk->pending_cmd);
> +	stk->pending_cmd = NULL;
> +
> +	if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
> +		ofono_error("TERMINAL RESPONSE to a UICC command failed");
> +		/* "The ME may retry to deliver the same Cell Broadcast
> +		 * page." */
>  		return;
> +	}
>  
> -	stk->driver->envelope(stk, tlv_len, tlv, stk_cbs_download_cb, stk);
> +	DBG("TERMINAL RESPONSE to a command reported no errors");
> +}
> +
> +void ofono_stk_proactive_command_cancel(struct ofono_stk *stk)
> +{
> +	if (!stk->pending_cmd)
> +		return;
> +
> +	stk->cancel_cmd(stk);

I notice this code has also been removed in future patches.  Please
clean up this patch.

>  }
>  
>  void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
>  					int length, const unsigned char *pdu)
>  {
> -	struct stk_command *cmd;
> +	struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
> +	struct stk_response rsp;
>  	char *buf;
> -	int i;
> +	int i, err;
> +	gboolean respond = TRUE;
>  
>  	buf = g_try_malloc(length * 2 + 1);
>  	if (!buf)
> @@ -93,18 +170,51 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk,
>  	for (i = 0; i < length; i ++)
>  		sprintf(buf + i * 2, "%02hhx", pdu[i]);
>  
> -	cmd = stk_command_new_from_pdu(pdu, length);
> -	if (!cmd) {
> +	stk->pending_cmd = stk_command_new_from_pdu(pdu, length);
> +	if (!stk->pending_cmd) {
>  		ofono_error("Can't parse proactive command: %s", buf);
>  
> -		/* TODO: return TERMINAL RESPONSE with permanent error */
> +		/*
> +		 * Nothing we can do, we'd need at least Command Details
> +		 * to be able to respond with an error.
> +		 */
>  		goto done;
>  	}
>  
> -	/* TODO: execute */
> -	ofono_info("Proactive command PDU: %s", buf);
> +	ofono_debug("Proactive command PDU: %s", buf);
> +
> +	memset(&rsp, 0, sizeof(rsp));
> +
> +	switch (stk->pending_cmd->status) {
> +	case STK_PARSE_RESULT_OK:
> +		switch (stk->pending_cmd->type) {
> +		default:
> +			rsp.result.type =
> +				STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD;
> +			break;
> +		}
> +
> +		if (respond)
> +			break;
> +		return;
> +
> +	case STK_PARSE_RESULT_TYPE_NOT_UNDERSTOOD:
> +	default:

Does this even work?  You might want to move this down to the end of the
switch/case.

> +		rsp.result.type = STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD;
> +		break;
> +
> +	case STK_PARSE_RESULT_MISSING_VALUE:
> +		rsp.result.type = STK_RESULT_TYPE_MINIMUM_NOT_MET;
> +		break;
> +
> +	case STK_PARSE_RESULT_DATA_NOT_UNDERSTOOD:
> +		rsp.result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD;
> +		break;
> +	}
>  
> -	stk_command_free(cmd);
> +	err = stk_respond(stk, &rsp, stk_command_cb);
> +	if (err)
> +		stk_command_cb(&error, stk);
>  
>  done:
>  	g_free(buf);

Regards,
-Denis