[RESEND PATCH 0/5] quectel: add support for BG95

[RESEND PATCH 0/5] quectel: add support for BG95

[Alexandru Ardelean]

// This is a RESEND series, because the send to the ofono@ofono.org email
// has failed.
// It's not entirely clear that the new list may be 'ofono@lists.linux.dev'

The BG95 modem has currently 7 sub-variants (BG95-{M1,M2,M3,M4,M5,M6,MF}),
but we've tested and validated BG95-M3. The AT commands covered in the docs
(and ofono) should work with all the sub-variants.

The BG95 (family) is a smaller brother of the BG96, with the main
difference that it does not suppot QMI.

It supports AT commands to configure the modem.
The AT command-set is roughly similar to the EC21 and EC200 (already
supported by ofono). Which is why the selected vendor is
OFONO_VENDOR_QUECTEL_EC2X.

There is only one AT command (in ofono) that will not work (as for the
EC2X); that is 'AT+QINDCFG="act",1'. The 'AT+QINDCFG="csq",1' command is
supported (in that same group). Which is why, this change does not add a
new vendor ID.

It supports LTE Cat M1 and Nb-IoT. And ofono (in the atmodem driver)
supports only EUTRAN. Which is why some changes (in the GPRS code) had to be
done for BG95 to work with LTE-only carriers that support these LTE types
of tech.

Alexandru Ardelean (3):
  gprs: rework have_active_contexts() to check for non-active contexts
  gprs: extend on_lte() to check for M1 and Nb-IOT techs
  plugins: quectel: re-organize code for ussd & lte init

Mircea Caprioru (2):
  atmodem: gprs-context: fix ATD99 init for modems with no slave channel
  quectel: add support for BG95

 drivers/atmodem/gprs-context.c |  8 +++----
 plugins/quectel.c              | 43 ++++++++++++++++++++++++++++------
 plugins/udevng.c               |  1 +
 src/gprs.c                     | 32 ++++++++++++++++++-------
 4 files changed, 64 insertions(+), 20 deletions(-)

-- 
2.34.1

[RESEND PATCH 1/5] atmodem: gprs-context: fix ATD99 init for modems with no slave channel

[Alexandru Ardelean]

From: Mircea Caprioru <mircea.caprioru@flowkernel.ro>

Patch 476fab4ea ("atmodem: Add support for NW DEACT notifications on slave
channel") has added a check to make sure we have a slave channel before
registering for "+CGEV:" notifications.

Patch 8e929df4f ("atmodem: use ATD99 to enter data state when needed") has
added the ATD99 detection after the slave channel check.

In our case, when we get a modem with no slave channels, the ATD99
detection will not get triggered and modems will use the AT+CGDATA="PPP"
method for connecting to the internet (when ATD99 would be supported).

This patch moves the check a bit lower in the code and removes the
early-return, to avoid for any potential future errors.
---
 drivers/atmodem/gprs-context.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index f9f3c0ab..efc3fe94 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -449,10 +449,6 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
 
 	ofono_gprs_context_set_data(gc, gcd);
 
-	chat = g_at_chat_get_slave(gcd->chat);
-	if (chat == NULL)
-		return 0;
-
 	switch (vendor) {
 	case OFONO_VENDOR_SIMCOM_SIM900:
 		gcd->use_atd99 = FALSE;
@@ -462,7 +458,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
 						at_cgdata_test_cb, gc, NULL);
 	}
 
-	g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
+	chat = g_at_chat_get_slave(gcd->chat);
+	if (chat != NULL)
+		g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
 
 	return 0;
 }
-- 
2.34.1

[RESEND PATCH 2/5] gprs: rework have_active_contexts() to check for non-active contexts

[Alexandru Ardelean]

While trying to add LTE support to the Quectel BG95-M3 modem, we've run
into a bit of a chicken-n-egg dilemma.

While ofono has enumerated (successfully) the GPRS contexts (from the
modem), we got only one (IP), but it's not active.

And trying to activate it, would yield a NotAttached error (via DBus).

The BG95-M3 modem supports Cat M1/Cat NB2/EGPRS LTE.

This situation would show up when trying to connect to an operator that
only has "lte-cat-m1" tech (and nothing else). If it the operator would
also advertise "gsm" tech, ofono would connect via "gsm".

Now, this could be a particularity of this modem (BG95-M3), in which case
it may be an idea to add a hook into the quectel plugin/driver to check the
attached status.

But for now, it seems that allowing the Attached state to be reported for
any present GPRS contexts (even non-active), seems to work.

For EUTRAN, this behavior is still maintained.
---
 src/gprs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 950b40fc..4befe88a 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1515,18 +1515,24 @@ void ofono_gprs_resume_notify(struct ofono_gprs *gprs)
 	update_suspended_property(gprs, FALSE);
 }
 
-static gboolean have_active_contexts(struct ofono_gprs *gprs)
+static gboolean have_contexts(struct ofono_gprs *gprs, bool must_be_active)
 {
 	GSList *l;
 	struct pri_context *ctx;
+	int ctx_count = 0;
 
 	for (l = gprs->contexts; l; l = l->next) {
 		ctx = l->data;
 
 		if (ctx->active == TRUE)
 			return TRUE;
+
+		ctx_count++;
 	}
 
+	if (!must_be_active && ctx_count > 0)
+		return TRUE;
+
 	return FALSE;
 }
 
@@ -1621,7 +1627,8 @@ static void gprs_attached_update(struct ofono_gprs *gprs)
 	ofono_bool_t attached;
 	int status = gprs->status;
 
-	if (on_lte(gprs))
+	if (on_lte(gprs)) {
+		int tech = ofono_netreg_get_technology(gprs->netreg);
 		/*
 		 * For LTE we attached status reflects successful context
 		 * activation.
@@ -1630,11 +1637,12 @@ static void gprs_attached_update(struct ofono_gprs *gprs)
 		 * expect the gprs status to be unknown. That must not
 		 * result in detaching...
 		 */
-		attached = have_active_contexts(gprs);
-	else
+		attached = have_contexts(gprs, tech == ACCESS_TECHNOLOGY_EUTRAN);
+	} else {
 		attached = gprs->driver_attached &&
 			(status == NETWORK_REGISTRATION_STATUS_REGISTERED ||
 				status == NETWORK_REGISTRATION_STATUS_ROAMING);
+	}
 
 	if (attached == gprs->attached)
 		return;
-- 
2.34.1

[RESEND PATCH 3/5] gprs: extend on_lte() to check for M1 and Nb-IOT techs

[Alexandru Ardelean]

The BG95 is an LTE modem with Cat M1 and NB-IoT techs.
So, when it happens that the modem registers to one of these techs (in our
case Cat M1), and there is no GSM advertised by the operator, the modem
will not attach.

Following the current logic of LTE, it looks like we just need to extend
the "on_lte()" function to allow for these techs.

Also, not checking the .read_settings callback existence. That looks like a
hack done for EUTRAN. In our case, the BG95 is controlled by AT commands,
so there is no .read_settings hook yet, and we don't seem to need it.
---
 src/gprs.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/src/gprs.c b/src/gprs.c
index 4befe88a..e2758545 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1615,11 +1615,19 @@ static void detach_active_contexts(struct ofono_gprs *gprs)
 
 static gboolean on_lte(struct ofono_gprs *gprs)
 {
-	if (ofono_netreg_get_technology(gprs->netreg) ==
-			ACCESS_TECHNOLOGY_EUTRAN && have_read_settings(gprs))
-		return TRUE;
+	int tech = ofono_netreg_get_technology(gprs->netreg);
 
-	return FALSE;
+	switch (tech) {
+	case ACCESS_TECHNOLOGY_NB_IOT_M1:
+	case ACCESS_TECHNOLOGY_NB_IOT_NB1:
+		return TRUE;
+	case ACCESS_TECHNOLOGY_EUTRAN:
+		if (have_read_settings(gprs))
+			return TRUE;
+		/* fall-through */
+	default:
+		return FALSE;
+	}
 }
 
 static void gprs_attached_update(struct ofono_gprs *gprs)
-- 
2.34.1

[RESEND PATCH 4/5] plugins: quectel: re-organize code for ussd & lte init

[Alexandru Ardelean]

The BG95 modem (family) supports LTE, but not USSD.
So, we'll split the init of the LTE separately, and add a helper function
(called quectel_model_supports_lte()) which will return true if the modem
supports LTE.
---
 plugins/quectel.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index f933723d..464845ff 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -128,6 +128,17 @@ enum quectel_power_event {
 
 static const char dbus_hw_interface[] = OFONO_SERVICE ".quectel.Hardware";
 
+static ofono_bool_t quectel_model_supports_lte(enum quectel_model model)
+{
+	switch (model) {
+	case QUECTEL_EC21:
+	case QUECTEL_EC200:
+		return TRUE;
+	default:
+		return FALSE;
+	}
+}
+
 static ofono_bool_t has_serial_connection(struct ofono_modem *modem)
 {
 
@@ -1356,10 +1367,11 @@ static void quectel_post_sim(struct ofono_modem *modem)
 	ofono_phonebook_create(modem, data->vendor, "atmodem", data->aux);
 	ofono_call_volume_create(modem, data->vendor, "atmodem", data->aux);
 
-	if (data->model == QUECTEL_EC21 || data->model == QUECTEL_EC200) {
+	if (data->model == QUECTEL_EC21 || data->model == QUECTEL_EC200)
 		ofono_ussd_create(modem, data->vendor, "atmodem", data->aux);
+
+	if (quectel_model_supports_lte(data->model))
 		ofono_lte_create(modem, data->vendor, "atmodem", data->aux);
-	}
 }
 
 static void quectel_post_online(struct ofono_modem *modem)
-- 
2.34.1

[RESEND PATCH 5/5] quectel: add support for BG95

[Alexandru Ardelean]

From: Mircea Caprioru <mircea.caprioru@flowkernel.ro>

This adds support for the Quectel BG95 modem (family).
There are about 7 models for BG95 (according to their docs):
BG95-{M1,M2,M3,M4,M5,M6,MF}.
But, we've tested and validated just the BG95-M3.

It does not support QMI (like BG96), it uses only AT commands.
The AT commands are similar between models, so the hope is that the work on
M3 works for all. At least the ones used by ofono are supported by all
models (according to the Quectel docs).

The behavior of the BG95 is quite similar with the one for EC21 and EC200
(in terms of AT commands), but it differs in a few aspects.
Which is why the OFONO_VENDOR_QUECTEL_EC2X ID is used.

The 'AT+QINDCFG' supports slightly different commands than EC2XX.
The one that is common with EC2XX is 'AT+QINDCFG="csq",1'.
The 'AT+QINDCFG="act",1' is not supported; that means that this command
will error out, but that's fine.

And it uses the Aux interface to talk to the GPRS context.

The rest of the AT commands that ofono uses to talk to EC2X are also
working for BG95.
---
 plugins/quectel.c | 27 ++++++++++++++++++++++-----
 plugins/udevng.c  |  1 +
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 464845ff..71594418 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -64,7 +64,7 @@ static const char *cpin_prefix[] = { "+CPIN:", NULL };
 static const char *cbc_prefix[] = { "+CBC:", NULL };
 static const char *qinistat_prefix[] = { "+QINISTAT:", NULL };
 static const char *cgmm_prefix[] = { "UC15", "Quectel_M95", "Quectel_MC60",
-					"EC21", "EC200", NULL };
+					"EC21", "EC200", "BG95", NULL };
 static const char *none_prefix[] = { NULL };
 
 static const uint8_t gsm0710_terminate[] = {
@@ -85,6 +85,7 @@ enum quectel_model {
 	QUECTEL_MC60,
 	QUECTEL_EC21,
 	QUECTEL_EC200,
+	QUECTEL_BG95,
 };
 
 struct quectel_data {
@@ -133,6 +134,7 @@ static ofono_bool_t quectel_model_supports_lte(enum quectel_model model)
 	switch (model) {
 	case QUECTEL_EC21:
 	case QUECTEL_EC200:
+	case QUECTEL_BG95:
 		return TRUE;
 	default:
 		return FALSE;
@@ -625,7 +627,8 @@ static void qinistat_cb(gboolean ok, GAtResult *result, gpointer user_data)
 		break;
 	case QUECTEL_M95:
 	case QUECTEL_MC60:
-		/* M95 and MC60 uses a counter to 3 */
+	case QUECTEL_BG95:
+		/* M95, MC60 and BG95 use a counter to 3 */
 		ready = 3;
 		break;
 	case QUECTEL_UNKNOWN:
@@ -834,7 +837,7 @@ static void setup_aux(struct ofono_modem *modem)
 
 	g_at_chat_set_slave(data->modem, data->aux);
 
-	if (data->model == QUECTEL_EC21) {
+	if (data->model == QUECTEL_EC21 || data->model == QUECTEL_BG95) {
 		g_at_chat_send(data->aux, "ATE0; &C0; +CMEE=1", none_prefix,
 				NULL, NULL, NULL);
 		g_at_chat_send(data->aux, "AT+QURCCFG=\"urcport\",\"uart1\"", none_prefix,
@@ -886,6 +889,14 @@ static void cgmm_cb(int ok, GAtResult *result, void *user_data)
 		DBG("%p model %s", modem, model);
 		data->vendor = OFONO_VENDOR_QUECTEL_EC2X;
 		data->model = QUECTEL_EC200;
+	} else if (strstr(model, "BG95")) {
+		/**
+		 * BG95 is roughly similar to EC2X in terms of the
+		 * AT commands which ofono uses to talk to it.
+		 */
+		DBG("%p model %s", modem, model);
+		data->vendor = OFONO_VENDOR_QUECTEL_EC2X;
+		data->model = QUECTEL_BG95;
 	} else {
 		ofono_warn("%p unknown model: '%s'", modem, model);
 		data->vendor = OFONO_VENDOR_QUECTEL;
@@ -1357,8 +1368,14 @@ static void quectel_post_sim(struct ofono_modem *modem)
 	DBG("%p", modem);
 
 	gprs = ofono_gprs_create(modem, data->vendor, "atmodem", data->aux);
-	gc = ofono_gprs_context_create(modem, data->vendor, "atmodem",
-					data->modem);
+
+	/* BG95 uses the aux interface to talk to the context */
+	if (data->model == QUECTEL_BG95)
+		gc = ofono_gprs_context_create(modem, data->vendor, "atmodem",
+						data->aux);
+	else
+		gc = ofono_gprs_context_create(modem, data->vendor, "atmodem",
+						data->modem);
 
 	if (gprs && gc)
 		ofono_gprs_add_context(gprs, gc);
diff --git a/plugins/udevng.c b/plugins/udevng.c
index 34ac1cc0..af6abf5c 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1843,6 +1843,7 @@ static struct {
 	{ "quectelqmi",	"qcserial",	"2c7c", "0125"	},
 	{ "quectelqmi",	"qmi_wwan",	"2c7c", "0296"	},
 	{ "quectelqmi",	"qcserial",	"2c7c", "0296"	},
+	{ "quectelqmi",	"qcserial",	"2c7c", "0700"	},
 	{ "ublox",	"cdc_acm",	"1546", "1010"	},
 	{ "ublox",	"cdc_ncm",	"1546", "1010"	},
 	{ "ublox",	"cdc_acm",	"1546", "1102"	},
-- 
2.34.1

Re: [RESEND PATCH 1/5] atmodem: gprs-context: fix ATD99 init for modems with no slave channel

[Denis Kenzior]

Hi Alexandru,

On 2/11/23 12:11, Alexandru Ardelean wrote:
> From: Mircea Caprioru <mircea.caprioru@flowkernel.ro>
> 
> Patch 476fab4ea ("atmodem: Add support for NW DEACT notifications on slave
> channel") has added a check to make sure we have a slave channel before
> registering for "+CGEV:" notifications.
> 
> Patch 8e929df4f ("atmodem: use ATD99 to enter data state when needed") has
> added the ATD99 detection after the slave channel check.
> 
> In our case, when we get a modem with no slave channels, the ATD99
> detection will not get triggered and modems will use the AT+CGDATA="PPP"
> method for connecting to the internet (when ATD99 would be supported).
> 
> This patch moves the check a bit lower in the code and removes the
> early-return, to avoid for any potential future errors.

So how exactly do you detect context deactivations?  The "atmodem" grps-context 
driver is really not setup to work without a second port that is available for 
AT commands.

> ---
>   drivers/atmodem/gprs-context.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index f9f3c0ab..efc3fe94 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -449,10 +449,6 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
>   
>   	ofono_gprs_context_set_data(gc, gcd);
>   
> -	chat = g_at_chat_get_slave(gcd->chat);
> -	if (chat == NULL)
> -		return 0;
> -
>   	switch (vendor) {
>   	case OFONO_VENDOR_SIMCOM_SIM900:
>   		gcd->use_atd99 = FALSE;
> @@ -462,7 +458,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
>   						at_cgdata_test_cb, gc, NULL);
>   	}
>   
> -	g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
> +	chat = g_at_chat_get_slave(gcd->chat);
> +	if (chat != NULL)
> +		g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);

I think this goes over 80 characters / line.

>   
>   	return 0;
>   }

Regards,
-Denis

Re: [RESEND PATCH 2/5] gprs: rework have_active_contexts() to check for non-active contexts

[Denis Kenzior]

Hi Alexandru,

On 2/11/23 12:11, Alexandru Ardelean wrote:
> While trying to add LTE support to the Quectel BG95-M3 modem, we've run
> into a bit of a chicken-n-egg dilemma.
> 
> While ofono has enumerated (successfully) the GPRS contexts (from the
> modem), we got only one (IP), but it's not active.
> 

How did it do that?

> And trying to activate it, would yield a NotAttached error (via DBus).
> 
> The BG95-M3 modem supports Cat M1/Cat NB2/EGPRS LTE.
> 
> This situation would show up when trying to connect to an operator that
> only has "lte-cat-m1" tech (and nothing else). If it the operator would
> also advertise "gsm" tech, ofono would connect via "gsm".

I'm not really sure how lte-cat-m1 works.  With LTE the requirement was that for 
a successful network registration on LTE, a context had to be 'active'.  This 
can be an IP context or an IMS one, or any type really.  So just by merely being 
registered to LTE indicates that you're "attached".

Now, this whole attached business is really for UMTS and avoiding data roaming. 
With LTE and newer technologies we should really redesign the API.  But that is 
quite a bit of work and oFono is in maintenance-only mode at the moment.

> 
> Now, this could be a particularity of this modem (BG95-M3), in which case
> it may be an idea to add a hook into the quectel plugin/driver to check the
> attached status.
> 
> But for now, it seems that allowing the Attached state to be reported for
> any present GPRS contexts (even non-active), seems to work.

How do you figure that?  gprs->contexts can be populated via provisioning or 
D-Bus API.  So just the presence of these contexts means nothing for the 
attached status.

> 
> For EUTRAN, this behavior is still maintained.
> ---
>   src/gprs.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 

Regards,
-Denis

Re: [RESEND PATCH 3/5] gprs: extend on_lte() to check for M1 and Nb-IOT techs

[Denis Kenzior]

Hi Alexandru,

On 2/11/23 12:11, Alexandru Ardelean wrote:
> The BG95 is an LTE modem with Cat M1 and NB-IoT techs.
> So, when it happens that the modem registers to one of these techs (in our
> case Cat M1), and there is no GSM advertised by the operator, the modem
> will not attach.
> 

oFono doesn't really grok these technology types.  It must be taught to handle 
them properly.

> Following the current logic of LTE, it looks like we just need to extend
> the "on_lte()" function to allow for these techs.

Or maybe treat them specially if they're not handled like LTE?

> 
> Also, not checking the .read_settings callback existence. That looks like a
> hack done for EUTRAN. In our case, the BG95 is controlled by AT commands,
> so there is no .read_settings hook yet, and we don't seem to need it.

For LTE support, .read_settings must be implemented.  That is because you need 
to obtain the default context details (see my reply to the previous patch). 
This is needed for matching default context to provisioned ones, etc.

If NB_IOT* acts differently than LTE in this regard, then modifying on_lte() is 
probably not the right approach.

> ---
>   src/gprs.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 

Regards,
-Denis

Re: [RESEND PATCH 4/5] plugins: quectel: re-organize code for ussd & lte init

[Denis Kenzior]

Hi Alexandru,

On 2/11/23 12:11, Alexandru Ardelean wrote:
> The BG95 modem (family) supports LTE, but not USSD.
> So, we'll split the init of the LTE separately, and add a helper function
> (called quectel_model_supports_lte()) which will return true if the modem
> supports LTE.
> ---
>   plugins/quectel.c | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)

Applied, thanks.

Regards,
-Denis

Re: [RESEND PATCH 1/5] atmodem: gprs-context: fix ATD99 init for modems with no slave channel

[Alexandru Ardelean]

On Mon, Feb 13, 2023 at 6:27 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Alexandru,
>
> On 2/11/23 12:11, Alexandru Ardelean wrote:
> > From: Mircea Caprioru <mircea.caprioru@flowkernel.ro>
> >
> > Patch 476fab4ea ("atmodem: Add support for NW DEACT notifications on slave
> > channel") has added a check to make sure we have a slave channel before
> > registering for "+CGEV:" notifications.
> >
> > Patch 8e929df4f ("atmodem: use ATD99 to enter data state when needed") has
> > added the ATD99 detection after the slave channel check.
> >
> > In our case, when we get a modem with no slave channels, the ATD99
> > detection will not get triggered and modems will use the AT+CGDATA="PPP"
> > method for connecting to the internet (when ATD99 would be supported).
> >
> > This patch moves the check a bit lower in the code and removes the
> > early-return, to avoid for any potential future errors.
>

So, broader context.
We're still learning how ofono works/operates.
Things are not quite all-the-way-clear to us, because we are reading
the BG95 AT command manual (which is ¯\_(ツ)_/¯ ), and trying to
fill-in-the-blanks (in our heads) about how ofono does it.

> So how exactly do you detect context deactivations?  The "atmodem" grps-context
> driver is really not setup to work without a second port that is available for
> AT commands.

Hmm, that is actually an interesting point.
The idea of the slave-channel being required, seems to have slipped me.
I will need to chase this in the quectel driver.
Thanks for the hint :)
It's quite probable that our patches (for BG95 support) are not very
good either.

>
> > ---
> >   drivers/atmodem/gprs-context.c | 8 +++-----
> >   1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> > index f9f3c0ab..efc3fe94 100644
> > --- a/drivers/atmodem/gprs-context.c
> > +++ b/drivers/atmodem/gprs-context.c
> > @@ -449,10 +449,6 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
> >
> >       ofono_gprs_context_set_data(gc, gcd);
> >
> > -     chat = g_at_chat_get_slave(gcd->chat);
> > -     if (chat == NULL)
> > -             return 0;
> > -
> >       switch (vendor) {
> >       case OFONO_VENDOR_SIMCOM_SIM900:
> >               gcd->use_atd99 = FALSE;
> > @@ -462,7 +458,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
> >                                               at_cgdata_test_cb, gc, NULL);
> >       }
> >
> > -     g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
> > +     chat = g_at_chat_get_slave(gcd->chat);
> > +     if (chat != NULL)
> > +             g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
>
> I think this goes over 80 characters / line.

ah, right;
will fix this if thi patch will not go away

>
> >
> >       return 0;
> >   }
>
> Regards,
> -Denis

Re: [RESEND PATCH 1/5] atmodem: gprs-context: fix ATD99 init for modems with no slave channel

[Alexandru Ardelean]

On Tue, Feb 14, 2023 at 10:54 AM Alexandru Ardelean <alex@shruggie.ro> wrote:
>
> On Mon, Feb 13, 2023 at 6:27 PM Denis Kenzior <denkenz@gmail.com> wrote:
> >
> > Hi Alexandru,
> >
> > On 2/11/23 12:11, Alexandru Ardelean wrote:
> > > From: Mircea Caprioru <mircea.caprioru@flowkernel.ro>
> > >
> > > Patch 476fab4ea ("atmodem: Add support for NW DEACT notifications on slave
> > > channel") has added a check to make sure we have a slave channel before
> > > registering for "+CGEV:" notifications.
> > >
> > > Patch 8e929df4f ("atmodem: use ATD99 to enter data state when needed") has
> > > added the ATD99 detection after the slave channel check.
> > >
> > > In our case, when we get a modem with no slave channels, the ATD99
> > > detection will not get triggered and modems will use the AT+CGDATA="PPP"
> > > method for connecting to the internet (when ATD99 would be supported).
> > >
> > > This patch moves the check a bit lower in the code and removes the
> > > early-return, to avoid for any potential future errors.
> >
>
> So, broader context.
> We're still learning how ofono works/operates.
> Things are not quite all-the-way-clear to us, because we are reading
> the BG95 AT command manual (which is ¯\_(ツ)_/¯ ), and trying to
> fill-in-the-blanks (in our heads) about how ofono does it.
>
> > So how exactly do you detect context deactivations?  The "atmodem" grps-context
> > driver is really not setup to work without a second port that is available for
> > AT commands.
>
> Hmm, that is actually an interesting point.
> The idea of the slave-channel being required, seems to have slipped me.
> I will need to chase this in the quectel driver.
> Thanks for the hint :)
> It's quite probable that our patches (for BG95 support) are not very
> good either.

Firstly, thanks for the trigger/hint about the second AT port.
We did a thorough check about that. (Well, at least I hope it was thorough :p )
And we ended up searching the Quectel forum (no idea why that didn't
come to mind sooner).

So, it seems we have an issue with our current modem FW.
It may be too old and we need to update it, to have things running smoother.
The kernel 'option' driver enumerates 4 /dev/ttyUSB{0,1,2,3} ports.
One thing that was confusing (for me at least), is, (in oFono) the Aux
port feels more like Modem port in the context of the atmodem driver +
quectel driver (and at least for BG95). But that's just semantic
probably.

So

/dev/ttyUSB0 - Diag
/dev/ttyUSB1 - GNSS
/dev/ttyUSB2 - AT commands (in oFono this is 'Aux')
/dev/ttyUSB3 - AT commands (in oFono this is 'Modem')

In our case, /dev/ttyUSB3 doesn't seem to respond to any AT command,
though it should.
Hence the suspicion that we need to update the modem FW.
Luckily, we have a serial /dev/ttySTM3 port, which we can temporarily
use to get URCs (context DEACT and such).

The thing about /dev/ttyUSB3 is that there is an AT command (which
incidentally) is not in our modem FW that makes the port behave like
an AT modem port, an RmNet port, or an ECM port.
We'll see what we'll do with this.

I will try to respond to the rest of the comments (in the series).
Apologies that I did not do that sooner.
We were really chasing this comment (which really helped).

For now, this BG95 patchset will need to be discarded (in the light of
the modem FW stuff).

Many thanks :)
Alexandru


>
> >
> > > ---
> > >   drivers/atmodem/gprs-context.c | 8 +++-----
> > >   1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> > > index f9f3c0ab..efc3fe94 100644
> > > --- a/drivers/atmodem/gprs-context.c
> > > +++ b/drivers/atmodem/gprs-context.c
> > > @@ -449,10 +449,6 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
> > >
> > >       ofono_gprs_context_set_data(gc, gcd);
> > >
> > > -     chat = g_at_chat_get_slave(gcd->chat);
> > > -     if (chat == NULL)
> > > -             return 0;
> > > -
> > >       switch (vendor) {
> > >       case OFONO_VENDOR_SIMCOM_SIM900:
> > >               gcd->use_atd99 = FALSE;
> > > @@ -462,7 +458,9 @@ static int at_gprs_context_probe(struct ofono_gprs_context *gc,
> > >                                               at_cgdata_test_cb, gc, NULL);
> > >       }
> > >
> > > -     g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
> > > +     chat = g_at_chat_get_slave(gcd->chat);
> > > +     if (chat != NULL)
> > > +             g_at_chat_register(chat, "+CGEV:", cgev_notify, FALSE, gc, NULL);
> >
> > I think this goes over 80 characters / line.
>
> ah, right;
> will fix this if thi patch will not go away
>
> >
> > >
> > >       return 0;
> > >   }
> >
> > Regards,
> > -Denis

Re: [RESEND PATCH 2/5] gprs: rework have_active_contexts() to check for non-active contexts

[Alexandru Ardelean]

On Mon, Feb 13, 2023 at 6:46 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Alexandru,
>
> On 2/11/23 12:11, Alexandru Ardelean wrote:
> > While trying to add LTE support to the Quectel BG95-M3 modem, we've run
> > into a bit of a chicken-n-egg dilemma.
> >
> > While ofono has enumerated (successfully) the GPRS contexts (from the
> > modem), we got only one (IP), but it's not active.
> >
>
> How did it do that?

So, we get the context via the AT+CGDCONT=? command

Feb 17 16:43:04 imow1 ofonod[31380]: Aux: > AT+CGDCONT=?\r
Feb 17 16:43:04 imow1 ofonod[31380]: Aux: < \r\n+CGDCONT:
(1-15),"IP",,,(0-2),(0-4),(0)\r\n+CGDCONT:
(1-15),"PPP",,,(0-2),(0-4),(0)\r\n+CGDCONT:
(1-15),"IPV6",,,(0-2),(0-4),(0)\r\n+CGDCONT:
(1-15),"IPV4V6",,,(0-2),(0-4),(0)\r\n+CGDCONT:
(1-15),"Non-IP",,,(0-2),(0-4),(0)\r\n\r\nOK\r\n

I'll be honest, I did not check that this is parsed correctly by
ofono. I just assumed that this is correct.

When querying via DBus for the contexts

dbus-send --system --dest=org.ofono --type=method_call
--reply-timeout=120000 --print-reply /quectel_0
org.ofono.ConnectionManager.GetContexts

The output is

method return time=1676645265.639429 sender=:1.68 -> destination=:1.73
serial=101 reply_serial=2
   array [
      struct {
         object path "/quectel_0/context1"
         array [
            dict entry(
               string "Name"
               variant                   string "Internet"
            )
            dict entry(
               string "Active"
               variant                   boolean false
            )
            dict entry(
               string "Type"
               variant                   string "internet"
            )
            dict entry(
               string "Protocol"
               variant                   string "ip"
            )
            dict entry(
               string "AccessPointName"
               variant                   string "stihliaxx.tma.iot"
            )
            dict entry(
               string "Username"
               variant                   string ""
            )
            dict entry(
               string "Password"
               variant                   string ""
            )
            dict entry(
               string "AuthenticationMethod"
               variant                   string "chap"
            )
            dict entry(
               string "Settings"
               variant                   array [
                  ]
            )
            dict entry(
               string "IPv6.Settings"
               variant                   array [
                  ]
            )
         ]
      }
   ]


>
> > And trying to activate it, would yield a NotAttached error (via DBus).
> >

With the above context, we run this command:

dbus-send --system --dest=org.ofono --type=method_call
--reply-timeout=120000 --print-reply /quectel_0/context1
org.ofono.ConnectionContext.SetProperty string:Active
variant:boolean:true

-  When modem registers with an operator via GSM, this works
-  When the modem registers to an operator with LTE Cat M1, we get a
NotAttached (GRPS) error

> > The BG95-M3 modem supports Cat M1/Cat NB2/EGPRS LTE.
> >
> > This situation would show up when trying to connect to an operator that
> > only has "lte-cat-m1" tech (and nothing else). If it the operator would
> > also advertise "gsm" tech, ofono would connect via "gsm".
>
> I'm not really sure how lte-cat-m1 works.  With LTE the requirement was that for
> a successful network registration on LTE, a context had to be 'active'.  This
> can be an IP context or an IMS one, or any type really.  So just by merely being
> registered to LTE indicates that you're "attached".

So, at least in the BG95 case, the LTE context is not 'active' (already).
And the on_lte() function only checks EUTRAN, leaving Cat-M1 (or
NB-IoT) to be considered 'GSM' (or non-LTE).

>
> Now, this whole attached business is really for UMTS and avoiding data roaming.
> With LTE and newer technologies we should really redesign the API.  But that is
> quite a bit of work and oFono is in maintenance-only mode at the moment.
>

Hopefully this doesn't sound too bad (to ask): is there a better
alternative that you could recommend to oFono (given that it's in
maintenance-only mode) ?
We picked-up oFono because it's in Yocto.
Maybe oFono is fine for our needs, but I guess the issues with the
BG95 FW have made it more difficult than it needed to be.

> >
> > Now, this could be a particularity of this modem (BG95-M3), in which case
> > it may be an idea to add a hook into the quectel plugin/driver to check the
> > attached status.
> >
> > But for now, it seems that allowing the Attached state to be reported for
> > any present GPRS contexts (even non-active), seems to work.
>
> How do you figure that?  gprs->contexts can be populated via provisioning or
> D-Bus API.  So just the presence of these contexts means nothing for the
> attached status.

Oh, I did not know about gprs->contexts being populate-able via DBus API.
Well, checking the presence of contexts (regardless of whether they're
active) is what worked for us with LTE Cat M1.
I am not saying/insisting that this is correct.
I am also using this patch like an RFC (without specifying the tag).

Btw: we're also not sure how Cat M1 is supposed to work.
We're just relying on oFono to take that load from us
Since QMI is not working too well for our BG95 FW, we need to use the
atmodem driver for a while.

>
> >
> > For EUTRAN, this behavior is still maintained.
> > ---
> >   src/gprs.c | 16 ++++++++++++----
> >   1 file changed, 12 insertions(+), 4 deletions(-)
> >
>
> Regards,
> -Denis
>

Re: [RESEND PATCH 1/5] atmodem: gprs-context: fix ATD99 init for modems with no slave channel

[Denis Kenzior]

Hi Alexandru,

> So, it seems we have an issue with our current modem FW.
> It may be too old and we need to update it, to have things running smoother.
> The kernel 'option' driver enumerates 4 /dev/ttyUSB{0,1,2,3} ports.
> One thing that was confusing (for me at least), is, (in oFono) the Aux
> port feels more like Modem port in the context of the atmodem driver +
> quectel driver (and at least for BG95). But that's just semantic
> probably.

You may also want to check whether your device isn't really an mbim or qmi 
device in disguise.  Some of the devices I've seen over the years will actually 
ship with a single (reduced functionality) AT port, but have to be driven by 
other protocols.

I know nothing of this hardware, just throwing this out there.

Regards,
-Denis

Re: [RESEND PATCH 2/5] gprs: rework have_active_contexts() to check for non-active contexts

[Denis Kenzior]

Hi Alexandru,

On 2/17/23 09:03, Alexandru Ardelean wrote:
> On Mon, Feb 13, 2023 at 6:46 PM Denis Kenzior <denkenz@gmail.com> wrote:
>>
>> Hi Alexandru,
>>
>> On 2/11/23 12:11, Alexandru Ardelean wrote:
>>> While trying to add LTE support to the Quectel BG95-M3 modem, we've run
>>> into a bit of a chicken-n-egg dilemma.
>>>
>>> While ofono has enumerated (successfully) the GPRS contexts (from the
>>> modem), we got only one (IP), but it's not active.
>>>
>>
>> How did it do that?
> 
> So, we get the context via the AT+CGDCONT=? command
> 
> Feb 17 16:43:04 imow1 ofonod[31380]: Aux: > AT+CGDCONT=?\r
> Feb 17 16:43:04 imow1 ofonod[31380]: Aux: < \r\n+CGDCONT:
> (1-15),"IP",,,(0-2),(0-4),(0)\r\n+CGDCONT:
> (1-15),"PPP",,,(0-2),(0-4),(0)\r\n+CGDCONT:
> (1-15),"IPV6",,,(0-2),(0-4),(0)\r\n+CGDCONT:
> (1-15),"IPV4V6",,,(0-2),(0-4),(0)\r\n+CGDCONT:
> (1-15),"Non-IP",,,(0-2),(0-4),(0)\r\n\r\nOK\r\n
> 

That's a capability check.  It just lists the context types supported by this 
modem, not the actual context definitions themselves.  Querying of the defined 
contexts would in theory be performed via 'AT+CGDCONT?'.  But oFono doesn't do 
this, it grabs the context settings via provisioning or the D-Bus API.

> I'll be honest, I did not check that this is parsed correctly by
> ofono. I just assumed that this is correct.
> 
> When querying via DBus for the contexts
> 
> dbus-send --system --dest=org.ofono --type=method_call
> --reply-timeout=120000 --print-reply /quectel_0
> org.ofono.ConnectionManager.GetContexts
> 
> The output is
> 
> method return time=1676645265.639429 sender=:1.68 -> destination=:1.73
> serial=101 reply_serial=2
>     array [
>        struct {
>           object path "/quectel_0/context1"
>           array [
>              dict entry(
>                 string "Name"
>                 variant                   string "Internet"
>              )
>              dict entry(
>                 string "Active"
>                 variant                   boolean false
>              )
>              dict entry(
>                 string "Type"
>                 variant                   string "internet"
>              )
>              dict entry(
>                 string "Protocol"
>                 variant                   string "ip"
>              )
>              dict entry(
>                 string "AccessPointName"
>                 variant                   string "stihliaxx.tma.iot"
>              )
>              dict entry(
>                 string "Username"
>                 variant                   string ""
>              )
>              dict entry(
>                 string "Password"
>                 variant                   string ""
>              )
>              dict entry(
>                 string "AuthenticationMethod"
>                 variant                   string "chap"
>              )
>              dict entry(
>                 string "Settings"
>                 variant                   array [
>                    ]
>              )
>              dict entry(
>                 string "IPv6.Settings"
>                 variant                   array [
>                    ]
>              )
>           ]
>        }
>     ]
> 
> 

That looks like it is coming from a provisioning database.

Regards,
-Denis

Re: [RESEND PATCH 1/5] atmodem: gprs-context: fix ATD99 init for modems with no slave channel

[Alexandru Ardelean]

On Fri, Feb 17, 2023 at 5:17 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Alexandru,
>
> > So, it seems we have an issue with our current modem FW.
> > It may be too old and we need to update it, to have things running smoother.
> > The kernel 'option' driver enumerates 4 /dev/ttyUSB{0,1,2,3} ports.
> > One thing that was confusing (for me at least), is, (in oFono) the Aux
> > port feels more like Modem port in the context of the atmodem driver +
> > quectel driver (and at least for BG95). But that's just semantic
> > probably.
>
> You may also want to check whether your device isn't really an mbim or qmi
> device in disguise.  Some of the devices I've seen over the years will actually
> ship with a single (reduced functionality) AT port, but have to be driven by
> other protocols.
>
> I know nothing of this hardware, just throwing this out there.

Well, I combed the Quectel forums for this BG95 and some of the issues
we are having are also shared by others.

We knew that QMI support for this modem is rather broken (from their
direct support contact).
Which is why we were trying on making the AT commands work.
This was also mentioned by Quectel when they upstreamed the VID + PID
combinations to the kernel:
https://lore.kernel.org/linux-usb/TYZPR06MB4270136AEC3FB06E20F6503486CF9@TYZPR06MB4270.apcprd06.prod.outlook.com/
https://lore.kernel.org/linux-usb/TYZPR06MB4270471E08BF0BCDBF24722186CE9@TYZPR06MB4270.apcprd06.prod.outlook.com/

Seems that our FW does not support device ports re-configuration (the
AT+QCFGEXT="usbnet" command does not work in our FW).
https://forums.quectel.com/t/bg95-unable-to-set-usbnet-to-rmnet/14308/10
(Though an update fixes that)

For now, we have an internal hack that uses the UART port as the
second AT port (in the atmodem driver).
So, basically, /dev/ttyUSB2 is Aux and a /dev/ttySTM3 is the Modem port.

I wouldn't know (now) what to propose for oFono upstream?
Wait for the QMI support to be finished by Quectel (if ever)?
Funny story: Quectel support seems to send FW updates via email if you
ask them on their forum:
https://forums.quectel.com/t/latest-firmware-bg95-m3/15403

>
> Regards,
> -Denis

Re: [RESEND PATCH 2/5] gprs: rework have_active_contexts() to check for non-active contexts

[Alexandru Ardelean]

On Fri, Feb 17, 2023 at 5:23 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Alexandru,
>
> On 2/17/23 09:03, Alexandru Ardelean wrote:
> > On Mon, Feb 13, 2023 at 6:46 PM Denis Kenzior <denkenz@gmail.com> wrote:
> >>
> >> Hi Alexandru,
> >>
> >> On 2/11/23 12:11, Alexandru Ardelean wrote:
> >>> While trying to add LTE support to the Quectel BG95-M3 modem, we've run
> >>> into a bit of a chicken-n-egg dilemma.
> >>>
> >>> While ofono has enumerated (successfully) the GPRS contexts (from the
> >>> modem), we got only one (IP), but it's not active.
> >>>
> >>
> >> How did it do that?
> >
> > So, we get the context via the AT+CGDCONT=? command
> >
> > Feb 17 16:43:04 imow1 ofonod[31380]: Aux: > AT+CGDCONT=?\r
> > Feb 17 16:43:04 imow1 ofonod[31380]: Aux: < \r\n+CGDCONT:
> > (1-15),"IP",,,(0-2),(0-4),(0)\r\n+CGDCONT:
> > (1-15),"PPP",,,(0-2),(0-4),(0)\r\n+CGDCONT:
> > (1-15),"IPV6",,,(0-2),(0-4),(0)\r\n+CGDCONT:
> > (1-15),"IPV4V6",,,(0-2),(0-4),(0)\r\n+CGDCONT:
> > (1-15),"Non-IP",,,(0-2),(0-4),(0)\r\n\r\nOK\r\n
> >
>
> That's a capability check.  It just lists the context types supported by this
> modem, not the actual context definitions themselves.  Querying of the defined
> contexts would in theory be performed via 'AT+CGDCONT?'.  But oFono doesn't do
> this, it grabs the context settings via provisioning or the D-Bus API.
>
> > I'll be honest, I did not check that this is parsed correctly by
> > ofono. I just assumed that this is correct.
> >
> > When querying via DBus for the contexts
> >
> > dbus-send --system --dest=org.ofono --type=method_call
> > --reply-timeout=120000 --print-reply /quectel_0
> > org.ofono.ConnectionManager.GetContexts
> >
> > The output is
> >
> > method return time=1676645265.639429 sender=:1.68 -> destination=:1.73
> > serial=101 reply_serial=2
> >     array [
> >        struct {
> >           object path "/quectel_0/context1"
> >           array [
> >              dict entry(
> >                 string "Name"
> >                 variant                   string "Internet"
> >              )
> >              dict entry(
> >                 string "Active"
> >                 variant                   boolean false
> >              )
> >              dict entry(
> >                 string "Type"
> >                 variant                   string "internet"
> >              )
> >              dict entry(
> >                 string "Protocol"
> >                 variant                   string "ip"
> >              )
> >              dict entry(
> >                 string "AccessPointName"
> >                 variant                   string "stihliaxx.tma.iot"
> >              )
> >              dict entry(
> >                 string "Username"
> >                 variant                   string ""
> >              )
> >              dict entry(
> >                 string "Password"
> >                 variant                   string ""
> >              )
> >              dict entry(
> >                 string "AuthenticationMethod"
> >                 variant                   string "chap"
> >              )
> >              dict entry(
> >                 string "Settings"
> >                 variant                   array [
> >                    ]
> >              )
> >              dict entry(
> >                 string "IPv6.Settings"
> >                 variant                   array [
> >                    ]
> >              )
> >           ]
> >        }
> >     ]
> >
> >
>
> That looks like it is coming from a provisioning database.

Ah, will check.
If that's the case, then this patch can definitely be dropped.

>
> Regards,
> -Denis

Re: [RESEND PATCH 3/5] gprs: extend on_lte() to check for M1 and Nb-IOT techs

[Alexandru Ardelean]

On Mon, Feb 13, 2023 at 6:52 PM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Alexandru,
>
> On 2/11/23 12:11, Alexandru Ardelean wrote:
> > The BG95 is an LTE modem with Cat M1 and NB-IoT techs.
> > So, when it happens that the modem registers to one of these techs (in our
> > case Cat M1), and there is no GSM advertised by the operator, the modem
> > will not attach.
> >
>
> oFono doesn't really grok these technology types.  It must be taught to handle
> them properly.

Ack.
It's what we want to do.

>
> > Following the current logic of LTE, it looks like we just need to extend
> > the "on_lte()" function to allow for these techs.
>
> Or maybe treat them specially if they're not handled like LTE?

Ah.
So, I may be a bit confused here about what LTE refers to (or which
you are referring to).
To me LTE is EUTRAN, Cat M1 and NB-IoT.
But maybe, you are referring to LTE as just EUTRAN, and the others are
specific cases that need to be handled differently.
That is fine from my side.

>
> >
> > Also, not checking the .read_settings callback existence. That looks like a
> > hack done for EUTRAN. In our case, the BG95 is controlled by AT commands,
> > so there is no .read_settings hook yet, and we don't seem to need it.
>
> For LTE support, .read_settings must be implemented.  That is because you need
> to obtain the default context details (see my reply to the previous patch).
> This is needed for matching default context to provisioned ones, etc.

Will see maybe about this .read_settings hook.
Maybe we treat LTE Cat M1 entirely differently.
Who knows?
But if we do get QMI to work (at some point), then this is not required anymore.

>
> If NB_IOT* acts differently than LTE in this regard, then modifying on_lte() is
> probably not the right approach.

Hmmm.
So we don't use NB-IoT. It's a requirement (for us) to disable it.
We're just using LTE Cat M1.
I guess I put it there inertially after looking into the list of LTE enums.


>
> > ---
> >   src/gprs.c | 16 ++++++++++++----
> >   1 file changed, 12 insertions(+), 4 deletions(-)
> >
>
> Regards,
> -Denis
>