[PATCH v3] ifx: Adding modem selftest for Infineon modem

[PATCH v3] ifx: Adding modem selftest for Infineon modem

[Robertino Benis]

[-- Attachment #1: Type: text/plain, Size: 3710 bytes --]

Hi all,

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.

Thank you,
-- r.

---
 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
+
 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 */
+	{ "RTC GTI Test", "at(a)rtc:rtc_gti_test_verify_32khz()" },
+	{ "Device Version Test", "at(a)vers:device_version_id()" },
+	{ NULL, NULL }
+};
+
 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;
+	}
+
+	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);
+
+	} 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);
 
 	return -EINPROGRESS;
 }
-- 
1.7.0.4

Re: [PATCH v3] ifx: Adding modem selftest for Infineon modem

[Robertino Benis]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

Hi guys,

> 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.

>  plugins/ifx.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++-------
>  1 files changed, 66 insertions(+), 10 deletions(-)

How did this submission (email from 12/23/2010) for modem self test look
like? Any closer to acceptance? We tested this code on couple of devices
and with different modems, with and without error condition on boot.

Thanks,
-- r. 

Re: [PATCH v3] ifx: Adding modem selftest for Infineon modem

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 5356 bytes --]

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", "at(a)rtc:rtc_gti_test_verify_32khz()" },
> +	{ "Device Version Test", "at(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

RE: [PATCH v3] ifx: Adding modem selftest for Infineon modem

[Bastian, Waldo]

[-- Attachment #1: Type: text/plain, Size: 3309 bytes --]

> >  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?

Typical response times are much less than 0.5 sec. 10 seconds is considered
much longer than any normal response time. 

> > +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.

It acts as a selftest command because if the modem fails to come out of reset
and/or start the AT command parser you will never get an OK back on it. In the 
error reporting we would like to be able to distinguish between the AT command
parser not working at all and any subsequent test command failing.

> > +	{ "RTC GTI Test", "at(a)rtc:rtc_gti_test_verify_32khz()" },
> > +	{ "Device Version Test", "at(a)vers:device_version_id()" },
> > +	{ NULL, NULL }
> > +};
>
> And I like to still see an answer why we have to trigger selftests here.

To make sure the modem is properly functioning.

> Can we just not have one AT command to check the modem health and be
> done with it?

The IFX modem we are targetting implements this with two AT commands.

> Another question that I did ask is to see some sample results from these
> test cases in failure and success case.

Robertino?

> Do we actually care about test
> description here at all or can we just drop it?

If you prefer cryptic messages then the AT command itself can act as the description.
Is that preferred?

> 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.

The design documentation wasn't clear on that point. The name mux_timeout
suggested that the timeout only applied to the mux, maybe it should be renamed to
setup_timeout to better describe the design intention?

Regardless, having a timeout for each command is conceptually cleaner as you will
know for sure which command is responsible for exceeding the timeout. 

Does oFono require the more ambiguous solution with a single timeout?

> 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.

In the patch it is the timeout for the first self test command.
I understand you are thinking about a potential future patch in which it would be
used as the overall setup timeout, I agree that in that case the name should be
changed accordingly as well, yes.

Cheers,
Waldo

RE: [PATCH v3] ifx: Adding modem selftest for Infineon modem

[Robertino Benis]

[-- Attachment #1: Type: text/plain, Size: 1771 bytes --]

Hi,

> The IFX modem we are targetting implements this with two AT commands.
> 
> > Another question that I did ask is to see some sample results from these
> > test cases in failure and success case.
> 
> Robertino?

I think you meant except form the log, such as this:

ofonod[1342]: Using multiplexer line discipline 29
ofonod[1342]: Master: > ATE0 +CMEE=1\r
ofonod[1342]: Master: < ATE0 +CMEE=1\r
ofonod[1342]: Master: < \r\nOK\r\n
ofonod[1342]: Master: > at(a)rtc:rtc_gti_test_verify_32khz()\r
ofonod[1342]: Master: < \r\n
ofonod[1342]: Master: < Verification passed\r\nOK\r\n     

ofonod[1342]: Master: > at(a)vers:device_version_id()\r
ofonod[1342]: Master: < \r\n
ofonod[1342]: Master: < \r\nCHIP ID = XG626 ES2\r\nFLASH TYPE =
NumonyxM18 NOR\r\nFLASH ID = 0x898881\r\nSmartiUE2 = 37664\r\nRF PMU =
10548\r\nPA PMU = 40244\r\nRF ASM = 0\r\nFEM PA = 21812\r\nOK\r\n  



Simulated Self test Failure

ofonod[1260]: Using multiplexer line discipline 29
ofonod[1260]: Master: > ATE0 +CMEE=1\r
ofonod[1260]: Master: < ATE0 +CMEE=1\r
ofonod[1260]: Master: < \r\nOK\r\n
ofonod[1260]: Master: > at(a)rtc:rtc_gti_test_verify_32khz()\r
ofonod[1260]: Master: < \r\n
ofonod[1260]: Master: < Verification passed\r\nOK\r\n
ofonod[1260]: Master: > at(a)vers:device_version_id\r
ofonod[1260]: Master: < \r\nERROR\r\n                
ofonod[1260]: Modem Device Version Test: FAIL        


> In the patch it is the timeout for the first self test command.
> I understand you are thinking about a potential future patch in which it would be
> used as the overall setup timeout, I agree that in that case the name should be
> changed accordingly as well, yes.

Do you think we should use overall setup timeout? 

Thanks,
-- r.

RE: [PATCH v3] ifx: Adding modem selftest for Infineon modem

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 4969 bytes --]

Hi Waldo,

> > 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?
> 
> Typical response times are much less than 0.5 sec. 10 seconds is considered
> much longer than any normal response time. 

that was the answer what I was looking for. Thanks.

> > > +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.
> 
> It acts as a selftest command because if the modem fails to come out of reset
> and/or start the AT command parser you will never get an OK back on it. In the 
> error reporting we would like to be able to distinguish between the AT command
> parser not working at all and any subsequent test command failing.

since we don't report any of these errors other than the logs this not
really helping you. I'd rather have the failing AT command in the log
than some text reference that you have to track down with the source
code to figure out what it means.

> > > +	{ "RTC GTI Test", "at(a)rtc:rtc_gti_test_verify_32khz()" },
> > > +	{ "Device Version Test", "at(a)vers:device_version_id()" },
> > > +	{ NULL, NULL }
> > > +};
> >
> > And I like to still see an answer why we have to trigger selftests here.
> 
> To make sure the modem is properly functioning.
> 
> > Can we just not have one AT command to check the modem health and be
> > done with it?
> 
> The IFX modem we are targetting implements this with two AT commands.

And that could have been changed to one command just giving a proper
result status on the modem health. That would have made sense to me.
Instead we are using some random commands to do this.

> > Another question that I did ask is to see some sample results from these
> > test cases in failure and success case.
> 
> Robertino?
> 
> > Do we actually care about test
> > description here at all or can we just drop it?
> 
> If you prefer cryptic messages then the AT command itself can act as the description.
> Is that preferred?

It is all cryptic at this point. As I said, a proper AT command to get
the result of the selftest sequence with proper result, that would be
not cryptic. This whole effort in trying to do selftest this way is not
how I would have this done.

> > 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.
> 
> The design documentation wasn't clear on that point. The name mux_timeout
> suggested that the timeout only applied to the mux, maybe it should be renamed to
> setup_timeout to better describe the design intention?
> 
> Regardless, having a timeout for each command is conceptually cleaner as you will
> know for sure which command is responsible for exceeding the timeout. 
> 
> Does oFono require the more ambiguous solution with a single timeout?

oFono does not require this, but it is also not helping you. If I read
your response right, then the selftest execution time is less than a
second anyway and thus it will return with either an error or with OK.
So the timeout comes from a modem not responding and that makes really
no difference with command causes this.

That's why I asked where these magic timeout values came from. I needed
to understand what are possible implications.

So with this in mind, just using the global mux setup timeout like we do
right now is fine. Individual timeouts are not buying you anything
additional.

> > 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.
> 
> In the patch it is the timeout for the first self test command.
> I understand you are thinking about a potential future patch in which it would be
> used as the overall setup timeout, I agree that in that case the name should be
> changed accordingly as well, yes.

Yep, that is how I want this done. It is way cleaner and since we don't
get any useful information out of the selftest anyway, that is good
enough.

Robertino, also the array of commands is rather pointless here. You can
just use g_at_chat_send in a row and have the callbacks you cancel all
remaining commands. That way the code becomes even more cleaner and we
don't have to bother with maintaining a command array.

Leave the ATE0.. command as it is. The only way that one fails if we run
into a timeout. And that works fine right now. So that is good enough.

Regards

Marcel