On 22 June 2010 23:42, Denis Kenzior <[email protected]> wrote:
> Hi Andrew,
>
>> ---
>> src/stk.c | 144
>> +++++++++++++++++++++++++++++++++++++++++++++++++++++++------ 1
> files
>> changed, 131 insertions(+), 13 deletions(-)
>>
>> diff --git a/src/stk.c b/src/stk.c
>> index f472a63..1dd5b77 100644
>> --- a/src/stk.c
>> +++ b/src/stk.c
>> @@ -40,14 +40,85 @@
>>
>> static GSList *g_drivers = NULL;
>>
>> +typedef void (*generic_cb_t)(const struct ofono_error *error,
>> + struct ofono_stk *stk);
>> +
>> +typedef void (*envelope_cb_t)(const struct ofono_error *error,
>> + const unsigned char *rdata,
>> + int length, struct ofono_stk *stk);
>
> User data?
Changed to just "data", as this is "response data".
>
>> +
>> +typedef gboolean (*command_handler_t)(const struct stk_command *cmd,
>> + struct stk_response *rsp,
>> + struct ofono_stk *stk);
>
> I think here we must have user data, especially if you're planning to use it
> outside stk (e.g. in voicecall atom). Any reason why *stk can't simply be
> void *?
Well, these types are just copies of the types in include/ofono/stk.h,
which do take void *, so these ones are just for local use in this
file to make working with the stk_respond / stk_send_envelope
functions easier.
>
>> +
>> struct ofono_stk {
>> const struct ofono_stk_driver *driver;
>> void *driver_data;
>> struct ofono_atom *atom;
>> + command_handler_t handlers[256];
>
> There are only 41 proactive commands, and we'll implement a subset. This
> seems a bit excessive. However, if we can't come up with anything more
> clever, I'm actually fine with it for fast access.
Ok, changed it to command_handler_t
handlers[STK_COMMAND_TYPE_END_SESSION + 1]; (that equals 0x82), is
that ok?
>
>> + struct stk_command *pending_cmd;
>> };
>>
>> +static void stk_respond(struct ofono_stk *stk, struct stk_command *cmd,
>> + struct stk_response *rsp, generic_cb_t cb)
>> +{
>
> Can we avoid passing in the original stk_command object? Since only one
> command is pending at a time, sounds like we should be able to set it
> elsewhere easily.
Ok, I'll remove the parameter completely since it was only used twice
in the file.
>
> Is the plan to make this public API at some point? If so it definitely
> requires userdata.
I was rather wanting them to be local, other files wouldn't need to
deal with stk_response or stk_envelope at all, instead stk.c would
pick the important bits from the structure and perhaps call a function
in the other file.
>
>> + struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
>> + const guint8 *tlv;
>> + unsigned int tlv_len;
>> +
>> + if (cmd == NULL)
>> + cmd = stk->pending_cmd;
>> + else
>> + stk->pending_cmd = cmd;
>> +
>> + rsp->src = STK_DEVICE_IDENTITY_TYPE_TERMINAL;
>> + rsp->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
>> + rsp->number = cmd->number;
>> + rsp->type = cmd->type;
>> + rsp->qualifier = cmd->qualifier;
>> +
>> + if (stk->driver->terminal_response == NULL) {
>> + cb(&error, stk);
>> + return;
>> + }
>> +
>> + tlv = stk_pdu_from_response(rsp, &tlv_len);
>> + if (!tlv) {
>> + cb(&error, stk);
>> + return;
>> + }
>> +
>> + stk->driver->terminal_response(stk, tlv_len, tlv,
>> + (ofono_stk_generic_cb_t) cb, stk);
>> +}
>> +
>> +static void stk_send_envelope(struct ofono_stk *stk, struct stk_envelope
>> *e, + envelope_cb_t cb)
>
> Same comments here. Should this be public API with userdata?
>
>> +{
>> + struct ofono_error error = { .type = OFONO_ERROR_TYPE_FAILURE };
>> + const guint8 *tlv;
>> + unsigned int tlv_len;
>> +
>> + e->dst = STK_DEVICE_IDENTITY_TYPE_UICC;
>> +
>> + if (stk->driver->envelope == NULL) {
>> + cb(&error, NULL, -1, stk);
>> + return;
>> + }
>> +
>> + tlv = stk_pdu_from_envelope(e, &tlv_len);
>> + if (!tlv) {
>> + cb(&error, NULL, -1, stk);
>> + return;
>> + }
>> +
>> + stk->driver->envelope(stk, tlv_len, tlv,
>> + (ofono_stk_envelope_cb_t) cb, stk);
>> +}
>> +
>> 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");
>> @@ -56,34 +126,47 @@ 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 stk_envelope e;
>>
>> - 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)
>> + stk_send_envelope(stk, &e, stk_cbs_download_cb);
>> +}
>> +
>> +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_notify(struct ofono_stk *stk,
>> int length, const unsigned char *pdu)
>> {
>> struct stk_command *cmd;
>> + struct stk_response rsp;
>> char *buf;
>> int i;
>>
>> @@ -97,14 +180,49 @@ void ofono_stk_proactive_command_notify(struct
>> ofono_stk *stk, ofono_error("Can't parse proactive command: %s", buf);
>> g_free(buf);
>>
>> - /* TODO: return TERMINAL RESPONSE with permanent error */
>> + /*
>> + * There's nothing we can do, we'd need at least Command
>> + * Details to respond with an error.
>> + */
>> return;
>> }
>> + g_free(buf);
>>
>> - /* TODO: execute */
>> + memset(&rsp, 0, sizeof(rsp));
>>
>> - g_free(buf);
>> - stk_command_free(cmd);
>> + switch (cmd->status) {
>> + case STK_PARSE_RESULT_OK:
>> + if (stk->handlers[cmd->type]) {
>> + if (stk->handlers[cmd->type](cmd, &rsp, stk) == TRUE)
>> + break;
>> +
>> + stk->pending_cmd = cmd;
>> + return;
>> + }
>> + /* Fall through */
>> +
>> + 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_respond(stk, cmd, &rsp, stk_command_cb);
>> +}
>> +
>> +static void stk_command_handler_register(struct ofono_stk *stk,
>> + enum stk_command_type type,
>> + command_handler_t handler)
>> +{
>> + stk->handlers[type] = handler;
>
> More error handling, might want to return an error if something is already
> registered.
Ok, added a check.
Regards
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
