7:43 p.m.
Hi Marcel,
please do not make up new commit prefixes. Use simple lower case
strings
like the modem plugin name, the subdirectory in drivers or for the core
the logical name.
Ok, I will do this.
> plugins/speedupcdma.c | 102
++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/plugins/speedupcdma.c b/plugins/speedupcdma.c
> index b7e54a1..1dcff88 100644
> --- a/plugins/speedupcdma.c
> +++ b/plugins/speedupcdma.c
> @@ -41,11 +41,25 @@
> #include <drivers/atmodem/atutil.h>
>
> static const char *none_prefix[] = { NULL };
> +static const char *sysinfo_prefix[] = { "^SYSINFO:", NULL };
> +
> +enum speedupcdma_sim_state {
> + SPEEDUPCDMA_SIM_STATE_VALID = 1,
> + SPEEDUPCDMA_SIM_STATE_ROMSIM = 240,
> + SPEEDUPCDMA_SIM_STATE_NOT_EXISTENT = 255
Add last , as well. We do this for enum so that if they get extended the
patches look cleaner and not have to reshuffle.
I see.
> +};
>
> struct speedupcdma_data {
> GAtChat *chat;
> + enum speedupcdma_sim_state sim_state;
> + guint sim_poll_timeout;
> + guint sim_poll_count;
> };
>
> +#define MAX_SIM_POLL_COUNT 5
> +
> +static gboolean query_sim_state(gpointer user_data);
> +
> static void speedupcdma_debug(const char *str, void *data)
> {
> const char *prefix = data;
> @@ -79,6 +93,90 @@ static void speedupcdma_remove(struct ofono_modem *modem)
> g_free(data);
> }
>
> +static gboolean notify_sim_state(struct ofono_modem *modem,
> + enum speedupcdma_sim_state sim_state)
> +{
> + struct speedupcdma_data *data = ofono_modem_get_data(modem);
> +
> + DBG("%d", sim_state);
> +
> + data->sim_state = sim_state;
> +
> + switch (sim_state) {
> + case SPEEDUPCDMA_SIM_STATE_NOT_EXISTENT:
> + /* SIM is not ready, try again a bit later */
> + return TRUE;
> + case SPEEDUPCDMA_SIM_STATE_VALID:
> + case SPEEDUPCDMA_SIM_STATE_ROMSIM:
> + if (data->sim_poll_timeout) {
> + g_source_remove(data->sim_poll_timeout);
> + data->sim_poll_timeout = 0;
> + }
> +
> + ofono_modem_set_powered(modem, TRUE);
Use break; here.
And can you include OFONO_AT_DEBUG traces to show what is going on.
> +
> + return FALSE;
> + }
> +
> + return FALSE;
> +}
> +
> +static void sysinfo_cb(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> + struct ofono_modem *modem = user_data;
> + struct speedupcdma_data *data = ofono_modem_get_data(modem);
> + gboolean rerun;
> + gint sim_state;
> + GAtResultIter iter;
> +
> + if (!ok)
> + return;
> +
> + g_at_result_iter_init(&iter, result);
> +
> + if (!g_at_result_iter_next(&iter, "^SYSINFO:"))
> + return;
> +
> + if (!g_at_result_iter_skip_next(&iter))
> + return;
> +
> + if (!g_at_result_iter_skip_next(&iter))
> + return;
> +
> + if (!g_at_result_iter_skip_next(&iter))
> + return;
> +
> + if (!g_at_result_iter_skip_next(&iter))
> + return;
> +
> + if (!g_at_result_iter_next_number(&iter, &sim_state))
> + return;
> +
> + rerun = notify_sim_state(modem, (enum speedupcdma_sim_state)
sim_state);
> +
> + if (rerun && data->sim_poll_count < MAX_SIM_POLL_COUNT) {
> + data->sim_poll_count++;
> + data->sim_poll_timeout = g_timeout_add_seconds(2,
> + query_sim_state,
> + modem);
> + }
> +}
This logic is copied from the Huawei GSM plugin and there I am not even
sure that it is correct. Please show us AT command traces. Since I do
not think that it is this complicated for CDMA.
I also think so.
After a full day of testing (while implementing NetworkRegistration for CDMA), I have
found a best way to handle this. In fact the correct way. As you said, I have copy/paste
this code from Huawei driver, and I have simply some part but not all. And I have to
rework this.
The Huawei plugin is a bad example to copy code from.
> +static gboolean query_sim_state(gpointer user_data)
> +{
> + struct ofono_modem *modem = user_data;
> + struct speedupcdma_data *data = ofono_modem_get_data(modem);
> +
> + DBG("");
> +
> + data->sim_poll_timeout = 0;
> +
> + g_at_chat_send(data->chat, "AT^SYSINFO", sysinfo_prefix,
> + sysinfo_cb, modem, NULL);
> +
> + return FALSE;
> +}
> +
> static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
> {
> struct ofono_modem *modem = user_data;
> @@ -93,8 +191,6 @@ static void cfun_enable(gboolean ok, GAtResult *result,
gpointer user_data)
> ofono_modem_set_powered(modem, FALSE);
> return;
> }
> -
> - ofono_modem_set_powered(modem, TRUE);
> }
>
> static int speedupcdma_enable(struct ofono_modem *modem)
> @@ -132,6 +228,8 @@ static int speedupcdma_enable(struct ofono_modem *modem)
> g_at_chat_send(data->chat, "AT+CFUN=1", none_prefix,
> cfun_enable, modem, NULL);
>
> + query_sim_state(modem);
> +
This is wrong code. So in case CFUN=1 fails, you still go ahead and
query the SIM state. That is rather pointless and might result on
unforseen feedback from the modem.
In addition you do not handle the set_powered in case ^SYSINFO fails or
you have polled max attempts.
Now looking over this. I do not like this at all. Please do not copy any
code from Huawei plugin and write this from scratch based on actual
traces and the documentation.
I totally agree, I will rework this part.
I will set the powered flag after CFUN, and use the SYSINFO to broadcast SIM info
(available or not).
Regards,
Bertrand
---------------------------------------------------------------------
Intel Corporation SAS (French simplified joint stock company)
Registered headquarters: "Les Montalets"- 2, rue de Paris,
92196 Meudon Cedex, France
Registration Number: 302 456 199 R.C.S. NANTERRE
Capital: 4,572,000 Euros
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.