libdfu: Fix dfu_device_wait_for_replug() so it can work with a DfuContext

Using g_usleep() works for the command line tool, but this starves the event
loop which means we get a flurry of removed:added:removed:added signals once
complete.

The context 'helpfully' sets up the new GUsbDevice which means we're doing
things with DfuTargets without DfuDevices and DfuTargets without GUsbDevices.
Basically, madness ensues.
This commit is contained in:
Richard Hughes 2015-11-25 13:57:00 +00:00
parent 6511f5011f
commit 4bb94e4bcd
2 changed files with 143 additions and 115 deletions

View File

@ -59,7 +59,7 @@ typedef struct {
DfuStatus status; DfuStatus status;
GPtrArray *targets; GPtrArray *targets;
GUsbDevice *dev; GUsbDevice *dev;
gboolean device_open; gboolean open_new_dev; /* if set new GUsbDevice */
gboolean dfuse_supported; gboolean dfuse_supported;
gchar *display_name; gchar *display_name;
gchar *platform_id; gchar *platform_id;
@ -223,10 +223,7 @@ dfu_device_finalize (GObject *object)
DfuDevicePrivate *priv = GET_PRIVATE (device); DfuDevicePrivate *priv = GET_PRIVATE (device);
/* don't rely on this */ /* don't rely on this */
if (priv->device_open) { g_usb_device_close (priv->dev, NULL);
g_debug ("auto-closing DfuDevice, call dfu_device_close()");
g_usb_device_close (priv->dev, NULL);
}
g_free (priv->display_name); g_free (priv->display_name);
g_free (priv->platform_id); g_free (priv->platform_id);
@ -1172,12 +1169,14 @@ dfu_device_open (DfuDevice *device, DfuDeviceOpenFlags flags,
return FALSE; return FALSE;
} }
/* just ignore */
if (priv->device_open)
return TRUE;
/* open */ /* open */
if (!g_usb_device_open (priv->dev, &error_local)) { if (!g_usb_device_open (priv->dev, &error_local)) {
if (g_error_matches (error_local,
G_USB_DEVICE_ERROR,
G_USB_DEVICE_ERROR_ALREADY_OPEN)) {
g_debug ("device already open, ignoring");
return TRUE;
}
if (g_error_matches (error_local, if (g_error_matches (error_local,
G_USB_DEVICE_ERROR, G_USB_DEVICE_ERROR,
G_USB_DEVICE_ERROR_PERMISSION_DENIED)) { G_USB_DEVICE_ERROR_PERMISSION_DENIED)) {
@ -1233,7 +1232,7 @@ dfu_device_open (DfuDevice *device, DfuDeviceOpenFlags flags,
} }
} }
priv->device_open = TRUE; priv->open_new_dev = TRUE;
return TRUE; return TRUE;
} }
@ -1264,15 +1263,21 @@ dfu_device_close (DfuDevice *device, GError **error)
return FALSE; return FALSE;
} }
/* only close if open */ /* close if open */
if (priv->device_open) { if (!g_usb_device_close (priv->dev, &error_local)) {
g_usb_device_release_interface (priv->dev, if (g_error_matches (error_local,
(gint) priv->iface_number, G_USB_DEVICE_ERROR,
0, NULL); G_USB_DEVICE_ERROR_NOT_OPEN)) {
if (!g_usb_device_close (priv->dev, error)) g_debug ("device not open, so ignoring error for close");
return FALSE; return TRUE;
priv->device_open = FALSE; }
g_set_error_literal (error,
DFU_ERROR,
DFU_ERROR_INTERNAL,
error_local->message);
return FALSE;
} }
priv->open_new_dev = FALSE;
return TRUE; return TRUE;
} }
@ -1284,23 +1289,27 @@ dfu_device_set_new_usb_dev (DfuDevice *device, GUsbDevice *dev,
GCancellable *cancellable, GError **error) GCancellable *cancellable, GError **error)
{ {
DfuDevicePrivate *priv = GET_PRIVATE (device); DfuDevicePrivate *priv = GET_PRIVATE (device);
gboolean reopen_device = FALSE;
/* same */ /* same */
if (priv->dev == dev) if (priv->dev == dev) {
g_warning ("setting GUsbDevice with same dev?!");
return TRUE; return TRUE;
}
/* device removed */ /* device removed */
if (dev == NULL) { if (dev == NULL) {
g_debug ("invalidating backing GUsbDevice");
g_clear_object (&priv->dev); g_clear_object (&priv->dev);
g_ptr_array_set_size (priv->targets, 0);
return TRUE; return TRUE;
} }
/* close */ /* close */
if (priv->device_open) { if (priv->dev != NULL) {
gboolean tmp = priv->open_new_dev;
if (!dfu_device_close (device, error)) if (!dfu_device_close (device, error))
return FALSE; return FALSE;
reopen_device = TRUE; priv->open_new_dev = tmp;
} }
/* set the new USB device */ /* set the new USB device */
@ -1309,13 +1318,12 @@ dfu_device_set_new_usb_dev (DfuDevice *device, GUsbDevice *dev,
/* should be the same */ /* should be the same */
if (g_strcmp0 (priv->platform_id, if (g_strcmp0 (priv->platform_id,
g_usb_device_get_platform_id (dev)) != 0) { g_usb_device_get_platform_id (dev)) != 0) {
g_warning ("platform ID changed"); g_warning ("platform ID changed when setting new GUsbDevice?!");
g_free (priv->platform_id); g_free (priv->platform_id);
priv->platform_id = g_strdup (g_usb_device_get_platform_id (dev)); priv->platform_id = g_strdup (g_usb_device_get_platform_id (dev));
} }
/* update all the targets */ /* update all the targets */
g_ptr_array_set_size (priv->targets, 0);
if (!dfu_device_add_targets (device)) { if (!dfu_device_add_targets (device)) {
g_set_error_literal (error, g_set_error_literal (error,
DFU_ERROR, DFU_ERROR,
@ -1325,7 +1333,8 @@ dfu_device_set_new_usb_dev (DfuDevice *device, GUsbDevice *dev,
} }
/* reclaim */ /* reclaim */
if (reopen_device) { if (priv->open_new_dev) {
g_debug ("automatically reopening device");
if (!dfu_device_open (device, DFU_DEVICE_OPEN_FLAG_NONE, if (!dfu_device_open (device, DFU_DEVICE_OPEN_FLAG_NONE,
cancellable, error)) cancellable, error))
return FALSE; return FALSE;
@ -1333,6 +1342,74 @@ dfu_device_set_new_usb_dev (DfuDevice *device, GUsbDevice *dev,
return TRUE; return TRUE;
} }
typedef struct {
DfuDevice *device;
GError **error;
GMainLoop *loop;
GUsbDevice *dev;
guint cnt;
guint timeout;
} DfuDeviceReplugHelper;
/**
* dfu_device_replug_helper_free:
**/
static void
dfu_device_replug_helper_free (DfuDeviceReplugHelper *helper)
{
if (helper->dev != NULL)
g_object_unref (helper->dev);
g_object_unref (helper->device);
g_main_loop_unref (helper->loop);
g_free (helper);
}
/**
* dfu_device_replug_helper_cb:
**/
static gboolean
dfu_device_replug_helper_cb (gpointer user_data)
{
DfuDeviceReplugHelper *helper = (DfuDeviceReplugHelper *) user_data;
DfuDevicePrivate *priv = GET_PRIVATE (helper->device);
/* did the backing GUsbDevice change */
if (helper->dev != priv->dev) {
g_debug ("device changed GUsbDevice %p->%p",
helper->dev, priv->dev);
g_set_object (&helper->dev, priv->dev);
/* success */
if (helper->dev != NULL) {
g_main_loop_quit (helper->loop);
return FALSE;
}
}
/* set a limit */
if (helper->cnt++ * 100 > helper->timeout) {
g_debug ("gave up waiting for device replug");
if (helper->dev == NULL) {
g_set_error_literal (helper->error,
DFU_ERROR,
DFU_ERROR_INVALID_DEVICE,
"target went away but did not come back");
} else {
g_set_error_literal (helper->error,
DFU_ERROR,
DFU_ERROR_INVALID_DEVICE,
"target did not disconnect");
}
g_main_loop_quit (helper->loop);
return FALSE;
}
/* continue waiting */
g_debug ("waiting for device replug for %ims -- state is %s",
helper->cnt * 100, dfu_state_to_string (priv->state));
return TRUE;
}
/** /**
* dfu_device_wait_for_replug: * dfu_device_wait_for_replug:
* @device: a #DfuDevice * @device: a #DfuDevice
@ -1341,6 +1418,7 @@ dfu_device_set_new_usb_dev (DfuDevice *device, GUsbDevice *dev,
* @error: a #GError, or %NULL * @error: a #GError, or %NULL
* *
* Waits for a DFU device to disconnect and reconnect. * Waits for a DFU device to disconnect and reconnect.
* This does rely on a #DfuContext being set up before this is called.
* *
* Return value: %TRUE for success * Return value: %TRUE for success
* *
@ -1351,73 +1429,25 @@ dfu_device_wait_for_replug (DfuDevice *device, guint timeout,
GCancellable *cancellable, GError **error) GCancellable *cancellable, GError **error)
{ {
DfuDevicePrivate *priv = GET_PRIVATE (device); DfuDevicePrivate *priv = GET_PRIVATE (device);
const guint poll_interval_ms = 100; DfuDeviceReplugHelper *helper;
gboolean went_away = FALSE; GError *error_tmp = NULL;
guint16 pid; const guint replug_poll = 100; /* ms */
guint16 vid;
guint i;
g_autofree gchar *platform_id = NULL;
g_autoptr(GUsbContext) usb_ctx = NULL;
g_return_val_if_fail (DFU_IS_DEVICE (device), FALSE); helper = g_new0 (DfuDeviceReplugHelper, 1);
g_return_val_if_fail (error == NULL || *error == NULL, FALSE); helper->loop = g_main_loop_new (NULL, FALSE);
helper->device = g_object_ref (device);
/* no backing USB device */ helper->dev = g_object_ref (priv->dev);
if (priv->dev == NULL) { helper->error = &error_tmp;
g_set_error (error, helper->timeout = timeout;
DFU_ERROR, g_timeout_add_full (G_PRIORITY_DEFAULT, replug_poll,
DFU_ERROR_INTERNAL, dfu_device_replug_helper_cb, helper,
"failed to wait for replug: no GUsbDevice for %s", (GDestroyNotify) dfu_device_replug_helper_free);
priv->platform_id); g_main_loop_run (helper->loop);
if (error_tmp != NULL) {
g_propagate_error (error, error_tmp);
return FALSE; return FALSE;
} }
return TRUE;
/* keep copies */
platform_id = g_strdup (g_usb_device_get_platform_id (priv->dev));
vid = g_usb_device_get_vid (priv->dev);
pid = g_usb_device_get_pid (priv->dev);
/* keep trying */
g_object_get (priv->dev, "context", &usb_ctx, NULL);
for (i = 0; i < timeout / poll_interval_ms; i++) {
g_autoptr(GUsbDevice) dev_tmp = NULL;
g_usleep (poll_interval_ms * 1000);
g_usb_context_enumerate (usb_ctx);
dev_tmp = g_usb_context_find_by_platform_id (usb_ctx, platform_id, NULL);
if (dev_tmp == NULL) {
went_away = TRUE;
continue;
}
/* some devices use the same PID for DFU mode */
if (priv->quirks & DFU_DEVICE_QUIRK_NO_PID_CHANGE) {
return dfu_device_set_new_usb_dev (device, dev_tmp,
cancellable, error);
}
/* VID:PID changed so find a DFU iface with the same alt */
if (vid != g_usb_device_get_vid (dev_tmp) ||
pid != g_usb_device_get_pid (dev_tmp)) {
return dfu_device_set_new_usb_dev (device, dev_tmp,
cancellable, error);
}
}
/* target went off into the woods */
if (went_away) {
g_set_error_literal (error,
DFU_ERROR,
DFU_ERROR_INVALID_DEVICE,
"target went away but did not come back");
return FALSE;
}
/* VID and PID did not change */
g_set_error_literal (error,
DFU_ERROR,
DFU_ERROR_INVALID_DEVICE,
"target came back with same VID:PID values");
return FALSE;
} }
/** /**

View File

@ -25,6 +25,7 @@
#include <stdlib.h> #include <stdlib.h>
#include "dfu-common.h" #include "dfu-common.h"
#include "dfu-context.h"
#include "dfu-device.h" #include "dfu-device.h"
#include "dfu-error.h" #include "dfu-error.h"
#include "dfu-firmware.h" #include "dfu-firmware.h"
@ -323,27 +324,25 @@ dfu_colorhug_plus_func (void)
GPtrArray *elements; GPtrArray *elements;
gboolean ret; gboolean ret;
gboolean seen_app_idle = FALSE; gboolean seen_app_idle = FALSE;
g_autoptr(DfuContext) context = NULL;
g_autoptr(DfuDevice) device = NULL; g_autoptr(DfuDevice) device = NULL;
g_autoptr(DfuDevice) device2 = NULL;
g_autoptr(DfuTarget) target = NULL; g_autoptr(DfuTarget) target = NULL;
g_autoptr(DfuImage) image = NULL; g_autoptr(DfuImage) image = NULL;
g_autoptr(GError) error = NULL; g_autoptr(GError) error = NULL;
g_autoptr(GUsbContext) usb_ctx = NULL;
g_autoptr(GUsbDevice) usb_device = NULL; /* create context */
context = dfu_context_new ();
ret = dfu_context_enumerate (context, &error);
g_assert_no_error (error);
g_assert (ret);
/* push appIDLE into dfuIDLE */ /* push appIDLE into dfuIDLE */
usb_ctx = g_usb_context_new (&error); device2 = dfu_context_get_device_by_vid_pid (context,
g_assert_no_error (error); 0x273f,
g_assert (usb_ctx != NULL); 0x1002,
g_usb_context_enumerate (usb_ctx); NULL);
usb_device = g_usb_context_find_by_vid_pid (usb_ctx, if (device2 != NULL) {
0x273f,
0x1002,
NULL);
if (usb_device != NULL) {
g_autoptr(DfuDevice) device2 = NULL;
device2 = dfu_device_new (usb_device);
g_assert (device2 != NULL);
ret = dfu_device_open (device2, DFU_DEVICE_OPEN_FLAG_NONE, NULL, &error); ret = dfu_device_open (device2, DFU_DEVICE_OPEN_FLAG_NONE, NULL, &error);
g_assert_no_error (error); g_assert_no_error (error);
g_assert (ret); g_assert (ret);
@ -360,25 +359,21 @@ dfu_colorhug_plus_func (void)
ret = dfu_device_close (device2, &error); ret = dfu_device_close (device2, &error);
g_assert_no_error (error); g_assert_no_error (error);
g_assert (ret); g_assert (ret);
/* we know the runtime VID:PID */
} }
/* find any DFU in dfuIDLE mode */ /* find any DFU in dfuIDLE mode */
usb_device = g_usb_context_find_by_vid_pid (usb_ctx, device = dfu_context_get_device_by_vid_pid (context,
0x273f, 0x273f,
0x1003, 0x1003,
NULL); NULL);
if (usb_device == NULL) if (device == NULL)
return; return;
/* check it's DFU-capable */ /* we don't know this unless we went from appIDLE -> dfuIDLE */
device = dfu_device_new (usb_device); if (device2 == NULL) {
g_assert (device != NULL); g_assert_cmpint (dfu_device_get_runtime_vid (device), ==, 0xffff);
g_assert_cmpint (dfu_device_get_runtime_pid (device), ==, 0xffff);
/* we don't know this yet */ }
g_assert_cmpint (dfu_device_get_runtime_vid (device), ==, 0xffff);
g_assert_cmpint (dfu_device_get_runtime_pid (device), ==, 0xffff);
/* open it */ /* open it */
ret = dfu_device_open (device, DFU_DEVICE_OPEN_FLAG_NONE, NULL, &error); ret = dfu_device_open (device, DFU_DEVICE_OPEN_FLAG_NONE, NULL, &error);
@ -548,6 +543,9 @@ main (int argc, char **argv)
/* only critical and error are fatal */ /* only critical and error are fatal */
g_log_set_fatal_mask (NULL, G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL); g_log_set_fatal_mask (NULL, G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL);
/* log everything */
g_setenv ("G_MESSAGES_DEBUG", "all", FALSE);
/* tests go here */ /* tests go here */
g_test_add_func ("/libdfu/enums", dfu_enums_func); g_test_add_func ("/libdfu/enums", dfu_enums_func);
g_test_add_func ("/libdfu/target(DfuSe}", dfu_target_dfuse_func); g_test_add_func ("/libdfu/target(DfuSe}", dfu_target_dfuse_func);