Hi, I've updated my branch ( https://github.com/tobygray/libusbx/tree/wince) with your review comments.
The only one I didn't do was pulling some of the common code out of windows_usb.c and wince_usb.c. In merging the poll_wince.c and poll_windows.c, I removed the use of _open() and _close() in https://github.com/tobygray/libusbx/commit/a45d9665965c2ae25e22d04d064f42b061f83922 as that had already been done for the Windows CE backend. I welcome any further comments or suggestions. I'm not entirely happy with the unified version of DLL_LOAD_PREFIXED in https://github.com/tobygray/libusbx/commit/f100d8a93932b372b4a310895d33688082938ec8 as the unicode and non-unicode versions of some of the functions makes the merged version a bit more complex. Regards, Toby On 05/08/12 23:24, Pete Batard wrote: > Finally got around to have a closer look at the winCE branch. > > I don't have much to say with regards to the bulk of the code which > looks OK to me (perhaps move STATUS_HALT_FLAG from wince_usb.c to the > header?). Note that the Windows backend chose not to reference WinUSB.h > and link against WinUSB.lob because it was a headache to do so for all > of MSVC, cygwin, MinGW and WDK, and hooking directly into the DLL > avoided adding an extra dependency. But maybe this approach doesn't make > as much sense for CEUSBKWrapper, and you could avoid DLL_LOAD and use a > CEUSBKWrapper.h directly to simplify things. Still, either way is fine > with me. > > Following are my notes, by order of importance: > > * Major > - some of the fixes that went into poll_windows.c have not been applied > to poll_wince.c (eg. "InterlockedExchange((LONG *)&compat_spinlock, 0)", > malloc -> calloc). Note that this can be alleviated if we just use a > single poll_windows.c to which we add the CE changes (see below). > > * Important > - I would very much like to have the xusb sample compiled in the > project. Currently only listdevs is. The rationale is that xusb can be > quite useful when trying to troubleshoot issues... > - A diff between poll_windows.c and poll_wince.c indicates that the > differences between the two are minimal enough to consider merging the > two into a single file, especially as it would avoid the missing fixes > above. My take on doing that would be: > - exit_polling() can be sorted out with a couple of WINCE #ifdefs > - usbi_pipe() and usbi_create_fd() could also be duplicated and put > behind #ifdefs > - Another #ifdef can be set in cancel_io, usbi_close and free_index. > We can keep the business about CancelIoEx as well as the extra > attributes in _poll_fd untouched as they should be ignored on CE. > - We can probably sort out the various Sleep() vs SleepEx() in a more > efficient manner by using a macro (eg. usbi_Sleep or something) that > either resolve to Sleep(t) or SleepEx(t, TRUE) according to the > availability of SleepEx. > > Note that I don't have much of an issue taking care of the above during > integration, if you don't have a chance to address them, especially as > I'm going to merge and reorganize some of your patches anyway. The only > issue is that I can't be able to test further than compiling the code. > But I can create a CE integration branch and give you some chance to > test it, before it gets merged into mainline. > > * Minor issues (can be left for after integration) > - The wince directory uses subdirectories for the project files > (libusb-1.0, listdevs), whereas the msvc one doesn't. With these > subdirectories only containing one file, I think they are a bit > unnecessary and we probably want to follow the same approach in both > instances. Likewise, the project files just places all the .c at the > same level, whereas you have an os/ and wince/ group. The .def is also > better placed in the "Resource Files" category > - The project is only set to build a DLL. Do you think there will be > many CE users to wanting to build a static library? If so, we may want > to have both a static and dynamic lib project as with the regular > Windows backend. > - The Windows solution file places the built libraries and executables > into a /Win32 or /x64 a the top level, whereas CE places those into > /wince/STANDARDSDK_500 (ARMV4I). For the sake of consistency, we > probably want to have the CE build directory at top level. > - Using /msvc for non CE Windows is going to become a misnomer, since > MSVC/VS also applies for CE users, so maybe we should rename it. Or we > could use something /msvc_wince for CE and which should make it more > explicit that /msvc is for Windows x86_32/x86_64 (and most likely > Windows RT) > - There's some easy factorization of the wince_usb/windows_usb.h that > could be placed into a windows_common.h (The safe_... and DLL_... macros). > - Same goes for some of the calls in windows_usb.c/wince_usb.c, though > this would require some update of the autotools scripts for Windows => > better left for later. > > For now, if you just push the addon patches that you think are relevant > from the elements mentioned above, ontop of what you already have in > your branch, and I'll take it from here (i.e. no need to re-do your > existing patches). > > Regards, > > /Pete > > ------------------------------------------------------------------------------ > 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 ------------------------------------------------------------------------------ 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
