Re: [PATCH] sim7100: query until AT channel is ready

Hi Sean,

On 5/23/25 8:17 AM, Sean Nyekjaer wrote:
> It's seen that the AT channel contains unknown data upon power on.
> Query the modem until it answers with OK. Create a timer to re-issue
> the AT command at regular intervals until the modem answers OK.
> 
> plugins/sim7100.c:sim7100_enable()
> plugins/sim7100.c:open_device() devkey=AT
> src/modem.c:get_modem_property() modem 0x261f20 property AT
> plugins/sim7100.c:open_device() devkey=PPP
> src/modem.c:get_modem_property() modem 0x261f20 property PPP
> AT: > AT\r
> AT: < \r\r*A
> PPP: < \r\r*A
> AT: < AT\r
> plugins/sim7100.c:init_timeout_cb() 0x261f20
> AT: > AT\r
> AT: < AT\r
> AT: < \r\nOK\r\n
> AT: > ATE0Q0V1\r
> AT: < ATE0Q0V1\r\r\nOK\r\n
> AT: > AT+CGMM\r
> AT: < \r\nA7672E-FASE\r\n\r\nOK\r\n
> plugins/sim7100.c:cgmm_cb() modem model: A7672E-FASE
> plugins/sim7100.c:cgmm_cb() before CFUN
> AT: > AT+CFUN=4\r
> AT: < \r\n+CGEV: ME DETACH\r\n\r\n+CGEV: NW PDN DEACT 1\r\n
> PPP: < \r\n+CGEV: ME DETACH\r\n\r\n+CGEV: NW PDN DEACT 1\r\n
> AT: < \r\nOK\r\n
> plugins/sim7100.c:cfun_set_on_cb()
> src/modem.c:modem_change_state() old state: 0, new state: 1
> ---
> 
> Is this the way to do it? It's quite weird that the serial channel
> doesn't respond in a sane maner after power up.

I've never seen this before.  oFono does perform a tcflush() when opening the 
port, so this garbage must be coming in after the port has been opened.  I'm not 
sure what else could be done besides what you're doing here.

> 
> Before this patch ofono would get stuck in enable and transmit CFUN=4,
> our software would then restart ofono and everything would be fine
> again.
> I have done nearly 50 power cycles with this patch all being successful.
> 
>   plugins/sim7100.c | 75 +++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 62 insertions(+), 13 deletions(-)
> 
> diff --git a/plugins/sim7100.c b/plugins/sim7100.c
> index b6389b09..d52e5b92 100644
> --- a/plugins/sim7100.c
> +++ b/plugins/sim7100.c
> @@ -22,6 +22,7 @@
>   #include <glib.h>
>   #include <gatchat.h>
>   #include <gattty.h>
> +#include <ell/ell.h>
>   
>   #define OFONO_API_SUBJECT_TO_CHANGE
>   #include <ofono/plugin.h>
> @@ -57,6 +58,9 @@ struct sim7100_data {
>   	GAtChat *at;
>   	GAtChat *ppp;
>   	enum sim7x00_model model;
> +	struct l_timeout *init_timeout;
> +	size_t init_count;
> +	guint init_cmd;
>   };
>   
>   static void sim7100_debug(const char *str, void *user_data)
> @@ -154,6 +158,47 @@ static void cgmm_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   									NULL);
>   }
>   
> +

nit: No double empty lines please.

> +static void init_cmd_cb(gboolean ok, GAtResult *result, void *user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct sim7100_data *data = ofono_modem_get_data(modem);
> +
> +	/* ensure modem is in a known state; verbose on, echo/quiet off */
> +	g_at_chat_send(data->at, "ATE0Q0V1", NULL, NULL, NULL, NULL);
> +
> +	g_at_chat_send(data->at, "AT+CGMM", NULL, cgmm_cb, modem, NULL);
> +
> +	l_timeout_remove(data->init_timeout);
> +	data->init_timeout = NULL;
> +}
> +
> +static void close_serial(struct ofono_modem *modem)
> +{
> +	struct sim7100_data *data = ofono_modem_get_data(modem);
> +
> +	g_at_chat_unref(data->ppp);
> +	g_at_chat_unref(data->at);
> +	data->at = data->ppp = NULL;
> +}
> +
> +static void init_timeout_cb(struct l_timeout *timeout, void *user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct sim7100_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	if (data->init_count++ >= 30) {
> +		ofono_error("failed to init modem after 30 attempts");
> +		close_serial(modem);

Do you also need ofono_modem_set_powered(modem, FALSE) here as well?  You're 
also leaking timeout here.  If this is a hot-pluggable modem, then the timeout 
also needs to be canceled / freed in .remove()

> +		return;
> +	}
> +
> +	g_at_chat_retry(data->at, data->init_cmd);
> +	l_timeout_modify_ms(timeout, 500);
> +}
> +
>   static int open_device(struct ofono_modem *modem, char *devkey, GAtChat **chat)
>   {
>   	DBG("devkey=%s", devkey);
> @@ -172,16 +217,22 @@ static int sim7100_enable(struct ofono_modem *modem)
>   
>   	DBG("");
>   
> -	err = open_device(modem, "AT", &data->at);
> -	if (err < 0)
> -		return err;
> -
> -	err = open_device(modem, "PPP", &data->ppp);
> -	if (err < 0)
> -		return err;
> -
> -	/* ensure modem is in a known state; verbose on, echo/quiet off */
> -	g_at_chat_send(data->at, "ATE0Q0V1", NULL, NULL, NULL, NULL);
> +	if (g_at_chat_get_channel(data->at) == NULL) {

What does this do?

> +		err = open_device(modem, "AT", &data->at);
> +		if (err < 0)
> +			return err;
> +
> +		err = open_device(modem, "PPP", &data->ppp);
> +		if (err < 0)
> +			return err;
> +
> +		data->init_count = 0;
> +		data->init_cmd = g_at_chat_send(data->at, "AT", NULL,
> +				init_cmd_cb, modem, NULL);
> +		data->init_timeout = l_timeout_create_ms(500, init_timeout_cb, modem,
> +				NULL);
> +		return -EINPROGRESS;
> +	}
>   
>   	/* query modem model string */
>   	g_at_chat_send(data->at, "AT+CGMM", NULL, cgmm_cb, modem, NULL);
> @@ -196,9 +247,7 @@ static void cfun_set_off_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   
>   	DBG("");
>   
> -	g_at_chat_unref(data->ppp);
> -	g_at_chat_unref(data->at);
> -	data->at = data->ppp = NULL;
> +	close_serial(modem);

Just FYI, this change now makes CI complain:

     plugins/sim7100.c: In function 'cfun_set_off_cb':
     plugins/sim7100.c:246:30: error: unused variable 'data' 
[-Werror=unused-variable]
       246 |         struct sim7100_data *data = ofono_modem_get_data(modem);
           |                              ^~~~
     cc1: all warnings being treated as errors

>   
>   	if (ok)
>   		ofono_modem_set_powered(modem, FALSE);

Regards,
-Denis