Hi Toby, As Pete likes to point out I have no authority in the libusbx project, so as far as libusbx is concerned I think the best might be if you could pretend that I had never posted this feedback on the libusbx list.
Please consider my feedback to be in the context of the libusb project, where I of course think it would be great to include also Windows CE support. Toby Gray wrote: > We've developed a Windows CE backend for libusbx and would like to > contribute it. Cool! > I've attached a series of patches, split up into the backend and > various changes to common code. I think you've split too much actually, but this is always easier to fix than the other way around, so no big problem. > This backend requires the Windows CE Standard 5.00 SDK. * Why? - I mean: What parts of the SDK are required? * The SDK is from 2004. What C compiler can be used, and which capabilities does it have? I.e. corresponding to roughly which Windows Visual Studio version? > Windows CE doesn't expose any user side USB APIs so we've also > developed a kernel driver. This kernel driver is released under > the LGPL and can be found on github at > https://github.com/RealVNC/CEUSBKWrapper Note that an LGPL kernel driver for the WinCE kernel may be problematic as far as licensing goes. But that's separate to the libusb backend, in any case. > The current limitations of this backend are: > * It only supports control and bulk transfers. The CEUSBKWrapper > driver doesn't (yet) support isochronous or interrupt transfers. > The kernel APIs do exist, so they just need to be exposed to user-side. What kind of effort does that require? > The attached patches are broken down as follows: > > 0001 - adds the WinCE backend code and build files. > 0002 - Updates .gitignore to include the WinCE build files. > 0003 - Adds support for the WinCE backend to common code. > 0005 - Updates some time API calls to call the appropriate APIs for WinCE. The above should be combined into a single Add Windows CE support patch. > 0004 - Replaces getenv() with NULL on WinCE due to it not being supported. Try to change this in a good way where getenv() is called, instead. Is there an equivalent to environment variables in Windows CE? Maybe the registry? The question is in which key the value would be. If no, then you can always simply make autoconf check for getenv and put the code in libusb_init() within #ifdef HAVE_GETENV. Just don't forget to initialize the dbg stack variable if you go that route. > 0006 - Fixes a handle leak in usbi_cond_destroy. This seems unrelated to Windows CE, and should indeed be a separate commit. > Subject: [PATCH 1/6] WinCE: Add backend and build files for Windows CE > backend. > > This backend requires the CEUSBKWrapper driver to be installed > on the WinCE device. Without this no devices will be listed. How about making libusb_init() return an error instead? Is any part of the libusb API useful without the ukw driver? > --- > libusb/os/poll_wince.c | 656 +++++++++++++++++++ > libusb/os/poll_wince.h | 117 ++++ > libusb/os/wince_usb.c | 1222 +++++++++++++++++++++++++++++++++++ > libusb/os/wince_usb.h | 156 +++++ How about refactoring the existing Windows code so that everything that is common between the two systems will exist only in a single place? There seems to be an awful lot of code duplication between them. If you consider that infeasible then please sell that a; write about why, and write about what is different between the normal windows backend and the wince one. > wince/config.h | 21 + > wince/errno.h | 12 + > wince/inttypes.h | 295 +++++++++ > wince/libusb-1.0/libusb-1.0.vcproj | 1229 > ++++++++++++++++++++++++++++++++++++ > wince/libusb.sln | 114 ++++ Why not libusb-1.0.sln ? > wince/signal.h | 1 + > wince/stdint.h | 256 ++++++++ > wince/sys/types.h | 1 + > wince/winresrc.h | 1 + Are the above simply duplicated? Please strive for heavy code reuse. > +++ b/libusb/os/wince_usb.c .. > +static int wince_open(struct libusb_device_handle *handle) > +{ > + // Nothing to do to open devices as a handle to it has > + // been retrieved by wince_get_device_list > + return LIBUSB_SUCCESS; > +} What happens in the kernel driver during _get_device_list? That should be a rather easy operation, and _open should be what establishes communications with the device. It's completely possible that open will indeed be a no-op, but please confirm. > +static void wince_close(struct libusb_device_handle *handle) > +{ > + // Nothing to do as wince_open does nothing. > +} Can multiple applications perform control transfers to the same device simultaneously? What happens in the kernel driver? What happens when two devices try to claim the same device interface? > +static int wince_submit_control_transfer(struct usbi_transfer *itransfer) > +{ > + struct libusb_transfer *transfer = > USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); > + struct libusb_context *ctx = DEVICE_CTX(transfer->dev_handle->dev); > + struct wince_transfer_priv *transfer_priv = (struct > wince_transfer_priv*)usbi_transfer_get_os_priv(itransfer); > + struct wince_device_priv *priv = > _device_priv(transfer->dev_handle->dev); > + BOOL direction_in, ret; > + struct winfd wfd; > + DWORD flags; > + HANDLE eventHandle; > + > + // Split out control setup header and data buffer > + PUKW_CONTROL_HEADER setup = (PUKW_CONTROL_HEADER) transfer->buffer; > + DWORD bufLen = transfer->length - sizeof(UKW_CONTROL_HEADER); > + PVOID buf = (PVOID) &transfer->buffer[sizeof(UKW_CONTROL_HEADER)]; > + > + transfer_priv->pollable_fd = INVALID_WINFD; > + direction_in = setup->bmRequestType & LIBUSB_ENDPOINT_IN; > + flags = direction_in ? UKW_TF_IN_TRANSFER : UKW_TF_OUT_TRANSFER; > + flags |= UKW_TF_SHORT_TRANSFER_OK; > + > + eventHandle = CreateEvent(NULL, FALSE, FALSE, NULL); > + if (eventHandle == NULL) { > + usbi_err(ctx, "Failed to create event for async transfer"); > + return LIBUSB_ERROR_NO_MEM; > + } > + > + wfd = usbi_create_fd(eventHandle, direction_in ? RW_READ : RW_WRITE, > itransfer, &wince_cancel_transfer); > + if (wfd.fd < 0) { > + CloseHandle(eventHandle); > + return LIBUSB_ERROR_NO_MEM; > + } > + > + transfer_priv->pollable_fd = wfd; > + ret = UkwIssueControlTransfer(priv->dev, flags, setup, buf, bufLen, > &transfer->actual_length, wfd.overlapped); > + if (!ret) { > + int libusbErr = translate_driver_error(GetLastError()); > + usbi_err(ctx, "UkwIssueControlTransfer failed: error %d", > GetLastError()); > + wince_clear_transfer_priv(itransfer); > + return libusbErr; > + } > + usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd, direction_in ? > POLLIN : POLLOUT); > + itransfer->flags |= USBI_TRANSFER_UPDATED_FDS; > + > + return LIBUSB_SUCCESS; > +} > + > +static int wince_submit_bulk_transfer(struct usbi_transfer *itransfer) > +{ > + struct libusb_transfer *transfer = > USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); > + struct libusb_context *ctx = DEVICE_CTX(transfer->dev_handle->dev); > + struct wince_transfer_priv *transfer_priv = (struct > wince_transfer_priv*)usbi_transfer_get_os_priv(itransfer); > + struct wince_device_priv *priv = > _device_priv(transfer->dev_handle->dev); > + BOOL direction_in, ret; > + struct winfd wfd; > + DWORD flags; > + HANDLE eventHandle; > + > + transfer_priv->pollable_fd = INVALID_WINFD; > + direction_in = transfer->endpoint & LIBUSB_ENDPOINT_IN; > + flags = direction_in ? UKW_TF_IN_TRANSFER : UKW_TF_OUT_TRANSFER; > + flags |= UKW_TF_SHORT_TRANSFER_OK; > + > + eventHandle = CreateEvent(NULL, FALSE, FALSE, NULL); > + if (eventHandle == NULL) { > + usbi_err(ctx, "Failed to create event for async transfer"); > + return LIBUSB_ERROR_NO_MEM; > + } > + > + wfd = usbi_create_fd(eventHandle, direction_in ? RW_READ : RW_WRITE, > itransfer, &wince_cancel_transfer); > + if (wfd.fd < 0) { > + CloseHandle(eventHandle); > + return LIBUSB_ERROR_NO_MEM; > + } > + > + transfer_priv->pollable_fd = wfd; > + ret = UkwIssueBulkTransfer(priv->dev, flags, transfer->endpoint, > transfer->buffer, > + transfer->length, &transfer->actual_length, wfd.overlapped); > + if (!ret) { > + int libusbErr = translate_driver_error(GetLastError()); > + usbi_err(ctx, "UkwIssueBulkTransfer failed: error %d", > GetLastError()); > + wince_clear_transfer_priv(itransfer); > + return libusbErr; > + } > + usbi_add_pollfd(ctx, transfer_priv->pollable_fd.fd, direction_in ? > POLLIN : POLLOUT); > + itransfer->flags |= USBI_TRANSFER_UPDATED_FDS; > + > + return LIBUSB_SUCCESS; > +} The code for control is quite similar to that for bulk transfers. Can this really not be refactored? > +static void wince_transfer_callback(struct usbi_transfer *itransfer, > uint32_t io_result, uint32_t io_size) > +{ > + struct libusb_transfer *transfer = > USBI_TRANSFER_TO_LIBUSB_TRANSFER(itransfer); > + struct wince_transfer_priv *transfer_priv = (struct > wince_transfer_priv*)usbi_transfer_get_os_priv(itransfer); > + struct wince_device_priv *priv = > _device_priv(transfer->dev_handle->dev); > + int status; > + > + usbi_dbg("handling I/O completion with errcode %d", io_result); > + > + if (io_result == ERROR_NOT_SUPPORTED) { > + /* The WinCE USB layer (and therefore the USB Kernel Wrapper > Driver) will report > + * USB_ERROR_STALL/ERROR_NOT_SUPPORTED in situations where the > endpoint isn't actually > + * stalled. > + * > + * One example of this is that some devices will occasionally > fail to reply to an IN > + * token. The WinCE USB layer carries on with the transaction > until it is completed > + * (or cancelled) but then completes it with USB_ERROR_STALL. > + * > + * This code therefore needs to confirm that there really is a > stall error, but checking > + * the pipe status and requesting the endpoint status from the > device. > + */ That's quite invasive, and potentially a significant performance hit. :( Also, did you do extensive testing to determine that it will actually work with devices? (I know it's supposed to, but..) > + default: > + usbi_err(ITRANSFER_CTX(itransfer), "detected I/O error: %s", > windows_error_str(io_result)); Maybe include the error number, which may be easier to look up in systems programming documentation. > +const struct usbi_os_backend wince_backend = { .. > + wince_clock_gettime, > +#if defined(USBI_TIMERFD_AVAILABLE) > + NULL, > +#endif I think neither Windows nor CE has timerfd, so no need to copypaste this unneccessary check around. > +}; > \ No newline at end of file Please make sure that the last line of every file includes a newline. > +++ b/wince/config.h > @@ -0,0 +1,21 @@ > +/* config.h. Manual config for WinCE. */ > + > +#ifndef _MSC_VER > +#warn "wince/config.h shouldn't be included for your development > environment." > +#error "Please make sure the msvc/ directory is removed from your build > path." I think this check is redundant since you have not added any support for WinCE to the autotools build files. I guess that there is simply no gcc+binutils for WinCE? Also note the msvc/ copypaste error in the error message. > +++ b/wince/errno.h > @@ -0,0 +1,12 @@ > +/* Dummy file to support lack of errno.h on WinCE. */ > + > +#define EPERM 1 > +#define EINTR 4 > +#define EIO 5 > +#define EBADF 9 > +#define ENOMEM 12 > +#define EACCES 13 > +#define EBUSY 16 > +#define EINVAL 22 > + > +extern int errno; So is it actually a dummy file, or are those values being used? If they are being used then I think dummy is a misnomer, perhaps compatibility is better? And perhaps there are WinCE defines that could be used rather than the magic numbers? > +++ b/wince/inttypes.h Did this file change between Windows and WinCE? > +++ b/wince/libusb-1.0/libusb-1.0.vcproj Was this file generated completely by the SDK Visual Studio, or did you start with a file in msvc/ ? Does MSVC enforce any particular style of line endings in the file for the WinCE project? > +++ b/wince/libusb.sln > @@ -0,0 +1,114 @@ > + Is the BOM required? > +++ b/wince/signal.h > @@ -0,0 +1 @@ > +/* Dummy file to support lack of signal.h on WinCE. */ > \ No newline at end of file Why not fix the include instead? > +++ b/wince/stdint.h I guess this file is similar between Windows and WinCE. > +++ b/wince/sys/types.h > @@ -0,0 +1 @@ > +/* Dummy file to support lack of sys/types.h on WinCE. */ .. > +++ b/wince/winresrc.h > @@ -0,0 +1 @@ > +/* Dummy file to support lack of winresrc.h on WinCE. */ > \ No newline at end of file Same here, why not fix the includes? > +++ b/.gitignore .. > @@ -37,3 +39,5 @@ doc/html > Debug > Release > *.user > +*.suo > +STANDARDSDK_500 (*) What is .suo? > Subject: [PATCH 4/6] WinCE: Adding workaround for getenv() not existing on > WinCE. Please make this change in a more excellent way. > +++ b/libusb/core.c > @@ -47,6 +47,11 @@ const struct usbi_os_backend * const usbi_backend = > &wince_backend; > #error "Unsupported OS" > #endif > > +#ifdef OS_WINCE > +// Workaround for WinCE not supporting getenv > +#define getenv(x) NULL > +#endif If there is absolutely nothing in WinCE which corresponds to an environment variable then put the call within #ifdef instead. > Subject: [PATCH 5/6] WinCE: Updating time function calls to use appropriate > APIs on WinCE. Squash together with the other WinCE commits. > From: Simon Haggett <simon.hagg...@realvnc.com> > Date: Tue, 10 Jul 2012 16:07:45 +0100 > Subject: [PATCH 6/6] Windows: usbi_cond_destroy() should free handles created > by usbi_cond_intwait(). Thanks, I've applied this fix to libusb.git. //Peter ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
