On 12/07/12 19:31, Peter Stuge wrote: > 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.
Thank you for spending time to provide your feedback. I've been lurking on the libusb-devel list from before libusbx branched so I'm familiar with the history of the two projects and how that relates to the individuals involved. > 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. Fair enough. I was going for bite-sized chunks for the common code changes with the intention that it would be easier to discuss individual changes. >> This backend requires the Windows CE Standard 5.00 SDK. > * Why? - I mean: What parts of the SDK are required? I'm not quite sure what question you are asking here. The SDK provides OS header files and libraries for the code to be compiled against; without the SDK there wouldn't be any anything for the compiler and linker to compile the source code against. I think I possibly should have expanded this statement considerably more. The information I was trying to convey was: Currently the WinCE project files target the "STANDARDSDK_500" platforms. These are provided by the Windows CE Standard 5.00 SDK. You should be able to compile libusb for WinCE by installing the SDK at the provided link. As far as I can tell WinCE 5 was the last WinCE release which had a standard SDK from Microsoft. If you want to compile for a more recent WinCE release then you will need to talk to the company providing the hardware or baseport for the SDK for your platform. Once you've got this SDK you'll need to add a new platform to the WinCE solution for your platform of interest. As the WinCE libusbx port doesn't need any features which are only available in versions beyond 5.0 it seemed sensible to target STANDARDSDK_500 as it's a platform that anyone can build for by installing the standard SDK from Microsoft. On a slight tangent: the CEUSBKWrapper driver currently only supports the memory model in Windows CE 6, but as the model in Windows CE 5 is simpler it shouldn't be too hard to change the UserBuffer class to support Windows CE 5 if someone wants to. > * 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? I've been using STANDARDSDK_500 with VS 2005, but running on a WinCE 6.0 platform. I believe that the SDK comes with appropriate C compilers, although they are in C:\Program Files\Microsoft Visual Studio 8\vc\ce\bin\x86_arm so it's hard to tell. Do you have a particular alternative compiler that you want to build WinCE binaries on? I'd be happy to help you to work out how to get it building on that. Can you send a link to it? Or were you just wondering about how to compile this? > >> 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. Drivers in the WinCE kernel are dynamically loaded and linked, sitting around as (kernel) dlls inside \windows waiting to be loaded. My reading of the LGPL is that it is acceptable to dynamically link to non-LGPL code. This also seems to be compatible with the license for WinCE. However if you do see a problem then can you say what you think it is? Is there an alternative license you can suggest that we could consider for CEUSBKWrapper? We obviously don't want to release code under an open source license which is incompatible with the licensing terms of the software it is intended to interact with. >> 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? Implementing UkwIssueInterruptTransfer and UkwIssueIsochronousTransfer :). I imagine UkwIssueInterruptTransfer will have lots of common code with the bulk transfer. Currently an attempt to issue and interrupt transfer will go via the bulk transfer code, which might just work as expected, we've not tested this. I have no idea about UkwIssueIsochronousTransfer as I've never done any USB development which has needed to use isochronous transfers directly. > > > 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. I've updated it to use a registry setting, see my other post. >> 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? I can't think of any use for the libusb API if you don't have the ukw driver. Is failing if no driver is available the behavior of all the other backends? > > >> --- >> 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. When I started the WinCE port I wasn't sure how close it would be to the Windows backend so I just copied and pasted so that I could hack away at the code as I pleased. I was also thinking that it would be more likely to be accepted as an experimental feature if the majority of code changes were outside common code. I can appreciate that a maintainer taking the approach of "make it perfect first time" is an valid viewpoint. However my personality is more that I would prefer to not have the risk of changing all the code of a pre-existing feature to add a new, unproven, feature. I would view that as something to be done later once it's been shown to be working and stable. That might just be because I don't trust myself to write code which works first time :) > > 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. One is two years old, one is only a few months old. As a user of libusbx on Windows I'd be hesitant about upgrading to a new release where I knew that the previously working Windows backend had been ripped apart and put back together with the backend from another OS by a couple of developers who were new to the libusb code structure. >> 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 ? I started work on this WinCE backend before libusbx was forked. At the time the desktop windows build had a single solution file called libusb.sln. Why are the currently solution files for libusb on windows not called libusb-1.0_2005.sln and libusb-1.0_2010.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. The empty header files are, again, to simplify the changes to common code. I felt it would make reviewing the changes to common code harder than necessary if the majority of changes was just putting #ifndef OS_WINCE blocks around various header file includes. But thinking about it some more I can just do this change in a separate git commit. I'll update my branch appropriately. > > >> +++ 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? It returns a list of handles to the devices it knows about. > 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. What communication with the device should _open be performing? My reading of the documentation for open inside the backend structure is that is shouldn't communicate with the device: * This function should not generate any bus I/O and should not block. >> +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? I believe so. We've certainly tested it with different libusb contexts within the same process. > What happens in the kernel driver? It will issue multiple control transfer requests. The USB driver inside WinCE then queues up these transfer requests, completing each one asynchronously when it completes. > What happens when two devices try to claim the same device interface? Assuming you mean two processes trying to claim the same device interface; then they are both currently allowed to. If another WinCE driver has claimed the interface, such as the mass storage driver, then no libusb processes will be able to claim the 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? Possibly. I'll have a look and see if it looks any neater with them sharing whatever code they can. > > >> +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..) This does seem to work for the device which fail to reply to an IN token in time and for devices which genuinely stall. However I do agree that it is an ugly hack. If I get the time I do want to look into seeing if it's possible to patch the WinCE USB layer to return an error code other than USB_ERROR_STALL. That will probably break other USB drivers running on the same system so it isn't a trivial change to do. > > >> + 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. windows_error_str() does include the error code value at the start of the string it returns: safe_sprintf(wErr_string, ERR_BUFFER_SIZE, _T("[%d] "), error_code); > > >> +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. Good point :) > > >> +}; >> \ No newline at end of file > Please make sure that the last line of every file includes a newline. Sorry, I should have checked that before submitting. > > >> +++ 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. Good point. I'll adjust the error message to one which describes the actual problem of attempting to compile on a non-Microsoft compiler not being supported. > > >> +++ 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? You are correct that it's not actually a dummy file, I'll adjust that comment. As errno isn't a WinCE concept this is really just present for compatibility. As such the defines are just reflecting the standard POSIX values. I'll add an appropriate comment. > > >> +++ b/wince/inttypes.h > Did this file change between Windows and WinCE? No. I'll sort this out with the dummy header files. > > >> +++ 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/ ? I think it's generated completely by Visual Studio, is it just the line endings which are a bit wrong? > > Does MSVC enforce any particular style of line endings in the file > for the WinCE project? I don't know. I'll find out and make it match the other vcproj files as necessary. > > >> +++ b/wince/libusb.sln >> @@ -0,0 +1,114 @@ >> + > Is the BOM required? The tool creating the file seemed to think it was. I'm not sure there is any value in removing it. Does it break something on other systems? > > >> +++ 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? See my comment above about avoid adding lots of #ifndef OS_WINCE to common code. > > >> +++ b/wince/stdint.h > I guess this file is similar between Windows and WinCE. I think it's identical. > > >> +++ 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? .suo files are "Solution User Options": http://msdn.microsoft.com/en-us/library/bb165909(v=vs.80).aspx <http://msdn.microsoft.com/en-us/library/bb165909%28v=vs.80%29.aspx> >> Subject: [PATCH 4/6] WinCE: Adding workaround for getenv() not existing on >> WinCE. > Please make this change in a more excellent way. I have attempted to increase the excellence of this change by at least 74% in the following commit :) https://github.com/tobygray/libusbx/commit/4b0f2413ae85d197db8721e2fda5a85355d95e1b > > >> +++ 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. > Excellent. Thanks for your review comments. Regards, Toby ------------------------------------------------------------------------------ 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
