8:50 p.m.
Quectel devices returns "Revision:" before the manufacture and revision.
Via dbus:
"Manufacturer" s "Revision: MTK 0828"
"Revision" s "Revision: M95FAR02A08"
---
plugins/quectel.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/plugins/quectel.c b/plugins/quectel.c
index 5d3ad470..2fd820a5 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -1228,6 +1228,8 @@ static void quectel_pre_sim(struct ofono_modem *modem)
DBG("%p", modem);
+ ofono_devinfo_create(modem, data->vendor, "atmodem", data->aux);
+
ofono_voicecall_create(modem, data->vendor, "atmodem", data->aux);
sim = ofono_sim_create(modem, data->vendor, "atmodem", data->aux);
--
2.34.1
3:45 a.m.
Hi Sean,
On 12/6/21 3:50 AM, Sean Nyekjaer wrote:
Quectel devices returns "Revision:" before the manufacture
and revision.
Via dbus:
"Manufacturer" s "Revision: MTK 0828"
"Revision" s "Revision: M95FAR02A08"
---
plugins/quectel.c | 2 ++
1 file changed, 2 insertions(+)
Applied. But you may want to add a quectel specific quirk handler in the
devinfo driver that strips the 'Revision: ' prefix. It should not be part of
the response.
Regards,
-Denis
2:40 a.m.
On Mon, Dec 06, 2021 at 10:45:35AM -0600, Denis Kenzior wrote:
Hi Sean,
On 12/6/21 3:50 AM, Sean Nyekjaer wrote:
> Quectel devices returns "Revision:" before the manufacture and revision.
> Via dbus:
> "Manufacturer" s "Revision: MTK 0828"
> "Revision" s "Revision: M95FAR02A08"
> ---
> plugins/quectel.c | 2 ++
> 1 file changed, 2 insertions(+)
>
Applied. But you may want to add a quectel specific quirk handler in the
devinfo driver that strips the 'Revision: ' prefix. It should not be part
of the response.
Regards,
-Denis
Hi Denis,
Good plan :)
I have been looking at it. [0]
Should I catch the callback directly in atmodem/devinfo.c instead?
We could also just look for a "Revision: " in query_revision_cb(), but
it's kinda hacky
/Sean
[0]:
diff --git a/drivers/atmodem/devinfo.c b/drivers/atmodem/devinfo.c
index ff7386cd..b64d9977 100644
--- a/drivers/atmodem/devinfo.c
+++ b/drivers/atmodem/devinfo.c
@@ -37,6 +37,11 @@
static const char *gcap_prefix[] = { "+GCAP:", NULL };
+struct dev_data {
+ GAtChat *chat;
+ unsigned int vendor;
+};
+
static void attr_cb(gboolean ok, GAtResult *result, gpointer user_data)
{
struct cb_data *cbd = user_data;
@@ -64,11 +69,11 @@ static void at_query_manufacturer(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
cbd->user = "+CGMI:";
- if (g_at_chat_send(chat, "AT+CGMI", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGMI", NULL, attr_cb, cbd, g_free) >
0)
return;
g_free(cbd);
@@ -80,11 +85,11 @@ static void at_query_model(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
cbd->user = "+CGMM:";
- if (g_at_chat_send(chat, "AT+CGMM", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGMM", NULL, attr_cb, cbd, g_free) >
0)
return;
g_free(cbd);
@@ -96,11 +101,11 @@ static void at_query_revision(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
cbd->user = "+CGMR:";
- if (g_at_chat_send(chat, "AT+CGMR", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGMR", NULL, attr_cb, cbd, g_free) >
0)
return;
g_free(cbd);
@@ -112,11 +117,11 @@ static void at_query_serial(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
cbd->user = "+CGSN:";
- if (g_at_chat_send(chat, "AT+CGSN", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGSN", NULL, attr_cb, cbd, g_free) >
0)
return;
g_free(cbd);
@@ -134,11 +139,16 @@ static void capability_cb(gboolean ok, GAtResult *result, gpointer
user_data)
static int at_devinfo_probe(struct ofono_devinfo *info, unsigned int vendor,
void *data)
{
- GAtChat *chat = g_at_chat_clone(data);
+ GAtChat *chat = data;
+ struct dev_data *dev;
+
+ dev = g_new0(struct dev_data, 1);
+ dev->chat = g_at_chat_clone(chat);
+ dev->vendor = vendor;
- ofono_devinfo_set_data(info, chat);
+ ofono_devinfo_set_data(info, dev);
- g_at_chat_send(chat, "AT+GCAP", gcap_prefix,
+ g_at_chat_send(dev->chat, "AT+GCAP", gcap_prefix,
capability_cb, info, NULL);
return 0;
@@ -146,11 +156,11 @@ static int at_devinfo_probe(struct ofono_devinfo *info, unsigned int
vendor,
static void at_devinfo_remove(struct ofono_devinfo *info)
{
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
- ofono_devinfo_set_data(info, NULL);
+ g_at_chat_unref(dev->chat);
- g_at_chat_unref(chat);
+ ofono_devinfo_set_data(info, NULL);
}
static const struct ofono_devinfo_driver driver = {
2:43 a.m.
Hi Sean,
Good plan :)
I have been looking at it. [0]
Yep, that looks like a good start.
Should I catch the callback directly in atmodem/devinfo.c instead?
You could try to intercept this in attr_cb itself. But you'd need access to
vendor selector somehow. Perhaps a custom data structure instead of cb_data.
Alternatively store the prefix in struct dev_data and set cb_data->user to
struct dev_data.
Something like:
static void attr_cb()
{
...
if (at_util_parse_attr(...)) {
...
}
if (dev->vendor == QUECTEL &&
(!strcmp(dev->prefix, ...) ||
!strcmp(dev->prefix, ...))) {
// Strip Revision:
}
...
}
We could also just look for a "Revision: " in query_revision_cb(), but
it's kinda hacky
That's the other approach. Introduce callback(s) for both revision and
manufacturer commands and handle the quirk there. Not sure what else we can do?
Regards,
-Denis
12:54 a.m.
On Tue, Dec 07, 2021 at 09:43:47AM -0600, Denis Kenzior wrote:
Hi Sean,
> Good plan :)
> I have been looking at it. [0]
Yep, that looks like a good start.
>
> Should I catch the callback directly in atmodem/devinfo.c instead?
You could try to intercept this in attr_cb itself. But you'd need access to
vendor selector somehow. Perhaps a custom data structure instead of
cb_data. Alternatively store the prefix in struct dev_data and set
cb_data->user to struct dev_data.
Something like:
static void attr_cb()
{
...
if (at_util_parse_attr(...)) {
...
}
if (dev->vendor == QUECTEL &&
(!strcmp(dev->prefix, ...) ||
!strcmp(dev->prefix, ...))) {
// Strip Revision:
}
...
}
[ ... ]
Hi Denis,
What do you think about this: [0]
In attr_cb() the +10 to attr pointer I need to do better.
While looking at the code, in attr_cb() do we leak the attr string? I
can't find where it's free'd :)
/Sean
[0]:
diff --git a/drivers/atmodem/devinfo.c b/drivers/atmodem/devinfo.c
index ff7386cd..263facd2 100644
--- a/drivers/atmodem/devinfo.c
+++ b/drivers/atmodem/devinfo.c
@@ -29,6 +29,7 @@
#include <ofono/log.h>
#include <ofono/modem.h>
#include <ofono/devinfo.h>
+#include "vendor.h"
#include "gatchat.h"
#include "gatresult.h"
@@ -37,9 +38,20 @@
static const char *gcap_prefix[] = { "+GCAP:", NULL };
+struct dev_data {
+ GAtChat *chat;
+ unsigned int vendor;
+};
+
+struct custom_cb_data {
+ struct cb_data *cbd;
+ unsigned int vendor;
+};
+
static void attr_cb(gboolean ok, GAtResult *result, gpointer user_data)
{
- struct cb_data *cbd = user_data;
+ struct custom_cb_data *ccd = user_data;
+ struct cb_data *cbd = ccd->cbd;
ofono_devinfo_query_cb_t cb = cbd->cb;
const char *prefix = cbd->user;
struct ofono_error error;
@@ -57,21 +69,33 @@ static void attr_cb(gboolean ok, GAtResult *result, gpointer
user_data)
return;
}
+ if (ccd->vendor == OFONO_VENDOR_QUECTEL_SERIAL &&
+ (!strcmp(prefix, "+CGMI:") ||
+ !strcmp(prefix, "+CGMR:"))) {
+ if (!strncmp("Revision: ", attr, 10)) {
+ cb(&error, attr+10, cbd->data);
+ return;
+ }
+ }
+
cb(&error, attr, cbd->data);
}
static void at_query_manufacturer(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
- struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct custom_cb_data *ccd = g_new0(struct custom_cb_data, 1);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
+ ccd->cbd = cb_data_new(cb, data);
+ ccd->vendor = dev->vendor;
- cbd->user = "+CGMI:";
+ ccd->cbd->user = "+CGMI:";
- if (g_at_chat_send(chat, "AT+CGMI", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGMI", NULL, attr_cb, ccd, g_free) >
0)
return;
- g_free(cbd);
+ g_free(ccd->cbd);
+ g_free(ccd);
CALLBACK_WITH_FAILURE(cb, NULL, data);
}
@@ -79,15 +103,18 @@ static void at_query_manufacturer(struct ofono_devinfo *info,
static void at_query_model(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
- struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct custom_cb_data *ccd = g_new0(struct custom_cb_data, 1);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
+ ccd->cbd = cb_data_new(cb, data);
+ ccd->vendor = dev->vendor;
- cbd->user = "+CGMM:";
+ ccd->cbd->user = "+CGMM:";
- if (g_at_chat_send(chat, "AT+CGMM", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGMM", NULL, attr_cb, ccd, g_free) >
0)
return;
- g_free(cbd);
+ g_free(ccd->cbd);
+ g_free(ccd);
CALLBACK_WITH_FAILURE(cb, NULL, data);
}
@@ -95,15 +122,18 @@ static void at_query_model(struct ofono_devinfo *info,
static void at_query_revision(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
- struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct custom_cb_data *ccd = g_new0(struct custom_cb_data, 1);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
+ ccd->cbd = cb_data_new(cb, data);
+ ccd->vendor = dev->vendor;
- cbd->user = "+CGMR:";
+ ccd->cbd->user = "+CGMR:";
- if (g_at_chat_send(chat, "AT+CGMR", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGMR", NULL, attr_cb, ccd, g_free) >
0)
return;
- g_free(cbd);
+ g_free(ccd->cbd);
+ g_free(ccd);
CALLBACK_WITH_FAILURE(cb, NULL, data);
}
@@ -111,15 +141,18 @@ static void at_query_revision(struct ofono_devinfo *info,
static void at_query_serial(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
- struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct custom_cb_data *ccd = g_new0(struct custom_cb_data, 1);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
+ ccd->cbd = cb_data_new(cb, data);
+ ccd->vendor = dev->vendor;
- cbd->user = "+CGSN:";
+ ccd->cbd->user = "+CGSN:";
- if (g_at_chat_send(chat, "AT+CGSN", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGSN", NULL, attr_cb, ccd, g_free) >
0)
return;
- g_free(cbd);
+ g_free(ccd->cbd);
+ g_free(ccd);
CALLBACK_WITH_FAILURE(cb, NULL, data);
}
@@ -134,11 +167,16 @@ static void capability_cb(gboolean ok, GAtResult *result, gpointer
user_data)
static int at_devinfo_probe(struct ofono_devinfo *info, unsigned int vendor,
void *data)
{
- GAtChat *chat = g_at_chat_clone(data);
+ GAtChat *chat = data;
+ struct dev_data *dev;
+
+ dev = g_new0(struct dev_data, 1);
+ dev->chat = g_at_chat_clone(chat);
+ dev->vendor = vendor;
- ofono_devinfo_set_data(info, chat);
+ ofono_devinfo_set_data(info, dev);
- g_at_chat_send(chat, "AT+GCAP", gcap_prefix,
+ g_at_chat_send(dev->chat, "AT+GCAP", gcap_prefix,
capability_cb, info, NULL);
return 0;
@@ -146,11 +184,11 @@ static int at_devinfo_probe(struct ofono_devinfo *info, unsigned int
vendor,
static void at_devinfo_remove(struct ofono_devinfo *info)
{
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
- ofono_devinfo_set_data(info, NULL);
+ g_at_chat_unref(dev->chat);
- g_at_chat_unref(chat);
+ ofono_devinfo_set_data(info, NULL);
}
static const struct ofono_devinfo_driver driver = {
2:16 a.m.
Hi Sean,
What do you think about this: [0]
In attr_cb() the +10 to attr pointer I need to do better.
While looking at the code, in attr_cb() do we leak the attr string? I
can't find where it's free'd :)
No, the string is declared as const char in both attr_cb and
at_util_parse_attr(). So it is returning a pointer to a buffer owned by GAtChat.
<snip>
static void attr_cb(gboolean ok, GAtResult *result, gpointer
user_data)
{
- struct cb_data *cbd = user_data;
+ struct custom_cb_data *ccd = user_data;
+ struct cb_data *cbd = ccd->cbd;
ofono_devinfo_query_cb_t cb = cbd->cb;
const char *prefix = cbd->user;
struct ofono_error error;
@@ -57,21 +69,33 @@ static void attr_cb(gboolean ok, GAtResult *result, gpointer
user_data)
return;
}
+ if (ccd->vendor == OFONO_VENDOR_QUECTEL_SERIAL &&
+ (!strcmp(prefix, "+CGMI:") ||
+ !strcmp(prefix, "+CGMR:"))) {
+ if (!strncmp("Revision: ", attr, 10)) {
+ cb(&error, attr+10, cbd->data);
+ return;
https://people.gnome.org/~ryanl/glib-docs/glib-String-Utility-Functions.h...
+ }
+ }
+
cb(&error, attr, cbd->data);
}
<snip>
@@ -111,15 +141,18 @@ static void at_query_revision(struct
ofono_devinfo *info,
static void at_query_serial(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
{
- struct cb_data *cbd = cb_data_new(cb, data);
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct custom_cb_data *ccd = g_new0(struct custom_cb_data, 1);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
+ ccd->cbd = cb_data_new(cb, data);
+ ccd->vendor = dev->vendor;
Why not just put everything you need from cb_data into your custom data object
instead of trying to allocate cb_data.
- cbd->user = "+CGSN:";
+ ccd->cbd->user = "+CGSN:";
- if (g_at_chat_send(chat, "AT+CGSN", NULL, attr_cb, cbd, g_free) > 0)
+ if (g_at_chat_send(dev->chat, "AT+CGSN", NULL, attr_cb, ccd, g_free) >
0)
return;
- g_free(cbd);
+ g_free(ccd->cbd);
+ g_free(ccd);
CALLBACK_WITH_FAILURE(cb, NULL, data);
}
<snip>
@@ -146,11 +184,11 @@ static int at_devinfo_probe(struct
ofono_devinfo *info, unsigned int vendor,
static void at_devinfo_remove(struct ofono_devinfo *info)
{
- GAtChat *chat = ofono_devinfo_get_data(info);
+ struct dev_data *dev = ofono_devinfo_get_data(info);
- ofono_devinfo_set_data(info, NULL);
+ g_at_chat_unref(dev->chat);
free dev?
- g_at_chat_unref(chat);
+ ofono_devinfo_set_data(info, NULL);
}
static const struct ofono_devinfo_driver driver = {
Regards,
-Denis
279
days inactive
286
days old
5 comments
2 participants
participants (2)
-
Denis Kenzior -
Sean Nyekjaer