Re: ModemManager: improving port probing

[Aleksander Morgado <aleksander lanedo com>]

> > 
> > 1. Plugin Manager
> > -----------------------------------------------
> > 
> > The MMManager object handled too many different tasks, that it made
> > sense to me to have an independent MMPluginManager object which takes
> > care of: 
> >  * Finding plugins and loading them
> >  * Controlling the requests to find the best plugin to support a given
> > port.
> > 
> > So whenever a new port is found/detected, we can just pass it to
> > mm_plugin_manager_find_port_support() and wait for the async method to
> > return the best plugin found for it. When devices with multiple ports
> > are detected, the Plugin Manager also takes care of propagating the
> > first best plugin found to support checks of ports in the same device.
> > 
> > This Plugin Manager work is at the beginning of the branch; and quite
> > independent to the rest of the changes (in other words, directly
> > mergeable to master if you guys like it).
> 
> Sounds like a good idea.  One thing to remember is that if a plugin
> claims a port, all other ports of that physical device should be given
> to that plugin (by comparing the port's udev parent or grandparent).
> Don't really want a situation where we might have two plugins trying to
> fight for the same actual device.
> 

Yes. When a plugin takes the first port of a device, the Plugin Manager
makes sure that only the same plugin tries to take all the other ports.

> > 
> > 
> > 2. Port Support Checks
> > -----------------------------------------------
> > 
> > The whole probing process is now done in a new MMPortProbe object
> > (somewhat equivalent to the MMPluginBaseSupportsTask object, which was
> > removed). The object serves as a temporary storage for already probed
> > information (as the MMPluginBaseSupportsTask one), but also allows
> > specifying what needs to be probed in the port and does also the real
> > probing.
> 
> Yay!  MMPluginBaseSupportsTask was getting really complicated.  Die die die.
> 

:-)

> > The MMPluginBase will control whether a probing is needed, and will also
> > process the results of the probing reported by the MMPortProbe object.
> > Specific plugins are able to set at construction time different property
> > values to modify the both the probing process and the pre- and
> > post-probing filters to run. See the ported Nokia plugin as example:
> > https://gitorious.org/aleksander/modemmanager/commit/d2992abdb1f386962063e6b61ae8bf26eb0f995a
> 
> I forget whether any of the plugins need to modify the probing commands
> they use at runtime, but would this approach preclude that?  x22x for
> example only sends the AT+GMR request if the vendor+product ID is
> 0x1bbb/0x0000.  We don't need it for other modems.
> 

If that is the case, it is pretty easy to extend it. The probing process
takes a default set of vendor probing commands, which have exactly the
same format as the custom init commands the plugin can use. So it would
be just a matter of letting the plugins specify the set of commands to
be used also in vendor probing.

> > The whole port support check done by every plugin can be summarized in
> > the following steps:
> >  * [MMPluginBase] Check whether we need to launch probing (pre-probing
> > filters).
> >  * [MMPortProbe] Launch port probing, get probing results.
> >  * [MMPluginBase] Check whether the results we got are the ones expected
> > (post-probing filters).
> >  * [MMPluginBase] Report port support check results.
> > 
> > 
> > 2.1 Pre-probing filters
> > 
> > Pre-probing filters are those which control whether the probing, as
> > requested by the specific plugin, takes place. The best example of this
> > filter type is the VENDOR_IDS one: The probing request will be aborted
> > (and therefore report as unsupported) unless the port reports a Vendor
> > ID equal to one in the given array.
> 
> Sounds fine too.  Though at least in one instance, Alcatel/TCT branded
> devices use the same vendor/product ID for modems with two different
> firmware types which should be handled by two different modems, and the
> only way we can differentiate that is by the AT+GMR response.
>  

I don't think this would be very difficult to include. I will take a
look at this specific case.

> >  * MM_PLUGIN_BASE_ALLOWED_VENDOR_IDS: A plugin can set this property to
> > a 0-terminated array of uint16s containing the udev-reported vendor IDs
> > it can handle, e.g.:
> > 
> >     static const guint16 vendor_ids[] = { 0x0421 , 0 };
> > 
> >     return MM_PLUGIN (
> >         g_object_new (MM_TYPE_PLUGIN_NOKIA,
> >                       ...
> >                       MM_PLUGIN_BASE_ALLOWED_VENDOR_IDS, vendor_ids,
> >                       NULL));
> > 
> > Other pre-probing filters considered and already implemented are
> > Subsystems, Drivers, Product ID and Udev Tags. See:
> > https://gitorious.org/aleksander/modemmanager/commit/142d0161070777a8a8cac44e8320b07b70fb437a
> > 
> > 
> > 2.2 Probing-specific configurations
> > 
> > If the port passed all pre-probing filters, a probing process can be
> > started. The specific probing to be done directly depends on the
> > requested post-probing filters. Some examples:
> >   ** Cinterion modems do not expose QCDM ports, therefore no QCDM port
> > probing is needed.
> >   ** ZTE modems are never RS232, therefore no Vendor/Product AT-probing
> > is needed.
> > 
> > If all the requested probing information is already cached, the probing
> > is not launched again. This may happen when probing a port in a RS232
> > modem, where capabilities and Vendor-string probing may be done by an
> > earlier plugin.
> > 
> > When any AT-specific probing is requested (capabilities, vendor,
> > product), we first check whether the port is an AT port or not (just
> > sending "AT" and not expecting any error). If the port is found to be an
> > AT port, capabilities/vendor/product probing can continue. We will only
> > check if the port is to be QCDM if the plugin object was created with
> > MM_PLUGIN_BASE_ALLOWED_QCDM set to TRUE.
> 
> Sounds fine, though in most cases a QCDM port can be exposed by any
> kernel serial driver.  But we'll know for some plugins that we never
> expect QCDM ports (Ericsson/mbm, Nokia) so we can ignore QCDM probing
> for those.  Most other plugins *could* handle QCDM ports though, even if
> they currently don't.
> 

Well, then we could default to include QCDM probing, and let plugins
decide that they don't want it.

> > As before, plugins are allowed to specify custom initialization
> > commands, in this case with the MM_PLUGIN_BASE_CUSTOM_INIT property
> > which expects a NULL-terminated array of the new MMPortProbeAtCommand
> > structs. This custom initialization can also be used to check whether
> > the port is an AT port or not. As an example, the Nokia plugin may use:
> 
> We've got a few cases of this:
> 
> 1) novatel: the main port needs to run DMAT=1 which flips the secondary
> port into AT command mode, and then the secondary port can be probed as
> an AT port
> 
> 2) huawei: turn off unsolicited messages on the secondary port, which if
> it's not opened and being listened to, the firmware appears to keep
> sending them, run out of buffer space because nothing is listening, and
> then crash.  we also try to get the portmap right away so that we have
> hints about what type of ports the others are
> 
> 3) longcheer/x22x: grab the AT+GMR response because this is the only way
> to distinguish between the X200 and the X060s modems, one of which is
> driven by longcheer, and the other by x22x, since they both use the same
> USB product/vendor but use completely different firmwares, and only the
> AT+GMR response is unique
> 
> 4) zte: same sort of thing as huawei, they flood the secondary port with
> unsolicited indications the first time you open it, and eventually fill
> up the firmware serial buffer and crash
> 

Not a big deal all this custom commands, I would say. I will try to
implement every case one by one, and then ping for help in testing.

> >     static gboolean
> >     parse_init (const gchar *response,
> >                 const GError *error,
> >                 GValue *result,
> >                 GError **result_error)
> >     {
> >         if (error)
> >             /* Go on to next command */
> >             return FALSE;
> > 
> >         /* If we didn't get any error, it is an AT port */
> >         g_value_init (result, G_TYPE_BOOLEAN);
> >         g_value_set_boolean (result, TRUE);
> >         return TRUE;
> >     }
> > 
> >     static gboolean
> >     parse_init_last (const gchar *response,
> >                      const GError *error,
> >                      GValue *result,
> >                      GError **result_error)
> >     {
> >         g_value_init (result, G_TYPE_BOOLEAN);
> >         /* On last error, report as not being an AT port */
> >         g_value_set_boolean (result, error ? FALSE : TRUE);
> >         return TRUE;
> >     }
> > 
> >     /* ------ */
> > 
> >     static const MMPortProbeAtCommand custom_init[] = {
> >         { "ATE1 E0", parse_init },
> >         { "ATE1 E0", parse_init },
> >         { "ATE1 E0", parse_init_last },
> >         { NULL }
> >     };
> 
> Looks good; though I wonder how we'd handle the x22x/longcheer case of
> only probing AT+GMR based on specific vendor/product IDs.  We don't
> *need* to restrict it to specific vendor/product IDs though, but other
> types of probing we might want to restrict with various other filters.
> 

We could say that those plugins need explicit AT vendor probing, as if
they were RS232 modems (where udev-reported IDs cannot be trusted). That
would probably cover the case, although maybe I'm missing something.

> >     return MM_PLUGIN (
> >         g_object_new (MM_TYPE_PLUGIN_NOKIA,
> >                       ...
> >                       MM_PLUGIN_BASE_CUSTOM_INIT, custom_init,
> >                       NULL));
> > 
> > This is, "ATE1 E0" will be sent up to 3 times, and if we kept getting
> > errors in all 3 tries, the port is reported as not being an AT port.
> > Being able to say whether a port is AT or not from the custom init is
> > just about convenience, the custom init command is of course not for
> > that and the callback is allowed to return without setting the output
> > 'result'. Also, if the callback sets 'result_error', the whole probing
> > process would get cancelled. See:
> > https://gitorious.org/aleksander/modemmanager/commit/40e66ea506101a0af6922bde24ad9579392f0429
> 
> This sounds fine.
> 

Great.

> > 
> > 2.3 Post-probing filters
> > 
> > Post-probing filters are those which control whether the plugin can
> > support the port once the desired probing results are ready. The best
> > example of this filter type is the CAPABILITIES one: The support request
> > will report unsupported unless the probed capabilities from the port
> > match the capabilities given in the property.
> > 
> >  * MM_PLUGIN_BASE_ALLOWED_CAPABILITIES: A plugin can set this property
> > to a guint representing a mask of flags where supported capabilities are
> > given, e.g.:
> > 
> >     static const guint32 capabilities =
> > MM_PORT_PROBE_CAPABILITY_GSM_OR_CDMA;
> > 
> >     return MM_PLUGIN (
> >         g_object_new (MM_TYPE_PLUGIN_NOKIA,
> >                       ...
> >                       MM_PLUGIN_BASE_ALLOWED_CAPABILITIES, capabilities,
> >                       NULL));
> > 
> > Other post-probing filters considered and implemented are Vendor Strings
> > and Product Strings (used only in plugins supporting RS232 devices).
> > See:
> > https://gitorious.org/aleksander/modemmanager/commit/bc1d62ddb58abbff3eea21196deac8cf9eff3473
> 
> Need the GMR results for x22x/longcheer for the Alcatel/TCT device too.
> Sounds fine though.
> 
> > 
> > 2.4 Port support results
> > 
> > Given a port, there is always a single plugin that may support it. MM
> > currently reports 'support level' values (given as an integer in the
> > [0-100] range), but the truth seems to be that only one single plugin
> > ends up reporting a support_level > 0 (due to the udev or AT-probed
> > vendor/product filtering). Therefore, a new SUPPORTED state was added to
> > the possible replies of the port support check done by the plugins, to
> > be used instead of reporting a support_level > 0.
> 
> Yeah, we can ditch support_level, that was an idea that never, ever
> happened and is pointless.  Only one plugin will support a modem at any
> given time.
> 

Great!

> > The whole supports_port() method was also changed so that it always
> > reports results asynchronously (so there is no longer need for
> > IN_PROGRESS status). This eases a lot the management of port support
> > requests within the Plugin Manager. See:
> > https://gitorious.org/aleksander/modemmanager/commit/b07189f14fe6e88a87cd8db5312d9fdf6734e680
> 
> Sweet.
> 
> > 
> > 3. TODOs
> > -----------------------------------------------
> > 
> > 3.1 Port Grabbing: In the same way that the port support checking was
> > fully moved to the MMPluginBase, port grabbing can also possibly be
> > completely moved out of the plugin implementations. For example, letting
> > plugins specify in a new property the GType of the modem object to
> > create when GSM capability is found, and such.
> 
> One thing we want to fix up with this is allowing plugins with multiple
> capabilities, since there are modems out there (Qualcomm ones) that
> support both GSM and CDMA that we just don't handle right now.  So this
> is intimately tied into the multi-capability modem changes we've talked
> about extensively.  Ideally the probing process does just enough to
> determine that the port is (a) an [AT, qcdm, qmi, wmc, cns, ethernet]
> port and (b) what plugin to use for that port and that's it; hopefully
> we can punt the GSM/CDMA/LTE capabilities decision until later because
> that's not always available via AT+GCAP for example.  This would help us
> get the modems detected more quickly too.
> 

I will dig into this, and comment later.

> Also of note is that we still need to ensure the blacklist/whitelist
> stuff works, since people have Uninteruptible Power Supplies that
> sometimes crash when you send them the wrong strings, and by default
> these get probed by MM right now unless the whitelist/blacklist is
> installed.
> 
> > 3.2 Port remaining plugins to the new probing process. I first wanted to
> > discuss all these ideas in the list before going on with that.
> > 
> > 
> > Comments?
> 
> I'd port at least ZTE over to the new probing process, I'm happy to test
> since I have like 50 ZTE modems.
> 

ZTE, Huawei and Wavecom will be next in the porting, as those are the
ones I've got at home.

> But basically, if you say its easier, clearer, faster, and less
> error-prone, I trust you that the solution you've come up with is those
> things and I'm happy to rely on your judgment.  I think the current
> probing code has gotten pretty ugly and a cleanup is most welcome.
> 

I'm happy to keep on developing with these ideas, and once ready, we can
see if they really make sense or not.

Cheers!

-- 
Aleksander