Re: [PATCH 2/7] extended support for LTE and NR. Some minor fixes. part 2 of 7

[Giacinto Cifelli <gciofono@gmail.com>]

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

Hi Denis,

On Wed, Sep 19, 2018 at 8:24 PM Denis Kenzior <denkenz@gmail.com> wrote:

> Hi Giacinto,
>
> > I had a look at this all, and I have a problem. I cannot check the
> > parameters as they are entered one by one.
> > Example: if I blank user/pwd when the authentication is changed to NONE,
> > then if changed again to CHAP, the module will reject it.
> > Shall I store the parameters, or keep them also in case of error?
>
> So in the case of ConnectionContext the parameters are not sent out to
> the driver until context activation is attempted.  Hence all the
> settings are immediate and the activation fails or it succeeds.
>
> However, in the LongTermEvolution driver setup the settings are
> immediate.  Thus the D-Bus API you propose is thus not really suitable
> and needs to be modified. Since we're kind of stuck with the
> 'DefaultAccessPointName' property at this point, the only two ways out
> of this I can think of are:
>
> - Have the driver handle this.  So if PAP/CHAP is selected but username
> or password are invalid, default to no authentication.  The assumption
> will be that eventually valid parameters are given.
>
> Or perhaps we only call out into the driver method once the parameters
> in their entirety are valid, accepting whatever the user puts in as long
> as the individual property input is valid according to core validity
> checks.
>

We cannot change the external API, but internally we are free to rearrange
the code.

how do you feel about the proposal below for a second set of functions, to
override the existing ones
for newer lte atoms?

in src/lte.c

static DBusMessage *lte_set_property(DBusConnection *conn,
DBusMessage *msg, void *data)
{
[...]
if (lte->driver->set_property)
lte->driver->set_property(...);
else if (lte->driver->set_default_attach_info)
lte->driver->set_default_attach_info(...);
else
return __ofono_error_not_implemented(msg);
[...]

in src/modem.c:

static void sim_state_watch(enum ofono_sim_state new_state, void *user)
{
[...]
case OFONO_SIM_STATE_READY:
modem_change_state(modem, MODEM_STATE_OFFLINE);

ofono_lte_set_reginfo(modem);

/* Modem is always online, proceed to online state. */
if (modem_is_always_online(modem) == TRUE)
set_online(modem, TRUE);

if (modem->online == TRUE)
modem_change_state(modem, MODEM_STATE_ONLINE);
else if (modem->get_online)
modem->driver->set_online(modem, 1, common_online_cb,
modem);

modem->get_online = FALSE;

break;
}

I see that if(modem_is_always_online) maybe it will come too late, but in
that case also the current solution
with set_default_attach_info suffers from the same limitation, so no
regression.

Of course then, again in src/lte.c, the function ofono_lte_set_reginfo will
look for an lte atom, if present will call it without parameter for sending
the AT+CGDCONT and AT+CGAUTH to the modem.

Would this work for you?



> ....
>
> >
> > Another way would be to have a SetParameters() function, and set them
> > all together, including the APN, and not allowing writing them
> > separately, apart from the APN which already exists.
> > I don't really like it, either.
> >
>
> As you point out, this is the second alternative.  AuthenticationMethod,
> Username & Password would need to be set via a method call and
> optionally exposed as [readonly] properties.  Protocol could still be
> handled as per DefaultAccessPointName or inside the aforementioned
> method call.
>
> > Or introduce an atom function that is called before
> modem->set_online(true)?
> >
>
> This might be trickier, but could also work...
>
> Regards,
> -Denis
>

Regards,
Giacinto

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