[PATCH] atmodem/sms: added missing return in at_cmt_notify before error processing

[Giacinto Cifelli]

---
 drivers/atmodem/sms.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index 7e78fcf..fb4d67a 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -455,6 +455,7 @@ static void at_cmt_notify(GAtResult *result, gpointer 
user_data)
 
if (data->vendor != OFONO_VENDOR_SIMCOM)
at_ack_delivery(sms);
+   return;
 
 err:
ofono_error("Unable to parse CMT notification");
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] mbim driver: fixed the initialization function

[Giacinto Cifelli]

---
 drivers/mbimmodem/mbimmodem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mbimmodem/mbimmodem.c b/drivers/mbimmodem/mbimmodem.c
index a4c9daa..2a01dd6 100644
--- a/drivers/mbimmodem/mbimmodem.c
+++ b/drivers/mbimmodem/mbimmodem.c
@@ -33,7 +33,7 @@ static int mbimmodem_init(void)
mbim_devinfo_init();
mbim_sim_init();
mbim_netreg_init();
-   mbim_sms_exit();
+   mbim_sms_init();
mbim_gprs_init();
mbim_gprs_context_init();
return 0;
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] mbim driver: call the release function in case MBIM OPEN fails

[Giacinto Cifelli]

---
 drivers/mbimmodem/mbim.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mbimmodem/mbim.c b/drivers/mbimmodem/mbim.c
index 9fcf44b..f4132d0 100644
--- a/drivers/mbimmodem/mbim.c
+++ b/drivers/mbimmodem/mbim.c
@@ -781,6 +781,9 @@ static bool open_read_handler(struct l_io *io, void 
*user_data)
/* Grab OPEN_DONE Status field */
if (l_get_le32(buf) != 0) {
close(fd);
+   if (device->disconnect_handler)
+   device->disconnect_handler(device->ready_data);
+   device->is_ready = false;
return false;
}
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] src/gprs.c: make sure that the context is properly released

[Giacinto Cifelli]

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

diff --git a/src/gprs.c b/src/gprs.c
index 8f5d195..40f43e3 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1642,6 +1642,9 @@ static void release_active_contexts(struct ofono_gprs 
*gprs)
 
if (gc->driver->detach_shutdown != NULL)
gc->driver->detach_shutdown(gc, ctx->context.cid);
+
+   /* Make sure the context is properly cleared */
+   release_context(ctx);
}
 }
 
@@ -2241,6 +2244,7 @@ static DBusMessage *gprs_remove_context(DBusConnection 
*conn,
}
 
DBG("Unregistering context: %s", ctx->path);
+   release_context(ctx);
context_dbus_unregister(ctx);
gprs->contexts = g_slist_remove(gprs->contexts, ctx);
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH 2/2] tools/auto-enable.c and tools/huawei-audio.c: removed calls to g_thread_init

[Giacinto Cifelli]

---
 tools/auto-enable.c  | 5 -
 tools/huawei-audio.c | 5 -
 2 files changed, 10 deletions(-)

diff --git a/tools/auto-enable.c b/tools/auto-enable.c
index 87fb0a8..5195aaa 100644
--- a/tools/auto-enable.c
+++ b/tools/auto-enable.c
@@ -492,11 +492,6 @@ int main(int argc, char **argv)
guint watch;
struct sigaction sa;
 
-#ifdef NEED_THREADS
-   if (g_thread_supported() == FALSE)
-   g_thread_init(NULL);
-#endif
-
context = g_option_context_new(NULL);
g_option_context_add_main_entries(context, options, NULL);
 
diff --git a/tools/huawei-audio.c b/tools/huawei-audio.c
index 9997a58..10b599f 100644
--- a/tools/huawei-audio.c
+++ b/tools/huawei-audio.c
@@ -775,11 +775,6 @@ int main(int argc, char **argv)
guint watch;
struct sigaction sa;
 
-#ifdef NEED_THREADS
-   if (g_thread_supported() == FALSE)
-   g_thread_init(NULL);
-#endif
-
context = g_option_context_new(NULL);
g_option_context_add_main_entries(context, options, NULL);
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] bugfix: src/gprs.c used the right binary NOT operator in bitwise operation instead of the boolean NOT operator

[Giacinto Cifelli]

---
 src/gprs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gprs.c b/src/gprs.c
index f17f31b..8f5d195 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -1015,7 +1015,7 @@ static void pri_read_settings_callback(const struct 
ofono_error *error,
 
value = pri_ctx->active;
 
-   gprs->flags &= !GPRS_FLAG_ATTACHING;
+   gprs->flags &= ~GPRS_FLAG_ATTACHING;
 
gprs->driver_attached = TRUE;
gprs_set_attached_property(gprs, TRUE);
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH 2/4] added default values handling for gprs authentication in atmodem and mbimmodem

[Giacinto Cifelli]

this is needed so that with the additional methods NONE and ANY
the compilation is not broken
---
 drivers/atmodem/gprs-context.c   | 2 ++
 drivers/mbimmodem/gprs-context.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
index 79ac4c8..0585850 100644
--- a/drivers/atmodem/gprs-context.c
+++ b/drivers/atmodem/gprs-context.c
@@ -304,6 +304,8 @@ static void at_gprs_activate_primary(struct 
ofono_gprs_context *gc,
snprintf(buf + len, sizeof(buf) - len - 3,
",\"PAP:%s\"", ctx->apn);
break;
+   default:
+   break;
}
break;
default:
diff --git a/drivers/mbimmodem/gprs-context.c b/drivers/mbimmodem/gprs-context.c
index 79793c9..ce33b92 100644
--- a/drivers/mbimmodem/gprs-context.c
+++ b/drivers/mbimmodem/gprs-context.c
@@ -75,6 +75,8 @@ static uint32_t auth_method_to_auth_protocol(enum 
ofono_gprs_auth_method method)
return 2; /* MBIMAuthProtocolChap */
case OFONO_GPRS_AUTH_METHOD_PAP:
return 1; /* MBIMAuthProtocolPap */
+   default:
+   return 0;
}
 
return 0;
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH 4/4] src/gprs.c: added support for NONE and ANY authentication methods

[Giacinto Cifelli]

---
 src/gprs.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/src/gprs.c b/src/gprs.c
index 377eced..f17f31b 100644
--- a/src/gprs.c
+++ b/src/gprs.c
@@ -261,6 +261,10 @@ static const char *gprs_auth_method_to_string(enum 
ofono_gprs_auth_method auth)
return "chap";
case OFONO_GPRS_AUTH_METHOD_PAP:
return "pap";
+   case OFONO_GPRS_AUTH_METHOD_NONE:
+   return "none";
+   default:
+   return NULL;
};
 
return NULL;
@@ -275,6 +279,9 @@ static gboolean gprs_auth_method_from_string(const char 
*str,
} else if (g_str_equal(str, "pap")) {
*auth = OFONO_GPRS_AUTH_METHOD_PAP;
return TRUE;
+   } else if (g_str_equal(str, "none")) {
+   *auth = OFONO_GPRS_AUTH_METHOD_NONE;
+   return TRUE;
}
 
return FALSE;
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH 3/4] plugins/file-provisioning.c: take into account auth_methods NONE and ANY

[Giacinto Cifelli]

added the support for the NONE and ANY authentication methods
---
 plugins/file-provision.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/plugins/file-provision.c b/plugins/file-provision.c
index d4846a6..f7412e6 100644
--- a/plugins/file-provision.c
+++ b/plugins/file-provision.c
@@ -104,6 +104,12 @@ static int config_file_provision_get_settings(const char 
*mcc,
else if (g_strcmp0(value, "pap") == 0)
(*settings)[0].auth_method =
OFONO_GPRS_AUTH_METHOD_PAP;
+   else if (g_strcmp0(value, "none") == 0)
+   (*settings)[0].auth_method =
+   OFONO_GPRS_AUTH_METHOD_NONE;
+   else if (g_strcmp0(value, "any") == 0)
+   (*settings)[0].auth_method =
+   OFONO_GPRS_AUTH_METHOD_ANY;
else
DBG("Unknown auth method: %s", value);
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH 1/4] gprs: added two authentication methods: NONE and ANY

[Giacinto Cifelli]

OFONO_GPRS_AUTH_METHOD_NONE disables the authentication
OFONO_GPRS_AUTH_METHOD_ANY tries first CHAP then PAP,
but only applies to supporting drivers and modules
---
 include/gprs-context.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/gprs-context.h b/include/gprs-context.h
index 20ca9ef..7e1dfcc 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -55,7 +55,9 @@ enum ofono_gprs_context_type {
 };
 
 enum ofono_gprs_auth_method {
-   OFONO_GPRS_AUTH_METHOD_CHAP = 0,
+   OFONO_GPRS_AUTH_METHOD_ANY = 0,
+   OFONO_GPRS_AUTH_METHOD_NONE,
+   OFONO_GPRS_AUTH_METHOD_CHAP,
OFONO_GPRS_AUTH_METHOD_PAP,
 };
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] ril driver: commented out pragma

[Giacinto Cifelli]

gcc 4.8.4 rejects the line:
and this breaks the default compilation.
Since they may be needed for other compilers,
I have just commented them out with '//', so that they stand out.
---
 drivers/rilmodem/call-forwarding.c  | 2 +-
 drivers/rilmodem/network-registration.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rilmodem/call-forwarding.c 
b/drivers/rilmodem/call-forwarding.c
index 4aff4d3..0bdab3f 100644
--- a/drivers/rilmodem/call-forwarding.c
+++ b/drivers/rilmodem/call-forwarding.c
@@ -38,7 +38,7 @@
 #include 
 #include "common.h"
 
-#pragma GCC diagnostic ignored "-Wrestrict"
+//#pragma GCC diagnostic ignored "-Wrestrict"
 
 #include "gril.h"
 
diff --git a/drivers/rilmodem/network-registration.c 
b/drivers/rilmodem/network-registration.c
index 809b3bc..9895c6d 100644
--- a/drivers/rilmodem/network-registration.c
+++ b/drivers/rilmodem/network-registration.c
@@ -37,7 +37,7 @@
 #include 
 #include 
 
-#pragma GCC diagnostic ignored "-Wrestrict"
+//#pragma GCC diagnostic ignored "-Wrestrict"
 
 #include 
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH] ril driver: commented out pragma\n\ngcc 4.8.4 rejects the line:\n#pragma GCC diagnostic ignored "-Wrestrict"\nand this breaks the default compilation.\nsince they may be needed for other c

[Giacinto Cifelli]

please ignore this. the commit message was wrongly formatted for some
reason.

On Thu, Sep 20, 2018 at 6:00 AM Giacinto Cifelli  wrote:

> ---
>  drivers/rilmodem/call-forwarding.c  | 2 +-
>  drivers/rilmodem/network-registration.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/rilmodem/call-forwarding.c
> b/drivers/rilmodem/call-forwarding.c
> index 4aff4d3..0bdab3f 100644
> --- a/drivers/rilmodem/call-forwarding.c
> +++ b/drivers/rilmodem/call-forwarding.c
> @@ -38,7 +38,7 @@
>  #include 
>  #include "common.h"
>
> -#pragma GCC diagnostic ignored "-Wrestrict"
> +//#pragma GCC diagnostic ignored "-Wrestrict"
>
>  #include "gril.h"
>
> diff --git a/drivers/rilmodem/network-registration.c
> b/drivers/rilmodem/network-registration.c
> index 809b3bc..9895c6d 100644
> --- a/drivers/rilmodem/network-registration.c
> +++ b/drivers/rilmodem/network-registration.c
> @@ -37,7 +37,7 @@
>  #include 
>  #include 
>
> -#pragma GCC diagnostic ignored "-Wrestrict"
> +//#pragma GCC diagnostic ignored "-Wrestrict"
>
>  #include 
>
> --
> 1.9.1
>
>
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] ril driver: commented out pragma\n\ngcc 4.8.4 rejects the line:\n#pragma GCC diagnostic ignored "-Wrestrict"\nand this breaks the default compilation.\nsince they may be needed for other compi

[Giacinto Cifelli]

---
 drivers/rilmodem/call-forwarding.c  | 2 +-
 drivers/rilmodem/network-registration.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/rilmodem/call-forwarding.c 
b/drivers/rilmodem/call-forwarding.c
index 4aff4d3..0bdab3f 100644
--- a/drivers/rilmodem/call-forwarding.c
+++ b/drivers/rilmodem/call-forwarding.c
@@ -38,7 +38,7 @@
 #include 
 #include "common.h"
 
-#pragma GCC diagnostic ignored "-Wrestrict"
+//#pragma GCC diagnostic ignored "-Wrestrict"
 
 #include "gril.h"
 
diff --git a/drivers/rilmodem/network-registration.c 
b/drivers/rilmodem/network-registration.c
index 809b3bc..9895c6d 100644
--- a/drivers/rilmodem/network-registration.c
+++ b/drivers/rilmodem/network-registration.c
@@ -37,7 +37,7 @@
 #include 
 #include 
 
-#pragma GCC diagnostic ignored "-Wrestrict"
+//#pragma GCC diagnostic ignored "-Wrestrict"
 
 #include 
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH 1/2] gemaltomodem: added Gemalto-specific gprs-context atom This atom name is not the same as the driver, because it might be necessary to add other grps-context atoms, according to the needs.

[Giacinto Cifelli]

---
 drivers/gemaltomodem/gemaltomodem.c  |   2 +
 drivers/gemaltomodem/gemaltomodem.h  |   3 +
 drivers/gemaltomodem/gprs-context-wwan.c | 446 +++
 3 files changed, 451 insertions(+)
 create mode 100644 drivers/gemaltomodem/gprs-context-wwan.c

diff --git a/drivers/gemaltomodem/gemaltomodem.c 
b/drivers/gemaltomodem/gemaltomodem.c
index 614ca81..7afd3c1 100644
--- a/drivers/gemaltomodem/gemaltomodem.c
+++ b/drivers/gemaltomodem/gemaltomodem.c
@@ -35,6 +35,7 @@
 static int gemaltomodem_init(void)
 {
gemalto_location_reporting_init();
+   gemaltowwan_gprs_context_init();
gemalto_voicecall_init();
return 0;
 }
@@ -42,6 +43,7 @@ static int gemaltomodem_init(void)
 static void gemaltomodem_exit(void)
 {
gemalto_location_reporting_exit();
+   gemaltowwan_gprs_context_exit();
gemalto_voicecall_exit();
 }
 
diff --git a/drivers/gemaltomodem/gemaltomodem.h 
b/drivers/gemaltomodem/gemaltomodem.h
index 4e6ed16..f45aea9 100644
--- a/drivers/gemaltomodem/gemaltomodem.h
+++ b/drivers/gemaltomodem/gemaltomodem.h
@@ -24,5 +24,8 @@
 extern void gemalto_location_reporting_init();
 extern void gemalto_location_reporting_exit();
 
+extern void gemaltowwan_gprs_context_init();
+extern void gemaltowwan_gprs_context_exit();
+
 extern void gemalto_voicecall_init();
 extern void gemalto_voicecall_exit();
diff --git a/drivers/gemaltomodem/gprs-context-wwan.c 
b/drivers/gemaltomodem/gprs-context-wwan.c
new file mode 100644
index 000..12378a3
--- /dev/null
+++ b/drivers/gemaltomodem/gprs-context-wwan.c
@@ -0,0 +1,446 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2017 Piotr Haber. All rights reserved.
+ *  Copyright (C) 2018 Sebastian Arnd. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include 
+#endif
+
+#define _GNU_SOURCE
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "gemaltomodem.h"
+
+static const char *none_prefix[] = { NULL };
+
+enum state {
+   STATE_IDLE,
+   STATE_ENABLING,
+   STATE_DISABLING,
+   STATE_ACTIVE,
+};
+
+struct gprs_context_data {
+   GAtChat *chat;
+   unsigned int active_context;
+   char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
+   char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
+   enum ofono_gprs_auth_method auth_method;
+   enum state state;
+   enum ofono_gprs_proto proto;
+   char address[64];
+   char netmask[64];
+   char gateway[64];
+   char dns1[64];
+   char dns2[64];
+   ofono_gprs_context_cb_t cb;
+   void *cb_data;
+   int use_wwan;
+};
+
+static gboolean gemalto_get_auth_command(struct ofono_modem *modem, int cid,
+   enum ofono_gprs_auth_method auth_method,
+   const char *username, const char *password,
+   char *buf, guint buflen)
+{
+   int gto_auth = ofono_modem_get_integer(modem, "Gemalto_Auth");
+   int len;
+   /*
+* 0: use cgauth
+* 1: use sgauth(pwd, user)
+* 2: use sgauth(user, pwd)
+*/
+
+   int auth_type;
+
+   switch (auth_method) {
+   case OFONO_GPRS_AUTH_METHOD_PAP:
+   auth_type = 1;
+   break;
+   case OFONO_GPRS_AUTH_METHOD_CHAP:
+   auth_type = 2;
+   break;
+   case OFONO_GPRS_AUTH_METHOD_NONE:
+   default:
+   auth_type = 0;
+   break;
+   }
+
+   if (auth_type != 0 && (!*username || !*password))
+   return FALSE;
+
+   switch (gto_auth) {
+   case 1:
+   case 2:
+   len = snprintf(buf, buflen, "AT^SGAUTH=%d", cid);
+   break;
+   case 0:
+   default:
+   len = snprintf(buf, buflen, "AT+CGAUTH=%d", cid);
+   break;
+   }
+
+   buflen -= len;
+
+   switch(auth_type) {
+   case 0:
+
+   switch (gto_auth) {
+   case 2:
+   snprintf(buf+len, buflen, ",0,\"\",\"\"");
+   break;
+   case 0:
+   case 1:
+   default:
+   snprintf(buf+len, buflen, ",0");
+   break;
+   }
+   break;
+
+   case 1:
+   case 2:
+
+   switch (gto_auth) {
+   case 1:
+

[PATCH 2/2] gemaltomodem: added gprs-context-wwan.c in the makefile

[Giacinto Cifelli]

---
 Makefile.am | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile.am b/Makefile.am
index e2363e2..fabda0a 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -397,6 +397,7 @@ builtin_modules += gemaltomodem
 builtin_sources += drivers/atmodem/atutil.h \
drivers/gemaltomodem/gemaltomodem.h \
drivers/gemaltomodem/gemaltomodem.c \
+   drivers/gemaltomodem/gprs-context-wwan.c \
drivers/gemaltomodem/voicecall.c \
drivers/gemaltomodem/location-reporting.c
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH 1/2] gemaltomodem: added voicecall atom

[Giacinto Cifelli]

---
 drivers/gemaltomodem/gemaltomodem.c |   3 +-
 drivers/gemaltomodem/gemaltomodem.h |   3 +
 drivers/gemaltomodem/voicecall.c| 965 
 3 files changed, 970 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gemaltomodem/voicecall.c

diff --git a/drivers/gemaltomodem/gemaltomodem.c 
b/drivers/gemaltomodem/gemaltomodem.c
index 91cf238..614ca81 100644
--- a/drivers/gemaltomodem/gemaltomodem.c
+++ b/drivers/gemaltomodem/gemaltomodem.c
@@ -35,13 +35,14 @@
 static int gemaltomodem_init(void)
 {
gemalto_location_reporting_init();
-
+   gemalto_voicecall_init();
return 0;
 }
 
 static void gemaltomodem_exit(void)
 {
gemalto_location_reporting_exit();
+   gemalto_voicecall_exit();
 }
 
 OFONO_PLUGIN_DEFINE(gemaltomodem, "Gemalto modem driver", VERSION,
diff --git a/drivers/gemaltomodem/gemaltomodem.h 
b/drivers/gemaltomodem/gemaltomodem.h
index 7ea1e8f..4e6ed16 100644
--- a/drivers/gemaltomodem/gemaltomodem.h
+++ b/drivers/gemaltomodem/gemaltomodem.h
@@ -23,3 +23,6 @@
 
 extern void gemalto_location_reporting_init();
 extern void gemalto_location_reporting_exit();
+
+extern void gemalto_voicecall_init();
+extern void gemalto_voicecall_exit();
diff --git a/drivers/gemaltomodem/voicecall.c b/drivers/gemaltomodem/voicecall.c
new file mode 100644
index 000..2a2af0d
--- /dev/null
+++ b/drivers/gemaltomodem/voicecall.c
@@ -0,0 +1,965 @@
+/*
+ *
+ *  oFono - Open Source Telephony
+ *
+ *  Copyright (C) 2008-2011  Intel Corporation. All rights reserved.
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 as
+ *  published by the Free Software Foundation.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include 
+#endif
+
+#define _GNU_SOURCE
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#include 
+#include 
+#include 
+
+#include "gatchat.h"
+#include "gatresult.h"
+
+#include "common.h"
+
+#include "gemaltomodem.h"
+
+static const char *clcc_prefix[] = { "+CLCC:", NULL };
+static const char *none_prefix[] = { NULL };
+
+/* According to 27.007 COLP is an intermediate status for ATD */
+static const char *atd_prefix[] = { "+COLP:", NULL };
+
+#define FLAG_NEED_CLIP 1
+
+struct voicecall_data {
+   GAtChat *chat;
+   GSList *calls;
+   unsigned int local_release;
+   unsigned int vendor;
+   unsigned char flags;
+};
+
+struct release_id_req {
+   struct ofono_voicecall *vc;
+   ofono_voicecall_cb_t cb;
+   void *data;
+   int id;
+};
+
+struct change_state_req {
+   struct ofono_voicecall *vc;
+   ofono_voicecall_cb_t cb;
+   void *data;
+   int affected_types;
+};
+
+static void generic_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+   struct change_state_req *req = user_data;
+   struct voicecall_data *vd = ofono_voicecall_get_data(req->vc);
+   struct ofono_error error;
+
+   decode_at_error(, g_at_result_final_response(result));
+
+   if (ok && req->affected_types) {
+   GSList *l;
+   struct ofono_call *call;
+
+   for (l = vd->calls; l; l = l->next) {
+   call = l->data;
+
+   if (req->affected_types & (1 << call->status))
+   vd->local_release |= (1 << call->id);
+   }
+   }
+
+   req->cb(, req->data);
+}
+
+static void gemalto_template(const char *cmd, struct ofono_voicecall *vc,
+   GAtResultFunc result_cb, unsigned int affected_types,
+   ofono_voicecall_cb_t cb, void *data)
+{
+   struct voicecall_data *vd = ofono_voicecall_get_data(vc);
+   struct change_state_req *req = g_try_new0(struct change_state_req, 1);
+
+   if (req == NULL)
+   goto error;
+
+   req->vc = vc;
+   req->cb = cb;
+   req->data = data;
+   req->affected_types = affected_types;
+
+   if (g_at_chat_send(vd->chat, cmd, none_prefix,
+   result_cb, req, g_free) > 0)
+   return;
+
+error:
+   g_free(req);
+
+   CALLBACK_WITH_FAILURE(cb, data);
+}
+
+static void gemalto_answer(struct ofono_voicecall *vc,
+   ofono_voicecall_cb_t cb, void *data)
+{
+   gemalto_template("ATA", vc, generic_cb, 0, cb, data);
+}
+
+static void gemalto_hangup_all(struct ofono_voicecall *vc,
+   ofono_voicecall_cb_t cb, void *data)
+{
+   unsigned 

[PATCH 2/2] gemaltomodem: added voicecall.c in the makefile

[Giacinto Cifelli]

---
 Makefile.am | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile.am b/Makefile.am
index 6dee4ce..e2363e2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -397,6 +397,7 @@ builtin_modules += gemaltomodem
 builtin_sources += drivers/atmodem/atutil.h \
drivers/gemaltomodem/gemaltomodem.h \
drivers/gemaltomodem/gemaltomodem.c \
+   drivers/gemaltomodem/voicecall.c \
drivers/gemaltomodem/location-reporting.c
 
 builtin_modules += xmm7modem
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] atmodem: added vendor Gemalto

[Giacinto Cifelli]

---
 drivers/atmodem/vendor.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
index 721796e..abe2d89 100644
--- a/drivers/atmodem/vendor.h
+++ b/drivers/atmodem/vendor.h
@@ -49,4 +49,5 @@ enum ofono_vendor {
OFONO_VENDOR_UBLOX_TOBY_L2,
OFONO_VENDOR_CINTERION,
OFONO_VENDOR_XMM,
+   OFONO_VENDOR_GEMALTO,
 };
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] atmodem: added inline function to parse AT+CESQ result for signal strenght It complements the function at_util_convert_signal_strength for AT+CSQ, which does not apply well to 3G and not at al

[Giacinto Cifelli]

---
 drivers/atmodem/atutil.h | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/atmodem/atutil.h b/drivers/atmodem/atutil.h
index 7113a4c..57573cb 100644
--- a/drivers/atmodem/atutil.h
+++ b/drivers/atmodem/atutil.h
@@ -115,6 +115,20 @@ static inline int at_util_convert_signal_strength(int 
strength)
return result;
 }
 
+static inline int at_util_convert_signal_strength_cesq(int strength_GSM, int 
strength_UTRAN, int strength_EUTRAN)
+{
+   int result = -1;
+
+   if (strength_GSM != 99)
+   result = (strength_GSM * 100) / 63;
+   else if (strength_UTRAN != 255)
+   result = (strength_UTRAN * 100) / 96;
+   else if (strength_EUTRAN != 255)
+   result = (strength_EUTRAN * 100) / 97;
+
+   return result;
+}
+
 #define CALLBACK_WITH_FAILURE(cb, args...) \
do {\
struct ofono_error cb_e;\
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

[PATCH] fixed typo in doc/emergency-call-handling.txt

[Giacinto Cifelli]

---
 doc/emergency-call-handling.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/emergency-call-handling.txt b/doc/emergency-call-handling.txt
index 69b217d..0436047 100644
--- a/doc/emergency-call-handling.txt
+++ b/doc/emergency-call-handling.txt
@@ -14,7 +14,7 @@ What oFono will do:
- Post online atoms will be created.
- Upon reception of Dial request, Emergency mode is activated.
- Once the call is ended, Emergency mode is deactivated.
-   - Modem remains in online mode with full funcationality.
+   - Modem remains in online mode with full functionality.
 
 Case 2: Call in SIM Present and PIN required state
 
-- 
1.9.1

___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

Hi Slava,

On 09/19/2018 06:14 PM, Slava Monich wrote:

On 20/09/18 00:45, Denis Kenzior wrote:
(ANY = PAP_CHAP, and don't ask me why we added new values to the 
beginning of the enum - it was before we started using binary 
plugins). I would be more than happy if upstream started to use the 
same enum!




That assumes that we should support your METHOD_ANY thing.  I've not 
heard any good arguments for that yet...


At least one Chinese modem I dealt with had AT+ EGPAU = ,, 
 where  is 0 for PAP, 1 for CHAP, 2 for NONE and 3 for 
PAP_CHAP. Also, Android RIL interface has value 3 reserved for PAP 
(see ril.h for older Androids and ApnAuthType in binder radio interface 
for Android 8+). To me that sounds like a valid use case.


So I went in and checked (someone can correct me if I'm wrong)

- The three-four generations of Intel modems I checked do not support this
- QMI doesn't support this
- MBIM doesn't support this (but supports MSChap/MSChapV2, funnily enough)
- 3GPP doesn't support this
- Telit doesn't support this
- Model Broadband Provider Info doesn't support this

Are you sure it isn't some weird vendor feature where they try all 
possibilities one at a time?  E.g. it tries CHAP first, then tries PAP, 
then tries None?


And how do you plan on supporting this on modems that physically do not 
support such an enumeration?  What would be the exact semantics?  Is 
this modem supported by oFono upstream?


This conversation is moot until you introduce support for such a modem 
or more specific information is introduced.  And until / unless this 
happens, we're not introducing random enumerations into the D-Bus API 
that we have to live with for a long time.


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Slava Monich]

On 20/09/18 00:45, Denis Kenzior wrote:
(ANY = PAP_CHAP, and don't ask me why we added new values to the 
beginning of the enum - it was before we started using binary 
plugins). I would be more than happy if upstream started to use the 
same enum!




That assumes that we should support your METHOD_ANY thing.  I've not 
heard any good arguments for that yet...


At least one Chinese modem I dealt with had AT+ EGPAU = ,, 
 where  is 0 for PAP, 1 for CHAP, 2 for NONE and 3 for 
PAP_CHAP. Also, Android RIL interface has value 3 reserved for PAP 
(see ril.h for older Androids and ApnAuthType in binder radio interface 
for Android 8+). To me that sounds like a valid use case.


-Slava
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: ublox TOBY-R200

[Denis Kenzior]

Hi Frank,

Please stop top-posting on this mailing list.  We follow the accepted 
best practice of open source mailing lists and require replies to be 
in-line.



Sep 19 20:17:41 gateway daemon.warn ofonod[152]: Context activated for
driver that doesn't support automatic context activation.


So this is a big clue that something is wrong.  The uBlox driver can 
create 'atmodem' or 'ubloxmodem' gprs contexts.  The atmodem 
gprs_context driver does not support .read_settings.  That is because we 
so far have not needed to support LTE contexts via PPP.  You would 
likely end up spiking the CPU to 100% and melt your device if you tried 
that ;)


The 'ubloxmodem' gprs_context driver does support read_settings, but I 
don't know enough about this hardware to guide you any further.


I'm new to oFono and connman so I'm going to try to get cellular 
connectivity working on a BeagleBone Black with u-blox TOBY-L201 (oFono 
supported modem) plugged into USB first.  I can activate a context 
successfully but cellular still says Connected = False in connmanctl on 
the BeagleBone Black.  Any pointers or links on how to do get connman 
working with oFono are appreciated.




This might be better asked on the ConnMan mailing list.

Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

Hi Slava,


I understand that! It makes things easier for you guys.

But we had to avoid certain compile-time dependencies in ofono, and a 
straightforward (and perhaps the only) way to achieve that was to use 
binary plugins. For us plugin API is not subject to change (plugins 
don't necessarily get upgraded together with ofono), meaning more 
changes between our fork and upstream in case if upstream breaks it, 
more maintenance work and more room for errors. Obviously, I would 
like to keep differences to a minimum.


So I sympathize, but really it might also be a hint to finally start 
getting things upstream.




I'm just humbly asking - if there's a way to keep plugin API backward 
compatible, please do it that way. There is at least one person in the 
world who would appreciate it.




The problem is, the next time this comes up there may be no way to avoid 
it...  Or we break binary compatibility inadvertently.  It is just not 
something we're going to be looking out for.




Actually, in our fork we have already modified enum 
ofono_gprs_auth_method and that's what we have there:


enum ofono_gprs_auth_method {
     OFONO_GPRS_AUTH_METHOD_ANY = 0,
     OFONO_GPRS_AUTH_METHOD_NONE,
     OFONO_GPRS_AUTH_METHOD_CHAP,
     OFONO_GPRS_AUTH_METHOD_PAP,
};


So you already made life very hard for yourself ;)



(ANY = PAP_CHAP, and don't ask me why we added new values to the 
beginning of the enum - it was before we started using binary plugins). 
I would be more than happy if upstream started to use the same enum!




That assumes that we should support your METHOD_ANY thing.  I've not 
heard any good arguments for that yet...


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: ublox TOBY-R200

[Frank Vasquez]

Hi Giacinto,

I attached a better antenna to our custom board but I'm still getting the
same results.

AT+CSQ
+CSQ: 24,4

OK

# ./enable-modem
Connecting modem /ublox_0...
# ./create-internet-context hologram
Found context /ublox_0/context1
Setting APN to hologram
# ./online-modem
Setting modem /ublox_0 online...
#
Sep 19 20:17:41 gateway daemon.warn ofonod[152]: Context activated for
driver that doesn't support automatic context activation.
Sep 19 20:17:41 gateway daemon.err connmand[148]: Failed to set regulatory
domain
# ./activate-context
Error activating /ublox_0/context1: org.ofono.Error.NotAttached: GPRS is
not attached

Reception appears to be good and I waited more than 2 minutes before
./activate-context.

I'm new to oFono and connman so I'm going to try to get cellular
connectivity working on a BeagleBone Black with u-blox TOBY-L201 (oFono
supported modem) plugged into USB first.  I can activate a context
successfully but cellular still says Connected = False in connmanctl on the
BeagleBone Black.  Any pointers or links on how to do get connman working
with oFono are appreciated.

Cheers,
Frank

On Tue, Sep 18, 2018 at 8:02 PM Giacinto Cifelli  wrote:

> Hi Frank,
>
> is it working? I had a look at ublox specification, and I think you need
> some more support in ofono for establishing the LTE default bearer.
> I will take this back once I have submitted some patches for this support
> (and have been accepted).
>
> Best regards,
> Giacinto
>
>
> On Sat, Sep 15, 2018 at 8:31 AM Giacinto Cifelli 
> wrote:
>
>> Hi Frank,
>>
>> I am happy that it started working.
>> Reviewing your email, I doubted that it was more a configure parameters
>> issue.
>> The rest below, I shall not top-post in this distribution list.
>>
>> On Sat, Sep 15, 2018 at 2:54 AM Frank Vasquez  wrote:
>>
>>> Hi Giacinto,
>>>
>>> I applied your patch for the TOBY-R200 and the results look very
>>> promising.
>>>
>>> # ./list-modems
>>> [ /ublox_0 ]
>>> Online = 1
>>> Powered = 1
>>> Lockdown = 0
>>> Emergency = 0
>>> Manufacturer = u-blox
>>> Model = TOBY-R200
>>> Revision = 30.31
>>> Serial = 352848080392646
>>> Interfaces = org.ofono.NetworkRegistration org.ofono.NetworkMonitor
>>> org.ofono.ConnectionManager org.ofono.LongTermEvolution
>>> org.ofono.AllowedAccessPoints org.ofono.VoiceCallManager
>>> org.ofono.SimManager
>>> Features = net gprs sim
>>> Type = hardware
>>> [ org.ofono.NetworkRegistration ]
>>> Status = searching
>>> Mode = auto
>>> Name =
>>> [ org.ofono.NetworkMonitor ]
>>> [ org.ofono.ConnectionManager ]
>>> Attached = 0
>>> Bearer = none
>>> RoamingAllowed = 0
>>> Powered = 1
>>> [ org.ofono.LongTermEvolution ]
>>> DefaultAccessPointName =
>>> [ org.ofono.AllowedAccessPoints ]
>>> [ org.ofono.VoiceCallManager ]
>>> EmergencyNumbers = 112 911
>>> [ org.ofono.SimManager ]
>>> Present = 1
>>> CardIdentifier = 8944501011176099176
>>> SubscriberIdentity = 234507098609917
>>> ServiceProviderName = Hologram
>>> FixedDialing = 0
>>> BarredDialing = 0
>>> MobileCountryCode = 234
>>> MobileNetworkCode = 50
>>> SubscriberNumbers =
>>> LockedPins =
>>> PreferredLanguages = en
>>> PinRequired = none
>>> Retries = [pin = 3] [pin2 = 3] [puk = 10] [puk2 = 10]
>>>
>>> # ./enable-modem
>>> Connecting modem /ublox_0...
>>> # ./create-internet-context hologram
>>> Found context /ublox_0/context1
>>> Setting APN to hologram
>>> # ./online-modem
>>> Setting modem /ublox_0 online...
>>> # ./activate-context
>>> Error activating /ublox_0/context1: org.ofono.Error.NotAttached: GPRS is
>>> not attached
>>>
>>>
>> Have you connected your antennas properly? Have you waited some 2 minutes
>> before trying the ./activate-context ?
>> If yes, then this module reports its attach status with some indicator
>> not recognized by ofono today (blind shot: +CEREG).
>>
>>
>>
>>> As you can see I was unable to activate-context but I got pretty close.
>>> I'm willing to apply more code changes if it means I can get cellular
>>> integration with connman.
>>>
>>
>> In general I test on Ubuntu with d-feed, so I don't know all the test
>> scripts, but there should be some to check whether the modem is reported
>> registered, to which technology, what the signal strength is, and if it is
>> reported attached.
>> If it is registered to LTE, it is attached, but ofono doesn't recognize
>> it automatically by radio technology, it needs a separate indicator.
>>
>>
>>>
>>> Cheers,
>>> Frank
>>>
>>> On Thu, Sep 13, 2018 at 9:19 PM Giacinto Cifelli 
>>> wrote:
>>>
 Hi Frank,

 both TOBYL2_COMPATIBLE_MODE and  TOBYL2_HIGH_THROUGHPUT_MODE behave
 the same in the code.
 (TOBYL2_MEDIUM_THROUGHPUT_MODE is recognized but discarded later in
 the code)
 Please replace the two attached files in the 

Re: [PATCH 2/3] sim800: add udev detection

[Clement Viel]

On Wed, Sep 19, 2018 at 03:03:57PM -0500, Denis Kenzior wrote:
Hi,

> Hi,
> 
> >>
> >>So I'm confused now.  If sim800 is a serial device, why do you have all this
> >>here?  Shouldn't this be handled by setup_serial_modem ?
> >>
> >
> >Because the modem was seen through a serial/USB converter, my kernel sees it 
> >as a USB device. So I used this function.
> >Should this be coded in a more generic way ?
> >
> 
> Yes, you should use the setup_serial_modem bits and the UDEV rule outlined
> in patch 3 should take care of the detection.
> 

All right, I'll change the detection and setup in next patch set.

> Regards,
> -Denis

Regards
Clem
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 2/3] sim800: add udev detection

[Denis Kenzior]

Hi,



So I'm confused now.  If sim800 is a serial device, why do you have all this
here?  Shouldn't this be handled by setup_serial_modem ?



Because the modem was seen through a serial/USB converter, my kernel sees it as 
a USB device. So I used this function.
Should this be coded in a more generic way ?



Yes, you should use the setup_serial_modem bits and the UDEV rule 
outlined in patch 3 should take care of the detection.


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 2/3] sim800: add udev detection

[Clement Viel]

On Wed, Sep 19, 2018 at 11:46:34AM -0500, Denis Kenzior wrote:
Hi,

> Hi,
> 
> On 09/18/2018 03:36 PM, ClémentViel wrote:
> >From: clem 
> >
> >---
> >  plugins/udevng.c | 49 +
> >  1 file changed, 49 insertions(+)
> >
> >diff --git a/plugins/udevng.c b/plugins/udevng.c
> >index 02d049e..9a1be6a 100644
> >--- a/plugins/udevng.c
> >+++ b/plugins/udevng.c
> >@@ -710,6 +710,53 @@ static gboolean setup_telitqmi(struct modem_info *modem)
> > return TRUE;
> >  }
> >+static gboolean setup_sim800(struct modem_info *modem)
> >+{
> >+const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
> >+GSList *list;
> >+
> >+DBG("%s", modem->syspath);
> >+
> >+for (list = modem->devices; list; list = list->next) {
> >+struct device_info *info = list->data;
> >+
> >+DBG("%s %s %s %s", info->devnode, info->interface,
> >+info->number, info->label);
> >+
> >+if (g_strcmp0(info->label, "aux") == 0) {
> >+DBG("setting aux as info->devnode");
> >+aux = info->devnode;
> >+if (mdm != NULL)
> >+break;
> >+} else if (g_strcmp0(info->label, "modem") == 0) {
> >+mdm = info->devnode;
> >+if (aux != NULL)
> >+break;
> >+} else if (g_strcmp0(info->interface, "255/0/0") == 0) {
> >+if (g_strcmp0(info->number, "00") == 0)
> >+mdm = info->devnode;
> >+else if (g_strcmp0(info->number, "01") == 0)
> >+gps = info->devnode;
> >+else if (g_strcmp0(info->number, "02") == 0)
> >+aux = info->devnode;
> >+else if (g_strcmp0(info->number, "03") == 0)
> >+mdm = info->devnode;
> >+}
> >+}
> >+
> 
> So I'm confused now.  If sim800 is a serial device, why do you have all this
> here?  Shouldn't this be handled by setup_serial_modem ?
> 

Because the modem was seen through a serial/USB converter, my kernel sees it as 
a USB device. So I used this function.
Should this be coded in a more generic way ?

> >+DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
> >+
> >+if (mdm == NULL) {
> >+DBG("modem did not set up");
> >+return FALSE;
> >+}
> >+
> >+ofono_modem_set_string(modem->modem, "Device", mdm);
> >+ofono_modem_set_string(modem->modem, "Modem", mdm);
> >+
> >+return TRUE;
> >+}
> >+
> >  /* TODO: Not used as we have no simcom driver */
> >  static gboolean setup_simcom(struct modem_info *modem)
> >  {
> >@@ -1282,6 +1329,7 @@ static struct {
> > { "telit",  setup_telit,"device/interface"  },
> > { "telitqmi",   setup_telitqmi  },
> > { "simcom", setup_simcom},
> >+{ "sim800", setup_sim800},
> > { "sim7100",setup_sim7100   },
> > { "zte",setup_zte   },
> > { "icera",  setup_icera },
> >@@ -1654,6 +1702,7 @@ static struct {
> > { "alcatel","option",   "1bbb", "0017"  },
> > { "novatel","option",   "1410"  },
> > { "zte","option",   "19d2"  },
> >+{ "sim800", "option",   "05c6", "9000"  },
> > { "simcom", "option",   "05c6", "9000"  },
> > { "sim7100","option",   "1e0e", "9001"  },
> > { "telit",  "usbserial","1bc7"  },
> >

I must use tabs instead of spacesI have no excuses...

> 
> Regards,
> -Denis

Regards
Clem
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 1/3] sim800: adding support for Simcom SIM800 modem

[Clement Viel]

On Wed, Sep 19, 2018 at 08:03:27PM +0300, Pičugins Arsenijs wrote:
Hi,

I guess I submit it in a hurry and forgot to add the firmware version.
BTW, your problem may be related to this. Are you sure your CMUX command is not 
returning an error ? Thus preventing ofono from enabling the modem ? I had this 
problem and Simcom sent me an updated firmware that supports the CMUX command.

> Hello!  You might want to describe how this is 
> different from sim900 that it warrants a fully separate 
> driver? I've tried to apply this patch yesterday 
> evening (since I too aminterested in SIM800 integration) and tried 
> to apply it, patch 2doesn't apply on current master for some 
> reason. Here's a diffbetween current SIM900 and this 
> SIM800: href="https://matrix.org/_matrix/media/v1/download/matrix.org/KqDxbcuYLxrcceUfRorxYgus;
>  
> data-external="true">https://matrix.org/_matrix/media/v1/download/matrix.org/KqDxbcuYLxrcceUfRorxYgus
>  tl;dr: apart from - adding phonebook 
> support (two lines, I don't understand it quite yet)- the sleep() 
> hack- a comment clarifying a problem with some unknown version of 
> firmware- DBG() log calls- sim900 = sim800 
> renaming it's no different than current SIM900 driver. 
> Unfortunately, thatdoesn't solve the problems I'm personally 
> experiencing (hang onCFUN, for one). 
> Cheers!Arsenijs 19.09.2018, 19:51, 
> "Denis Kenzior" denk...@gmail.com: type="cite">Hi,On 09/18/2018 03:36 PM, ClémentViel 
> wrote: From: clem  href="mailto:vielclem...@gmail.com;>vielclem...@gmail.com 
> You might want to describe how this is different from 
> sim900 that itwarrants a fully separate driver?If there are 
> only minor differences, then this can be handled via UDEVattributes or 
> querying +CGMM, etc.  ---   Makefile.am | 4 + />   plugins/sim800.c | 424 
> +++   2 files 
> changed, 428 insertions(+)   create mode 100644 plugins/sim800.c 
> snip  + +static 
> void sim800_post_sim(struct ofono_modem *modem) +{ + struct 
> sim800_data *data = ofono_modem_get_data(modem); + struct ofono_gprs 
> *gprs; + struct ofono_gprs_context *gc; + + DBG("%p", 
> modem); + + /* Dirty Hack : give some time to sim800 for 
> multiplexing + * to be effective and avoid VOICE_DLC to be + * 
> flooded thus leading to a "famine" situation + */ + + 
> sleep(2);No sleeps inside plugins. That blocks the 
> entire daemon and we can'thave that. How does this help you anyway? 
> GAtChat is a queue, so only1 command is outstanding at a time. 
>  + ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", /> + data-dlcs[SMS_DLC]); + + + gprs = 
> ofono_gprs_create(modem, 0, "atmodem", data-dlcs[GPRS_DLC]); + if 
> (gprs == NULL) + return; + + gc = 
> ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM, + "atmodem", 
> data-dlcs[GPRS_DLC]); + if (gc) + 
> ofono_gprs_add_context(gprs, gc); +} + />Regards,-Denis />___ofono mailing list /> href="mailto:ofono@ofono.org;>ofono@ofono.org href="https://lists.ofono.org/mailman/listinfo/ofono;>https://lists.ofono.org/mailman/listinfo/ofono
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 1/3] sim800: adding support for Simcom SIM800 modem

[Clement Viel]

On Wed, Sep 19, 2018 at 11:51:26AM -0500, Denis Kenzior wrote:

Hi,

> Hi,
> 
> On 09/18/2018 03:36 PM, ClémentViel wrote:
> >From: clem 
> >
> 
> You might want to describe how this is different from sim900 that it
> warrants a fully separate driver?
> 
> If there are only minor differences, then this can be handled via UDEV
> attributes or querying +CGMM, etc.
> 
> >---
> >  Makefile.am  |   4 +
> >  plugins/sim800.c | 424 
> > +++
> >  2 files changed, 428 insertions(+)
> >  create mode 100644 plugins/sim800.c
> >

I guess I went straightforward and because sim800 wasn't supported I wanted to 
add it as a plugin.
Sim800 is really close from sim900, in fact theey are compatible. But because 
sim800 series are newer.
sim800 have more AT commands than sim900  so maybe the compatibility won't be 
complete if the two drivers are merged...


> 
> 
> 
> >+
> >+static void sim800_post_sim(struct ofono_modem *modem)
> >+{
> >+struct sim800_data *data = ofono_modem_get_data(modem);
> >+struct ofono_gprs *gprs;
> >+struct ofono_gprs_context *gc;
> >+
> >+DBG("%p", modem);
> >+
> >+/* Dirty Hack : give some time to sim800 for multiplexing
> >+ *  to be effective and avoid VOICE_DLC to 
> >be
> >+ *  flooded thus leading to a "famine" 
> >situation
> >+ */
> >+
> >+sleep(2);
> 
> No sleeps inside plugins.  That blocks the entire daemon and we can't have
> that.  How does this help you anyway?  GAtChat is a queue, so only 1 command
> is outstanding at a time.

Yep that's why the "Dirty Hack" comment, I'll try another trick, there is an 
URC that we can trigger on.

> 
> >+ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
> >+data->dlcs[SMS_DLC]);
> >+
> >+
> >+gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
> >+if (gprs == NULL)
> >+return;
> >+
> >+gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
> >+"atmodem", data->dlcs[GPRS_DLC]);
> >+if (gc)
> >+ofono_gprs_add_context(gprs, gc);
> >+}
> >+
> 
> Regards,
> -Denis

Regards 
Clement
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 3/3] sim800: adding documentation to inform about the udev rule the user must add

[Pičugins Arsenijs]

>+To enable SIM900 module support you need to put the following BTW, there's a typo in your documentation patch - you're stillmentioning "SIM900" in the description. Cheers!Arsenijs 19.09.2018, 22:25, "Clement Viel" :On Wed, Sep 19, 2018 at 11:43:49AM -0500, Denis Kenzior wrote: Hi, On 09/18/2018 03:36 PM, ClémentViel wrote: >From: clem  Can you fix your Author information (see AUTHORS for an example). Otherwise I cannot take these...Yep, I'll do it the right way   > >--- > doc/sim800-modem.txt | 7 +++ > 1 file changed, 7 insertions(+) > create mode 100644 doc/sim800-modem.txt > >diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt >new file mode 100644 >index 000..6731879 >--- /dev/null >+++ b/doc/sim800-modem.txt >@@ -0,0 +1,7 @@ >+SIM800 modem usage >+=== >+ >+To enable SIM900 module support you need to put the following >+udev rule into appropriate file in /{etc,lib}/udev/rules.d: >+ >+KERNEL=="ttyUSB0", ENV{OFONO_DRIVER}="sim800" > So sim800 is a serial device right? I take it you're running it from a serial converter here?Yes, there is a converter. I let "ttyUSB0" as the starter kit provided by SimCom comes with a converter.Regards, -Denis___ofono mailing listofono@ofono.orghttps://lists.ofono.org/mailman/listinfo/ofono___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 3/3] sim800: adding documentation to inform about the udev rule the user must add

[Clement Viel]

On Wed, Sep 19, 2018 at 11:43:49AM -0500, Denis Kenzior wrote:
> Hi,
> 
> On 09/18/2018 03:36 PM, ClémentViel wrote:
> >From: clem 
> 
> Can you fix your Author information (see AUTHORS for an example). Otherwise
> I cannot take these...

Yep, I'll do it the right way

> 
> >
> >---
> >  doc/sim800-modem.txt | 7 +++
> >  1 file changed, 7 insertions(+)
> >  create mode 100644 doc/sim800-modem.txt
> >
> >diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
> >new file mode 100644
> >index 000..6731879
> >--- /dev/null
> >+++ b/doc/sim800-modem.txt
> >@@ -0,0 +1,7 @@
> >+SIM800 modem usage
> >+===
> >+
> >+To enable SIM900 module support you need to put the following
> >+udev rule into appropriate file in /{etc,lib}/udev/rules.d:
> >+
> >+KERNEL=="ttyUSB0", ENV{OFONO_DRIVER}="sim800"
> >
> 
> So sim800 is a serial device right?  I take it you're running it from a
> serial converter here?

Yes, there is a converter. I let "ttyUSB0" as the starter kit provided by 
SimCom comes with a converter.

> 


> Regards,
> -Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

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.






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
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Giacinto Cifelli]

On Wed, Sep 19, 2018 at 6:30 PM Denis Kenzior  wrote:

> Hi Giacinto,
>
> > so, first the documentation?
>
> Correct.  Whenever you touch something that affects the D-Bus API, it is
> really preferable to have the D-Bus API changes in the preceding commit.
>   That way the reviewers can cross-reference what is being proposed to
> the actual implementation.  Anything that breaks the D-Bus API can also
> be spotted much earlier.
>
> oFono's D-Bus API is stable.  That means we can add new things, but we
> cannot change existing ones as that will break existing clients.
>
> We also cannot break existing settings, as that would break oFono
> installations on existing devices if they are upgraded.
>
> This is a handicap that we have to live with right now.
>
> 
>
> >
> > So there can be no unification with the GPRS naming now that the D-Bus
> > API is set?
>
> I'm afraid not.  But I'm not sure why this is a problem?
>
> >
> >  > +#define LTE_PROTO "Protocol"
> >  > +#define LTE_USERNAME "Username"
> >  > +#define LTE_PASSWORD "Password"
> >  > +#define LTE_AUTH_METHOD "AuthenticationMethod"
> >  >
> >  >   struct ofono_lte {
> >  >   const struct ofono_lte_driver *driver;
> >  > @@ -50,13 +55,82 @@ struct ofono_lte {
> >  >   DBusMessage *pending;
> >  >   struct ofono_lte_default_attach_info pending_info;
> >  >   struct ofono_lte_default_attach_info info;
> >  > + const char *prop_changed;
> >
> > ??  What memory location is this const char pointing to?
> >
> >
> > it is initialized to null with the containing structure.
>
> That is not what I'm asking.  This pointer points into data owned by
> DBusMessage and the semantics of whether it is valid or not are not
> enforced.  Avoid that at all cost...
>
> >
> > What about reading also the previous key?
>
> Ideally you shouldn't change the key in the first place.  But if you
> are, then yes you need to be able to read legacy settings versions in
> order to be backwards-compatible.
>
> >
> > You do realize you're never actually calling into the driver method,
> > right?  So none of these changes actually go out to the modem.  Have
> > you
> > actually tested any of this?
> >
> >
> > Yes, it works. Actually the only call is when the APN is set, as
> > mentioned in the lte-api.txt.
>
> No, it really doesn't...
>
> > And at that point all parameters are also set in the module.
> > It is not possible to set separately protocol and apn, and auth_method,
> > username, and password.
> > For ublox modules, the auth_method is also part of the APN name.
> >
> > So we kept the call into the module when the APN is set, and previously
> > to it all other parameters are set.
>
> You simply cannot do that.  You cannot assume that the client knows how
> your API is implemented underneath.  So if they set the APN first and
> then change the username, that has to work.  This is why the
> default_attach_info contains everything.  If anything is changed the
> entire structure is sent to the modem and every parameter is updated.
>

Hi Denis,

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?

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.

Or introduce an atom function that is called before modem->set_online(true)?

thanks,
Giacinto


>
> >
> > You have also mentioned that somewhere we should also verify that with
> > AUTH_NONE there are no user/pwd.
> > This also can only be verified at the end.
> >
> > Any suggestions to improve this, given these limitations?
> >
>
> See above.  Any parameter change has to trigger
> set_default_attach_info() call.  If something is invalid, then you have
> to return a D-Bus error.
>
> For example, if my method is set to 'none' and I try to do
> LongTermEvolution.SetProperty("DefaultUsername", "foo") that should
> return an error.
>
> >
> >  > + return dbus_message_ref(msg);;
> >  >   }
> >  >
> >  >   static DBusMessage *lte_set_property(DBusConnection *conn,
> >  > - DBusMessage *msg, void
> *data)
> >  > + DBusMessage *msg,
> > void *data)
> >  >   {
> >  >   struct ofono_lte *lte = data;
> >  >   DBusMessageIter iter;
> >  >   DBusMessageIter var;
> >  >   const char *property;
> >  >   const char *str;
> >  > + enum ofono_gprs_auth_method auth_method;
> >  > + enum ofono_gprs_proto proto;
> >  > +
> >  > + if (lte->driver->set_default_attach_info == NULL)
> > 

Re: [PATCH 1/3] sim800: adding support for Simcom SIM800 modem

[Pičugins Arsenijs]

Hello! > You might want to describe how this is different from sim900 that it> warrants a fully separate driver? I've tried to apply this patch yesterday evening (since I too aminterested in SIM800 integration) and tried to apply it, patch 2doesn't apply on current master for some reason. Here's a diffbetween current SIM900 and this SIM800:https://matrix.org/_matrix/media/v1/download/matrix.org/KqDxbcuYLxrcceUfRorxYgus tl;dr: apart from - adding phonebook support (two lines, I don't understand it quite yet)- the sleep() hack- a comment clarifying a problem with some unknown version of firmware- DBG() log calls- sim900 => sim800 renaming it's no different than current SIM900 driver. Unfortunately, thatdoesn't solve the problems I'm personally experiencing (hang onCFUN, for one). Cheers!Arsenijs 19.09.2018, 19:51, "Denis Kenzior" :Hi,On 09/18/2018 03:36 PM, ClémentViel wrote: From: clem  You might want to describe how this is different from sim900 that itwarrants a fully separate driver?If there are only minor differences, then this can be handled via UDEVattributes or querying +CGMM, etc.  ---   Makefile.am | 4 +   plugins/sim800.c | 424 +++   2 files changed, 428 insertions(+)   create mode 100644 plugins/sim800.c   + +static void sim800_post_sim(struct ofono_modem *modem) +{ + struct sim800_data *data = "" /> + struct ofono_gprs *gprs; + struct ofono_gprs_context *gc; + + DBG("%p", modem); + + /* Dirty Hack : give some time to sim800 for multiplexing + * to be effective and avoid VOICE_DLC to be + * flooded thus leading to a "famine" situation + */ + + sleep(2);No sleeps inside plugins. That blocks the entire daemon and we can'thave that. How does this help you anyway? GAtChat is a queue, so only1 command is outstanding at a time.  + ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem", + data->dlcs[SMS_DLC]); + + + gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]); + if (gprs == NULL) + return; + + gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM, + "atmodem", data->dlcs[GPRS_DLC]); + if (gc) + ofono_gprs_add_context(gprs, gc); +} +Regards,-Denis___ofono mailing listofono@ofono.orghttps://lists.ofono.org/mailman/listinfo/ofono___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Giacinto Cifelli]

On Wed, Sep 19, 2018 at 6:54 PM Slava Monich  wrote:

> On 19/09/18 19:32, Denis Kenzior wrote:
> > Hi Slava,
> >
> >> Anything in include is external API. We do maintain not just
> >> out-of-tree, but binary plugins. Backward (binary!) compatibility a
> >> must for us. Please do not break it.
> >
> > That is why we have 'OFONO_API_SUBJECT_TO_CHANGE' as a reminder. We
> > mean it.  D-Bus API is stable, we never made any guarantees about the
> > internal APIs.
> >
>
> I understand that! It makes things easier for you guys.
>
> But we had to avoid certain compile-time dependencies in ofono, and a
> straightforward (and perhaps the only) way to achieve that was to use
> binary plugins. For us plugin API is not subject to change (plugins
> don't necessarily get upgraded together with ofono), meaning more
> changes between our fork and upstream in case if upstream breaks it,
> more maintenance work and more room for errors. Obviously, I would like
> to keep differences to a minimum.
>
> I'm just humbly asking - if there's a way to keep plugin API backward
> compatible, please do it that way. There is at least one person in the
> world who would appreciate it.
>
>
Hi Slava,

this special case will not be backward compatible.
This enum for the authentication methods is handled through switch/cases in
the code, and some of them miss default. so an additional value will give
an unpredictable behavior (depending also on how the switch has been
optimized by the compiler).

Do you think it still makes sense to add an OFONO_GPRS_AUTH_METHOD_ANY ?
It looks like Denis will not take it anyway if it is not explained clearly
what it is for and how it is used. I have to say, I cannot see its benefit
either.


> Cheers,
> -Slava
> ___
> ofono mailing list
> ofono@ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono


BR,
Giacinto
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Slava Monich]

On 19/09/18 19:32, Denis Kenzior wrote:

Hi Slava,

Anything in include is external API. We do maintain not just 
out-of-tree, but binary plugins. Backward (binary!) compatibility a 
must for us. Please do not break it.


That is why we have 'OFONO_API_SUBJECT_TO_CHANGE' as a reminder. We 
mean it.  D-Bus API is stable, we never made any guarantees about the 
internal APIs.




I understand that! It makes things easier for you guys.

But we had to avoid certain compile-time dependencies in ofono, and a 
straightforward (and perhaps the only) way to achieve that was to use 
binary plugins. For us plugin API is not subject to change (plugins 
don't necessarily get upgraded together with ofono), meaning more 
changes between our fork and upstream in case if upstream breaks it, 
more maintenance work and more room for errors. Obviously, I would like 
to keep differences to a minimum.


I'm just humbly asking - if there's a way to keep plugin API backward 
compatible, please do it that way. There is at least one person in the 
world who would appreciate it.


Cheers,
-Slava
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 1/3] sim800: adding support for Simcom SIM800 modem

[Denis Kenzior]

Hi,

On 09/18/2018 03:36 PM, ClémentViel wrote:

From: clem 



You might want to describe how this is different from sim900 that it 
warrants a fully separate driver?


If there are only minor differences, then this can be handled via UDEV 
attributes or querying +CGMM, etc.



---
  Makefile.am  |   4 +
  plugins/sim800.c | 424 +++
  2 files changed, 428 insertions(+)
  create mode 100644 plugins/sim800.c






+
+static void sim800_post_sim(struct ofono_modem *modem)
+{
+   struct sim800_data *data = ofono_modem_get_data(modem);
+   struct ofono_gprs *gprs;
+   struct ofono_gprs_context *gc;
+
+   DBG("%p", modem);
+
+   /* Dirty Hack : give some time to sim800 for multiplexing
+*  to be effective and avoid VOICE_DLC to 
be
+*  flooded thus leading to a "famine" 
situation
+*/
+
+   sleep(2);


No sleeps inside plugins.  That blocks the entire daemon and we can't 
have that.  How does this help you anyway?  GAtChat is a queue, so only 
1 command is outstanding at a time.



+   ofono_sms_create(modem, OFONO_VENDOR_SIMCOM, "atmodem",
+   data->dlcs[SMS_DLC]);
+
+
+   gprs = ofono_gprs_create(modem, 0, "atmodem", data->dlcs[GPRS_DLC]);
+   if (gprs == NULL)
+   return;
+
+   gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
+   "atmodem", data->dlcs[GPRS_DLC]);
+   if (gc)
+   ofono_gprs_add_context(gprs, gc);
+}
+


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 2/3] sim800: add udev detection

[Denis Kenzior]

Hi,

On 09/18/2018 03:36 PM, ClémentViel wrote:

From: clem 

---
  plugins/udevng.c | 49 +
  1 file changed, 49 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 02d049e..9a1be6a 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -710,6 +710,53 @@ static gboolean setup_telitqmi(struct modem_info *modem)
return TRUE;
  }
  
+static gboolean setup_sim800(struct modem_info *modem)

+{
+   const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
+   GSList *list;
+
+   DBG("%s", modem->syspath);
+
+   for (list = modem->devices; list; list = list->next) {
+   struct device_info *info = list->data;
+
+   DBG("%s %s %s %s", info->devnode, info->interface,
+   info->number, info->label);
+
+   if (g_strcmp0(info->label, "aux") == 0) {
+   DBG("setting aux as info->devnode");
+   aux = info->devnode;
+   if (mdm != NULL)
+   break;
+   } else if (g_strcmp0(info->label, "modem") == 0) {
+   mdm = info->devnode;
+   if (aux != NULL)
+   break;
+   } else if (g_strcmp0(info->interface, "255/0/0") == 0) {
+   if (g_strcmp0(info->number, "00") == 0)
+   mdm = info->devnode;
+   else if (g_strcmp0(info->number, "01") == 0)
+   gps = info->devnode;
+   else if (g_strcmp0(info->number, "02") == 0)
+   aux = info->devnode;
+   else if (g_strcmp0(info->number, "03") == 0)
+   mdm = info->devnode;
+   }
+   }
+


So I'm confused now.  If sim800 is a serial device, why do you have all 
this here?  Shouldn't this be handled by setup_serial_modem ?



+   DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
+
+   if (mdm == NULL) {
+   DBG("modem did not set up");
+   return FALSE;
+   }
+
+   ofono_modem_set_string(modem->modem, "Device", mdm);
+   ofono_modem_set_string(modem->modem, "Modem", mdm);
+
+   return TRUE;
+}
+
  /* TODO: Not used as we have no simcom driver */
  static gboolean setup_simcom(struct modem_info *modem)
  {
@@ -1282,6 +1329,7 @@ static struct {
{ "telit",setup_telit,"device/interface"},
{ "telitqmi", setup_telitqmi  },
{ "simcom",   setup_simcom},
+   { "sim800", setup_sim800},
{ "sim7100",  setup_sim7100   },
{ "zte",  setup_zte   },
{ "icera",setup_icera },
@@ -1654,6 +1702,7 @@ static struct {
{ "alcatel",  "option", "1bbb", "0017"  },
{ "novatel",  "option", "1410"},
{ "zte",  "option", "19d2"},
+   { "sim800", "option",   "05c6", "9000"  },
{ "simcom",   "option", "05c6", "9000"  },
{ "sim7100",  "option", "1e0e", "9001"  },
{ "telit","usbserial",  "1bc7"},



Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

Re: [PATCH 3/3] sim800: adding documentation to inform about the udev rule the user must add

[Denis Kenzior]

Hi,

On 09/18/2018 03:36 PM, ClémentViel wrote:

From: clem 


Can you fix your Author information (see AUTHORS for an example). 
Otherwise I cannot take these...




---
  doc/sim800-modem.txt | 7 +++
  1 file changed, 7 insertions(+)
  create mode 100644 doc/sim800-modem.txt

diff --git a/doc/sim800-modem.txt b/doc/sim800-modem.txt
new file mode 100644
index 000..6731879
--- /dev/null
+++ b/doc/sim800-modem.txt
@@ -0,0 +1,7 @@
+SIM800 modem usage
+===
+
+To enable SIM900 module support you need to put the following
+udev rule into appropriate file in /{etc,lib}/udev/rules.d:
+
+KERNEL=="ttyUSB0", ENV{OFONO_DRIVER}="sim800"



So sim800 is a serial device right?  I take it you're running it from a 
serial converter here?


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

Hi Slava,

Anything in include is external API. We do maintain not just 
out-of-tree, but binary plugins. Backward (binary!) compatibility a must 
for us. Please do not break it.


That is why we have 'OFONO_API_SUBJECT_TO_CHANGE' as a reminder.  We 
mean it.  D-Bus API is stable, we never made any guarantees about the 
internal APIs.


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

Hi Giacinto,


so, first the documentation?


Correct.  Whenever you touch something that affects the D-Bus API, it is 
really preferable to have the D-Bus API changes in the preceding commit. 
 That way the reviewers can cross-reference what is being proposed to 
the actual implementation.  Anything that breaks the D-Bus API can also 
be spotted much earlier.


oFono's D-Bus API is stable.  That means we can add new things, but we 
cannot change existing ones as that will break existing clients.


We also cannot break existing settings, as that would break oFono 
installations on existing devices if they are upgraded.


This is a handicap that we have to live with right now.





So there can be no unification with the GPRS naming now that the D-Bus 
API is set?


I'm afraid not.  But I'm not sure why this is a problem?



 > +#define LTE_PROTO "Protocol"
 > +#define LTE_USERNAME "Username"
 > +#define LTE_PASSWORD "Password"
 > +#define LTE_AUTH_METHOD "AuthenticationMethod"
 >
 >   struct ofono_lte {
 >       const struct ofono_lte_driver *driver;
 > @@ -50,13 +55,82 @@ struct ofono_lte {
 >       DBusMessage *pending;
 >       struct ofono_lte_default_attach_info pending_info;
 >       struct ofono_lte_default_attach_info info;
 > +     const char *prop_changed;

??  What memory location is this const char pointing to?


it is initialized to null with the containing structure.


That is not what I'm asking.  This pointer points into data owned by 
DBusMessage and the semantics of whether it is valid or not are not 
enforced.  Avoid that at all cost...




What about reading also the previous key?


Ideally you shouldn't change the key in the first place.  But if you 
are, then yes you need to be able to read legacy settings versions in 
order to be backwards-compatible.




You do realize you're never actually calling into the driver method,
right?  So none of these changes actually go out to the modem.  Have
you
actually tested any of this?


Yes, it works. Actually the only call is when the APN is set, as 
mentioned in the lte-api.txt.


No, it really doesn't...


And at that point all parameters are also set in the module.
It is not possible to set separately protocol and apn, and auth_method, 
username, and password.

For ublox modules, the auth_method is also part of the APN name.

So we kept the call into the module when the APN is set, and previously 
to it all other parameters are set.


You simply cannot do that.  You cannot assume that the client knows how 
your API is implemented underneath.  So if they set the APN first and 
then change the username, that has to work.  This is why the 
default_attach_info contains everything.  If anything is changed the 
entire structure is sent to the modem and every parameter is updated.




You have also mentioned that somewhere we should also verify that with 
AUTH_NONE there are no user/pwd.

This also can only be verified at the end.

Any suggestions to improve this, given these limitations?



See above.  Any parameter change has to trigger 
set_default_attach_info() call.  If something is invalid, then you have 
to return a D-Bus error.


For example, if my method is set to 'none' and I try to do 
LongTermEvolution.SetProperty("DefaultUsername", "foo") that should 
return an error.




 > +     return dbus_message_ref(msg);;
 >   }
 >
 >   static DBusMessage *lte_set_property(DBusConnection *conn,
 > -                                     DBusMessage *msg, void *data)
 > +                                             DBusMessage *msg,
void *data)
 >   {
 >       struct ofono_lte *lte = data;
 >       DBusMessageIter iter;
 >       DBusMessageIter var;
 >       const char *property;
 >       const char *str;
 > +     enum ofono_gprs_auth_method auth_method;
 > +     enum ofono_gprs_proto proto;
 > +
 > +     if (lte->driver->set_default_attach_info == NULL)
 > +             return __ofono_error_not_implemented(msg);
 > +
 > +     if (lte->pending)
 > +             return __ofono_error_busy(msg);
 >
 >       if (!dbus_message_iter_init(msg, ))
 >               return __ofono_error_invalid_args(msg);
 > @@ -192,13 +428,58 @@ static DBusMessage
*lte_set_property(DBusConnection *conn,
 >
 >       dbus_message_iter_recurse(, );
 >
 > -     if (!strcmp(property, DEFAULT_APN_KEY)) {
 > +     lte->prop_changed=property;
 > +
 > +     if (!strcmp(property, LTE_PROTO)) {
 > +
 > +             if (dbus_message_iter_get_arg_type() !=
DBUS_TYPE_STRING)
 > +                     return __ofono_error_invalid_args(msg);
 > +
 > +             dbus_message_iter_get_basic(, );
 > +
 > +             if (gprs_proto_from_string(str, ))
 > +                     return lte_set_proto(lte, conn, msg, proto);

The return from this callback is 

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

[Slava Monich]

On 19/09/18 18:21, Denis Kenzior wrote:


Hi Giacinto,

I would favour also renumbering with NONE on top, but I am not sure 
of the side effects everywhere, in case the values are used directly 
in commands.


Actually there is no problem doing that.  The internal API is not 
stable.  Besides, it will give all the guys who insist on maintaining 
out-of-tree plugins some extra work ;)




Anything in include is external API. We do maintain not just 
out-of-tree, but binary plugins. Backward (binary!) compatibility a must 
for us. Please do not break it.


-Slava
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Giacinto Cifelli]

Hi Denis,

On Wed, Sep 19, 2018 at 5:04 PM Denis Kenzior  wrote:

> Hi Giacinto,
>
> On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:
> > ---
> >   src/gprs.c |  13 ++-
> >   src/lte.c  | 372
> ++---
> >   src/main.c |   5 -
> >   3 files changed, 341 insertions(+), 49 deletions(-)
>
> So you seem to have 3 completely unrelated things going on here...  At
> the very minimum this should be 3 commits.
>
> Also, you're adding LTE D-Bus implementation without updating or
> proposing changes to doc/lte-api.txt.
>

so, first the documentation?


>
> > diff --git a/src/gprs.c b/src/gprs.c
> > index 377eced..40f43e3 100644
> > --- a/src/gprs.c
> > +++ b/src/gprs.c
> > @@ -261,6 +261,10 @@ static const char *gprs_auth_method_to_string(enum
> ofono_gprs_auth_method auth)
> >   return "chap";
> >   case OFONO_GPRS_AUTH_METHOD_PAP:
> >   return "pap";
> > + case OFONO_GPRS_AUTH_METHOD_NONE:
> > + return "none";
> > + default:
> > + return NULL;
> >   };
>
> Okay, but this patch likely needs to also ensure that username /
> password are not settable if method is NONE.  And follow up with an
> update of all things that depend on OFONO_GPRS_AUTH_METHOD usage.  E.g.
> drivers, provisioning plugins, etc.
>

Ok, I will take care of this as well.

>
> >   return NULL;
> > @@ -275,6 +279,9 @@ static gboolean gprs_auth_method_from_string(const
> char *str,
> >   } else if (g_str_equal(str, "pap")) {
> >   *auth = OFONO_GPRS_AUTH_METHOD_PAP;
> >   return TRUE;
> > + } else if (g_str_equal(str, "none")) {
> > + *auth = OFONO_GPRS_AUTH_METHOD_NONE;
> > + return TRUE;
> >   }
> >
> >   return FALSE;
> > @@ -1008,7 +1015,7 @@ static void pri_read_settings_callback(const
> struct ofono_error *error,
> >
> >   value = pri_ctx->active;
> >
> > - gprs->flags &= !GPRS_FLAG_ATTACHING;
> > + gprs->flags &= ~GPRS_FLAG_ATTACHING;
>
> Okay, but this is a separate fix and should be documented properly.
>

Okay


>
> >
> >   gprs->driver_attached = TRUE;
> >   gprs_set_attached_property(gprs, TRUE);
> > @@ -1635,6 +1642,9 @@ static void release_active_contexts(struct
> ofono_gprs *gprs)
> >
> >   if (gc->driver->detach_shutdown != NULL)
> >   gc->driver->detach_shutdown(gc, ctx->context.cid);
> > +
> > + /* Make sure the context is properly cleared */
> > + release_context(ctx);
>
> As above, seems to be an unrelated fix.
>

It is. I will submit another patch. the same below.


> >   }
> >   }
> >
> > @@ -2234,6 +2244,7 @@ static DBusMessage
> *gprs_remove_context(DBusConnection *conn,
> >   }
> >
> >   DBG("Unregistering context: %s", ctx->path);
> > + release_context(ctx);
>
> As above.  You can't just lump these changes into something unrelated.
> You need to submit these fixes separately and describe what each one is
> fixing and why.
>
> >   context_dbus_unregister(ctx);
> >   gprs->contexts = g_slist_remove(gprs->contexts, ctx);
> >
> > diff --git a/src/lte.c b/src/lte.c
> > index a6d26b3..21b6a19 100644
> > --- a/src/lte.c
> > +++ b/src/lte.c
> > @@ -3,6 +3,7 @@
> >*  oFono - Open Source Telephony
> >*
> >*  Copyright (C) 2016  Endocode AG. All rights reserved.
> > + *  Copyright (C) 2018 Gemalto M2M
> >*
> >*  This program is free software; you can redistribute it and/or
> modify
> >*  it under the terms of the GNU General Public License version 2 as
> > @@ -39,7 +40,11 @@
> >
> >   #define SETTINGS_STORE "lte"
> >   #define SETTINGS_GROUP "Settings"
> > -#define DEFAULT_APN_KEY "DefaultAccessPointName"
> > +#define LTE_APN "AccessPointName"
>
> No.  You can't do that.  The D-Bus API is stable and cannot be changed.
> This is why you propose D-Bus API changes first, so that they can be
> reviewed separately for any impacts.
>

So there can be no unification with the GPRS naming now that the D-Bus API
is set?



> > +#define LTE_PROTO "Protocol"
> > +#define LTE_USERNAME "Username"
> > +#define LTE_PASSWORD "Password"
> > +#define LTE_AUTH_METHOD "AuthenticationMethod"
> >
> >   struct ofono_lte {
> >   const struct ofono_lte_driver *driver;
> > @@ -50,13 +55,82 @@ struct ofono_lte {
> >   DBusMessage *pending;
> >   struct ofono_lte_default_attach_info pending_info;
> >   struct ofono_lte_default_attach_info info;
> > + const char *prop_changed;
>
> ??  What memory location is this const char pointing to?
>

it is initialized to null with the containing structure.


> Why don't you just use an enum.  Or even better, don't do this at all
> and simply compare pending_info & info to generate the right signal.
>

I will try to change it this way.


>
> >   };
> >
> >   static GSList *g_drivers = NULL;
> >
> > +static const char *gprs_proto_to_string(enum ofono_gprs_proto proto)
> > +{
> > + switch (proto) {
> 

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

[Denis Kenzior]

Hi Giacinto,


From the ./HACKING file I understood that the requirement is to
split according to the top-level directories only, and to make sure not 
to break compilation.


The rule provides a baseline minimum.  It also implies that sometimes 
breaking compilation is inevitable.  So long as the entire series is 
results in a clean build, that is fine.  Other projects are stricter 
here in order to not break 'git bisect' functionality.  But we are not.



Definitely I will add more comments to the commits, too.


Don't be afraid to do that.  The more commits and the smaller the 
patches, the easier it is to review things.  In fact, as a rule of 
thumb, it is much faster to get 10 small patches upstream than 1 big patch.



well, this is the interesting part of this series of patches.
What the lte atom does is really parallel to the gprs-context, at the 
end is a big code duplication.


I see them as related but the gprs change itself can stand on its own. 
Also much of the duplicated code can be factored out into common.c


A good rule of thumb is to ask yourself this question: "Are these 
changes useful just by themselves?"  If the answer is yes, then you 
should separate them into a distinct commit.  And this is part of the 
reason why it is easier to get upstream when it is broken down into 
smaller chunks.  Even if some part of the series is controversial, the 
other parts might not be and could easily be applied right away.



Isn't there a smart way to reduce this?


Are you referring to the number of commits? Why would you want to?

... or code duplication?  In which case yes, you need to factor things 
out into common.c.




already fixed in the rest of this patch series. But this implies that I 
will have to submit again a multi-part patch for this change.


Which is why a few days ago I suggested you start with a small subset to 
learn the process.  It is far easier to re-factor smaller chunks than 
3-4k of code.




this means submitting a patch for the D-Bus API before the others ?


Yes, preferably.


In any case it has to go with the rest if it has to compile.
I still haven't figured out completely how to split properly to ensure 
compilation.




Browse git history and see how this is done if you're not sure.  Our git 
history tends to be quite clean.  But in general, this is a practical 
skill and like anything will take some time to get comfortable with.


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Giacinto Cifelli]

On Wed, Sep 19, 2018 at 4:09 PM Denis Kenzior  wrote:

> Hi Giacinto,
>
> Your patch series has 7 commits with the same header.  That is nonsense.
> Each commit header should be specific to what is contained inside and
> preferably followed by a commit description.
>
> Read any one of the many 'How to submit a kernel patch' howtos in order
> to understand the best practices here.  oFono does not use Signed-off-by
> lines and a few things are different, but the commit header/description
> requirements and the overall patch submission process are the same.
>

Hi Denis,

I will break down things further. This series of patches is only the first
one to support our modules.
I have submitted this first part also to gather comments.
>From the ./HACKING file I understood that the requirement is to
split according to the top-level directories only, and to make sure not to
break compilation.
Definitely I will add more comments to the commits, too.


>
> On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:
> > ---
> >   include/gprs-context.h |  1 +
> >   include/lte.h  | 11 +--
> >   2 files changed, 10 insertions(+), 2 deletions(-)
>
> This really should be two patches because you're changing unrelated things.
>

well, this is the interesting part of this series of patches.
What the lte atom does is really parallel to the gprs-context, at the end
is a big code duplication.
Isn't there a smart way to reduce this?


> >
> > diff --git a/include/gprs-context.h b/include/gprs-context.h
> > index 20ca9ef..8869c12 100644
> > --- a/include/gprs-context.h
> > +++ b/include/gprs-context.h
> > @@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
> >   enum ofono_gprs_auth_method {
> >   OFONO_GPRS_AUTH_METHOD_CHAP = 0,
> >   OFONO_GPRS_AUTH_METHOD_PAP,
> > + OFONO_GPRS_AUTH_METHOD_NONE,
>
> So strictly speaking we already support NONE as a method if username is
> empty. I don't have a problem with this change, but it does imply that
> you need to fix up the existing code depending on this enumeration to
> behave properly.
>

already fixed in the rest of this patch series. But this implies that I
will have to submit again a multi-part patch for this change.


> >   };
> >
> >   struct ofono_gprs_primary_context {
> > diff --git a/include/lte.h b/include/lte.h
> > index ef84ab9..38587c3 100644
> > --- a/include/lte.h
> > +++ b/include/lte.h
> > @@ -3,6 +3,7 @@
> >*  oFono - Open Source Telephony
> >*
> >*  Copyright (C) 2016  Endocode AG. All rights reserved.
> > + *  Copyright (C) 2018 Gemalto M2M
> >*
> >*  This program is free software; you can redistribute it and/or
> modify
> >*  it under the terms of the GNU General Public License version 2 as
> > @@ -28,14 +29,18 @@ extern "C" {
> >
> >   #include 
> >
> > -struct ofono_lte;
> > -
>
> So why are you changing the order seemingly randomly?  And anyway, this
> is wrong.  See doc/coding-style.txt item M9.
>
> >   struct ofono_lte_default_attach_info {
> >   char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
> > + enum ofono_gprs_proto proto;
> > + enum ofono_gprs_auth_method auth_method;
> > + char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
> > + char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
>
> Okay, but you might want to start at the D-Bus API level first...
>

this means submitting a patch for the D-Bus API before the others ?
In any case it has to go with the rest if it has to compile.
I still haven't figured out completely how to split properly to ensure
compilation.


> >   };
> >
> >   typedef void (*ofono_lte_cb_t)(const struct ofono_error *error, void
> *data);
> >
> > +struct ofono_lte;
> > +
> >   struct ofono_lte_driver {
> >   const char *name;
> >   int (*probe)(struct ofono_lte *lte, unsigned int vendor, void
> *data);
> > @@ -61,6 +66,8 @@ void ofono_lte_set_data(struct ofono_lte *lte, void
> *data);
> >
> >   void *ofono_lte_get_data(const struct ofono_lte *lte);
> >
> > +struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte);
> > + >   #ifdef __cplusplus
> >   }
> >   #endif
> >
>
> Regards,
> -Denis
>

Regards,
Giacinto
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

Hi Giacinto,

I would favour also renumbering with NONE on top, but I am not sure of 
the side effects everywhere, in case the values are used directly in 
commands.


Actually there is no problem doing that.  The internal API is not 
stable.  Besides, it will give all the guys who insist on maintaining 
out-of-tree plugins some extra work ;)


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

Hi Slava,

On 09/19/2018 03:35 AM, Slava Monich wrote:

On 19/09/18 08:37, Giacinto Cifelli wrote:

---
  include/gprs-context.h |  1 +
  include/lte.h  | 11 +--
  2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/gprs-context.h b/include/gprs-context.h
index 20ca9ef..8869c12 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
  enum ofono_gprs_auth_method {
  OFONO_GPRS_AUTH_METHOD_CHAP = 0,
  OFONO_GPRS_AUTH_METHOD_PAP,
+    OFONO_GPRS_AUTH_METHOD_NONE,


I think there should be OFONO_GPRS_AUTH_METHOD_ANY (or 
OFONO_GPRS_AUTH_METHOD_PAP_CHAP) here as well, for completeness. Many 
modems support that too (and we had to add it in our fork).


There is no such thing in 3GPP, so how does that work?

And by the way,  none of the provisioning databases I'm aware of have 
this possibility and CHAP is actually a sane default.  While there are 
probably 2 providers left on this planet that might still insist on PAP, 
the 3GPP specs actually mandate CHAP anyway.






  };
  struct ofono_gprs_primary_context {
diff --git a/include/lte.h b/include/lte.h
index ef84ab9..38587c3 100644
--- a/include/lte.h
+++ b/include/lte.h
@@ -3,6 +3,7 @@
   *  oFono - Open Source Telephony
   *
   *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
   *
   *  This program is free software; you can redistribute it and/or 
modify

   *  it under the terms of the GNU General Public License version 2 as
@@ -28,14 +29,18 @@ extern "C" {
  #include 
-struct ofono_lte;
-
  struct ofono_lte_default_attach_info {
  char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
+    enum ofono_gprs_proto proto;
+    enum ofono_gprs_auth_method auth_method;
+    char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
+    char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
  };


This is starting to look suspiciously similar to struct 
ofono_gprs_primary_context (the only thing left is cid). Is it really 
necessary to maintain two copies of essentially the same structure or is 
there some room for unification here?




I don't really see a problem here.  One is active context details, the 
other is default attach details.  But if OFONO_GPRS_* defines are going 
to be used in multiple include/ofono files, then these defines should be 
moved to types.h.


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

Hi Giacinto,

On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:

---
  drivers/atmodem/atutil.h |  14 +
  drivers/atmodem/cbs.c|   1 +
  drivers/atmodem/gprs-context.c   |   2 +
  drivers/atmodem/gprs.c   | 554 --
  drivers/atmodem/lte.c| 283 -
  drivers/atmodem/network-registration.c   | 134 +++--
  drivers/atmodem/sim.c|  10 +-
  drivers/atmodem/sms.c|  21 +-
  drivers/atmodem/vendor.h |   1 +
  drivers/gemaltomodem/gemaltomodem.c  |   5 +-
  drivers/gemaltomodem/gemaltomodem.h  |  19 +-
  drivers/gemaltomodem/gprs-context-wwan.c | 445 ++
  drivers/gemaltomodem/voicecall.c | 965 +++
  drivers/mbimmodem/gprs-context.c |   2 +
  drivers/mbimmodem/mbim.c |   3 +
  drivers/mbimmodem/mbimmodem.c|   2 +-
  drivers/rilmodem/call-forwarding.c   |   2 +-
  drivers/rilmodem/network-registration.c  |   2 +-
  18 files changed, 2343 insertions(+), 122 deletions(-)
  create mode 100644 drivers/gemaltomodem/gprs-context-wwan.c
  create mode 100644 drivers/gemaltomodem/voicecall.c



I'm not going to even bother looking at this until it is broken up 
properly and actually has a commit description that is longer than an 
empty string.


Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Denis Kenzior]

Hi Giacinto,

Your patch series has 7 commits with the same header.  That is nonsense.
Each commit header should be specific to what is contained inside and 
preferably followed by a commit description.


Read any one of the many 'How to submit a kernel patch' howtos in order 
to understand the best practices here.  oFono does not use Signed-off-by 
lines and a few things are different, but the commit header/description 
requirements and the overall patch submission process are the same.


On 09/19/2018 12:37 AM, Giacinto Cifelli wrote:

---
  include/gprs-context.h |  1 +
  include/lte.h  | 11 +--
  2 files changed, 10 insertions(+), 2 deletions(-)


This really should be two patches because you're changing unrelated things.



diff --git a/include/gprs-context.h b/include/gprs-context.h
index 20ca9ef..8869c12 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
  enum ofono_gprs_auth_method {
OFONO_GPRS_AUTH_METHOD_CHAP = 0,
OFONO_GPRS_AUTH_METHOD_PAP,
+   OFONO_GPRS_AUTH_METHOD_NONE,


So strictly speaking we already support NONE as a method if username is 
empty. I don't have a problem with this change, but it does imply that 
you need to fix up the existing code depending on this enumeration to 
behave properly.



  };
  
  struct ofono_gprs_primary_context {

diff --git a/include/lte.h b/include/lte.h
index ef84ab9..38587c3 100644
--- a/include/lte.h
+++ b/include/lte.h
@@ -3,6 +3,7 @@
   *  oFono - Open Source Telephony
   *
   *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
   *
   *  This program is free software; you can redistribute it and/or modify
   *  it under the terms of the GNU General Public License version 2 as
@@ -28,14 +29,18 @@ extern "C" {
  
  #include 
  
-struct ofono_lte;

-


So why are you changing the order seemingly randomly?  And anyway, this 
is wrong.  See doc/coding-style.txt item M9.



  struct ofono_lte_default_attach_info {
char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
+   enum ofono_gprs_proto proto;
+   enum ofono_gprs_auth_method auth_method;
+   char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
+   char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];


Okay, but you might want to start at the D-Bus API level first...


  };
  
  typedef void (*ofono_lte_cb_t)(const struct ofono_error *error, void *data);
  
+struct ofono_lte;

+
  struct ofono_lte_driver {
const char *name;
int (*probe)(struct ofono_lte *lte, unsigned int vendor, void *data);
@@ -61,6 +66,8 @@ void ofono_lte_set_data(struct ofono_lte *lte, void *data);
  
  void *ofono_lte_get_data(const struct ofono_lte *lte);
  
+struct ofono_modem *ofono_lte_get_modem(const struct ofono_lte *lte);

+ >   #ifdef __cplusplus
  }
  #endif



Regards,
-Denis
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Giacinto Cifelli]

On Wed, Sep 19, 2018 at 10:35 AM Slava Monich 
wrote:

> On 19/09/18 08:37, Giacinto Cifelli wrote:
> > ---
> >   include/gprs-context.h |  1 +
> >   include/lte.h  | 11 +--
> >   2 files changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/gprs-context.h b/include/gprs-context.h
> > index 20ca9ef..8869c12 100644
> > --- a/include/gprs-context.h
> > +++ b/include/gprs-context.h
> > @@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
> >   enum ofono_gprs_auth_method {
> >   OFONO_GPRS_AUTH_METHOD_CHAP = 0,
> >   OFONO_GPRS_AUTH_METHOD_PAP,
> > + OFONO_GPRS_AUTH_METHOD_NONE,
>
> I think there should be OFONO_GPRS_AUTH_METHOD_ANY (or
> OFONO_GPRS_AUTH_METHOD_PAP_CHAP) here as well, for completeness. Many
> modems support that too (and we had to add it in our fork).
>
>
I agree. Let me collect all comments, then I will also add it.
I would favour also renumbering with NONE on top, but I am not sure of the
side effects everywhere, in case the values are used directly in commands.


>
> >   };
> >
> >   struct ofono_gprs_primary_context {
> > diff --git a/include/lte.h b/include/lte.h
> > index ef84ab9..38587c3 100644
> > --- a/include/lte.h
> > +++ b/include/lte.h
> > @@ -3,6 +3,7 @@
> >*  oFono - Open Source Telephony
> >*
> >*  Copyright (C) 2016  Endocode AG. All rights reserved.
> > + *  Copyright (C) 2018 Gemalto M2M
> >*
> >*  This program is free software; you can redistribute it and/or
> modify
> >*  it under the terms of the GNU General Public License version 2 as
> > @@ -28,14 +29,18 @@ extern "C" {
> >
> >   #include 
> >
> > -struct ofono_lte;
> > -
> >   struct ofono_lte_default_attach_info {
> >   char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
> > + enum ofono_gprs_proto proto;
> > + enum ofono_gprs_auth_method auth_method;
> > + char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
> > + char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
> >   };
>
> This is starting to look suspiciously similar to struct
> ofono_gprs_primary_context (the only thing left is cid). Is it really
> necessary to maintain two copies of essentially the same structure or is
> there some room for unification here?
>
>
There is a lot of duplication between gprs and lte, yes, if you look in the
rest of the patches there is a lot in common.
I havent found a way to do it smoothly. I even had to copy the
gprs_proto_to/from_string, gprs_auth_method_to/from_string in src/gprs.c in
src/lte.c, because I could not export them in common.c (without removing
the enums).
If there is a smart way to do it, I am more than willing to recraft the
code. Like this every change is to be duplicated.


> Cheers,
> -Slava
>

Regards,
Giacinto

___
> ofono mailing list
> ofono@ofono.org
> https://lists.ofono.org/mailman/listinfo/ofono
>
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono

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

[Slava Monich]

On 19/09/18 08:37, Giacinto Cifelli wrote:

---
  include/gprs-context.h |  1 +
  include/lte.h  | 11 +--
  2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/gprs-context.h b/include/gprs-context.h
index 20ca9ef..8869c12 100644
--- a/include/gprs-context.h
+++ b/include/gprs-context.h
@@ -57,6 +57,7 @@ enum ofono_gprs_context_type {
  enum ofono_gprs_auth_method {
OFONO_GPRS_AUTH_METHOD_CHAP = 0,
OFONO_GPRS_AUTH_METHOD_PAP,
+   OFONO_GPRS_AUTH_METHOD_NONE,


I think there should be OFONO_GPRS_AUTH_METHOD_ANY (or 
OFONO_GPRS_AUTH_METHOD_PAP_CHAP) here as well, for completeness. Many 
modems support that too (and we had to add it in our fork).




  };
  
  struct ofono_gprs_primary_context {

diff --git a/include/lte.h b/include/lte.h
index ef84ab9..38587c3 100644
--- a/include/lte.h
+++ b/include/lte.h
@@ -3,6 +3,7 @@
   *  oFono - Open Source Telephony
   *
   *  Copyright (C) 2016  Endocode AG. All rights reserved.
+ *  Copyright (C) 2018 Gemalto M2M
   *
   *  This program is free software; you can redistribute it and/or modify
   *  it under the terms of the GNU General Public License version 2 as
@@ -28,14 +29,18 @@ extern "C" {
  
  #include 
  
-struct ofono_lte;

-
  struct ofono_lte_default_attach_info {
char apn[OFONO_GPRS_MAX_APN_LENGTH + 1];
+   enum ofono_gprs_proto proto;
+   enum ofono_gprs_auth_method auth_method;
+   char username[OFONO_GPRS_MAX_USERNAME_LENGTH + 1];
+   char password[OFONO_GPRS_MAX_PASSWORD_LENGTH + 1];
  };


This is starting to look suspiciously similar to struct 
ofono_gprs_primary_context (the only thing left is cid). Is it really 
necessary to maintain two copies of essentially the same structure or is 
there some room for unification here?


Cheers,
-Slava
___
ofono mailing list
ofono@ofono.org
https://lists.ofono.org/mailman/listinfo/ofono