* [PATCH 6/6] stk: Introduce BIP command handlers @ 2011-04-08 16:33 Philippe Nunes 2011-04-12 5:19 ` Denis Kenzior 0 siblings, 1 reply; 5+ messages in thread From: Philippe Nunes @ 2011-04-08 16:33 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 18311 bytes --] --- src/stk.c | 506 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 files changed, 484 insertions(+), 22 deletions(-) diff --git a/src/stk.c b/src/stk.c index c86cbfb..e033df7 100644 --- a/src/stk.c +++ b/src/stk.c @@ -79,6 +79,10 @@ struct ofono_stk { __ofono_sms_sim_download_cb_t sms_pp_cb; void *sms_pp_userdata; + struct stk_channel channel; + struct stk_channel_data rx_buffer; + struct stk_channel_data tx_buffer; + gboolean link_on_demand; }; struct envelope_op { @@ -104,6 +108,13 @@ static void timers_update(struct ofono_stk *stk); result.additional_len = sizeof(addn_info); \ result.additional = addn_info; \ +/* + * According the structure and coding of the Terminal response defined in + * TS 102 223 section 6.8, the maximum number of bytes possible for the channel + * data object is 243 + */ +#define CHANNEL_DATA_OBJECT_MAX_LENGTH 243 + static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, ofono_stk_generic_cb_t cb) { @@ -112,6 +123,9 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, DBG(""); + if (stk->pending_cmd == NULL) + return 0; + if (stk->driver->terminal_response == NULL) return -ENOSYS; @@ -128,6 +142,7 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, stk_command_free(stk->pending_cmd); stk->pending_cmd = NULL; stk->cancel_cmd = NULL; + stk->respond_on_exit = FALSE; stk->driver->terminal_response(stk, tlv_len, tlv, cb, stk); @@ -473,14 +488,48 @@ static void emit_menu_changed(struct ofono_stk *stk) g_dbus_send_message(conn, signal); } +static void stk_close_channel(struct ofono_stk *stk) +{ + /* + * TODO + * Deactivate PDP context + * Send the Terminal Response once the PDP context is deactivated + */ + + /* Temporary implementation */ + g_free(stk->rx_buffer.data.array); + g_free(stk->tx_buffer.data.array); + stk->rx_buffer.data.array = NULL; + stk->tx_buffer.data.array = NULL; + + stk->channel.id = 0; + stk->channel.status = STK_CHANNEL_PACKET_DATA_SERVICE_NOT_ACTIVATED; + + if (stk->pending_cmd && + stk->pending_cmd->type == STK_COMMAND_TYPE_CLOSE_CHANNEL) + send_simple_response(stk, STK_RESULT_TYPE_SUCCESS); + else { + /* + * TODO + * Send Event channel status + */ + } +} + static void user_termination_cb(enum stk_agent_result result, void *user_data) { struct ofono_stk *stk = user_data; - if (result == STK_AGENT_RESULT_TERMINATE) { - stk->respond_on_exit = FALSE; + if (result != STK_AGENT_RESULT_TERMINATE) + return; + + if (stk->pending_cmd) { send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); + stk->cancel_cmd(stk); } + + if (stk->channel.id) + stk_close_channel(stk); } static void stk_alpha_id_set(struct ofono_stk *stk, @@ -511,6 +560,147 @@ static void stk_alpha_id_unset(struct ofono_stk *stk) stk_agent_request_cancel(stk->current_agent); } + +static void stk_open_channel(struct ofono_stk *stk) +{ + const struct stk_command_open_channel *oc; + struct stk_response rsp; + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; + + if (stk->pending_cmd == NULL || + stk->pending_cmd->type != STK_COMMAND_TYPE_OPEN_CHANNEL) + return; + + oc = &stk->pending_cmd->open_channel; + + memset(&rsp, 0, sizeof(rsp)); + rsp.result.type = STK_RESULT_TYPE_SUCCESS; + + stk->rx_buffer.data.array = g_try_malloc(oc->buf_size); + if (stk->rx_buffer.data.array == NULL) { + rsp.result.type = STK_RESULT_TYPE_NOT_CAPABLE; + goto out; + } + + stk->tx_buffer.data.array = g_try_malloc(oc->buf_size); + if (stk->tx_buffer.data.array == NULL) { + rsp.result.type = STK_RESULT_TYPE_NOT_CAPABLE; + goto out; + } + + stk->rx_buffer.data.len = oc->buf_size; + stk->tx_buffer.data.len = oc->buf_size; + stk->link_on_demand = (stk->pending_cmd->qualifier & + STK_OPEN_CHANNEL_FLAG_IMMEDIATE) ? FALSE : TRUE; + + /* + * TODO + * Setup the channel-> either create a new PDP context or + * use a default one + * Send the Terminal Response or wait until the PDP context is activated + * in case of immediate link establishment not in background. + */ +out: + + if (stk_respond(stk, &rsp, stk_command_cb)) + stk_command_cb(&failure, stk); +} + +static void stk_send_data(struct ofono_stk *stk, + struct stk_common_byte_array data, + unsigned char qualifier) +{ + struct stk_response rsp; + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; + unsigned int offset; + + memset(&rsp, 0, sizeof(rsp)); + rsp.result.type = STK_RESULT_TYPE_SUCCESS; + + if (data.len > stk->tx_buffer.data.len) { + rsp.result.type = STK_RESULT_TYPE_BIP_ERROR; + goto out; + } + + if (qualifier == STK_SEND_DATA_STORE_DATA) { + + /* + * check if the requested number of bytes of empty space + * is available + */ + if (data.len > stk->tx_buffer.tx_avail) { + rsp.result.type = STK_RESULT_TYPE_BIP_ERROR; + goto out; + } + + offset = stk->tx_buffer.data.len - stk->tx_buffer.tx_avail; + memcpy(stk->tx_buffer.data.array + offset, data.array, + data.len); + + stk->tx_buffer.tx_avail -= data.len; + rsp.send_data.tx_avail = stk->tx_buffer.tx_avail; + goto out; + } + + if (stk->channel.status == STK_CHANNEL_PACKET_DATA_SERVICE_NOT_ACTIVATED + && stk->link_on_demand == TRUE) { + /* + * TODO + * activate the context, update the channel status + * once the context is activated, send the data immediately + * and flush the tx buffer + */ + } else { + /* + * TODO + * send the data immediately, flush the tx buffer + */ + stk->tx_buffer.tx_avail = stk->tx_buffer.data.len; + rsp.send_data.tx_avail = stk->tx_buffer.data.len; + } + +out: + + if (stk_respond(stk, &rsp, stk_command_cb)) + stk_command_cb(&failure, stk); +} + +static void stk_receive_data(struct ofono_stk *stk, unsigned char data_len) +{ + struct stk_response rsp; + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; + + memset(&rsp, 0, sizeof(rsp)); + rsp.result.type = STK_RESULT_TYPE_SUCCESS; + + if (stk->rx_buffer.rx_remaining == 0) { + rsp.result.type = STK_RESULT_TYPE_MISSING_INFO; + goto out; + } + + if (data_len > stk->rx_buffer.rx_remaining) { + rsp.result.type = STK_RESULT_TYPE_MISSING_INFO; + data_len = stk->rx_buffer.rx_remaining; + } + + if (data_len > CHANNEL_DATA_OBJECT_MAX_LENGTH) + data_len = CHANNEL_DATA_OBJECT_MAX_LENGTH; + + rsp.receive_data.rx_data.array = stk->rx_buffer.data.array; + rsp.receive_data.rx_data.len = data_len; + stk->rx_buffer.rx_remaining -= data_len; + rsp.receive_data.rx_remaining = stk->rx_buffer.rx_remaining; + +out: + if (stk_respond(stk, &rsp, stk_command_cb)) + stk_command_cb(&failure, stk); + + if (rsp.receive_data.rx_data.len && stk->rx_buffer.rx_remaining > 0) + memmove(stk->rx_buffer.data.array, stk->rx_buffer.data.array + + data_len, stk->rx_buffer.rx_remaining); + +} + static int duration_to_msecs(const struct stk_duration *duration) { int msecs = duration->interval; @@ -598,7 +788,6 @@ static void default_agent_notify(gpointer user_data) stk->default_agent = NULL; stk->current_agent = stk->session_agent; - stk->respond_on_exit = FALSE; } static void session_agent_notify(gpointer user_data) @@ -617,7 +806,6 @@ static void session_agent_notify(gpointer user_data) stk->session_agent = NULL; stk->current_agent = stk->default_agent; - stk->respond_on_exit = FALSE; if (stk->remove_agent_source) { g_source_remove(stk->remove_agent_source); @@ -1159,8 +1347,6 @@ static void request_selection_cb(enum stk_agent_result result, uint8_t id, { struct ofono_stk *stk = user_data; - stk->respond_on_exit = FALSE; - switch (result) { case STK_AGENT_RESULT_OK: { @@ -1243,8 +1429,6 @@ static void display_text_cb(enum stk_agent_result result, void *user_data) static unsigned char screen_busy_result[] = { 0x01 }; static struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE }; - stk->respond_on_exit = FALSE; - /* * There are four possible paths for DisplayText with immediate * response flag set: @@ -1386,8 +1570,6 @@ static void request_confirmation_cb(enum stk_agent_result result, struct stk_command_get_inkey *cmd = &stk->pending_cmd->get_inkey; struct stk_response rsp; - stk->respond_on_exit = FALSE; - switch (result) { case STK_AGENT_RESULT_OK: memset(&rsp, 0, sizeof(rsp)); @@ -1442,8 +1624,6 @@ static void request_key_cb(enum stk_agent_result result, char *string, struct stk_command_get_inkey *cmd = &stk->pending_cmd->get_inkey; struct stk_response rsp; - stk->respond_on_exit = FALSE; - switch (result) { case STK_AGENT_RESULT_OK: memset(&rsp, 0, sizeof(rsp)); @@ -1560,8 +1740,6 @@ static void request_string_cb(enum stk_agent_result result, char *string, gboolean packed = (qualifier & (1 << 3)) != 0; struct stk_response rsp; - stk->respond_on_exit = FALSE; - switch (result) { case STK_AGENT_RESULT_OK: memset(&rsp, 0, sizeof(rsp)); @@ -1699,8 +1877,6 @@ static void confirm_call_cb(enum stk_agent_result result, gboolean confirm, struct stk_response rsp; int err; - stk->respond_on_exit = FALSE; - switch (result) { case STK_AGENT_RESULT_TIMEOUT: confirm = FALSE; @@ -2300,8 +2476,6 @@ static void dtmf_sent_cb(int error, void *user_data) { struct ofono_stk *stk = user_data; - stk->respond_on_exit = FALSE; - stk_alpha_id_unset(stk); if (error == ENOENT) { @@ -2413,8 +2587,6 @@ static void play_tone_cb(enum stk_agent_result result, void *user_data) { struct ofono_stk *stk = user_data; - stk->respond_on_exit = FALSE; - switch (result) { case STK_AGENT_RESULT_OK: case STK_AGENT_RESULT_TIMEOUT: @@ -2547,8 +2719,6 @@ static void confirm_launch_browser_cb(enum stk_agent_result result, struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; struct stk_response rsp; - stk->respond_on_exit = FALSE; - switch (result) { case STK_AGENT_RESULT_TIMEOUT: confirm = FALSE; @@ -2612,6 +2782,273 @@ static gboolean handle_command_launch_browser(const struct stk_command *cmd, return FALSE; } + +static void open_channel_cancel(struct ofono_stk *stk) +{ + /* TODO */ +} + +static void confirm_open_channel_cb(enum stk_agent_result result, + gboolean confirm, + void *user_data) +{ + struct ofono_stk *stk = user_data; + + switch (result) { + case STK_AGENT_RESULT_TERMINATE: + send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); + return; + + case STK_AGENT_RESULT_TIMEOUT: + confirm = FALSE; + /* Fall through */ + + case STK_AGENT_RESULT_OK: + if (confirm) + break; + /* Fall through */ + + default: + send_simple_response(stk, STK_RESULT_TYPE_TERMINAL_BUSY); + return; + } + + stk_open_channel(stk); +} + +static gboolean handle_command_open_channel(const struct stk_command *cmd, + struct stk_response *rsp, + struct ofono_stk *stk) +{ + struct ofono_modem *modem = __ofono_atom_get_modem(stk->atom); + const struct stk_command_open_channel *oc = &cmd->open_channel; + struct ofono_atom *gprs_atom; + int err; + + /* Check first if channel is available */ + if (stk->channel.id) { + unsigned char addnl_info[1]; + + addnl_info[0] = STK_RESULT_ADDNL_BIP_PB_NO_CHANNEL_AVAIL; + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, + addnl_info); + return TRUE; + } + + gprs_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_GPRS); + if (gprs_atom == NULL || !__ofono_atom_get_registered(gprs_atom)) { + rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE; + return TRUE; + } + + stk->respond_on_exit = TRUE; + stk->cancel_cmd = open_channel_cancel; + + /* + * Don't ask for user confirmation if AID is a null data object + * or is not provided + */ + if (oc->alpha_id && strlen(oc->alpha_id) > 0) { + char *alpha_id; + + alpha_id = dbus_apply_text_attributes(oc->alpha_id, + &oc->text_attr); + if (alpha_id == NULL) { + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; + return TRUE; + } + + err = stk_agent_confirm_open_channel(stk->current_agent, + alpha_id, + &oc->icon_id, + confirm_open_channel_cb, + stk, NULL, + stk->timeout * 1000); + g_free(alpha_id); + + if (err < 0) { + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; + return TRUE; + } + + return FALSE; + } + + stk_open_channel(stk); + + return FALSE; +} + +static gboolean handle_command_close_channel(const struct stk_command *cmd, + struct stk_response *rsp, + struct ofono_stk *stk) +{ + const struct stk_command_close_channel *cc = &cmd->close_channel; + int err; + + /* Check if channel identifier is valid */ + if (cmd->dst != stk->channel.id) { + unsigned char addnl_info[1]; + + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, + addnl_info); + return TRUE; + } + + /* + * Don't inform the user about the link closing phase if AID is + * a null data object or is not provided + */ + if (cc->alpha_id && strlen(cc->alpha_id) > 0) { + char *alpha_id; + + alpha_id = dbus_apply_text_attributes(cc->alpha_id, + &cc->text_attr); + if (alpha_id == NULL) { + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; + return TRUE; + } + + err = stk_agent_display_action(stk->current_agent, alpha_id, + &cc->icon_id, + user_termination_cb, + stk, NULL); + g_free(alpha_id); + + if (err < 0) { + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; + return TRUE; + } + } + + stk->respond_on_exit = TRUE; + stk->cancel_cmd = stk_request_cancel; + + stk_close_channel(stk); + + return FALSE; +} + +static gboolean handle_command_receive_data(const struct stk_command *cmd, + struct stk_response *rsp, + struct ofono_stk *stk) +{ + const struct stk_command_receive_data *rd = &cmd->receive_data; + int err; + + /* Check if channel identifier is valid or already closed */ + if (cmd->dst != stk->channel.id) { + unsigned char addnl_info[1]; + + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, + addnl_info); + return TRUE; + } + + /* + * Don't inform the user during data transfer if AID is + * a null data object or is not provided + */ + if (rd->alpha_id && strlen(rd->alpha_id) > 0) { + char *alpha_id; + + alpha_id = dbus_apply_text_attributes(rd->alpha_id, + &rd->text_attr); + if (alpha_id == NULL) { + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; + return TRUE; + } + + err = stk_agent_display_action(stk->current_agent, alpha_id, + &rd->icon_id, + user_termination_cb, + stk, NULL); + g_free(alpha_id); + + if (err < 0) { + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; + return TRUE; + } + } + + stk->respond_on_exit = TRUE; + stk->cancel_cmd = stk_request_cancel; + + stk_receive_data(stk, rd->data_len); + + return FALSE; +} + +static gboolean handle_command_send_data(const struct stk_command *cmd, + struct stk_response *rsp, + struct ofono_stk *stk) +{ + const struct stk_command_send_data *sd = &cmd->send_data; + int err; + unsigned char addnl_info[1]; + + /* Check if channel identifier is valid or already closed */ + if (cmd->dst != stk->channel.id) { + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, + addnl_info); + return TRUE; + } + + /* Check if the link was dropped */ + if (stk->channel.status == STK_CHANNEL_LINK_DROPPED) { + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_CLOSED; + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, + addnl_info); + return TRUE; + } + + /* + * Don't inform the user during data transfer if AID is + * a null data object or is not provided + */ + if (sd->alpha_id && strlen(sd->alpha_id) > 0) { + char *alpha_id; + + alpha_id = dbus_apply_text_attributes(sd->alpha_id, + &sd->text_attr); + if (alpha_id == NULL) { + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; + return TRUE; + } + + err = stk_agent_display_action(stk->current_agent, alpha_id, + &sd->icon_id, + user_termination_cb, + stk, NULL); + g_free(alpha_id); + + if (err < 0) { + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; + return TRUE; + } + } + + stk->respond_on_exit = TRUE; + stk->cancel_cmd = stk_request_cancel; + + stk_send_data(stk, sd->data, cmd->qualifier); + + return FALSE; +} + +static gboolean handle_command_get_channel_status(const struct stk_command *cmd, + struct stk_response *rsp, + struct ofono_stk *stk) +{ + rsp->result.type = STK_RESULT_TYPE_SUCCESS; + rsp->channel_status.channel.id = stk->channel.id; + rsp->channel_status.channel.status = stk->channel.status; + return TRUE; +} + static void stk_proactive_command_cancel(struct ofono_stk *stk) { if (stk->immediate_response) @@ -2805,6 +3242,31 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk, &rsp, stk); break; + case STK_COMMAND_TYPE_OPEN_CHANNEL: + respond = handle_command_open_channel(stk->pending_cmd, + &rsp, stk); + break; + + case STK_COMMAND_TYPE_CLOSE_CHANNEL: + respond = handle_command_close_channel(stk->pending_cmd, + &rsp, stk); + break; + + case STK_COMMAND_TYPE_RECEIVE_DATA: + respond = handle_command_receive_data(stk->pending_cmd, + &rsp, stk); + break; + + case STK_COMMAND_TYPE_SEND_DATA: + respond = handle_command_send_data(stk->pending_cmd, + &rsp, stk); + break; + + case STK_COMMAND_TYPE_GET_CHANNEL_STATUS: + respond = handle_command_get_channel_status(stk->pending_cmd, + &rsp, stk); + break; + default: rsp.result.type = STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD; break; -- 1.7.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 6/6] stk: Introduce BIP command handlers 2011-04-08 16:33 [PATCH 6/6] stk: Introduce BIP command handlers Philippe Nunes @ 2011-04-12 5:19 ` Denis Kenzior 2011-04-13 14:02 ` Philippe Nunes 0 siblings, 1 reply; 5+ messages in thread From: Denis Kenzior @ 2011-04-12 5:19 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 20645 bytes --] Hi Philippe, On 04/08/2011 11:33 AM, Philippe Nunes wrote: > --- > src/stk.c | 506 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 files changed, 484 insertions(+), 22 deletions(-) > > diff --git a/src/stk.c b/src/stk.c > index c86cbfb..e033df7 100644 > --- a/src/stk.c > +++ b/src/stk.c > @@ -79,6 +79,10 @@ struct ofono_stk { > > __ofono_sms_sim_download_cb_t sms_pp_cb; > void *sms_pp_userdata; > + struct stk_channel channel; > + struct stk_channel_data rx_buffer; > + struct stk_channel_data tx_buffer; > + gboolean link_on_demand; > }; > > struct envelope_op { > @@ -104,6 +108,13 @@ static void timers_update(struct ofono_stk *stk); > result.additional_len = sizeof(addn_info); \ > result.additional = addn_info; \ > > +/* > + * According the structure and coding of the Terminal response defined in > + * TS 102 223 section 6.8, the maximum number of bytes possible for the channel > + * data object is 243 > + */ > +#define CHANNEL_DATA_OBJECT_MAX_LENGTH 243 > + > static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, > ofono_stk_generic_cb_t cb) > { > @@ -112,6 +123,9 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, > > DBG(""); > > + if (stk->pending_cmd == NULL) > + return 0; > + This seems fishy, why do you need this? Is this a bug you discovered or a behavior change necessitated by BIP support? > if (stk->driver->terminal_response == NULL) > return -ENOSYS; > > @@ -128,6 +142,7 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, > stk_command_free(stk->pending_cmd); > stk->pending_cmd = NULL; > stk->cancel_cmd = NULL; > + stk->respond_on_exit = FALSE; Same comment as above here > > stk->driver->terminal_response(stk, tlv_len, tlv, cb, stk); > > @@ -473,14 +488,48 @@ static void emit_menu_changed(struct ofono_stk *stk) > g_dbus_send_message(conn, signal); > } > > +static void stk_close_channel(struct ofono_stk *stk) > +{ > + /* > + * TODO > + * Deactivate PDP context > + * Send the Terminal Response once the PDP context is deactivated > + */ > + > + /* Temporary implementation */ > + g_free(stk->rx_buffer.data.array); > + g_free(stk->tx_buffer.data.array); > + stk->rx_buffer.data.array = NULL; > + stk->tx_buffer.data.array = NULL; > + > + stk->channel.id = 0; > + stk->channel.status = STK_CHANNEL_PACKET_DATA_SERVICE_NOT_ACTIVATED; > + > + if (stk->pending_cmd && > + stk->pending_cmd->type == STK_COMMAND_TYPE_CLOSE_CHANNEL) doc/coding-style.txt M4 > + send_simple_response(stk, STK_RESULT_TYPE_SUCCESS); > + else { > + /* > + * TODO > + * Send Event channel status > + */ > + } > +} > + > static void user_termination_cb(enum stk_agent_result result, void *user_data) > { > struct ofono_stk *stk = user_data; > > - if (result == STK_AGENT_RESULT_TERMINATE) { > - stk->respond_on_exit = FALSE; > + if (result != STK_AGENT_RESULT_TERMINATE) > + return; > + > + if (stk->pending_cmd) { > send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); > + stk->cancel_cmd(stk); I don't think this will work out. send_simple_responses NULLs out cancel_cmd... I'm guessing you're trying to guarantee that send_dtmf is properly canceled when the agent sends an EndSession error... > } > + > + if (stk->channel.id) > + stk_close_channel(stk); > } > > static void stk_alpha_id_set(struct ofono_stk *stk, > @@ -511,6 +560,147 @@ static void stk_alpha_id_unset(struct ofono_stk *stk) > stk_agent_request_cancel(stk->current_agent); > } > > + > +static void stk_open_channel(struct ofono_stk *stk) > +{ > + const struct stk_command_open_channel *oc; > + struct stk_response rsp; > + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; > + > + if (stk->pending_cmd == NULL || > + stk->pending_cmd->type != STK_COMMAND_TYPE_OPEN_CHANNEL) > + return; > + > + oc = &stk->pending_cmd->open_channel; > + > + memset(&rsp, 0, sizeof(rsp)); > + rsp.result.type = STK_RESULT_TYPE_SUCCESS; > + > + stk->rx_buffer.data.array = g_try_malloc(oc->buf_size); > + if (stk->rx_buffer.data.array == NULL) { > + rsp.result.type = STK_RESULT_TYPE_NOT_CAPABLE; > + goto out; > + } > + > + stk->tx_buffer.data.array = g_try_malloc(oc->buf_size); > + if (stk->tx_buffer.data.array == NULL) { > + rsp.result.type = STK_RESULT_TYPE_NOT_CAPABLE; > + goto out; > + } > + > + stk->rx_buffer.data.len = oc->buf_size; > + stk->tx_buffer.data.len = oc->buf_size; > + stk->link_on_demand = (stk->pending_cmd->qualifier & > + STK_OPEN_CHANNEL_FLAG_IMMEDIATE) ? FALSE : TRUE; > + > + /* > + * TODO > + * Setup the channel-> either create a new PDP context or > + * use a default one > + * Send the Terminal Response or wait until the PDP context is activated > + * in case of immediate link establishment not in background. > + */ > +out: > + > + if (stk_respond(stk, &rsp, stk_command_cb)) > + stk_command_cb(&failure, stk); > +} > + > +static void stk_send_data(struct ofono_stk *stk, > + struct stk_common_byte_array data, > + unsigned char qualifier) > +{ > + struct stk_response rsp; > + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; > + unsigned int offset; > + > + memset(&rsp, 0, sizeof(rsp)); > + rsp.result.type = STK_RESULT_TYPE_SUCCESS; > + > + if (data.len > stk->tx_buffer.data.len) { > + rsp.result.type = STK_RESULT_TYPE_BIP_ERROR; > + goto out; > + } > + > + if (qualifier == STK_SEND_DATA_STORE_DATA) { > + > + /* > + * check if the requested number of bytes of empty space > + * is available > + */ > + if (data.len > stk->tx_buffer.tx_avail) { > + rsp.result.type = STK_RESULT_TYPE_BIP_ERROR; > + goto out; > + } > + > + offset = stk->tx_buffer.data.len - stk->tx_buffer.tx_avail; > + memcpy(stk->tx_buffer.data.array + offset, data.array, > + data.len); > + > + stk->tx_buffer.tx_avail -= data.len; > + rsp.send_data.tx_avail = stk->tx_buffer.tx_avail; > + goto out; > + } > + > + if (stk->channel.status == STK_CHANNEL_PACKET_DATA_SERVICE_NOT_ACTIVATED > + && stk->link_on_demand == TRUE) { > + /* > + * TODO > + * activate the context, update the channel status > + * once the context is activated, send the data immediately > + * and flush the tx buffer > + */ > + } else { > + /* > + * TODO > + * send the data immediately, flush the tx buffer > + */ > + stk->tx_buffer.tx_avail = stk->tx_buffer.data.len; > + rsp.send_data.tx_avail = stk->tx_buffer.data.len; > + } > + > +out: > + > + if (stk_respond(stk, &rsp, stk_command_cb)) > + stk_command_cb(&failure, stk); > +} > + > +static void stk_receive_data(struct ofono_stk *stk, unsigned char data_len) > +{ > + struct stk_response rsp; > + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; > + > + memset(&rsp, 0, sizeof(rsp)); > + rsp.result.type = STK_RESULT_TYPE_SUCCESS; > + > + if (stk->rx_buffer.rx_remaining == 0) { > + rsp.result.type = STK_RESULT_TYPE_MISSING_INFO; > + goto out; > + } > + > + if (data_len > stk->rx_buffer.rx_remaining) { > + rsp.result.type = STK_RESULT_TYPE_MISSING_INFO; > + data_len = stk->rx_buffer.rx_remaining; > + } > + > + if (data_len > CHANNEL_DATA_OBJECT_MAX_LENGTH) > + data_len = CHANNEL_DATA_OBJECT_MAX_LENGTH; > + > + rsp.receive_data.rx_data.array = stk->rx_buffer.data.array; > + rsp.receive_data.rx_data.len = data_len; > + stk->rx_buffer.rx_remaining -= data_len; > + rsp.receive_data.rx_remaining = stk->rx_buffer.rx_remaining; > + > +out: > + if (stk_respond(stk, &rsp, stk_command_cb)) > + stk_command_cb(&failure, stk); > + > + if (rsp.receive_data.rx_data.len && stk->rx_buffer.rx_remaining > 0) > + memmove(stk->rx_buffer.data.array, stk->rx_buffer.data.array + > + data_len, stk->rx_buffer.rx_remaining); > + > +} > + > static int duration_to_msecs(const struct stk_duration *duration) > { > int msecs = duration->interval; > @@ -598,7 +788,6 @@ static void default_agent_notify(gpointer user_data) > > stk->default_agent = NULL; > stk->current_agent = stk->session_agent; > - stk->respond_on_exit = FALSE; You really need to put these into a separate patch with a huge description of what you're trying to fix and why. respond_on_exit is pretty much tricky to get right, and really has to be reviewed independently from BIP command handlers... > } > > static void session_agent_notify(gpointer user_data) > @@ -617,7 +806,6 @@ static void session_agent_notify(gpointer user_data) > > stk->session_agent = NULL; > stk->current_agent = stk->default_agent; > - stk->respond_on_exit = FALSE; Same comment here > > if (stk->remove_agent_source) { > g_source_remove(stk->remove_agent_source); > @@ -1159,8 +1347,6 @@ static void request_selection_cb(enum stk_agent_result result, uint8_t id, > { > struct ofono_stk *stk = user_data; > > - stk->respond_on_exit = FALSE; > - And here > switch (result) { > case STK_AGENT_RESULT_OK: > { > @@ -1243,8 +1429,6 @@ static void display_text_cb(enum stk_agent_result result, void *user_data) > static unsigned char screen_busy_result[] = { 0x01 }; > static struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE }; > > - stk->respond_on_exit = FALSE; > - And here > /* > * There are four possible paths for DisplayText with immediate > * response flag set: > @@ -1386,8 +1570,6 @@ static void request_confirmation_cb(enum stk_agent_result result, > struct stk_command_get_inkey *cmd = &stk->pending_cmd->get_inkey; > struct stk_response rsp; > > - stk->respond_on_exit = FALSE; > - And here > switch (result) { > case STK_AGENT_RESULT_OK: > memset(&rsp, 0, sizeof(rsp)); > @@ -1442,8 +1624,6 @@ static void request_key_cb(enum stk_agent_result result, char *string, > struct stk_command_get_inkey *cmd = &stk->pending_cmd->get_inkey; > struct stk_response rsp; > > - stk->respond_on_exit = FALSE; > - And here > switch (result) { > case STK_AGENT_RESULT_OK: > memset(&rsp, 0, sizeof(rsp)); > @@ -1560,8 +1740,6 @@ static void request_string_cb(enum stk_agent_result result, char *string, > gboolean packed = (qualifier & (1 << 3)) != 0; > struct stk_response rsp; > > - stk->respond_on_exit = FALSE; > - > switch (result) { > case STK_AGENT_RESULT_OK: > memset(&rsp, 0, sizeof(rsp)); > @@ -1699,8 +1877,6 @@ static void confirm_call_cb(enum stk_agent_result result, gboolean confirm, > struct stk_response rsp; > int err; > > - stk->respond_on_exit = FALSE; > - > switch (result) { > case STK_AGENT_RESULT_TIMEOUT: > confirm = FALSE; > @@ -2300,8 +2476,6 @@ static void dtmf_sent_cb(int error, void *user_data) > { > struct ofono_stk *stk = user_data; > > - stk->respond_on_exit = FALSE; > - > stk_alpha_id_unset(stk); > > if (error == ENOENT) { > @@ -2413,8 +2587,6 @@ static void play_tone_cb(enum stk_agent_result result, void *user_data) > { > struct ofono_stk *stk = user_data; > > - stk->respond_on_exit = FALSE; > - > switch (result) { > case STK_AGENT_RESULT_OK: > case STK_AGENT_RESULT_TIMEOUT: > @@ -2547,8 +2719,6 @@ static void confirm_launch_browser_cb(enum stk_agent_result result, > struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; > struct stk_response rsp; > > - stk->respond_on_exit = FALSE; > - And... you get the idea :) > switch (result) { > case STK_AGENT_RESULT_TIMEOUT: > confirm = FALSE; > @@ -2612,6 +2782,273 @@ static gboolean handle_command_launch_browser(const struct stk_command *cmd, > return FALSE; > } > > + > +static void open_channel_cancel(struct ofono_stk *stk) > +{ > + /* TODO */ > +} > + > +static void confirm_open_channel_cb(enum stk_agent_result result, > + gboolean confirm, > + void *user_data) > +{ > + struct ofono_stk *stk = user_data; > + > + switch (result) { > + case STK_AGENT_RESULT_TERMINATE: > + send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); > + return; > + > + case STK_AGENT_RESULT_TIMEOUT: > + confirm = FALSE; > + /* Fall through */ > + > + case STK_AGENT_RESULT_OK: > + if (confirm) > + break; > + /* Fall through */ > + > + default: > + send_simple_response(stk, STK_RESULT_TYPE_TERMINAL_BUSY); > + return; > + } > + > + stk_open_channel(stk); > +} > + > +static gboolean handle_command_open_channel(const struct stk_command *cmd, > + struct stk_response *rsp, > + struct ofono_stk *stk) > +{ > + struct ofono_modem *modem = __ofono_atom_get_modem(stk->atom); > + const struct stk_command_open_channel *oc = &cmd->open_channel; > + struct ofono_atom *gprs_atom; > + int err; > + > + /* Check first if channel is available */ > + if (stk->channel.id) { > + unsigned char addnl_info[1]; > + > + addnl_info[0] = STK_RESULT_ADDNL_BIP_PB_NO_CHANNEL_AVAIL; > + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, > + addnl_info); > + return TRUE; > + } > + > + gprs_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_GPRS); > + if (gprs_atom == NULL || !__ofono_atom_get_registered(gprs_atom)) { > + rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE; > + return TRUE; > + } > + > + stk->respond_on_exit = TRUE; > + stk->cancel_cmd = open_channel_cancel; > + > + /* > + * Don't ask for user confirmation if AID is a null data object > + * or is not provided > + */ > + if (oc->alpha_id && strlen(oc->alpha_id) > 0) { > + char *alpha_id; > + > + alpha_id = dbus_apply_text_attributes(oc->alpha_id, > + &oc->text_attr); > + if (alpha_id == NULL) { > + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; > + return TRUE; > + } > + > + err = stk_agent_confirm_open_channel(stk->current_agent, > + alpha_id, > + &oc->icon_id, > + confirm_open_channel_cb, > + stk, NULL, > + stk->timeout * 1000); > + g_free(alpha_id); > + > + if (err < 0) { > + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; > + return TRUE; > + } > + > + return FALSE; > + } > + > + stk_open_channel(stk); > + > + return FALSE; > +} > + > +static gboolean handle_command_close_channel(const struct stk_command *cmd, > + struct stk_response *rsp, > + struct ofono_stk *stk) > +{ > + const struct stk_command_close_channel *cc = &cmd->close_channel; > + int err; > + > + /* Check if channel identifier is valid */ > + if (cmd->dst != stk->channel.id) { > + unsigned char addnl_info[1]; > + > + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; > + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, > + addnl_info); > + return TRUE; > + } > + > + /* > + * Don't inform the user about the link closing phase if AID is > + * a null data object or is not provided > + */ > + if (cc->alpha_id && strlen(cc->alpha_id) > 0) { > + char *alpha_id; > + > + alpha_id = dbus_apply_text_attributes(cc->alpha_id, > + &cc->text_attr); > + if (alpha_id == NULL) { > + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; > + return TRUE; > + } > + > + err = stk_agent_display_action(stk->current_agent, alpha_id, > + &cc->icon_id, > + user_termination_cb, > + stk, NULL); > + g_free(alpha_id); > + > + if (err < 0) { > + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; > + return TRUE; > + } This seems awfully close to what stk_alpha_id_set does. Any reason why you're not using it? ... > + } > + > + stk->respond_on_exit = TRUE; > + stk->cancel_cmd = stk_request_cancel; > + > + stk_close_channel(stk); > + > + return FALSE; > +} > + > +static gboolean handle_command_receive_data(const struct stk_command *cmd, > + struct stk_response *rsp, > + struct ofono_stk *stk) > +{ > + const struct stk_command_receive_data *rd = &cmd->receive_data; > + int err; > + > + /* Check if channel identifier is valid or already closed */ > + if (cmd->dst != stk->channel.id) { > + unsigned char addnl_info[1]; > + > + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; > + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, > + addnl_info); > + return TRUE; > + } > + > + /* > + * Don't inform the user during data transfer if AID is > + * a null data object or is not provided > + */ > + if (rd->alpha_id && strlen(rd->alpha_id) > 0) { > + char *alpha_id; > + > + alpha_id = dbus_apply_text_attributes(rd->alpha_id, > + &rd->text_attr); > + if (alpha_id == NULL) { > + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; > + return TRUE; > + } > + > + err = stk_agent_display_action(stk->current_agent, alpha_id, > + &rd->icon_id, > + user_termination_cb, > + stk, NULL); > + g_free(alpha_id); > + > + if (err < 0) { > + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; > + return TRUE; > + } ... Regardless, copy-pasting this much code is a bad idea. You should really use a function. > + } > + > + stk->respond_on_exit = TRUE; > + stk->cancel_cmd = stk_request_cancel; > + > + stk_receive_data(stk, rd->data_len); > + > + return FALSE; > +} > + > +static gboolean handle_command_send_data(const struct stk_command *cmd, > + struct stk_response *rsp, > + struct ofono_stk *stk) > +{ > + const struct stk_command_send_data *sd = &cmd->send_data; > + int err; > + unsigned char addnl_info[1]; > + > + /* Check if channel identifier is valid or already closed */ > + if (cmd->dst != stk->channel.id) { > + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; > + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, > + addnl_info); > + return TRUE; > + } > + > + /* Check if the link was dropped */ > + if (stk->channel.status == STK_CHANNEL_LINK_DROPPED) { > + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_CLOSED; > + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, > + addnl_info); > + return TRUE; > + } > + > + /* > + * Don't inform the user during data transfer if AID is > + * a null data object or is not provided > + */ > + if (sd->alpha_id && strlen(sd->alpha_id) > 0) { > + char *alpha_id; > + > + alpha_id = dbus_apply_text_attributes(sd->alpha_id, > + &sd->text_attr); > + if (alpha_id == NULL) { > + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; > + return TRUE; > + } > + > + err = stk_agent_display_action(stk->current_agent, alpha_id, > + &sd->icon_id, > + user_termination_cb, > + stk, NULL); > + g_free(alpha_id); > + > + if (err < 0) { > + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; > + return TRUE; > + } > + } > + > + stk->respond_on_exit = TRUE; > + stk->cancel_cmd = stk_request_cancel; > + > + stk_send_data(stk, sd->data, cmd->qualifier); > + > + return FALSE; > +} > + > +static gboolean handle_command_get_channel_status(const struct stk_command *cmd, > + struct stk_response *rsp, > + struct ofono_stk *stk) > +{ > + rsp->result.type = STK_RESULT_TYPE_SUCCESS; > + rsp->channel_status.channel.id = stk->channel.id; > + rsp->channel_status.channel.status = stk->channel.status; > + return TRUE; > +} > + > static void stk_proactive_command_cancel(struct ofono_stk *stk) > { > if (stk->immediate_response) > @@ -2805,6 +3242,31 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk, > &rsp, stk); > break; > > + case STK_COMMAND_TYPE_OPEN_CHANNEL: > + respond = handle_command_open_channel(stk->pending_cmd, > + &rsp, stk); > + break; > + > + case STK_COMMAND_TYPE_CLOSE_CHANNEL: > + respond = handle_command_close_channel(stk->pending_cmd, > + &rsp, stk); > + break; > + > + case STK_COMMAND_TYPE_RECEIVE_DATA: > + respond = handle_command_receive_data(stk->pending_cmd, > + &rsp, stk); > + break; > + > + case STK_COMMAND_TYPE_SEND_DATA: > + respond = handle_command_send_data(stk->pending_cmd, > + &rsp, stk); > + break; > + > + case STK_COMMAND_TYPE_GET_CHANNEL_STATUS: > + respond = handle_command_get_channel_status(stk->pending_cmd, > + &rsp, stk); > + break; > + > default: > rsp.result.type = STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD; > break; Regards, -Denis ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 6/6] stk: Introduce BIP command handlers 2011-04-12 5:19 ` Denis Kenzior @ 2011-04-13 14:02 ` Philippe Nunes 2011-04-13 14:37 ` Denis Kenzior 0 siblings, 1 reply; 5+ messages in thread From: Philippe Nunes @ 2011-04-13 14:02 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 23812 bytes --] Hi Denis, On 04/12/2011 07:19 AM, Denis Kenzior wrote: > Hi Philippe, > > On 04/08/2011 11:33 AM, Philippe Nunes wrote: >> --- >> src/stk.c | 506 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--- >> 1 files changed, 484 insertions(+), 22 deletions(-) >> >> diff --git a/src/stk.c b/src/stk.c >> index c86cbfb..e033df7 100644 >> --- a/src/stk.c >> +++ b/src/stk.c >> @@ -79,6 +79,10 @@ struct ofono_stk { >> >> __ofono_sms_sim_download_cb_t sms_pp_cb; >> void *sms_pp_userdata; >> + struct stk_channel channel; >> + struct stk_channel_data rx_buffer; >> + struct stk_channel_data tx_buffer; >> + gboolean link_on_demand; >> }; >> >> struct envelope_op { >> @@ -104,6 +108,13 @@ static void timers_update(struct ofono_stk *stk); >> result.additional_len = sizeof(addn_info); \ >> result.additional = addn_info; \ >> >> +/* >> + * According the structure and coding of the Terminal response defined in >> + * TS 102 223 section 6.8, the maximum number of bytes possible for the channel >> + * data object is 243 >> + */ >> +#define CHANNEL_DATA_OBJECT_MAX_LENGTH 243 >> + >> static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, >> ofono_stk_generic_cb_t cb) >> { >> @@ -112,6 +123,9 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, >> >> DBG(""); >> >> + if (stk->pending_cmd == NULL) >> + return 0; >> + > > This seems fishy, why do you need this? Is this a bug you discovered or > a behavior change necessitated by BIP support? No, in fact, just looking to the code below (session_agent_notify), I found logical and more safe to add also this check in the function stk_respond: if (stk->current_agent == stk->session_agent && stk->respond_on_exit) { if (stk->pending_cmd) stk->cancel_cmd(stk); DBG("Sending Terminate response for session agent"); send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); } Now, I agree that if the flag 'respond_on_exit' is TRUE, it requires necessarily a pending command especially with the modification I did below (since the flag is set back to False just after the pending_cmd is set to NULL). So, I can forget this check but logically we could remove also the check before calling stk->cancel_cmd. > >> if (stk->driver->terminal_response == NULL) >> return -ENOSYS; >> >> @@ -128,6 +142,7 @@ static int stk_respond(struct ofono_stk *stk, struct stk_response *rsp, >> stk_command_free(stk->pending_cmd); >> stk->pending_cmd = NULL; >> stk->cancel_cmd = NULL; >> + stk->respond_on_exit = FALSE; > > Same comment as above here > >> >> stk->driver->terminal_response(stk, tlv_len, tlv, cb, stk); >> >> @@ -473,14 +488,48 @@ static void emit_menu_changed(struct ofono_stk *stk) >> g_dbus_send_message(conn, signal); >> } >> >> +static void stk_close_channel(struct ofono_stk *stk) >> +{ >> + /* >> + * TODO >> + * Deactivate PDP context >> + * Send the Terminal Response once the PDP context is deactivated >> + */ >> + >> + /* Temporary implementation */ >> + g_free(stk->rx_buffer.data.array); >> + g_free(stk->tx_buffer.data.array); >> + stk->rx_buffer.data.array = NULL; >> + stk->tx_buffer.data.array = NULL; >> + >> + stk->channel.id = 0; >> + stk->channel.status = STK_CHANNEL_PACKET_DATA_SERVICE_NOT_ACTIVATED; >> + >> + if (stk->pending_cmd&& >> + stk->pending_cmd->type == STK_COMMAND_TYPE_CLOSE_CHANNEL) > > doc/coding-style.txt M4 In this case, as it is not possible to indent the second line condition, should I indent one more the body? > >> + send_simple_response(stk, STK_RESULT_TYPE_SUCCESS); >> + else { >> + /* >> + * TODO >> + * Send Event channel status >> + */ >> + } >> +} >> + >> static void user_termination_cb(enum stk_agent_result result, void *user_data) >> { >> struct ofono_stk *stk = user_data; >> >> - if (result == STK_AGENT_RESULT_TERMINATE) { >> - stk->respond_on_exit = FALSE; >> + if (result != STK_AGENT_RESULT_TERMINATE) >> + return; >> + >> + if (stk->pending_cmd) { >> send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); >> + stk->cancel_cmd(stk); > > I don't think this will work out. send_simple_responses NULLs out > cancel_cmd... > > I'm guessing you're trying to guarantee that send_dtmf is properly > canceled when the agent sends an EndSession error... yes, that was the idea. I need to call the cancel function before sending the response... > >> } >> + >> + if (stk->channel.id) >> + stk_close_channel(stk); >> } >> >> static void stk_alpha_id_set(struct ofono_stk *stk, >> @@ -511,6 +560,147 @@ static void stk_alpha_id_unset(struct ofono_stk *stk) >> stk_agent_request_cancel(stk->current_agent); >> } >> >> + >> +static void stk_open_channel(struct ofono_stk *stk) >> +{ >> + const struct stk_command_open_channel *oc; >> + struct stk_response rsp; >> + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; >> + >> + if (stk->pending_cmd == NULL || >> + stk->pending_cmd->type != STK_COMMAND_TYPE_OPEN_CHANNEL) >> + return; >> + >> + oc =&stk->pending_cmd->open_channel; >> + >> + memset(&rsp, 0, sizeof(rsp)); >> + rsp.result.type = STK_RESULT_TYPE_SUCCESS; >> + >> + stk->rx_buffer.data.array = g_try_malloc(oc->buf_size); >> + if (stk->rx_buffer.data.array == NULL) { >> + rsp.result.type = STK_RESULT_TYPE_NOT_CAPABLE; >> + goto out; >> + } >> + >> + stk->tx_buffer.data.array = g_try_malloc(oc->buf_size); >> + if (stk->tx_buffer.data.array == NULL) { >> + rsp.result.type = STK_RESULT_TYPE_NOT_CAPABLE; >> + goto out; >> + } >> + >> + stk->rx_buffer.data.len = oc->buf_size; >> + stk->tx_buffer.data.len = oc->buf_size; >> + stk->link_on_demand = (stk->pending_cmd->qualifier& >> + STK_OPEN_CHANNEL_FLAG_IMMEDIATE) ? FALSE : TRUE; >> + >> + /* >> + * TODO >> + * Setup the channel-> either create a new PDP context or >> + * use a default one >> + * Send the Terminal Response or wait until the PDP context is activated >> + * in case of immediate link establishment not in background. >> + */ >> +out: >> + >> + if (stk_respond(stk,&rsp, stk_command_cb)) >> + stk_command_cb(&failure, stk); >> +} >> + >> +static void stk_send_data(struct ofono_stk *stk, >> + struct stk_common_byte_array data, >> + unsigned char qualifier) >> +{ >> + struct stk_response rsp; >> + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; >> + unsigned int offset; >> + >> + memset(&rsp, 0, sizeof(rsp)); >> + rsp.result.type = STK_RESULT_TYPE_SUCCESS; >> + >> + if (data.len> stk->tx_buffer.data.len) { >> + rsp.result.type = STK_RESULT_TYPE_BIP_ERROR; >> + goto out; >> + } >> + >> + if (qualifier == STK_SEND_DATA_STORE_DATA) { >> + >> + /* >> + * check if the requested number of bytes of empty space >> + * is available >> + */ >> + if (data.len> stk->tx_buffer.tx_avail) { >> + rsp.result.type = STK_RESULT_TYPE_BIP_ERROR; >> + goto out; >> + } >> + >> + offset = stk->tx_buffer.data.len - stk->tx_buffer.tx_avail; >> + memcpy(stk->tx_buffer.data.array + offset, data.array, >> + data.len); >> + >> + stk->tx_buffer.tx_avail -= data.len; >> + rsp.send_data.tx_avail = stk->tx_buffer.tx_avail; >> + goto out; >> + } >> + >> + if (stk->channel.status == STK_CHANNEL_PACKET_DATA_SERVICE_NOT_ACTIVATED >> + && stk->link_on_demand == TRUE) { >> + /* >> + * TODO >> + * activate the context, update the channel status >> + * once the context is activated, send the data immediately >> + * and flush the tx buffer >> + */ >> + } else { >> + /* >> + * TODO >> + * send the data immediately, flush the tx buffer >> + */ >> + stk->tx_buffer.tx_avail = stk->tx_buffer.data.len; >> + rsp.send_data.tx_avail = stk->tx_buffer.data.len; >> + } >> + >> +out: >> + >> + if (stk_respond(stk,&rsp, stk_command_cb)) >> + stk_command_cb(&failure, stk); >> +} >> + >> +static void stk_receive_data(struct ofono_stk *stk, unsigned char data_len) >> +{ >> + struct stk_response rsp; >> + struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; >> + >> + memset(&rsp, 0, sizeof(rsp)); >> + rsp.result.type = STK_RESULT_TYPE_SUCCESS; >> + >> + if (stk->rx_buffer.rx_remaining == 0) { >> + rsp.result.type = STK_RESULT_TYPE_MISSING_INFO; >> + goto out; >> + } >> + >> + if (data_len> stk->rx_buffer.rx_remaining) { >> + rsp.result.type = STK_RESULT_TYPE_MISSING_INFO; >> + data_len = stk->rx_buffer.rx_remaining; >> + } >> + >> + if (data_len> CHANNEL_DATA_OBJECT_MAX_LENGTH) >> + data_len = CHANNEL_DATA_OBJECT_MAX_LENGTH; >> + >> + rsp.receive_data.rx_data.array = stk->rx_buffer.data.array; >> + rsp.receive_data.rx_data.len = data_len; >> + stk->rx_buffer.rx_remaining -= data_len; >> + rsp.receive_data.rx_remaining = stk->rx_buffer.rx_remaining; >> + >> +out: >> + if (stk_respond(stk,&rsp, stk_command_cb)) >> + stk_command_cb(&failure, stk); >> + >> + if (rsp.receive_data.rx_data.len&& stk->rx_buffer.rx_remaining> 0) >> + memmove(stk->rx_buffer.data.array, stk->rx_buffer.data.array + >> + data_len, stk->rx_buffer.rx_remaining); >> + >> +} >> + >> static int duration_to_msecs(const struct stk_duration *duration) >> { >> int msecs = duration->interval; >> @@ -598,7 +788,6 @@ static void default_agent_notify(gpointer user_data) >> >> stk->default_agent = NULL; >> stk->current_agent = stk->session_agent; >> - stk->respond_on_exit = FALSE; > > You really need to put these into a separate patch with a huge > description of what you're trying to fix and why. respond_on_exit is > pretty much tricky to get right, and really has to be reviewed > independently from BIP command handlers... Indeed, in practice, this modification is coming from BIP handler implementation, but this is uncorrelated from BIP. Here, the idea of this modification is to update the flag only once the Terminal response has been sent. My understanding of this flag is to allow to send a Terminal response "Proactive UICC session terminated by the user" once the stk agent exits (user quits). But this case still subsists even after the user confirms to setup a voice call or to setup a BIP channel. For such cases, we need to establish the link before returning the terminal response, which can take a while. Actually, in the callback 'confirm_call_cb', the flag stk->respond_on_exit is set back to FALSE whereas the Terminal response is not already sent. Fact is that the user may exit the stk agent before the call is connected. In this case, no terminal response is sent, leading to a potential mismatch regarding the expected answer from SIM. So I found more safe and logical to reinitialise this flag only in the function stk_respond after the terminal response is sent. If you agree with this approach, I'm willing to submit a dedicated patch for that. > >> } >> >> static void session_agent_notify(gpointer user_data) >> @@ -617,7 +806,6 @@ static void session_agent_notify(gpointer user_data) >> >> stk->session_agent = NULL; >> stk->current_agent = stk->default_agent; >> - stk->respond_on_exit = FALSE; > > Same comment here > >> >> if (stk->remove_agent_source) { >> g_source_remove(stk->remove_agent_source); >> @@ -1159,8 +1347,6 @@ static void request_selection_cb(enum stk_agent_result result, uint8_t id, >> { >> struct ofono_stk *stk = user_data; >> >> - stk->respond_on_exit = FALSE; >> - > > And here > >> switch (result) { >> case STK_AGENT_RESULT_OK: >> { >> @@ -1243,8 +1429,6 @@ static void display_text_cb(enum stk_agent_result result, void *user_data) >> static unsigned char screen_busy_result[] = { 0x01 }; >> static struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE }; >> >> - stk->respond_on_exit = FALSE; >> - > > And here > >> /* >> * There are four possible paths for DisplayText with immediate >> * response flag set: >> @@ -1386,8 +1570,6 @@ static void request_confirmation_cb(enum stk_agent_result result, >> struct stk_command_get_inkey *cmd =&stk->pending_cmd->get_inkey; >> struct stk_response rsp; >> >> - stk->respond_on_exit = FALSE; >> - > > And here > >> switch (result) { >> case STK_AGENT_RESULT_OK: >> memset(&rsp, 0, sizeof(rsp)); >> @@ -1442,8 +1624,6 @@ static void request_key_cb(enum stk_agent_result result, char *string, >> struct stk_command_get_inkey *cmd =&stk->pending_cmd->get_inkey; >> struct stk_response rsp; >> >> - stk->respond_on_exit = FALSE; >> - > > And here > >> switch (result) { >> case STK_AGENT_RESULT_OK: >> memset(&rsp, 0, sizeof(rsp)); >> @@ -1560,8 +1740,6 @@ static void request_string_cb(enum stk_agent_result result, char *string, >> gboolean packed = (qualifier& (1<< 3)) != 0; >> struct stk_response rsp; >> >> - stk->respond_on_exit = FALSE; >> - >> switch (result) { >> case STK_AGENT_RESULT_OK: >> memset(&rsp, 0, sizeof(rsp)); >> @@ -1699,8 +1877,6 @@ static void confirm_call_cb(enum stk_agent_result result, gboolean confirm, >> struct stk_response rsp; >> int err; >> >> - stk->respond_on_exit = FALSE; >> - >> switch (result) { >> case STK_AGENT_RESULT_TIMEOUT: >> confirm = FALSE; >> @@ -2300,8 +2476,6 @@ static void dtmf_sent_cb(int error, void *user_data) >> { >> struct ofono_stk *stk = user_data; >> >> - stk->respond_on_exit = FALSE; >> - >> stk_alpha_id_unset(stk); >> >> if (error == ENOENT) { >> @@ -2413,8 +2587,6 @@ static void play_tone_cb(enum stk_agent_result result, void *user_data) >> { >> struct ofono_stk *stk = user_data; >> >> - stk->respond_on_exit = FALSE; >> - >> switch (result) { >> case STK_AGENT_RESULT_OK: >> case STK_AGENT_RESULT_TIMEOUT: >> @@ -2547,8 +2719,6 @@ static void confirm_launch_browser_cb(enum stk_agent_result result, >> struct ofono_error failure = { .type = OFONO_ERROR_TYPE_FAILURE }; >> struct stk_response rsp; >> >> - stk->respond_on_exit = FALSE; >> - > > And... you get the idea :) > >> switch (result) { >> case STK_AGENT_RESULT_TIMEOUT: >> confirm = FALSE; >> @@ -2612,6 +2782,273 @@ static gboolean handle_command_launch_browser(const struct stk_command *cmd, >> return FALSE; >> } >> >> + >> +static void open_channel_cancel(struct ofono_stk *stk) >> +{ >> + /* TODO */ >> +} >> + >> +static void confirm_open_channel_cb(enum stk_agent_result result, >> + gboolean confirm, >> + void *user_data) >> +{ >> + struct ofono_stk *stk = user_data; >> + >> + switch (result) { >> + case STK_AGENT_RESULT_TERMINATE: >> + send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); >> + return; >> + >> + case STK_AGENT_RESULT_TIMEOUT: >> + confirm = FALSE; >> + /* Fall through */ >> + >> + case STK_AGENT_RESULT_OK: >> + if (confirm) >> + break; >> + /* Fall through */ >> + >> + default: >> + send_simple_response(stk, STK_RESULT_TYPE_TERMINAL_BUSY); >> + return; >> + } >> + >> + stk_open_channel(stk); >> +} >> + >> +static gboolean handle_command_open_channel(const struct stk_command *cmd, >> + struct stk_response *rsp, >> + struct ofono_stk *stk) >> +{ >> + struct ofono_modem *modem = __ofono_atom_get_modem(stk->atom); >> + const struct stk_command_open_channel *oc =&cmd->open_channel; >> + struct ofono_atom *gprs_atom; >> + int err; >> + >> + /* Check first if channel is available */ >> + if (stk->channel.id) { >> + unsigned char addnl_info[1]; >> + >> + addnl_info[0] = STK_RESULT_ADDNL_BIP_PB_NO_CHANNEL_AVAIL; >> + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, >> + addnl_info); >> + return TRUE; >> + } >> + >> + gprs_atom = __ofono_modem_find_atom(modem, OFONO_ATOM_TYPE_GPRS); >> + if (gprs_atom == NULL || !__ofono_atom_get_registered(gprs_atom)) { >> + rsp->result.type = STK_RESULT_TYPE_NOT_CAPABLE; >> + return TRUE; >> + } >> + >> + stk->respond_on_exit = TRUE; >> + stk->cancel_cmd = open_channel_cancel; >> + >> + /* >> + * Don't ask for user confirmation if AID is a null data object >> + * or is not provided >> + */ >> + if (oc->alpha_id&& strlen(oc->alpha_id)> 0) { >> + char *alpha_id; >> + >> + alpha_id = dbus_apply_text_attributes(oc->alpha_id, >> + &oc->text_attr); >> + if (alpha_id == NULL) { >> + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; >> + return TRUE; >> + } >> + >> + err = stk_agent_confirm_open_channel(stk->current_agent, >> + alpha_id, >> + &oc->icon_id, >> + confirm_open_channel_cb, >> + stk, NULL, >> + stk->timeout * 1000); >> + g_free(alpha_id); >> + >> + if (err< 0) { >> + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; >> + return TRUE; >> + } >> + >> + return FALSE; >> + } >> + >> + stk_open_channel(stk); >> + >> + return FALSE; >> +} >> + >> +static gboolean handle_command_close_channel(const struct stk_command *cmd, >> + struct stk_response *rsp, >> + struct ofono_stk *stk) >> +{ >> + const struct stk_command_close_channel *cc =&cmd->close_channel; >> + int err; >> + >> + /* Check if channel identifier is valid */ >> + if (cmd->dst != stk->channel.id) { >> + unsigned char addnl_info[1]; >> + >> + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; >> + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, >> + addnl_info); >> + return TRUE; >> + } >> + >> + /* >> + * Don't inform the user about the link closing phase if AID is >> + * a null data object or is not provided >> + */ >> + if (cc->alpha_id&& strlen(cc->alpha_id)> 0) { >> + char *alpha_id; >> + >> + alpha_id = dbus_apply_text_attributes(cc->alpha_id, >> + &cc->text_attr); >> + if (alpha_id == NULL) { >> + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; >> + return TRUE; >> + } >> + >> + err = stk_agent_display_action(stk->current_agent, alpha_id, >> + &cc->icon_id, >> + user_termination_cb, >> + stk, NULL); >> + g_free(alpha_id); >> + >> + if (err< 0) { >> + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; >> + return TRUE; >> + } > > This seems awfully close to what stk_alpha_id_set does. Any reason why > you're not using it? ... Not really, I can indeed refer to stk_alpha_id_set to apply the text attributes and call properly stk_agent_display_action. > >> + } >> + >> + stk->respond_on_exit = TRUE; >> + stk->cancel_cmd = stk_request_cancel; >> + >> + stk_close_channel(stk); >> + >> + return FALSE; >> +} >> + >> +static gboolean handle_command_receive_data(const struct stk_command *cmd, >> + struct stk_response *rsp, >> + struct ofono_stk *stk) >> +{ >> + const struct stk_command_receive_data *rd =&cmd->receive_data; >> + int err; >> + >> + /* Check if channel identifier is valid or already closed */ >> + if (cmd->dst != stk->channel.id) { >> + unsigned char addnl_info[1]; >> + >> + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; >> + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, >> + addnl_info); >> + return TRUE; >> + } >> + >> + /* >> + * Don't inform the user during data transfer if AID is >> + * a null data object or is not provided >> + */ >> + if (rd->alpha_id&& strlen(rd->alpha_id)> 0) { >> + char *alpha_id; >> + >> + alpha_id = dbus_apply_text_attributes(rd->alpha_id, >> + &rd->text_attr); >> + if (alpha_id == NULL) { >> + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; >> + return TRUE; >> + } >> + >> + err = stk_agent_display_action(stk->current_agent, alpha_id, >> + &rd->icon_id, >> + user_termination_cb, >> + stk, NULL); >> + g_free(alpha_id); >> + >> + if (err< 0) { >> + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; >> + return TRUE; >> + } > > ... Regardless, copy-pasting this much code is a bad idea. You should > really use a function. yes, you're right. > >> + } >> + >> + stk->respond_on_exit = TRUE; >> + stk->cancel_cmd = stk_request_cancel; >> + >> + stk_receive_data(stk, rd->data_len); >> + >> + return FALSE; >> +} >> + >> +static gboolean handle_command_send_data(const struct stk_command *cmd, >> + struct stk_response *rsp, >> + struct ofono_stk *stk) >> +{ >> + const struct stk_command_send_data *sd =&cmd->send_data; >> + int err; >> + unsigned char addnl_info[1]; >> + >> + /* Check if channel identifier is valid or already closed */ >> + if (cmd->dst != stk->channel.id) { >> + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_ID_NOT_VALID; >> + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, >> + addnl_info); >> + return TRUE; >> + } >> + >> + /* Check if the link was dropped */ >> + if (stk->channel.status == STK_CHANNEL_LINK_DROPPED) { >> + addnl_info[1] = STK_RESULT_ADDNL_BIP_PB_CHANNEL_CLOSED; >> + ADD_ERROR_RESULT(rsp->result, STK_RESULT_TYPE_BIP_ERROR, >> + addnl_info); >> + return TRUE; >> + } >> + >> + /* >> + * Don't inform the user during data transfer if AID is >> + * a null data object or is not provided >> + */ >> + if (sd->alpha_id&& strlen(sd->alpha_id)> 0) { >> + char *alpha_id; >> + >> + alpha_id = dbus_apply_text_attributes(sd->alpha_id, >> + &sd->text_attr); >> + if (alpha_id == NULL) { >> + rsp->result.type = STK_RESULT_TYPE_DATA_NOT_UNDERSTOOD; >> + return TRUE; >> + } >> + >> + err = stk_agent_display_action(stk->current_agent, alpha_id, >> + &sd->icon_id, >> + user_termination_cb, >> + stk, NULL); >> + g_free(alpha_id); >> + >> + if (err< 0) { >> + rsp->result.type = STK_RESULT_TYPE_TERMINAL_BUSY; >> + return TRUE; >> + } >> + } >> + >> + stk->respond_on_exit = TRUE; >> + stk->cancel_cmd = stk_request_cancel; >> + >> + stk_send_data(stk, sd->data, cmd->qualifier); >> + >> + return FALSE; >> +} >> + >> +static gboolean handle_command_get_channel_status(const struct stk_command *cmd, >> + struct stk_response *rsp, >> + struct ofono_stk *stk) >> +{ >> + rsp->result.type = STK_RESULT_TYPE_SUCCESS; >> + rsp->channel_status.channel.id = stk->channel.id; >> + rsp->channel_status.channel.status = stk->channel.status; >> + return TRUE; >> +} >> + >> static void stk_proactive_command_cancel(struct ofono_stk *stk) >> { >> if (stk->immediate_response) >> @@ -2805,6 +3242,31 @@ void ofono_stk_proactive_command_notify(struct ofono_stk *stk, >> &rsp, stk); >> break; >> >> + case STK_COMMAND_TYPE_OPEN_CHANNEL: >> + respond = handle_command_open_channel(stk->pending_cmd, >> + &rsp, stk); >> + break; >> + >> + case STK_COMMAND_TYPE_CLOSE_CHANNEL: >> + respond = handle_command_close_channel(stk->pending_cmd, >> + &rsp, stk); >> + break; >> + >> + case STK_COMMAND_TYPE_RECEIVE_DATA: >> + respond = handle_command_receive_data(stk->pending_cmd, >> + &rsp, stk); >> + break; >> + >> + case STK_COMMAND_TYPE_SEND_DATA: >> + respond = handle_command_send_data(stk->pending_cmd, >> + &rsp, stk); >> + break; >> + >> + case STK_COMMAND_TYPE_GET_CHANNEL_STATUS: >> + respond = handle_command_get_channel_status(stk->pending_cmd, >> + &rsp, stk); >> + break; >> + >> default: >> rsp.result.type = STK_RESULT_TYPE_COMMAND_NOT_UNDERSTOOD; >> break; > > Regards, > -Denis > Regards, Philippe. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 6/6] stk: Introduce BIP command handlers 2011-04-13 14:02 ` Philippe Nunes @ 2011-04-13 14:37 ` Denis Kenzior 2011-04-18 23:56 ` Andrzej Zaborowski 0 siblings, 1 reply; 5+ messages in thread From: Denis Kenzior @ 2011-04-13 14:37 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 4928 bytes --] Hi Philippe, >>> @@ -112,6 +123,9 @@ static int stk_respond(struct ofono_stk *stk, >>> struct stk_response *rsp, >>> >>> DBG(""); >>> >>> + if (stk->pending_cmd == NULL) >>> + return 0; >>> + >> >> This seems fishy, why do you need this? Is this a bug you discovered or >> a behavior change necessitated by BIP support? > > No, in fact, just looking to the code below (session_agent_notify), I > found logical and more safe to add also this check in the function > stk_respond: > > if (stk->current_agent == stk->session_agent && stk->respond_on_exit) { > if (stk->pending_cmd) > stk->cancel_cmd(stk); > > DBG("Sending Terminate response for session agent"); > send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); > } > > Now, I agree that if the flag 'respond_on_exit' is TRUE, it requires > necessarily a pending command especially with the modification I did > below (since the flag is set back to False just after the pending_cmd is > set to NULL). > So, I can forget this check but logically we could remove also the check > before calling stk->cancel_cmd. Briefly looking at it the if (stk->pending_cmd) does indeed look to be redundant. Feel free to send such a patch, but Andrew has to take a look as well. >>> + >>> + if (stk->pending_cmd&& >>> + stk->pending_cmd->type == STK_COMMAND_TYPE_CLOSE_CHANNEL) >> >> doc/coding-style.txt M4 > > > In this case, as it is not possible to indent the second line condition, > should I indent one more the body? No, indenting the body is a bad idea. You will have to get creative, e.g. breaking up the == over two lines. >>> static void user_termination_cb(enum stk_agent_result result, void >>> *user_data) >>> { >>> struct ofono_stk *stk = user_data; >>> >>> - if (result == STK_AGENT_RESULT_TERMINATE) { >>> - stk->respond_on_exit = FALSE; >>> + if (result != STK_AGENT_RESULT_TERMINATE) >>> + return; >>> + >>> + if (stk->pending_cmd) { >>> send_simple_response(stk, STK_RESULT_TYPE_USER_TERMINATED); >>> + stk->cancel_cmd(stk); >> >> I don't think this will work out. send_simple_responses NULLs out >> cancel_cmd... >> >> I'm guessing you're trying to guarantee that send_dtmf is properly >> canceled when the agent sends an EndSession error... > > yes, that was the idea. I need to call the cancel function before > sending the response... > You will probably need a dedicated callback for this case. >>> static int duration_to_msecs(const struct stk_duration *duration) >>> { >>> int msecs = duration->interval; >>> @@ -598,7 +788,6 @@ static void default_agent_notify(gpointer user_data) >>> >>> stk->default_agent = NULL; >>> stk->current_agent = stk->session_agent; >>> - stk->respond_on_exit = FALSE; >> >> You really need to put these into a separate patch with a huge >> description of what you're trying to fix and why. respond_on_exit is >> pretty much tricky to get right, and really has to be reviewed >> independently from BIP command handlers... > > Indeed, in practice, this modification is coming from BIP handler > implementation, but this is uncorrelated from BIP. > Here, the idea of this modification is to update the flag only once the > Terminal response has been sent. > My understanding of this flag is to allow to send a Terminal response > "Proactive UICC session terminated by the user" once the stk agent exits > (user quits). Yep, that is correct. > > But this case still subsists even after the user confirms to setup a > voice call or to setup a BIP channel. For such cases, we need to > establish the link before returning the terminal response, which can > take a while. > > Actually, in the callback 'confirm_call_cb', the flag > stk->respond_on_exit is set back to FALSE whereas the Terminal response > is not already sent. Fact is that the user may exit the stk agent before > the call is connected. In this case, no terminal response is sent, > leading to a potential mismatch regarding the expected answer from SIM. This is a good point. Andrew's idea was that once the Dial has been sent, the EndSesion can be triggered by hanging up the call and tracking the session agent is no longer needed (as Setup Call is usually the end of a session anyway) I don't mind triggering EndSession via the Agent exiting as well in this case. > > So I found more safe and logical to reinitialise this flag only in the > function stk_respond after the terminal response is sent. > > If you agree with this approach, I'm willing to submit a dedicated patch > for that. > Sure, it does seem that respond_on_exit clearing can be simplified somewhat. However, it was hard to judge when it was mixed with 500 lines of additional changes ;) Regards, -Denis ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 6/6] stk: Introduce BIP command handlers 2011-04-13 14:37 ` Denis Kenzior @ 2011-04-18 23:56 ` Andrzej Zaborowski 0 siblings, 0 replies; 5+ messages in thread From: Andrzej Zaborowski @ 2011-04-18 23:56 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1739 bytes --] Hi, On 13 April 2011 16:37, Denis Kenzior <denkenz@gmail.com> wrote: ... >> Indeed, in practice, this modification is coming from BIP handler >> implementation, but this is uncorrelated from BIP. >> Here, the idea of this modification is to update the flag only once the >> Terminal response has been sent. >> My understanding of this flag is to allow to send a Terminal response >> "Proactive UICC session terminated by the user" once the stk agent exits >> (user quits). > > Yep, that is correct. > >> >> But this case still subsists even after the user confirms to setup a >> voice call or to setup a BIP channel. For such cases, we need to >> establish the link before returning the terminal response, which can >> take a while. >> >> Actually, in the callback 'confirm_call_cb', the flag >> stk->respond_on_exit is set back to FALSE whereas the Terminal response >> is not already sent. Fact is that the user may exit the stk agent before >> the call is connected. In this case, no terminal response is sent, >> leading to a potential mismatch regarding the expected answer from SIM. I can't see the mismatch happening, ofono always sends the response sooner or later. > > This is a good point. Andrew's idea was that once the Dial has been > sent, the EndSesion can be triggered by hanging up the call and tracking > the session agent is no longer needed (as Setup Call is usually the end > of a session anyway) > > I don't mind triggering EndSession via the Agent exiting as well in this > case. Ah, I see that how that might work. Although the spec only mentions the "session terminated by the user" response in the "confirmation phase" and not in the "setup phase". Best regards ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-04-18 23:56 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-04-08 16:33 [PATCH 6/6] stk: Introduce BIP command handlers Philippe Nunes 2011-04-12 5:19 ` Denis Kenzior 2011-04-13 14:02 ` Philippe Nunes 2011-04-13 14:37 ` Denis Kenzior 2011-04-18 23:56 ` Andrzej Zaborowski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.