Re: [PATCH 3/4] plugins: add a new driver for Quectel UC15 modules

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Philip,

>
> I don't think there's any reason this needs to be there specifically.  I
> probably put it there when I was debugging a USB-related reset problem.
> It can probably either go away (I've not done more than cursory testing
> with the modem connected to a UART - many things may be broken) or move
> to quectel_enable() (in which case the modem will behave a little bit
> more like one expects a modem to behave when connected to a UART).

Just wondering.  We usually send &C0 fairly early in the process.

> 
>> Any reason for checking that CFUN succeeded?  If so, then it might be a
>> good idea to document that in comments / commit description.
> 
> It turns out that AT+CFUN=0 -> AT+CFUN=1 transitions can return OK
> before the SIM is completely out of reset (temperature related).  In
> that case, AT+CPIN will return any number of bizarre error codes (from
> SIM not inserted to memory failure).  Trying again a bit later just
> works.

Should you be using at_util_sim_state_query_new for this?  Or does
CFUN=1 needs to be sent again?

> 
>> What if it failed?  Are you relying on the core timeout mechanisms?
> 
> It it doesn't behave as expected after retrying as often and waiting as
> long as this, it's unlikely to ever work.  The core will timeout waiting
> for the driver to indicate the modem is powered.  I would expect higher
> layers to see this as a hint that something may be wrong with powering
> up the modem.
> 
> Can you suggest a better way to fail here?

I was thinking that the driver should notify the core as soon as it
determines that the hardware is inoperable.  The core timeout handling
is really just a failsafe.  But either way works...

> 
>>> +	if (!ok) {
>>> +		if (data->enable_tries > MAX_RETRIES) {
>>> +			g_at_chat_unref(data->aux);
>>> +			data->aux = NULL;
>>> +			g_at_chat_unref(data->modem);
>>> +			data->modem = NULL;
>>> +			ofono_modem_set_powered(modem, FALSE);
>>> +			DBG("modem won't enable - giving up");
>>> +		}
>>> +		goto retry;
>>
>> Is the timer still running in case MAX_RETRIES is reached?  In general
>> we prefer using single-shot timers and re-starting them if the command
>> fails.  Certain commands might take several seconds to return on some
>> hardware.
> 
> I am fairly confident it's safe: I ran into this failure case quite
> often during development and testing.  I schedule quectel_start() in
> quectel_enable() and quectel_start() keeps rescheduling cfun_enable()
> until it succeeds or runs out of retries.  I carefully check if the
> timer is still running everywhere I try to tear things down.

The thing is that running out of retries does not stop the timer.  At
least I don't see g_source_remove for it here.  Neither does the
quectel_start() function check for max retries.

> 
> It's probably not too hard to rework this to use a single-shot timer.
> 

That might make things a bit more reliable and readable as well.

>>> +	/* Give the modem a bit more time to power up completely. */
>>> +	data->enable_tries = 0;
>>> +	data->enable_timer = g_timeout_add_seconds(RESET_DELAY,
>>> +					quectel_start, modem);
>>
>> Is this delay always necessary?  enable() is called in other situations
>> besides a hot-plugging...
> 
> I mainly tested this with hot-plugging and not-so-hot-plugging (keeping
> the module powered but kicking its reset line) and it seems necessary in
> both cases.  I haven't tested toggling enable in software much, but just
> doing AT+CFUN=0 ; AT+CFUN=1 in a loop shows a fair amount of variance
> even without touching power or reset.  May be a hardware issue, but I've
> not talked to Quectel about it.
>  

Just wondering whether upping the max retries and sending this command
right away might be 'better' for the non-hotplug case.

>>> +	g_at_chat_send(data->aux, "AT+CFUN=0", NULL,
>>> +					cfun_disable, modem, NULL);
>>
>> What behavior does this modem exhibit when using CFUN=0?  Many devices
>> jump off the USB bus when this is sent.  Hence why most drivers use CFUN=4.
> 
> It stays on the bus but turns off the radio and goes into lower power
> mode.  I have no strong feelings about using CFUN=4 instead, but
> intuitively that feels more like "offline" than "disabled".  Am I
> missing some other distinction between set_online and enable/disable?

If the modem stays on the bus, using CFUN=0 is just fine.  As I said,
most others jump off the bus, which makes using CFUN=0 not a good idea :)

Regards,
-Denis