Hi Robertino, > This is another attempt to submit a patch that triggers Infineon > modem selftest during ofono boot. Patch addresses issues raised > by Marcel from the previous submissions. > ---
as a general comment, changelogs and notes for the reviewer have to between --- and the diffstat. They should not be part of the commit message. The commit messages becomes part of ofono.git, the comments are just for the reviewer. > plugins/ifx.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++++------- > 1 files changed, 66 insertions(+), 10 deletions(-) > > diff --git a/plugins/ifx.c b/plugins/ifx.c > index c0a69c2..e0eb982 100644 > --- a/plugins/ifx.c > +++ b/plugins/ifx.c > @@ -71,6 +71,8 @@ > #define GPRS3_DLC 4 > #define AUX_DLC 5 > > +#define IFX_SELF_TESTS_TIMEOUT 10 > + I asked this 3 times now, where is this magic value of 10 seconds coming from. What is the average expected execution time of each test? > static char *dlc_prefixes[NUM_DLC] = { "Voice: ", "Net: ", "GPRS1: ", > "GPRS2: ", "GPRS3: ", "Aux: " }; > > @@ -81,6 +83,16 @@ static const char *dlc_nodes[NUM_DLC] = { "/dev/ttyGSM1", > "/dev/ttyGSM2", > static const char *none_prefix[] = { NULL }; > static const char *xdrv_prefix[] = { "+XDRV:", NULL }; > > +static struct { > + char *test_desc; > + char *at_cmd; > +} const mst[] = { > + { "AT Command Test", "ATE0 +CMEE=1" }, /* set echo & error reporting */ I really don't like to put ATE0 into this. It is not a selftest or test command. > + { "RTC GTI Test", "a...@rtc:rtc_gti_test_verify_32khz()" }, > + { "Device Version Test", "a...@vers:device_version_id()" }, > + { NULL, NULL } > +}; And I like to still see an answer why we have to trigger selftests here. Can we just not have one AT command to check the modem health and be done with it? Another question that I did ask is to see some sample results from these test cases in failure and success case. Do we actually care about test description here at all or can we just drop it? > struct ifx_data { > GIOChannel *device; > GAtMux *mux; > @@ -99,6 +111,7 @@ struct ifx_data { > int audio_loopback; > struct ofono_sim *sim; > gboolean have_sim; > + int self_test_idx; > }; > > static void ifx_debug(const char *str, void *user_data) > @@ -545,6 +558,52 @@ static gboolean mux_timeout_cb(gpointer user_data) > return FALSE; > } > > +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 (data->mux_init_timeout > 0) { > + g_source_remove(data->mux_init_timeout); > + data->mux_init_timeout = 0; > + } This is rather pointless. We are not starting a timeout for every single command. The mux_timeout handling was designed for spawning the overall setup process and not just one command. > + if (!ok) { > + ofono_error("Modem %s: FAIL", > + mst[data->self_test_idx].test_desc); > + g_at_chat_unref(data->dlcs[AUX_DLC]); > + data->dlcs[AUX_DLC] = NULL; > + > + g_io_channel_unref(data->device); > + data->device = NULL; > + > + ofono_modem_set_powered(modem, FALSE); > + > + return; > + } > + > + data->self_test_idx++; > + > + if (mst[data->self_test_idx].at_cmd != NULL) { > + g_at_chat_send(data->dlcs[AUX_DLC], > + mst[data->self_test_idx].at_cmd, > + NULL, ifx_self_test_cb, modem, NULL); > + > + data->mux_init_timeout = g_timeout_add_seconds( > + IFX_SELF_TESTS_TIMEOUT, mux_timeout_cb, modem); > + Just using return here and bothering with the else branch would result in a lot simpler to read code. > + } else { /* 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); > + } > +} > + > static int ifx_enable(struct ofono_modem *modem) > { > struct ifx_data *data = ofono_modem_get_data(modem); > @@ -595,18 +654,15 @@ static int ifx_enable(struct ofono_modem *modem) > if (getenv("OFONO_AT_DEBUG")) > g_at_chat_set_debug(chat, ifx_debug, "Master: "); > > - g_at_chat_send(chat, "ATE0 +CMEE=1", NULL, > - NULL, NULL, NULL); > - > - data->frame_size = 1509; > - > - g_at_chat_send(chat, "AT+CMUX=0,0,,1509,10,3,30,,", NULL, > - mux_setup_cb, modem, NULL); > + /* Execute Modem Self tests */ > + 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], mst[data->self_test_idx].at_cmd, > + NULL, ifx_self_test_cb, modem, NULL); > > - data->dlcs[AUX_DLC] = chat; > + data->mux_init_timeout = g_timeout_add_seconds( > + IFX_SELF_TESTS_TIMEOUT, mux_timeout_cb, modem); I am fine with using the established mutliplexer timeout handling here, but essentially this is not a SELF_TESTS timeout. This is overall setup timeout. So can we please get the naming right here. I really dislike code where function names and constants are not named properly. This causes major confusion later on. Regards Marcel _______________________________________________ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono