9:48 p.m.
From: Richard Röjfors <richard(a)puffinpack.se>
Previously the valid "unknown" registration status was set
during startup, but its a bit problematic for gprs.
There might be cases where a LTE context is activated
before netreg is finished updating its status.
And then gprs could make faulty actions.
Instead we set the status to -1 when the drivers supports
fetching the status manually, which is done during startup.
During the time the status is -1, gprs postpones actions until
the status is valid (>= 0).
---
src/gprs.c | 12 ++++++++++--
src/network.c | 5 ++++-
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/gprs.c b/src/gprs.c
index bdb8c8a2..07802c36 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1661,6 +1661,14 @@ static void gprs_netreg_update(struct ofono_gprs *gprs)
{
ofono_bool_t attach;
+ /* This function can get called by other reasons than netreg
+ * updating its status. So check if we have a valid netreg status yet.
+ * The only reason for not having a valid status is basically during
+ * startup while the netreg atom is fetching the status.
+ */
+ if (gprs->netreg_status < 0)
+ return;
+
attach = gprs->netreg_status == NETWORK_REGISTRATION_STATUS_REGISTERED;
attach = attach || (gprs->roaming_allowed &&
@@ -3083,8 +3091,8 @@ struct ofono_gprs *ofono_gprs_create(struct ofono_modem *modem,
break;
}
- gprs->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
- gprs->netreg_status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
+ gprs->status = -1;
+ gprs->netreg_status = -1;
gprs->used_pids = l_uintset_new(MAX_CONTEXTS);
return gprs;
diff --git a/src/network.c b/src/network.c
index 2824eae6..1db01bcf 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1874,7 +1874,7 @@ struct ofono_netreg *ofono_netreg_create(struct ofono_modem *modem,
if (netreg == NULL)
return NULL;
- netreg->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
+ netreg->status = -1;
netreg->location = -1;
netreg->cellid = -1;
netreg->technology = -1;
@@ -1893,6 +1893,9 @@ struct ofono_netreg *ofono_netreg_create(struct ofono_modem *modem,
continue;
netreg->driver = drv;
+ /* Check if we can init the status properly using the driver */
+ if (netreg->driver->registration_status == NULL)
+ netreg->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
break;
}
--
2.20.1
11:04 p.m.
Hi Richard,
On 8/1/19 6:48 AM, richard.rojfors(a)gmail.com wrote:
From: Richard Röjfors <richard(a)puffinpack.se>
Previously the valid "unknown" registration status was set
during startup, but its a bit problematic for gprs.
There might be cases where a LTE context is activated
before netreg is finished updating its status.
And then gprs could make faulty actions.
Instead we set the status to -1 when the drivers supports
fetching the status manually, which is done during startup.
During the time the status is -1, gprs postpones actions until
the status is valid (>= 0).
---
src/gprs.c | 12 ++++++++++--
src/network.c | 5 ++++-
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/src/gprs.c b/src/gprs.c
index bdb8c8a2..07802c36 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1661,6 +1661,14 @@ static void gprs_netreg_update(struct ofono_gprs *gprs)
{
ofono_bool_t attach;
+ /* This function can get called by other reasons than netreg
+ * updating its status. So check if we have a valid netreg status yet.
+ * The only reason for not having a valid status is basically during
+ * startup while the netreg atom is fetching the status.
+ */
Note doc/coding-style.txt item M2 for our commenting style...
+ if (gprs->netreg_status < 0)
+ return;
+
attach = gprs->netreg_status == NETWORK_REGISTRATION_STATUS_REGISTERED;
attach = attach || (gprs->roaming_allowed &&
@@ -3083,8 +3091,8 @@ struct ofono_gprs *ofono_gprs_create(struct ofono_modem *modem,
break;
}
- gprs->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
- gprs->netreg_status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
+ gprs->status = -1;
Why do you need to mess with this? It seems to be not used
+ gprs->netreg_status = -1;
gprs->used_pids = l_uintset_new(MAX_CONTEXTS);
Wouldn't changes to gprs.c be sufficient, without touching netreg.c?
return gprs;
diff --git a/src/network.c b/src/network.c
index 2824eae6..1db01bcf 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1874,7 +1874,7 @@ struct ofono_netreg *ofono_netreg_create(struct ofono_modem
*modem,
if (netreg == NULL)
return NULL;
- netreg->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
+ netreg->status = -1;
Okay, but a minor problem with this is that there now exists a brief
moment in type where the Status property has a weird value...
netreg->location = -1;
netreg->cellid = -1;
netreg->technology = -1;
@@ -1893,6 +1893,9 @@ struct ofono_netreg *ofono_netreg_create(struct ofono_modem
*modem,
continue;
netreg->driver = drv;
+ /* Check if we can init the status properly using the driver */
+ if (netreg->driver->registration_status == NULL)
+ netreg->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
break;
}
Regards,
-Denis
11:33 p.m.
Hi Denis,
Den fre 2 aug. 2019 kl 13:04 skrev Denis Kenzior <denkenz(a)gmail.com>:
Hi Richard,
On 8/1/19 6:48 AM, richard.rojfors(a)gmail.com wrote:
> From: Richard Röjfors <richard(a)puffinpack.se>
>
> Previously the valid "unknown" registration status was set
> during startup, but its a bit problematic for gprs.
> There might be cases where a LTE context is activated
> before netreg is finished updating its status.
> And then gprs could make faulty actions.
> Instead we set the status to -1 when the drivers supports
> fetching the status manually, which is done during startup.
> During the time the status is -1, gprs postpones actions until
> the status is valid (>= 0).
> ---
> src/gprs.c | 12 ++++++++++--
> src/network.c | 5 ++++-
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/src/gprs.c b/src/gprs.c
> index bdb8c8a2..07802c36 100644
> --- a/src/gprs.c
> +++ b/src/gprs.c
> @@ -1661,6 +1661,14 @@ static void gprs_netreg_update(struct ofono_gprs
*gprs)
> {
> ofono_bool_t attach;
>
> + /* This function can get called by other reasons than netreg
> + * updating its status. So check if we have a valid netreg status
yet.
> + * The only reason for not having a valid status is basically
during
> + * startup while the netreg atom is fetching the status.
> + */
Note doc/coding-style.txt item M2 for our commenting style...
Will fix the comment!
> + if (gprs->netreg_status < 0)
> + return;
> +
> attach = gprs->netreg_status ==
NETWORK_REGISTRATION_STATUS_REGISTERED;
>
> attach = attach || (gprs->roaming_allowed &&
> @@ -3083,8 +3091,8 @@ struct ofono_gprs *ofono_gprs_create(struct
ofono_modem *modem,
> break;
> }
>
> - gprs->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
> - gprs->netreg_status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
> + gprs->status = -1;
Why do you need to mess with this? It seems to be not used
This was just of convenience reasons so it would look the same for gprs and
netreg status.
> + gprs->netreg_status = -1;
> gprs->used_pids = l_uintset_new(MAX_CONTEXTS);
>
Wouldn't changes to gprs.c be sufficient, without touching netreg.c?
No unfortunately not. gprs reads the netreg status when it attaches to the
atom. Which would be unknown if we do not touch netreg.c.
.. and unknown is a valid status which can not be distinguished from "not
yet updated".
> return gprs;
> diff --git a/src/network.c b/src/network.c
> index 2824eae6..1db01bcf 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -1874,7 +1874,7 @@ struct ofono_netreg *ofono_netreg_create(struct
ofono_modem *modem,
> if (netreg == NULL)
> return NULL;
>
> - netreg->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
> + netreg->status = -1;
Okay, but a minor problem with this is that there now exists a brief
moment in type where the Status property has a weird value...
I looked at this, and most statuses, even some defined results as unknown.
Any unhandled values as negative would do that too.
If you are refering to the D-Bus property?
> netreg->location = -1;
> netreg->cellid = -1;
> netreg->technology = -1;
> @@ -1893,6 +1893,9 @@ struct ofono_netreg *ofono_netreg_create(struct
ofono_modem *modem,
> continue;
>
> netreg->driver = drv;
> + /* Check if we can init the status properly using the
driver */
> + if (netreg->driver->registration_status == NULL)
> + netreg->status =
NETWORK_REGISTRATION_STATUS_UNKNOWN;
> break;
> }
>
>
--Richard
11:39 p.m.
Hi Richard,
> + gprs->netreg_status = -1;
> gprs->used_pids = l_uintset_new(MAX_CONTEXTS);
>
Wouldn't changes to gprs.c be sufficient, without touching netreg.c?
No unfortunately not. gprs reads the netreg status when it attaches to
the atom. Which would be unknown if we do not touch netreg.c.
.. and unknown is a valid status which can not be distinguished from
"not yet updated".
Hmm, okay. But then, can't you set gprs->netreg_status to -1 inside
netreg_watch and just wait until netreg_status_changed to get it set
properly?
That seems to be your real culprit, the gprs->netreg_status assignment
from ofono_netreg_get_status()
> return gprs;
> diff --git a/src/network.c b/src/network.c
> index 2824eae6..1db01bcf 100644
> --- a/src/network.c
> +++ b/src/network.c
> @@ -1874,7 +1874,7 @@ struct ofono_netreg
*ofono_netreg_create(struct ofono_modem *modem,
> if (netreg == NULL)
> return NULL;
>
> - netreg->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
> + netreg->status = -1;
Okay, but a minor problem with this is that there now exists a brief
moment in type where the Status property has a weird value...
I looked at this, and most statuses, even some defined results as
unknown. Any unhandled values as negative would do that too.
If you are refering to the D-Bus property?
Yes. Most of the time -1 simply means the property is omitted. In this
case that is not actually what happens. Refer to src/common.c
registration_status_to_string()
Regards,
-Denis
12:50 a.m.
Hi Denis,
Den fre 2 aug. 2019 kl 13:39 skrev Denis Kenzior <denkenz(a)gmail.com>:
Hi Richard,
> > + gprs->netreg_status = -1;
> > gprs->used_pids = l_uintset_new(MAX_CONTEXTS);
> >
>
> Wouldn't changes to gprs.c be sufficient, without touching netreg.c?
>
>
> No unfortunately not. gprs reads the netreg status when it attaches to
> the atom. Which would be unknown if we do not touch netreg.c.
> .. and unknown is a valid status which can not be distinguished from
> "not yet updated".
Hmm, okay. But then, can't you set gprs->netreg_status to -1 inside
netreg_watch and just wait until netreg_status_changed to get it set
properly?
That would work in my case.
I was just worried in case any driver would have
updated netreg with status before calling ofono_netreg_register, then
we would not get the status since there wont be any initial callback.
But if that is not allowed/happens anywhere I agree that its better.
That seems to be your real culprit, the gprs->netreg_status assignment
from ofono_netreg_get_status()
>
>
> > return gprs;
> > diff --git a/src/network.c b/src/network.c
> > index 2824eae6..1db01bcf 100644
> > --- a/src/network.c
> > +++ b/src/network.c
> > @@ -1874,7 +1874,7 @@ struct ofono_netreg
> *ofono_netreg_create(struct ofono_modem *modem,
> > if (netreg == NULL)
> > return NULL;
> >
> > - netreg->status = NETWORK_REGISTRATION_STATUS_UNKNOWN;
> > + netreg->status = -1;
>
> Okay, but a minor problem with this is that there now exists a brief
> moment in type where the Status property has a weird value...
>
>
> I looked at this, and most statuses, even some defined results as
> unknown. Any unhandled values as negative would do that too.
> If you are refering to the D-Bus property?
Yes. Most of the time -1 simply means the property is omitted. In this
case that is not actually what happens. Refer to src/common.c
registration_status_to_string()
--Richard
1143
days inactive
1144
days old
4 comments
3 participants
participants (3)
-
Denis Kenzior -
Richard Röjfors -
richard.rojfors＠gmail.com