Re: [PATCH] base-bearer: stop connection status monitoring if no TTY available

2017-03-24 Thread Aleksander Morgado 
On 24/03/17 21:20, Aleksander Morgado wrote:
>>> +/* If CID not defined, error out */
>>>  if (!MM_BROADBAND_BEARER (self)->priv->cid) {
>>> -g_task_return_new_error (task, MM_CORE_ERROR,
>>> MM_CORE_ERROR_FAILED,
>>> +g_task_return_new_error (task, MM_CORE_ERROR,
>>> MM_CORE_ERROR_UNSUPPORTED,
>> Did you mean to change this case from FAILED -> UNSUPPORTED?
> Yes, but on a second thought, you're right, that should be kept
> FAILED. I'll update it.

Pushed to git master after updating that specific error back to FAILED,
and also the error string returned when no available TTY was found,
which was also incorrect.

-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] base-bearer: stop connection status monitoring if no TTY available

2017-03-24 Thread Aleksander Morgado 
On Fri, Mar 24, 2017 at 9:15 PM, Dan Williams  wrote:
>> +/* If CID not defined, error out */
>>  if (!MM_BROADBAND_BEARER (self)->priv->cid) {
>> -g_task_return_new_error (task, MM_CORE_ERROR,
>> MM_CORE_ERROR_FAILED,
>> +g_task_return_new_error (task, MM_CORE_ERROR,
>> MM_CORE_ERROR_UNSUPPORTED,
>
> Did you mean to change this case from FAILED -> UNSUPPORTED?

Yes, but on a second thought, you're right, that should be kept
FAILED. I'll update it.


-- 
Aleksander
https://aleksander.es
___
ModemManager-devel mailing list
ModemManager-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/modemmanager-devel

Re: [PATCH] base-bearer: stop connection status monitoring if no TTY available

2017-03-24 Thread Dan Williams 
On Fri, 2017-03-24 at 14:55 +0100, Aleksander Morgado wrote:
> On modems with a single TTY for both control and data, we cannot use
> the TTY to load connection status once we're in connected mode:
> 
> Connection through a plain serial AT port (ttyUSB2)
> (ttyUSB2): --> 'ATD*99***2#'
> (ttyUSB2): <-- 'CONNECT 1'
> (ttyUSB2): port now connected
> Connected bearer '/org/freedesktop/ModemManager1/Bearer/0'
> Modem /org/freedesktop/ModemManager1/Modem/0: state changed
> (connecting -> connected)
> Simple connect state (8/8): All done
> checking if connected failed: Couldn't check current list of
> active PDP contexts: No AT port available to run command
> checking if connected failed: Couldn't check current list of
> active PDP contexts: No AT port available to run command
> checking if connected failed: Couldn't check current list of
> active PDP contexts: No AT port available to run command
> ...
> 
> So, disable connection monitoring right away if that situation is
> detected:
> 
> Connection through a plain serial AT port (ttyUSB2)
> (ttyUSB2): --> 'ATD*99***2#'
> (ttyUSB2): <-- 'CONNECT 1'
> (ttyUSB2): port now connected
> Connected bearer '/org/freedesktop/ModemManager1/Bearer/0'
> Modem /org/freedesktop/ModemManager1/Modem/0: state changed
> (connecting -> connected)
> Simple connect state (8/8): All done
> Connection monitoring is unsupported by the device
> ...
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=100376
> ---
> 
> Hey everyone,
> 
> Plugins can now report a MM_CORE_UNSUPPORTED error in
> load_connection_status() in order to instruct the upper layers to
> stop doing the connection status monitoring for the current
> connection. This is implemented in the generic broadband bearer for
> the specific case of single-TTY modems, where the control TTY can no
> longer be used for AT commands once it gets in connected mode.
> 
> Comments?
> 
> ---
>  src/mm-base-bearer.c  | 20 ++--
>  src/mm-broadband-bearer.c | 43 +--
> 
>  2 files changed, 47 insertions(+), 16 deletions(-)
> 
> diff --git a/src/mm-base-bearer.c b/src/mm-base-bearer.c
> index 0cca8534..02084dff 100644
> --- a/src/mm-base-bearer.c
> +++ b/src/mm-base-bearer.c
> @@ -92,6 +92,8 @@ struct _MMBaseBearerPrivate {
> 
>  /* Connection status monitoring */
>  guint connection_monitor_id;
> +/* Flag to specify whether connection monitoring is supported or
> not */
> +gboolean load_connection_status_unsupported;
> 
>  /*-- 3GPP specific --*/
>  guint deferred_3gpp_unregistration_id;
> @@ -162,7 +164,18 @@ load_connection_status_ready (MMBaseBearer
> *self,
> 
>  status = MM_BASE_BEARER_GET_CLASS (self)-
> >load_connection_status_finish (self, res, );
>  if (status == MM_BEARER_CONNECTION_STATUS_UNKNOWN) {
> -mm_warn ("checking if connected failed: %s", error-
> >message);
> +/* Only warn if not reporting an "unsupported" error */
> +if (!g_error_matches (error, MM_CORE_ERROR,
> MM_CORE_ERROR_UNSUPPORTED)) {
> +mm_warn ("checking if connected failed: %s", error-
> >message);
> +g_error_free (error);
> +return;
> +}
> +
> +/* If we're being told that connection monitoring is
> unsupported, just
> + * ignore the error and remove the timeout. */
> +mm_dbg ("Connection monitoring is unsupported by the
> device");
> +self->priv->load_connection_status_unsupported = TRUE;
> +connection_monitor_stop (self);
>  g_error_free (error);
>  return;
>  }
> @@ -176,7 +189,7 @@ load_connection_status_ready (MMBaseBearer *self,
>  static gboolean
>  connection_monitor_cb (MMBaseBearer *self)
>  {
> -/* If the implementation knows how to update stat values, run it
> */
> +/* If the implementation knows how to load connection status,
> run it */
>  MM_BASE_BEARER_GET_CLASS (self)->load_connection_status (
>  self,
>  (GAsyncReadyCallback)load_connection_status_ready,
> @@ -192,6 +205,9 @@ connection_monitor_start (MMBaseBearer *self)
>  !MM_BASE_BEARER_GET_CLASS (self)-
> >load_connection_status_finish)
>  return;
> 
> +if (self->priv->load_connection_status_unsupported)
> +return;
> +
>  /* Schedule */
>  g_assert (!self->priv->connection_monitor_id);
>  self->priv->connection_monitor_id = g_timeout_add_seconds
> (BEARER_CONNECTION_MONITOR_TIMEOUT,
> diff --git a/src/mm-broadband-bearer.c b/src/mm-broadband-bearer.c
> index 1188f60a..f2834b1b 100644
> --- a/src/mm-broadband-bearer.c
> +++ b/src/mm-broadband-bearer.c
> @@ -1983,30 +1983,45 @@ load_connection_status
> (MMBaseBearer*self,
>  GAsyncReadyCallback  callback,
>  gpointer user_data)
>  {
> -GTask   *task;
> -MMBaseModem *modem = NULL;

[PATCH] base-bearer: stop connection status monitoring if no TTY available

2017-03-24 Thread Aleksander Morgado 
On modems with a single TTY for both control and data, we cannot use
the TTY to load connection status once we're in connected mode:

Connection through a plain serial AT port (ttyUSB2)
(ttyUSB2): --> 'ATD*99***2#'
(ttyUSB2): <-- 'CONNECT 1'
(ttyUSB2): port now connected
Connected bearer '/org/freedesktop/ModemManager1/Bearer/0'
Modem /org/freedesktop/ModemManager1/Modem/0: state changed (connecting -> 
connected)
Simple connect state (8/8): All done
checking if connected failed: Couldn't check current list of active PDP 
contexts: No AT port available to run command
checking if connected failed: Couldn't check current list of active PDP 
contexts: No AT port available to run command
checking if connected failed: Couldn't check current list of active PDP 
contexts: No AT port available to run command
...

So, disable connection monitoring right away if that situation is
detected:

Connection through a plain serial AT port (ttyUSB2)
(ttyUSB2): --> 'ATD*99***2#'
(ttyUSB2): <-- 'CONNECT 1'
(ttyUSB2): port now connected
Connected bearer '/org/freedesktop/ModemManager1/Bearer/0'
Modem /org/freedesktop/ModemManager1/Modem/0: state changed (connecting -> 
connected)
Simple connect state (8/8): All done
Connection monitoring is unsupported by the device
...

https://bugs.freedesktop.org/show_bug.cgi?id=100376
---

Hey everyone,

Plugins can now report a MM_CORE_UNSUPPORTED error in load_connection_status() 
in order to instruct the upper layers to stop doing the connection status 
monitoring for the current connection. This is implemented in the generic 
broadband bearer for the specific case of single-TTY modems, where the control 
TTY can no longer be used for AT commands once it gets in connected mode.

Comments?

---
 src/mm-base-bearer.c  | 20 ++--
 src/mm-broadband-bearer.c | 43 +--
 2 files changed, 47 insertions(+), 16 deletions(-)

diff --git a/src/mm-base-bearer.c b/src/mm-base-bearer.c
index 0cca8534..02084dff 100644
--- a/src/mm-base-bearer.c
+++ b/src/mm-base-bearer.c
@@ -92,6 +92,8 @@ struct _MMBaseBearerPrivate {

 /* Connection status monitoring */
 guint connection_monitor_id;
+/* Flag to specify whether connection monitoring is supported or not */
+gboolean load_connection_status_unsupported;

 /*-- 3GPP specific --*/
 guint deferred_3gpp_unregistration_id;
@@ -162,7 +164,18 @@ load_connection_status_ready (MMBaseBearer *self,

 status = MM_BASE_BEARER_GET_CLASS (self)->load_connection_status_finish 
(self, res, );
 if (status == MM_BEARER_CONNECTION_STATUS_UNKNOWN) {
-mm_warn ("checking if connected failed: %s", error->message);
+/* Only warn if not reporting an "unsupported" error */
+if (!g_error_matches (error, MM_CORE_ERROR, 
MM_CORE_ERROR_UNSUPPORTED)) {
+mm_warn ("checking if connected failed: %s", error->message);
+g_error_free (error);
+return;
+}
+
+/* If we're being told that connection monitoring is unsupported, just
+ * ignore the error and remove the timeout. */
+mm_dbg ("Connection monitoring is unsupported by the device");
+self->priv->load_connection_status_unsupported = TRUE;
+connection_monitor_stop (self);
 g_error_free (error);
 return;
 }
@@ -176,7 +189,7 @@ load_connection_status_ready (MMBaseBearer *self,
 static gboolean
 connection_monitor_cb (MMBaseBearer *self)
 {
-/* If the implementation knows how to update stat values, run it */
+/* If the implementation knows how to load connection status, run it */
 MM_BASE_BEARER_GET_CLASS (self)->load_connection_status (
 self,
 (GAsyncReadyCallback)load_connection_status_ready,
@@ -192,6 +205,9 @@ connection_monitor_start (MMBaseBearer *self)
 !MM_BASE_BEARER_GET_CLASS (self)->load_connection_status_finish)
 return;

+if (self->priv->load_connection_status_unsupported)
+return;
+
 /* Schedule */
 g_assert (!self->priv->connection_monitor_id);
 self->priv->connection_monitor_id = g_timeout_add_seconds 
(BEARER_CONNECTION_MONITOR_TIMEOUT,
diff --git a/src/mm-broadband-bearer.c b/src/mm-broadband-bearer.c
index 1188f60a..f2834b1b 100644
--- a/src/mm-broadband-bearer.c
+++ b/src/mm-broadband-bearer.c
@@ -1983,30 +1983,45 @@ load_connection_status (MMBaseBearer*self,
 GAsyncReadyCallback  callback,
 gpointer user_data)
 {
-GTask   *task;
-MMBaseModem *modem = NULL;
+GTask  *task;
+MMBaseModem*modem = NULL;
+MMPortSerialAt *port;

 task = g_task_new (self, NULL, callback, user_data);

+g_object_get (MM_BASE_BEARER (self),
+  MM_BASE_BEARER_MODEM, ,
+  NULL);
+
+/* If CID not defined, error out */