[PATCH 2/2] plugin for the Thuraya XT satellite phone/modem
Aleksander Morgado
aleksander at aleksander.es
Thu Feb 11 08:57:09 UTC 2016
Hey Thomas,
I see no email reply to the original review I did to the plugin code,
and this patch doesn't explain what was changed w.r.t. the original
submission, which makes it a bit difficult to re-review :/ Ideally,
when you get a patch review you would reply to each comment in the
review and say "ok", or "agreed" or "no this doesn't make any sense"
or whatever. Otherwise it gets a bit difficult to do the re-reviews
and forces me to recheck everything from scratch.
So among other things, you simplified the patch to avoid needing a
custom bearer, custom unsolicited messaging setup, custom flow control
and custom init sequence, at least. The plugin looks much better and
cleaner now indeed :)
But, removing the custom bearer completely means that you no longer
specify a custom "ip-timeout" property in the bearer. This property
tells the upper layers (e.g. NetworkManager) how much time they need
to wait maximum to get the IP settings. By default it's 20s if I'm not
mistaken, which is ok for mobile networks, but my tests with the
Iridium plugin told me that it wasn't enough for satellite modems, so
Iridium uses 200s there. Question being, maybe we should keep 200s
also for the Thuraya plugin? If you can use the default bearer
implementation, then you could just subclass the create_bearer()
method, create a normal MMBroadbandBearer, and then g_object_set
(bearer, "ip-timeout", 200, NULL);. What do you think?
See more comments below.
On Tue, Feb 2, 2016 at 6:13 PM, Thomas Sailer
<sailer at sailer.dynip.lugs.ch> wrote:
> From: Thomas Sailer <t.sailer at alumni.ethz.ch>
>
> Signed-off-by: Thomas Sailer <t.sailer at alumni.ethz.ch>
> ---
> plugins/Makefile.am | 10 ++
> plugins/thuraya/mm-broadband-modem-thuraya.c | 184 +++++++++++++++++++++++++++
> plugins/thuraya/mm-broadband-modem-thuraya.h | 50 ++++++++
> plugins/thuraya/mm-plugin-thuraya.c | 93 ++++++++++++++
> plugins/thuraya/mm-plugin-thuraya.h | 49 +++++++
> 5 files changed, 386 insertions(+)
>
> diff --git a/plugins/Makefile.am b/plugins/Makefile.am
> index 3cc5e2f..53b0b9b 100644
> --- a/plugins/Makefile.am
> +++ b/plugins/Makefile.am
> @@ -125,6 +125,7 @@ pkglib_LTLIBRARIES = \
> libmm-plugin-nokia-icera.la \
> libmm-plugin-cinterion.la \
> libmm-plugin-iridium.la \
> + libmm-plugin-thuraya.la \
> libmm-plugin-motorola.la \
> libmm-plugin-novatel.la \
> libmm-plugin-novatel-lte.la \
> @@ -461,6 +462,15 @@ libmm_plugin_iridium_la_SOURCES = \
> libmm_plugin_iridium_la_CPPFLAGS = $(PLUGIN_COMMON_COMPILER_FLAGS)
> libmm_plugin_iridium_la_LDFLAGS = $(PLUGIN_COMMON_LINKER_FLAGS)
>
> +# Thuraya XT
> +libmm_plugin_thuraya_la_SOURCES = \
> + thuraya/mm-plugin-thuraya.c \
> + thuraya/mm-plugin-thuraya.h \
> + thuraya/mm-broadband-modem-thuraya.c \
> + thuraya/mm-broadband-modem-thuraya.h
> +libmm_plugin_thuraya_la_CPPFLAGS = $(PLUGIN_COMMON_COMPILER_FLAGS)
> +libmm_plugin_thuraya_la_LDFLAGS = $(PLUGIN_COMMON_LINKER_FLAGS)
> +
> # Common Novatel modem support library
> noinst_LTLIBRARIES += libmm-utils-novatel.la
> libmm_utils_novatel_la_SOURCES = \
> diff --git a/plugins/thuraya/mm-broadband-modem-thuraya.c b/plugins/thuraya/mm-broadband-modem-thuraya.c
> new file mode 100644
> index 0000000..acc1c9c
> --- /dev/null
> +++ b/plugins/thuraya/mm-broadband-modem-thuraya.c
> @@ -0,0 +1,184 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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:
> + *
> + * Copyright (C) 2011 - 2012 Ammonit Measurement GmbH
> + * Author: Aleksander Morgado <aleksander at lanedo.com>
> + * Copyright (C) 2016
> + * Author: Thomas Sailer <t.sailer at alumni.ethz.ch>
I would just add one line:
Copyright (C) 2016 Thomas Sailer <t.sailer at alumni.ethz.ch>
> + */
> +
> +#include <config.h>
> +
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include <ctype.h>
> +
> +#include "ModemManager.h"
> +#include "mm-log.h"
> +#include "mm-errors-types.h"
> +#include "mm-base-modem-at.h"
> +#include "mm-iface-modem.h"
> +#include "mm-iface-modem-3gpp.h"
> +#include "mm-broadband-modem-thuraya.h"
> +#include "mm-broadband-bearer.h"
> +#include "mm-modem-helpers.h"
> +
> +static void iface_modem_init (MMIfaceModem *iface);
> +static void iface_modem_3gpp_init (MMIfaceModem3gpp *iface);
> +
> +G_DEFINE_TYPE_EXTENDED (MMBroadbandModemThuraya, mm_broadband_modem_thuraya, MM_TYPE_BROADBAND_MODEM, 0,
> + G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM, iface_modem_init)
> + G_IMPLEMENT_INTERFACE (MM_TYPE_IFACE_MODEM_3GPP, iface_modem_3gpp_init) );
> +
> +/*****************************************************************************/
> +/* Operator Code and Name loading (3GPP interface) */
> +
> +static gchar *
> +load_operator_code_finish (MMIfaceModem3gpp *self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + /* Only "90103" operator code is assumed */
> + return g_strdup ("90106");
> +}
> +
> +static gchar *
> +load_operator_name_finish (MMIfaceModem3gpp *self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + /* Only "THURAYA" operator name is assumed */
> + return g_strdup ("THURAYA");
> +}
> +
> +static void
> +load_operator_name_or_code (MMIfaceModem3gpp *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + GSimpleAsyncResult *result;
> +
> + result = g_simple_async_result_new (G_OBJECT (self),
> + callback,
> + user_data,
> + load_operator_name_or_code);
> + g_simple_async_result_set_op_res_gboolean (result, TRUE);
> + g_simple_async_result_complete_in_idle (result);
> + g_object_unref (result);
> +}
> +
> +/*****************************************************************************/
> +/* Load supported modes (Modem inteface) */
> +
> +static GArray *
> +load_supported_modes_finish (MMIfaceModem *self,
> + GAsyncResult *res,
> + GError **error)
> +{
> + return g_array_ref (g_simple_async_result_get_op_res_gpointer (G_SIMPLE_ASYNC_RESULT (res)));
> +}
> +
> +static void
> +load_supported_modes (MMIfaceModem *self,
> + GAsyncReadyCallback callback,
> + gpointer user_data)
> +{
> + GSimpleAsyncResult *result;
> + GArray *combinations;
> + MMModemModeCombination mode;
> +
> + result = g_simple_async_result_new (G_OBJECT (self),
> + callback,
> + user_data,
> + load_supported_modes);
> +
> + /* Build list of combinations */
> + combinations = g_array_sized_new (FALSE, FALSE, sizeof (MMModemModeCombination), 1);
> +
> + /* Report any, Thuraya connections are packet-switched */
> + mode.allowed = MM_MODEM_MODE_ANY;
> + mode.preferred = MM_MODEM_MODE_NONE;
> + g_array_append_val (combinations, mode);
> +
> + g_simple_async_result_set_op_res_gpointer (result, combinations, (GDestroyNotify) g_array_unref);
> + g_simple_async_result_complete_in_idle (result);
> + g_object_unref (result);
> +}
> +
> +/*****************************************************************************/
> +
> +MMBroadbandModemThuraya *
> +mm_broadband_modem_thuraya_new (const gchar *device,
> + const gchar **drivers,
> + const gchar *plugin,
> + guint16 vendor_id,
> + guint16 product_id)
> +{
> + return g_object_new (MM_TYPE_BROADBAND_MODEM_THURAYA,
> + MM_BASE_MODEM_DEVICE, device,
> + MM_BASE_MODEM_DRIVERS, drivers,
> + MM_BASE_MODEM_PLUGIN, plugin,
> + MM_BASE_MODEM_VENDOR_ID, vendor_id,
> + MM_BASE_MODEM_PRODUCT_ID, product_id,
> + /* Allow only up to 3 consecutive timeouts in the serial port */
> + MM_BASE_MODEM_MAX_TIMEOUTS, 3,
The previous setting was needed in the Iridium plugin because it was
hanging off a RS232 port, and we needed a way to detect that the modem
was gone. I don't think it's needed in this USB modem.
> + /* Only PS network is supported by the Thuraya modem */
> + //MM_IFACE_MODEM_3GPP_CS_NETWORK_SUPPORTED, FALSE,
Ok, now it says CS only, but the comment says "PS" :)
> + NULL);
> +}
> +
> +static void
> +mm_broadband_modem_thuraya_init (MMBroadbandModemThuraya *self)
> +{
> +}
> +
> +static void
> +iface_modem_init (MMIfaceModem *iface)
> +{
> + /* No need to power-up/power-down the modem */
> + iface->load_power_state = NULL;
> + iface->load_power_state_finish = NULL;
> + iface->modem_power_up = NULL;
> + iface->modem_power_up_finish = NULL;
> + iface->modem_power_down = NULL;
> + iface->modem_power_down_finish = NULL;
> +
> + /* Supported modes cannot be queried */
> + iface->load_supported_modes = load_supported_modes;
> + iface->load_supported_modes_finish = load_supported_modes_finish;
> +}
> +
> +static void
> +iface_modem_3gpp_init (MMIfaceModem3gpp *iface)
> +{
> + /* Fixed operator code and name to be reported */
> + iface->load_operator_name = load_operator_name_or_code;
> + iface->load_operator_name_finish = load_operator_name_finish;
> + iface->load_operator_code = load_operator_name_or_code;
> + iface->load_operator_code_finish = load_operator_code_finish;
> +
> + /* Don't try to scan networks with AT+COPS=?.
> + * The Thuraya XT does not seem to properly support AT+COPS=?.
> + * When issuing this command, it seems to get sufficiently confused
> + * to drop the signal. Furthermore, it is useless anyway as there is only
> + * one network supported, Thuraya.
> + */
> + iface->scan_networks = NULL;
> + iface->scan_networks_finish = NULL;
> +}
> +
> +static void
> +mm_broadband_modem_thuraya_class_init (MMBroadbandModemThurayaClass *klass)
> +{
> +}
> diff --git a/plugins/thuraya/mm-broadband-modem-thuraya.h b/plugins/thuraya/mm-broadband-modem-thuraya.h
> new file mode 100644
> index 0000000..42df9b9
> --- /dev/null
> +++ b/plugins/thuraya/mm-broadband-modem-thuraya.h
> @@ -0,0 +1,50 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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:
> + *
> + * Copyright (C) 2008 - 2009 Novell, Inc.
> + * Copyright (C) 2009 - 2011 Red Hat, Inc.
> + * Copyright (C) 2011 Google Inc.
> + * Copyright (C) 2016 Thomas Sailer <t.sailer at alumni.ethz.ch>
> + */
> +
> +#ifndef MM_BROADBAND_MODEM_THURAYA_H
> +#define MM_BROADBAND_MODEM_THURAYA_H
> +
> +#include "mm-broadband-modem.h"
> +
> +#define MM_TYPE_BROADBAND_MODEM_THURAYA (mm_broadband_modem_thuraya_get_type ())
> +#define MM_BROADBAND_MODEM_THURAYA(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), MM_TYPE_BROADBAND_MODEM_THURAYA, MMBroadbandModemThuraya))
> +#define MM_BROADBAND_MODEM_THURAYA_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), MM_TYPE_BROADBAND_MODEM_THURAYA, MMBroadbandModemThurayaClass))
> +#define MM_IS_BROADBAND_MODEM_THURAYA(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), MM_TYPE_BROADBAND_MODEM_THURAYA))
> +#define MM_IS_BROADBAND_MODEM_THURAYA_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), MM_TYPE_BROADBAND_MODEM_THURAYA))
> +#define MM_BROADBAND_MODEM_THURAYA_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), MM_TYPE_BROADBAND_MODEM_THURAYA, MMBroadbandModemThurayaClass))
> +
> +typedef struct _MMBroadbandModemThuraya MMBroadbandModemThuraya;
> +typedef struct _MMBroadbandModemThurayaClass MMBroadbandModemThurayaClass;
> +
> +struct _MMBroadbandModemThuraya {
> + MMBroadbandModem parent;
> +};
> +
> +struct _MMBroadbandModemThurayaClass{
> + MMBroadbandModemClass parent;
> +};
> +
> +GType mm_broadband_modem_thuraya_get_type (void);
> +
> +MMBroadbandModemThuraya *mm_broadband_modem_thuraya_new (const gchar *device,
> + const gchar **drivers,
> + const gchar *plugin,
> + guint16 vendor_id,
> + guint16 product_id);
> +
> +#endif /* MM_BROADBAND_MODEM_THURAYA_H */
> diff --git a/plugins/thuraya/mm-plugin-thuraya.c b/plugins/thuraya/mm-plugin-thuraya.c
> new file mode 100644
> index 0000000..3b5ca46
> --- /dev/null
> +++ b/plugins/thuraya/mm-plugin-thuraya.c
> @@ -0,0 +1,93 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * 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., 59 Temple Place - Suite 330,
> + * Boston, MA 02111-1307, USA.
> + *
> + * Copyright (C) 2011 - 2012 Ammonit Measurement GmbH
> + * Author: Aleksander Morgado <aleksander at lanedo.com>
> + * Copyright (C) 2016
> + * Author: Thomas Sailer <t.sailer at alumni.ethz.ch>
> + */
> +
> +#include <string.h>
> +#include <gmodule.h>
> +
> +#define _LIBMM_INSIDE_MM
> +#include <libmm-glib.h>
> +
> +#include "mm-plugin-thuraya.h"
> +#include "mm-broadband-modem.h"
> +#include "mm-broadband-modem-thuraya.h"
> +#include "mm-private-boxed-types.h"
> +#include "mm-log.h"
> +
> +G_DEFINE_TYPE (MMPluginThuraya, mm_plugin_thuraya, MM_TYPE_PLUGIN)
> +
> +int mm_plugin_major_version = MM_PLUGIN_MAJOR_VERSION;
> +int mm_plugin_minor_version = MM_PLUGIN_MINOR_VERSION;
> +
> +static MMBaseModem *
> +create_modem (MMPlugin *self,
> + const gchar *sysfs_path,
> + const gchar **drivers,
> + guint16 vendor,
> + guint16 product,
> + GList *probes,
> + GError **error)
> +{
> + return MM_BASE_MODEM (mm_broadband_modem_thuraya_new (sysfs_path,
> + drivers,
> + mm_plugin_get_name (self),
> + vendor,
> + product));
> +}
> +
> +/*****************************************************************************/
> +
> +G_MODULE_EXPORT MMPlugin *
> +mm_plugin_create (void)
> +{
> + static const gchar *subsystems[] = { "tty", NULL };
> + static const guint16 vendor_ids[] = { 0x1a26, 0 };
> + static const guint16 product_ids[] = { 0x09cf, 0 };
> + static const gchar *vendor_strings[] = { "manufacturer apsi", NULL };
> + static const mm_str_pair product_strings[] = {{"manufacturer apsi", "thuraya xt" },
> + { NULL, NULL }};
> +
> + return MM_PLUGIN (
> + g_object_new (MM_TYPE_PLUGIN_THURAYA,
> + MM_PLUGIN_NAME, "Thuraya",
> + MM_PLUGIN_ALLOWED_SUBSYSTEMS, subsystems,
> + MM_PLUGIN_ALLOWED_VENDOR_STRINGS, vendor_strings,
> + MM_PLUGIN_ALLOWED_PRODUCT_STRINGS, product_strings,
> + MM_PLUGIN_ALLOWED_VENDOR_IDS, vendor_ids,
> + MM_PLUGIN_ALLOWED_PRODUCT_IDS, product_ids,
> + MM_PLUGIN_ALLOWED_AT, TRUE,
> + NULL));
I see that you're still using MM_PLUGIN_ALLOWED_VENDOR_STRINGS and
MM_PLUGIN_ALLOWED_PRODUCT_STRINGS. Do we really really need those?
Isn't this a USB-only modem, not an RS232 one? The _STRINGS checks are
only needed for plugins that may support RS232 modems.
> +}
> +
> +static void
> +mm_plugin_thuraya_init (MMPluginThuraya *self)
> +{
> +}
> +
> +static void
> +mm_plugin_thuraya_class_init (MMPluginThurayaClass *klass)
> +{
> + MMPluginClass *plugin_class = MM_PLUGIN_CLASS (klass);
> +
> + plugin_class->create_modem = create_modem;
> +}
> diff --git a/plugins/thuraya/mm-plugin-thuraya.h b/plugins/thuraya/mm-plugin-thuraya.h
> new file mode 100644
> index 0000000..d6ca2cd
> --- /dev/null
> +++ b/plugins/thuraya/mm-plugin-thuraya.h
> @@ -0,0 +1,49 @@
> +/* -*- Mode: C; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> +
> +/*
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of the
> + * License, or (at your option) any later version.
> + *
> + * 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., 59 Temple Place - Suite 330,
> + * Boston, MA 02111-1307, USA.
> + *
> + * Copyright (C) 2011 - 2012 Ammonit Measurement GmbH
> + * Author: Aleksander Morgado <aleksander at lanedo.com>
> + * Copyright (C) 2016
> + * Author: Thomas Sailer <t.sailer at alumni.ethz.ch>
> + */
> +
> +#ifndef MM_PLUGIN_THURAYA_H
> +#define MM_PLUGIN_THURAYA_H
> +
> +#include "mm-plugin.h"
> +
> +#define MM_TYPE_PLUGIN_THURAYA (mm_plugin_thuraya_get_type ())
> +#define MM_PLUGIN_THURAYA(obj) (G_TYPE_CHECK_INSTANCE_CAST ((obj), MM_TYPE_PLUGIN_THURAYA, MMPluginThuraya))
> +#define MM_PLUGIN_THURAYA_CLASS(klass) (G_TYPE_CHECK_CLASS_CAST ((klass), MM_TYPE_PLUGIN_THURAYA, MMPluginThurayaClass))
> +#define MM_IS_PLUGIN_THURAYA(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), MM_TYPE_PLUGIN_THURAYA))
> +#define MM_IS_PLUGIN_THURAYA_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), MM_TYPE_PLUGIN_THURAYA))
> +#define MM_PLUGIN_THURAYA_GET_CLASS(obj) (G_TYPE_INSTANCE_GET_CLASS ((obj), MM_TYPE_PLUGIN_THURAYA, MMPluginThurayaClass))
> +
> +typedef struct {
> + MMPlugin parent;
> +} MMPluginThuraya;
> +
> +typedef struct {
> + MMPluginClass parent;
> +} MMPluginThurayaClass;
> +
> +GType mm_plugin_thuraya_get_type (void);
> +
> +G_MODULE_EXPORT MMPlugin *mm_plugin_create (void);
> +
> +#endif /* MM_PLUGIN_THURAYA_H */
> --
> 2.5.0
>
--
Aleksander
https://aleksander.es
More information about the ModemManager-devel
mailing list