[PATCH] ublox: use ID_MM_UBLOX_PORT_READY_DELAY udev flag as init

[Matthew Starr]

> -----Original Message-----
> From: Aleksander Morgado [mailto:aleksander at aleksander.es]
> Sent: Thursday, May 03, 2018 6:49 AM
> 
> Hey!
> 
> On Wed, May 2, 2018 at 10:34 PM, Matthew Starr <mstarr at hedonline.com>
> wrote:
> > Added reading the ID_MM_UBLOX_PORT_READY_DELAY udev flag value
> and
> > using it as an init delay when a value is set.
> >
> > The 20 second delay for the TOBY-L4 +READ URC has been reimplemented
> > using the new ID_MM_UBLOX_PORT_READY_DELAY udev value.
> 
> This looks simple enough I think yes.
> Some small comments below.
> 
> > ---
> >  plugins/ublox/77-mm-ublox-port-types.rules |  2 ++
> >  plugins/ublox/mm-plugin-ublox.c            | 16 ++++++++++------
> >  2 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/plugins/ublox/77-mm-ublox-port-types.rules
> b/plugins/ublox/77-mm-ublox-port-types.rules
> > index 31128dac..168d1571 100644
> > --- a/plugins/ublox/77-mm-ublox-port-types.rules
> > +++ b/plugins/ublox/77-mm-ublox-port-types.rules
> > @@ -11,9 +11,11 @@ SUBSYSTEMS=="usb",
> ATTRS{bInterfaceNumber}=="?*", ENV{.MM_USBIFNUM}="$attr{bInte
> >  #  ttyACM0 (if #2): secondary (ignore)
> >  #  ttyACM1 (if #4): debug port (ignore)
> >  #  ttyACM2 (if #6): primary
> > +#      Wait up to 20s for the +READY URC
> >  #  ttyACM3 (if #8): AT port for FOTA (ignore)
> >  ATTRS{idVendor}=="1546", ATTRS{idProduct}=="1010",
> ENV{.MM_USBIFNUM}=="02", ENV{ID_MM_PORT_IGNORE}="1"
> >  ATTRS{idVendor}=="1546", ATTRS{idProduct}=="1010",
> ENV{.MM_USBIFNUM}=="04", ENV{ID_MM_PORT_IGNORE}="1"
> > +ATTRS{idVendor}=="1546", ATTRS{idProduct}=="1010",
> ENV{.MM_USBIFNUM}=="06",
> ENV{ID_MM_UBLOX_PORT_READY_DELAY}="20"
> >  ATTRS{idVendor}=="1546", ATTRS{idProduct}=="1010",
> ENV{.MM_USBIFNUM}=="08", ENV{ID_MM_PORT_IGNORE}="1"
> >
> >  # TOBY-R2 port types
> > diff --git a/plugins/ublox/mm-plugin-ublox.c b/plugins/ublox/mm-plugin-
> ublox.c
> > index f4553a60..fdff239b 100644
> > --- a/plugins/ublox/mm-plugin-ublox.c
> > +++ b/plugins/ublox/mm-plugin-ublox.c
> > @@ -50,9 +50,6 @@ create_modem (MMPlugin     *self,
> >
> /**********************************************************
> *******************/
> >  /* Custom init context */
> >
> > -/* Wait up to 20s for the +READY URC */
> > -#define READY_WAIT_TIME_SECS 20
> > -
> >  typedef struct {
> >      MMPortSerialAt *port;
> >      GRegex         *ready_regex;
> > @@ -147,8 +144,13 @@ wait_for_ready (GTask *task)
> >                                                     task,
> >                                                     NULL);
> >
> > +    mm_dbg ("(%s/%s) waiting %d seconds for init timeout",
> > +            mm_port_probe_get_port_subsys (probe),
> > +            mm_port_probe_get_port_name   (probe),
> > +            mm_kernel_device_get_property_as_int
> (mm_port_probe_peek_port(probe),
> "ID_MM_UBLOX_PORT_READY_DELAY"));
> > +
> >      /* Otherwise, let the custom init timeout in some seconds. */
> > -    ctx->timeout_id = g_timeout_add_seconds (READY_WAIT_TIME_SECS,
> (GSourceFunc) ready_timeout, task);
> > +    ctx->timeout_id = g_timeout_add_seconds
> (mm_kernel_device_get_property_as_int
> (mm_port_probe_peek_port(probe),
> "ID_MM_UBLOX_PORT_READY_DELAY"), (GSourceFunc) ready_timeout,
> task);
> 
> How about having a single call reading the udev tag value in
> ublox_custom_init() and passing the value to wait_for_ready()?
> 
> E.g.
> static void
> wait_for_ready (GTask *task,
>                 guint wait_timeout_secs)
> {
> ...
> }
> 

The only issue with that is wait_for_ready() is also called by the quick_at_ready() function which is a callback function, so I can't pass in the wait_timeout_secs to that instance.  I was thinking instead of adding wait_timeout_secs to the CustomInitContext struct since that is already available to the wait_for_ready() function no matter how it is called. Then ctx->wait_timeout_secs can be set in the ublox_custom_init() function, unless you have another recommendation.

> >  }
> >
> >  static void
> > @@ -225,8 +227,10 @@ ublox_custom_init (MMPortProbe         *probe,
> >          return;
> >      }
> >
> > -    /* Device hotplugged, wait for READY URC */
> > -    wait_for_ready (task);
> > +    if (mm_kernel_device_get_property_as_int
> (mm_port_probe_peek_port(probe),
> "ID_MM_UBLOX_PORT_READY_DELAY") != 0) {
> 
> Given that we're reading an int, possibly better > 0 instead of != 0.
> 

Yes, it will also catch the case where the mm_kernel_device_get_property_as_int() function encounters an error and returns -1.  In that case it would just default to no delay.

Based on my recommendation above of adding wait_timeout_secs to the CustomInitContext struct, I would also add an additional check for a value less than 0 and reset the wait_timeout_secs to 0 for the case where the wait_for_ready() function is called from the quick_at_ready() callback.

> > +           /* Device hotplugged and has a defined ready delay, wait for READY
> URC */
> > +           wait_for_ready (task);
> > +    }
> >  }
> >
> >
> /**********************************************************
> *******************/
> > --
> > 2.14.1
> >
> > _______________________________________________
> > ModemManager-devel mailing list
> > ModemManager-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
> 

Regards,
Matthew Starr