Re: vendor models and options

[Giacinto Cifelli <gciofono@gmail.com>]

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

Hi Denis,

On Fri, Sep 14, 2018 at 6:01 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> So first of all, please stop top-posting on this mailing list.  All
> comments should be in-line.
>
>
understood.


> > enum gemalto_auth_type {
> > OV_GEMALTO_AUTH_CGAUTH = 0,
>
> CGAUTH is a release 11 feature and should be simply probed for in the
> gprs-context initialization procedure, similar to how we test for
> +CGDATA support.
>

> OV_GEMALTO_AUTH_SGAUTH_PWD_USER = 1,
> > OV_GEMALTO_AUTH_SGAUTH_USER_PWD = 2,
>
> You will need to explain the difference between SGAUTH 1 & 2 to me.  Is
> it the parameter order that is different between firmware versions?
> Can't these simply be queried properly using AT^SGAUTH=?
>
> > };
>
> yes, CGAUTH can be queried, but there is no fallback command in case it
fails, so it has to be pre-defined based on the model used.
No query commands would not return the name of the parameters, which in
this case is the only difference between username and password.



> So are you introducing a new gprs-context driver for gemalto or using
>
these for the 'atmodem' driver?
>
>
I am introducing a specific gprs-context for some models, using atmodem for
others, and mbim and qmi for other models.
I try to stick to gprs (not gprs-context) for most of them, because this is
pretty much the same for most of them. For some models that have a lot of
differences we are going to use specific atoms.
We use the same code for the lte atom as well, unless there is a way to
factorize it.

Just to explain the target, overall we are introducing support for more
than 50 models (not firmware versions, plain different models).
Historically, an entire family of models share the same USB enumeration,
despite the differences in hardware and software (for example for voice
support), so there will be about two dozen USB PID, but more models behind.

>
> > struct ov_gemalto {
> > enum gemalto_auth_type auth;
> > gboolean vts_with_quotes;
>
> Sheesh, your firmware guys somehow managed to screw up the VTS command
> syntax?  That command has been in there for 20 years...
>

and our Siemens/Cinterion/Gemalto from earlier than that.
For backward compatibility, model after model, we kept the pre-standard
syntax in some models, and introduced the standard one in other models
whenever possible.


>
> Anyhow, I strongly discourage you from modifying the atmodem voicecall.c
> driver.  Every vendor should have voice call state reporting commands
> that are far superior to the AT+CLCC polling we do in the 'atmodem'
> voicecall.c driver.  So it should be fairly trivial to probe for +VTS
> syntax there.
>
>
we already discussed this.
We do plan to have our own voicecall.c using URCs instead of polling, among
the rest.
We planned to do it a bit later, but I think you are right, we can already
clone the atmodem/voicecall for this change, then introducing the rest of
the changes later.


> Worst case you can use ofono_modem_set_* family to set a particular
> modem specific property which can be available to all atom drivers via
> ofono_modem_get_*.  So this VTS syntax can be handled that way.
>
> > };
> >
>
>
I think ofono_modem_set_*/ofono_modem_get_* is just what we need for the
two examples above (and others).


> <snip>
>
> >
> > and then change the *_create() functions to use 'struct
> > ofono_vendor_parameters *' instead of 'unsigned int vendor', with
> > default &ov_generic.
> >
> > In the example above, I have shown differences for the various Gemalto
> > authentication commands (3 options), that  cross with the options for
> VTS.
> > And then most of the settings are common for Gemalto models (currently
> > factored with the vendor=OFONO_VENDOR_CINTERION).
> > If  tomorrow I have to declare models for these simple settings, I have
> > to add at least 6 with the current system, and having a full driver just
> > because of these two AT commands seems needless.
>
> No, this is all completely unneeded as far as I'm concerned.  If you
> want to use +CGAUTH, then just probe it properly and don't pass a vendor
> at all.  The VTS stuff I already covered.
>
>
well, the point is that we need to use CGAUTH for some models, AND the rest
of the code selected with OFONO_VENDOR_CINTERION within the same atom.
But the aforementioned ofono_modem_set_*/ofono_modem_get_* is the mechanism
that we need.


> >
> > The code would be simplified because in general in a switch there will
> > be only 1 vendor, with the current system there would be 6.
> >
> > Is it ok if I update the code with this vendor system? I will have to
> > submit the entire tree, not by subdirectories, to have a consistent and
> > compiling commit.
> >
>
> Right now I'm completely unconvinced that such an invasive change is
> warranted.  So no, please don't do this.
>

After your comments, I am convinced as well that we don't need this.
You were also right that with an example we could explain each other better.


>
> Regards,
> -Denis
>

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 7180 bytes --]