Hi Robertino,
> static const char *none_prefix[] = { NULL };
> static const char *xdrv_prefix[] = { "+XDRV:", NULL };
> +static const char *empty_prefix[] = { "", NULL };
this is essentially to just passing NULL as argument. What are you
trying to achieve here?
> +static const char *modem_self_tests[][2] = {
> + { "RTC Test", "a...@rtc:rtc_gti_test_verify_32khz()" },
> + { "Version Info Test", "a...@vers:device_version_id()"
> },
> + { NULL, NULL } };
The coding style is broken here. Please check the oFono source code for
example on how to format these type of array correctly.
In addition the string array is wrong. It should be a struct array. The
magic of [1] and [2] for picking the command or the text string is not
intuitive and leads to unreadable code.
> struct ifx_data {
> GIOChannel *device;
> @@ -99,6 +105,8 @@ struct ifx_data {
> int audio_loopback;
> struct ofono_sim *sim;
> gboolean have_sim;
> + guint self_test_timeout;
> + int self_test_idx;
> };
>
> static void ifx_debug(const char *str, void *user_data)
> @@ -545,6 +553,82 @@ static gboolean mux_timeout_cb(gpointer user_data)
> return FALSE;
> }
>
> +static gboolean self_test_timeout_cb(gpointer user_data)
> +{
> + struct ofono_modem *modem = user_data;
> + struct ifx_data *data = ofono_modem_get_data(modem);
> +
> + ofono_error("Timeout with modem self_test");
You need an extra empty line here. The error is logically separated.
> + g_source_remove(data->self_test_timeout);
You can not remove a source in its own callback. Have you tested this
code path actually?
> + data->self_test_timeout = 0;
> +
> + shutdown_device(data);
You can not use shutdown_device here since the multiplexer is not ready
yet. If you decide the execute the selftests before bringing up the
multiplexer then this is a different error path.
You need to cleanup data->dlcs[AUX_DLC] and data->device in this failure
case.
> + ofono_modem_set_powered(modem, FALSE);
This needs an extra empty before the return statement.
> + return FALSE;
> +}
> +
> +static void ifx_self_test_get_device_resp(GAtResult *result, int test_id)
> +{
The function is just wrong in conjunction with your array approach.
> + GAtResultIter iter;
> + const char *str;
> +
> + ofono_info("Modem Response: %s",
> + modem_self_tests[test_id][0]);
Can you give an example on a positive and negative selftest response
here. I don't wanna spam the logs with details if everything is fine.
This should use ofono_error and that only in case of an error.
> + g_at_result_iter_init(&iter, result);
> +
> + while (g_at_result_iter_next(&iter, NULL)) {
> +
> + if (g_at_result_iter_next_string(&iter, &str))
> + ofono_info("Modem Response: %s", str);
> + }
What is this while loop actually doing? How does a response look like.
> +}
> +
> +static void ifx_self_test_cb(gboolean ok, GAtResult *result,
> + gpointer user_data)
> +{
> + struct ofono_modem *modem = user_data;
> + struct ifx_data *data = ofono_modem_get_data(modem);
> +
> + if (ok) {
This needs to be a if (!ok) here actually and then leaving this function
with a simple error case.
Having the positive handling nested is not a good idea and we don't
really do it that way.
> +
> + if (data->self_test_timeout > 0) {
> + g_source_remove(data->self_test_timeout);
> + data->self_test_timeout = 0;
> + }
> +
> + ifx_self_test_get_device_resp(result, data->self_test_idx);
> + data->self_test_idx++;
> +
> + if (modem_self_tests[data->self_test_idx][1] != NULL) {
> +
No empty line here.
> + g_at_chat_send(data->dlcs[AUX_DLC],
> + modem_self_tests[data->self_test_idx][1],
> + empty_prefix, ifx_self_test_cb, modem, NULL);
> +
> + data->self_test_timeout = g_timeout_add_seconds(10,
> + self_test_timeout_cb, modem);
> + return;
No empty line here, but this is not needed in case of (!ok) anyway.
> + } else {
> +
No empty line here either.
> + /* Enable MUX Channels */
> + data->frame_size = 1509;
> + g_at_chat_send(data->dlcs[AUX_DLC],
> + "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
> + mux_setup_cb, modem, NULL);
> +
> + data->mux_init_timeout = g_timeout_add_seconds(5,
> + mux_timeout_cb, modem);
> + return;
> + }
> + }
With proper if (error) { ... ; return; } coding style this function
becomes a lot more readable.
> +
> + ofono_error("Modem %s: FAIL", modem_self_tests[data->self_test_idx][0]);
> + shutdown_device(data);
Again here, the shutdown_device call can not be used since the
multiplexer is not yet brought up.
> + ofono_modem_set_powered(modem, FALSE);
> +}
> +
> static int ifx_enable(struct ofono_modem *modem)
> {
> struct ifx_data *data = ofono_modem_get_data(modem);
> @@ -598,15 +682,17 @@ static int ifx_enable(struct ofono_modem *modem)
> g_at_chat_send(chat, "ATE0 +CMEE=1", NULL,
> NULL, NULL, NULL);
Now here is the question if we wanna execute ATE0 first or run the
selftests first?
> - data->frame_size = 1509;
> + /* Execute Modem Self tests */
>
> - g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL,
> - mux_setup_cb, modem, NULL);
> + data->dlcs[AUX_DLC] = chat;
> + data->self_test_idx = 0;
>
> - data->mux_init_timeout = g_timeout_add_seconds(5, mux_timeout_cb,
> - modem);
> + g_at_chat_send(data->dlcs[AUX_DLC],
> + modem_self_tests[data->self_test_idx][1],
> + empty_prefix, ifx_self_test_cb, modem, NULL);
>
> - data->dlcs[AUX_DLC] = chat;
> + data->self_test_timeout = g_timeout_add_seconds(10,
> + self_test_timeout_cb, modem);
Where does these 10 seconds come from?
Regards
Marcel
_______________________________________________
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
