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

Hi Denis,

On Wed, May 28, 2025 at 01:17:19PM +0100, Denis Kenzior wrote:
> 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.

Will remove!

> 
> > +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?
Sure? We first set ofono_modem_set_powered(modem, TRUE) after
AT+CFUN=4.

> You're
> also leaking timeout here.  If this is a hot-pluggable modem, then the timeout
> also needs to be canceled / freed in .remove()

Will do.

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

Will remove, it was cp from the quectel plugin :)

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

/Sean