Den mån 30 sep. 2019 kl 19:00 skrev Denis Kenzior <[email protected]>:
> Hi Richard, > > On 9/29/19 2:39 PM, [email protected] wrote: > > From: Richard Röjfors <[email protected]> > > > > It turns out that both L2xx and L4xx modems are a bit > > buggy when it comes to send CREG URC's when the tech changes. > > Try to overcome this by subscribing to both UREG and CREG, > > and poll the other when any of the URC's are received. > > Protect from doing simultanious polls though. > > nit: typo, simultaneous > Thanks :) > > Also, this patch doesn't apply? > This is strange, I did a git send-email. Anyway, I have changes to do according to comments below, so I will make sure to rebase on master. > > denkenz@localhost ~/ofono-master $ git am --3way ~/merge/\[PATCH\]\ > ublox\:\ netreg\:\ Also\ subscribe\ to\ UREG\ URC\'s.eml > Applying: ublox: netreg: Also subscribe to UREG URC's > Using index info to reconstruct a base tree... > error: patch failed: drivers/ubloxmodem/network-registration.c:47 > error: drivers/ubloxmodem/network-registration.c: patch does not apply > error: Did you hand edit your patch? > It does not apply to blobs recorded in its index. > Patch failed at 0001 ublox: netreg: Also subscribe to UREG URC's > hint: Use 'git am --show-current-patch' to see the failed patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". > > > --- > > drivers/ubloxmodem/network-registration.c | 198 ++++++++++++++++------ > > 1 file changed, 148 insertions(+), 50 deletions(-) > > > > diff --git a/drivers/ubloxmodem/network-registration.c > b/drivers/ubloxmodem/network-registration.c > > index 69af4644..ad905fb3 100644 > > --- a/drivers/ubloxmodem/network-registration.c > > +++ b/drivers/ubloxmodem/network-registration.c > > @@ -47,11 +47,15 @@ > > static const char *none_prefix[] = { NULL }; > > static const char *cmer_prefix[] = { "+CMER:", NULL }; > > static const char *ureg_prefix[] = { "+UREG:", NULL }; > > +static const char *creg_prefix[] = { "+CREG:", NULL }; > > + > > +#define UBLOX_UPDATING_STATUS 0x01 > > > > struct netreg_data { > > struct at_netreg_data at_data; > > > > const struct ublox_model *model; > > + int flags; > > Nowadays we prefer to use bool foo : 1 notation to manipulate bit fields. > > So maybe something like: > > bool updating_status : 1; > Will filx. > > > }; > > > > struct tech_query { > > @@ -213,13 +217,72 @@ static void ctze_notify(GAtResult *result, > gpointer user_data) > > ofono_netreg_time_notify(netreg, &nd->time); > > } > > > > -static void ublox_query_tech_cb(gboolean ok, GAtResult *result, > > +static int ublox_ureg_state_to_tech(int state) > > +{ > > + switch (state) { > > + case 1: > > + return ACCESS_TECHNOLOGY_GSM; > > + case 2: > > + return ACCESS_TECHNOLOGY_GSM_EGPRS; > > + case 3: > > + return ACCESS_TECHNOLOGY_UTRAN; > > + case 4: > > + return ACCESS_TECHNOLOGY_UTRAN_HSDPA; > > + case 5: > > + return ACCESS_TECHNOLOGY_UTRAN_HSUPA; > > + case 6: > > + return ACCESS_TECHNOLOGY_UTRAN_HSDPA_HSUPA; > > + case 7: > > + return ACCESS_TECHNOLOGY_EUTRAN; > > + case 8: > > + return ACCESS_TECHNOLOGY_GSM; > > + case 9: > > + return ACCESS_TECHNOLOGY_GSM_EGPRS; > > + default: > > + /* Not registered for PS (0) or something unknown (>9)... > */ > > + return -1; > > + } > > +} > > + > > +static gboolean is_registered(int status) > > +{ > > + return status == NETWORK_REGISTRATION_STATUS_REGISTERED || > > + status == NETWORK_REGISTRATION_STATUS_ROAMING; > > +} > > + > > +static void ublox_creg_cb(gboolean ok, GAtResult *result, > > + gpointer user_data) > > +{ > > + struct tech_query *tq = user_data; > > + struct netreg_data *nd = ofono_netreg_get_data(tq->netreg); > > + int status, lac, ci, tech; > > One variable declaration per line please. > Will fix > > > + > > + nd->flags &= ~UBLOX_UPDATING_STATUS; > > + > > + if (!ok) > > + return; > > + > > + if (at_util_parse_reg(result, "+CREG:", NULL, &status, > > + &lac, &ci, &tech, OFONO_VENDOR_GENERIC) == > FALSE) > > + return; > > + > > + /* The query provided a tech, use that */ > > + if (is_registered(status) && tq->tech != -1) > > + tech = tq->tech; > > + > > + ofono_netreg_status_notify(tq->netreg, status, lac, ci, tech); > > +} > > + > > +static void ublox_ureg_cb(gboolean ok, GAtResult *result, > > gpointer user_data) > > { > > struct tech_query *tq = user_data; > > + struct netreg_data *nd = ofono_netreg_get_data(tq->netreg); > > GAtResultIter iter; > > gint enabled, state; > > - int tech = -1; > > + int tech = tq->tech; > > + > > + nd->flags &= ~UBLOX_UPDATING_STATUS; > > > > if (!ok) > > goto error; > > <snip> > > > +static void ureg_notify(GAtResult *result, gpointer user_data) > > +{ > > + struct ofono_netreg *netreg = user_data; > > + int state; > > + struct netreg_data *nd = ofono_netreg_get_data(netreg); > > + struct tech_query *tq; > > + GAtResultIter iter; > > + > > + if (nd->flags & UBLOX_UPDATING_STATUS) > > + return; > > + > > + g_at_result_iter_init(&iter, result); > > + > > + if (!g_at_result_iter_next(&iter, "+UREG:")) > > + return; > > + > > + if (!g_at_result_iter_next_number(&iter, &state)) > > + return; > > + > > + tq = g_new0(struct tech_query, 1); > > + > > + tq->tech = ublox_ureg_state_to_tech(state); > > + tq->netreg = netreg; > > + > > + if (g_at_chat_send(nd->at_data.chat, "AT+CREG?", creg_prefix, > > + ublox_creg_cb, tq, g_free) > 0) { > > + nd->flags |= UBLOX_UPDATING_STATUS; > > + return; > > + } > > + > > + g_free(tq); > > +} > > + > > So it seems to me you're firing off a CREG? when UREG unsolicited comes > in but also firing off UREG? when CREG unsolicited comes in. > Yes that's the idea, but in case we are already firing off something, at least we avoid acting it on the second URC. The reason for fire off when we get CREG is that the tech is sometimes incorrect in the CREG. The reason for fire off when we get UREG is that the CREG is sometimes missing, so we need to poll CREG to get status, lac, ci. > Briefly looking at some random ublox docs via google, I understand that > UREG was meant to tell you what Packet Switching technology was active. > One can still be connected to Voice Circuit Switched service with no PS > active. So I'm not entirely sure whether the two can be assumed to be > related the way you do it here...? > This is completely true. And in that case it will signal UREG status 0, and we then fall back to use the status from CREG. I'm afraid this is the best we can do, since I have seen missing CREG's and CREG's with incorrect status. In those occasions it has fired UREG URC's at least.. > > I really see nothing that can go wrong in being paranoid and re-querying > CREG/UREG, so I'm ok taking this up. Just wondering whether you'll end > up fixing this again later. > I see your point. All this is to try doing the best to work around bugs in the modem. I'm afraid we have to live with it. Even if it gets fixed in later FW there will be modems out there that works like this. What I read from Jonas and comments in the code is that the L4xx seems even worse. For the L210 it seems like the URC's are missing only sometimes. But those situation could be annoying. We could switch from E-UTRAN to UTRAN and should attach and activate context. But ofono will still think we are on E-UTRAN and just waiting for the context to show up...(if the URCs are missing) --Richard
_______________________________________________ ofono mailing list -- [email protected] To unsubscribe send an email to [email protected]
