meson.build: release version 0.1.2

There's no reason we should wait 3s before retrying a failed AT command. 
This can even cause issues with suspend inhibition: 3 retries of a 
failed command would take 9s, while logind ignores the inhibitor after 
only 5s.

Reducing the delay to 500ms should be enough and wouldn't interfere with 
suspend inhibition.

Fixes #3

meson.build

@@ -8,7 +8,7 @@
 project (
   'eg25manager',
   'c',
-  version : '0.0.1',
+  version : '0.1.2',
   license : 'GPLv3+',
   meson_version : '>= 0.50.0',
   default_options :
@@ -43,6 +43,7 @@ endif
 mgr_deps = [
     dependency('glib-2.0'),
     dependency('gio-unix-2.0'),
+    dependency('gudev-1.0'),
     dependency('libgpiod'),
     dependency('libusb-1.0'),
     dependency('mm-glib'),

src/at.c

@@ -52,19 +52,24 @@ static int configure_serial(const char *tty)
 static gboolean send_at_command(struct EG25Manager *manager)
 {
     char command[256];
-    struct AtCommand *at_cmd = g_list_nth_data(manager->at_cmds, 0);
+    struct AtCommand *at_cmd = manager->at_cmds ? g_list_nth_data(manager->at_cmds, 0) : NULL;
+    int ret, len = 0;
 
     if (at_cmd) {
-        if (at_cmd->subcmd == NULL && at_cmd->value == NULL)
-            sprintf(command, "AT+%s?\r\n", at_cmd->cmd);
+        if (at_cmd->subcmd == NULL && at_cmd->value == NULL && at_cmd->expected == NULL)
+            len = sprintf(command, "AT+%s\r\n", at_cmd->cmd);
+        else if (at_cmd->subcmd == NULL && at_cmd->value == NULL)
+            len = sprintf(command, "AT+%s?\r\n", at_cmd->cmd);
         else if (at_cmd->subcmd == NULL && at_cmd->value)
-            sprintf(command, "AT+%s=%s\r\n", at_cmd->cmd, at_cmd->value);
+            len = sprintf(command, "AT+%s=%s\r\n", at_cmd->cmd, at_cmd->value);
         else if (at_cmd->subcmd && at_cmd->value == NULL)
-            sprintf(command, "AT+%s=\"%s\"\r\n", at_cmd->cmd, at_cmd->subcmd);
+            len = sprintf(command, "AT+%s=\"%s\"\r\n", at_cmd->cmd, at_cmd->subcmd);
         else if (at_cmd->subcmd && at_cmd->value)
-            sprintf(command, "AT+%s=\"%s\",%s\r\n", at_cmd->cmd, at_cmd->subcmd, at_cmd->value);
+            len = sprintf(command, "AT+%s=\"%s\",%s\r\n", at_cmd->cmd, at_cmd->subcmd, at_cmd->value);
 
-        write(manager->at_fd, command, strlen(command));
+        ret = write(manager->at_fd, command, len);
+        if (ret < len)
+            g_warning("Couldn't write full AT command: wrote %d/%d bytes", ret, len);
 
         g_message("Sending command: %s", g_strstrip(command));
     } else if (manager->modem_state < EG25_STATE_CONFIGURED) {
@@ -75,8 +80,7 @@ static gboolean send_at_command(struct EG25Manager *manager)
         else
             manager->modem_state = EG25_STATE_CONFIGURED;
     } else if (manager->modem_state == EG25_STATE_SUSPENDING) {
-        g_message("suspend sequence is over, drop inhibitor");
-        suspend_inhibit(manager, FALSE);
+        modem_suspend_post(manager);
     } else if (manager->modem_state == EG25_STATE_RESETTING) {
         manager->modem_state = EG25_STATE_POWERED;
     }
@@ -86,7 +90,10 @@ static gboolean send_at_command(struct EG25Manager *manager)
 
 static void next_at_command(struct EG25Manager *manager)
 {
-    struct AtCommand *at_cmd = g_list_nth_data(manager->at_cmds, 0);
+    struct AtCommand *at_cmd = manager->at_cmds ? g_list_nth_data(manager->at_cmds, 0) : NULL;
+
+    if (!at_cmd)
+        return;
 
     if (at_cmd->cmd)
         g_free(at_cmd->cmd);
@@ -104,20 +111,26 @@ static void next_at_command(struct EG25Manager *manager)
 
 static void retry_at_command(struct EG25Manager *manager)
 {
-    struct AtCommand *at_cmd = g_list_nth_data(manager->at_cmds, 0);
+    struct AtCommand *at_cmd = manager->at_cmds ? g_list_nth_data(manager->at_cmds, 0) : NULL;
+
+    if (!at_cmd)
+        return;
 
     at_cmd->retries++;
     if (at_cmd->retries > 3) {
         g_critical("Command %s retried %d times, aborting...", at_cmd->cmd, at_cmd->retries);
         next_at_command(manager);
     } else {
-        g_timeout_add_seconds(3, G_SOURCE_FUNC(send_at_command), manager);
+        g_timeout_add(500, G_SOURCE_FUNC(send_at_command), manager);
     }
 }
 
 static void process_at_result(struct EG25Manager *manager, char *response)
 {
-    struct AtCommand *at_cmd = g_list_nth_data(manager->at_cmds, 0);
+    struct AtCommand *at_cmd = manager->at_cmds ? g_list_nth_data(manager->at_cmds, 0) : NULL;
+
+    if (!at_cmd)
+        return;
 
     if (at_cmd->expected && !strstr(response, at_cmd->expected)) {
         if (at_cmd->value)
@@ -195,6 +208,9 @@ static gboolean modem_response(gint fd,
             retry_at_command(manager);
         else if (strstr(response, "OK"))
             process_at_result(manager, response);
+        else
+            // Not a recognized response, try running next command, just in case
+            next_at_command(manager);
     }
 
     return TRUE;
@@ -224,9 +240,9 @@ void at_sequence_configure(struct EG25Manager *manager)
 {
     /*
      * Default parameters in megi's driver which differ with our own:
-     *   - urc/ri/* are always set the same way on both BH and CE
-     *   - urc/ri/* pulse duration is 1 ms and urc/delay is 0 (no need to delay
-     *     URCs if the pulse is that short)
+     *   - urc/ri/x are always set the same way on both BH and CE
+     *   - urc/ri/x pulse duration is 1 ms and urc/delay is 0 (no need to delay
+     *     URCs if the pulse is that short, so this is expected)
      *   - apready is disabled
      *
      * Parameters set in megi's kernel but not here:
@@ -252,14 +268,18 @@ void at_sequence_configure(struct EG25Manager *manager)
         append_at_command(manager, "QCFG", "apready", NULL, "1,0,500");
     }
     append_at_command(manager, "QURCCFG", "urcport", NULL, "\"usbat\"");
-    append_at_command(manager, "QGPS", NULL, NULL, "1");
+    if (manager->manage_gnss)
+        append_at_command(manager, "QGPS", NULL, NULL, "1");
     append_at_command(manager, "QSCLK", NULL, "1", NULL);
+    // Make sure URC cache is disabled
+    append_at_command(manager, "QCFG", "urc/cache", "0", NULL);
     send_at_command(manager);
 }
 
 void at_sequence_suspend(struct EG25Manager *manager)
 {
-    append_at_command(manager, "QGPS", NULL, "0", NULL);
+    if (manager->manage_gnss)
+        append_at_command(manager, "QGPSEND", NULL, NULL, NULL);
     append_at_command(manager, "QCFG", "urc/cache", "1", NULL);
     send_at_command(manager);
 }
@@ -267,7 +287,8 @@ void at_sequence_suspend(struct EG25Manager *manager)
 void at_sequence_resume(struct EG25Manager *manager)
 {
     append_at_command(manager, "QCFG", "urc/cache", "0", NULL);
-    append_at_command(manager, "QGPS", NULL, "1", NULL);
+    if (manager->manage_gnss)
+        append_at_command(manager, "QGPS", NULL, "1", NULL);
     send_at_command(manager);
 }
 

src/gpio.c

@@ -131,13 +131,13 @@ int gpio_init(struct EG25Manager *manager)
 
         manager->gpio_out[i] = gpiod_chip_get_line(manager->gpiochip[chipidx], offset);
         if (!manager->gpio_out[i]) {
-            g_critical("Unable to get output GPIO %d", i);
+            g_error("Unable to get output GPIO %d", i);
             return 1;
         }
 
         ret = gpiod_line_request_output(manager->gpio_out[i], "eg25manager", 0);
         if (ret < 0) {
-            g_critical("Unable to request output GPIO %d", i);
+            g_error("Unable to request output GPIO %d", i);
             return 1;
         }
     }
@@ -177,7 +177,7 @@ gboolean gpio_check_poweroff(struct EG25Manager *manager, gboolean keep_down)
     if (manager->gpio_in[GPIO_IN_STATUS] &&
         gpiod_line_get_value(manager->gpio_in[GPIO_IN_STATUS]) == 1) {
 
-        if (keep_down) {
+        if (keep_down && manager->gpio_out[GPIO_OUT_RESET]) {
             // Asserting RESET line to prevent modem from rebooting
             gpiod_line_set_value(manager->gpio_out[GPIO_OUT_RESET], 1);
         }

src/manager.c

@@ -9,6 +9,7 @@
 #include "manager.h"
 #include "mm-iface.h"
 #include "suspend.h"
+#include "udev.h"
 
 #include <fcntl.h>
 #include <signal.h>
@@ -30,6 +31,7 @@ static gboolean quit_app(struct EG25Manager *manager)
     at_destroy(manager);
     mm_iface_destroy(manager);
     suspend_destroy(manager);
+    udev_destroy(manager);
 
     if (manager->modem_state >= EG25_STATE_STARTED) {
         g_message("Powering down the modem...");
@@ -110,45 +112,77 @@ void modem_configure(struct EG25Manager *manager)
     at_sequence_configure(manager);
 }
 
+static gboolean modem_reset_done(struct EG25Manager* manager)
+{
+    manager->modem_state = EG25_STATE_RESUMING;
+    manager->reset_timer = 0;
+    return FALSE;
+}
+
 void modem_reset(struct EG25Manager *manager)
 {
-    int fd;
+    int fd, ret, len;
+
+    if (manager->reset_timer)
+        return;
+
+    if (manager->suspend_timer) {
+        g_source_remove(manager->suspend_timer);
+        manager->suspend_timer = 0;
+    }
+
+    if (!manager->modem_usb_id) {
+        g_warning("Unknown modem USB ID");
+        goto error;
+    }
+
+    len = strlen(manager->modem_usb_id);
+
+    manager->modem_state = EG25_STATE_RESETTING;
 
     fd = open("/sys/bus/usb/drivers/usb/unbind", O_WRONLY);
     if (fd < 0)
         goto error;
-    write(fd, manager->modem_usb_id, strlen(manager->modem_usb_id));
+    ret = write(fd, manager->modem_usb_id, len);
+    if (ret < len)
+        g_warning("Couldn't unbind modem: wrote %d/%d bytes", ret, len);
     close(fd);
 
     fd = open("/sys/bus/usb/drivers/usb/bind", O_WRONLY);
     if (fd < 0)
         goto error;
-    write(fd, manager->modem_usb_id, strlen(manager->modem_usb_id));
+    ret = write(fd, manager->modem_usb_id, len);
+    if (ret < len)
+        g_warning("Couldn't bind modem: wrote %d/%d bytes", ret, len);
     close(fd);
 
-    manager->modem_state = EG25_STATE_CONFIGURED;
+    /*
+     * 3s is long enough to make sure the modem has been bound back and
+     * short enough to ensure it hasn't been acquired by ModemManager
+     */
+    manager->reset_timer = g_timeout_add_seconds(3, G_SOURCE_FUNC(modem_reset_done), manager);
+
     return;
 
 error:
     // Everything else failed, reset the modem
     at_sequence_reset(manager);
-    manager->modem_state = EG25_STATE_RESETTING;
 }
 
 void modem_suspend_pre(struct EG25Manager *manager)
 {
-    manager->modem_state = EG25_STATE_SUSPENDING;
-    gpio_sequence_suspend(manager);
+    at_sequence_suspend(manager);
 }
 
 void modem_suspend_post(struct EG25Manager *manager)
 {
-    at_sequence_suspend(manager);
+    gpio_sequence_suspend(manager);
+    g_message("suspend sequence is over, drop inhibitor");
+    suspend_inhibit(manager, FALSE);
 }
 
 void modem_resume_pre(struct EG25Manager *manager)
 {
-    manager->modem_state = EG25_STATE_RESUMING;
     gpio_sequence_resume(manager);
 }
 
@@ -159,14 +193,27 @@ void modem_resume_post(struct EG25Manager *manager)
 
 int main(int argc, char *argv[])
 {
+    g_autoptr(GOptionContext) opt_context = NULL;
+    g_autoptr(GError) err = NULL;
     struct EG25Manager manager;
     char compatible[32];
-    int fd;
+    int fd, ret;
+    const GOptionEntry options[] = {
+        { "gnss", 'g', 0, G_OPTION_ARG_NONE, &manager.manage_gnss, "Manage the GNSS feature.", NULL },
+        { NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }
+    };
 
     memset(&manager, 0, sizeof(manager));
     manager.at_fd = -1;
     manager.suspend_inhibit_fd = -1;
 
+    opt_context = g_option_context_new ("- Power management for the Quectel EG25 modem");
+    g_option_context_add_main_entries (opt_context, options, NULL);
+    if (!g_option_context_parse (opt_context, &argc, &argv, &err)) {
+        g_warning ("%s", err->message);
+        return 1;
+    }
+
     manager.loop = g_main_loop_new(NULL, FALSE);
 
     fd = open("/proc/device-tree/compatible", O_RDONLY);
@@ -174,8 +221,8 @@ int main(int argc, char *argv[])
         g_critical("Unable to read 'compatible' string from device tree");
         return 1;
     }
-    read(fd, compatible, sizeof(compatible));
-    if (!strstr(compatible, "pine64,pinephone-1.2"))
+    ret = read(fd, compatible, sizeof(compatible));
+    if (ret > 0 && !strstr(compatible, "pine64,pinephone-1.2"))
         manager.braveheart = TRUE;
     close(fd);
 
@@ -183,6 +230,7 @@ int main(int argc, char *argv[])
     gpio_init(&manager);
     mm_iface_init(&manager);
     suspend_init(&manager);
+    udev_init(&manager);
 
     g_idle_add(G_SOURCE_FUNC(modem_start), &manager);
 

src/manager.h

@@ -8,6 +8,7 @@
 
 #include <glib.h>
 #include <gpiod.h>
+#include <gudev/gudev.h>
 #include <libmm-glib.h>
 
 enum EG25State {
@@ -26,6 +27,12 @@ enum EG25State {
 
 struct EG25Manager {
     GMainLoop *loop;
+    guint reset_timer;
+    gboolean manage_gnss;
+
+    int at_fd;
+    guint at_source;
+    GList *at_cmds;
 
     enum EG25State modem_state;
     gchar *modem_usb_id;
@@ -37,12 +44,9 @@ struct EG25Manager {
 
     GDBusProxy *suspend_proxy;
     int suspend_inhibit_fd;
-    guint suspend_source;
+    guint suspend_timer;
 
-
-    int at_fd;
-    guint at_source;
-    GList *at_cmds;
+    GUdevClient *udev;
 
     struct gpiod_chip *gpiochip[2];
     struct gpiod_line *gpio_out[5];

src/meson.build

@@ -12,6 +12,7 @@ executable (
         'manager.c', 'manager.h',
         'mm-iface.c', 'mm-iface.h',
         'suspend.c', 'suspend.h',
+        'udev.c', 'udev.h',
     ],
     dependencies : mgr_deps,
     install : true

src/mm-iface.c

@@ -35,7 +35,10 @@ static void add_modem(struct EG25Manager *manager, GDBusObject *object)
     g_assert(manager->mm_modem != NULL);
 
     if (manager->modem_state == EG25_STATE_RESUMING) {
-        g_source_remove(manager->suspend_source);
+        if (manager->suspend_timer) {
+            g_source_remove(manager->suspend_timer);
+            manager->suspend_timer = 0;
+        }
         modem_resume_post(manager);
         manager->modem_state = EG25_STATE_CONFIGURED;
     }
@@ -84,13 +87,8 @@ static void interface_removed_cb(struct EG25Manager *manager,
 
     g_message("ModemManager interface `%s' removed on object `%s'", info->name, path);
 
-    if (g_strcmp0(info->name, MM_DBUS_INTERFACE_MODEM) == 0) {
+    if (g_strcmp0(info->name, MM_DBUS_INTERFACE_MODEM) == 0)
         manager->mm_modem = NULL;
-        if (manager->modem_usb_id) {
-            g_free(manager->modem_usb_id);
-            manager->modem_usb_id = NULL;
-        }
-    }
 }
 
 
@@ -132,14 +130,7 @@ static void object_removed_cb(struct EG25Manager *manager, GDBusObject *object)
     path = g_dbus_object_get_object_path(object);
     g_message("ModemManager object `%s' removed", path);
 
-    if (manager->modem_state == EG25_STATE_SUSPENDING)
-        modem_suspend_post(manager);
-
     manager->mm_modem = NULL;
-    if (manager->modem_usb_id) {
-        g_free(manager->modem_usb_id);
-        manager->modem_usb_id = NULL;
-    }
 }
 
 
@@ -147,7 +138,7 @@ static void mm_manager_new_cb(GDBusConnection    *connection,
                               GAsyncResult       *res,
                               struct EG25Manager *manager)
 {
-    GError *error = NULL;
+    g_autoptr (GError) error = NULL;
 
     manager->mm_manager = mm_manager_new_finish(res, &error);
     if (!manager->mm_manager)
@@ -199,6 +190,10 @@ void mm_iface_destroy(struct EG25Manager *manager)
         g_clear_object(&manager->mm_manager);
         manager->mm_manager = NULL;
     }
+    if (manager->modem_usb_id) {
+        g_free(manager->modem_usb_id);
+        manager->modem_usb_id = NULL;
+    }
     if (manager->mm_watch != 0) {
         g_bus_unwatch_name(manager->mm_watch);
         manager->mm_watch = 0;

src/suspend.c

@@ -20,6 +20,7 @@
 static gboolean check_modem_resume(struct EG25Manager *manager)
 {
     g_message("Modem wasn't probed in time, restart it!");
+    manager->suspend_timer = 0;
     modem_reset(manager);
 
     return FALSE;
@@ -42,14 +43,13 @@ static void inhibit_done(GObject *source,
 {
     GDBusProxy *suspend_proxy = G_DBUS_PROXY(source);
     struct EG25Manager *manager = user_data;
-    GError *error = NULL;
+    g_autoptr (GError) error = NULL;
     GVariant *res;
     GUnixFDList *fd_list;
 
     res = g_dbus_proxy_call_with_unix_fd_list_finish(suspend_proxy, &fd_list, result, &error);
     if (!res) {
         g_warning("inhibit failed: %s", error->message);
-        g_error_free(error);
     } else {
         if (!fd_list || g_unix_fd_list_get_length(fd_list) != 1)
             g_warning("didn't get a single fd back");
@@ -66,7 +66,8 @@ static void take_inhibitor(struct EG25Manager *manager)
 {
     GVariant *variant_arg;
 
-    g_assert(manager->suspend_inhibit_fd == -1);
+    if(manager->suspend_inhibit_fd != -1)
+        drop_inhibitor(manager);
 
     variant_arg = g_variant_new ("(ssss)", "sleep", "eg25manager",
                                  "eg25manager needs to prepare modem for sleep", "delay");
@@ -92,12 +93,14 @@ static void signal_cb(GDBusProxy *proxy,
 
     if (is_about_to_suspend) {
         g_message("system is about to suspend");
+        manager->modem_state = EG25_STATE_SUSPENDING;
         modem_suspend_pre(manager);
     } else {
         g_message("system is resuming");
         take_inhibitor(manager);
         modem_resume_pre(manager);
-        manager->suspend_source = g_timeout_add_seconds(8, G_SOURCE_FUNC(check_modem_resume), manager);
+        manager->modem_state = EG25_STATE_RESUMING;
+        manager->suspend_timer = g_timeout_add_seconds(9, G_SOURCE_FUNC(check_modem_resume), manager);
     }
 }
 
@@ -124,13 +127,12 @@ static void on_proxy_acquired(GObject *object,
                               GAsyncResult *res,
                               struct EG25Manager *manager)
 {
-    GError *error = NULL;
+    g_autoptr (GError) error = NULL;
     char *owner;
 
     manager->suspend_proxy = g_dbus_proxy_new_for_bus_finish(res, &error);
     if (!manager->suspend_proxy) {
         g_warning("failed to acquire logind proxy: %s", error->message);
-        g_clear_error(&error);
         return;
     }
 
@@ -156,6 +158,10 @@ void suspend_init(struct EG25Manager *manager)
 void suspend_destroy(struct EG25Manager *manager)
 {
     drop_inhibitor(manager);
+    if (manager->suspend_timer) {
+        g_source_remove(manager->suspend_timer);
+        manager->suspend_timer = 0;
+    }
     if (manager->suspend_proxy) {
         g_object_unref(manager->suspend_proxy);
         manager->suspend_proxy = NULL;

src/udev.c

@@ -0,0 +1,41 @@
+/*
+ * Copyright (C) 2020 Arnaud Ferraris <arnaud.ferraris@gmail.com>
+ *
+ * SPDX-License-Identifier: GPL-3.0-or-later
+ */
+
+#include "udev.h"
+
+#include <string.h>
+
+static void udev_event_cb(GUdevClient *client, gchar *action, GUdevDevice *device, gpointer data)
+{
+    struct EG25Manager *manager = data;
+
+    if (strcmp(action, "unbind") != 0 || manager->modem_state == EG25_STATE_RESETTING || !manager->modem_usb_id)
+        return;
+
+    if (strncmp(g_udev_device_get_name(device), manager->modem_usb_id, strlen(manager->modem_usb_id)) == 0 &&
+        manager->reset_timer == 0) {
+        g_message("Lost modem, resetting...");
+        modem_reset(manager);
+    }
+}
+
+void udev_init (struct EG25Manager *manager)
+{
+    const char * const subsystems[] = { "usb", NULL };
+
+    manager->udev = g_udev_client_new(subsystems);
+    g_signal_connect(manager->udev, "uevent", G_CALLBACK(udev_event_cb), manager);
+
+    return;
+}
+
+void udev_destroy (struct EG25Manager *manager)
+{
+    if (manager->udev) {
+        g_object_unref(manager->udev);
+        manager->udev = NULL;
+    }
+}

src/udev.h

@@ -0,0 +1,12 @@
+/*
+ * Copyright (C) 2020 Arnaud Ferraris <arnaud.ferraris@gmail.com>
+ *
+ * SPDX-License-Identifier: GPL-3.0-or-later
+ */
+
+#pragma once
+
+#include "manager.h"
+
+void udev_init (struct EG25Manager *data);
+void udev_destroy (struct EG25Manager *data);