On 03/15/2012 05:13 PM, Scott Talbert wrote: >>> I started doing a code-review on this and then I realized it came at the >>> same >>> time as my last code review, so you haven't had a chance to incorporate >>> any of >>> that stuff in yet, so I shall hold off. >> >> Okay, I just uploaded patch v3 to the bug report. I believe v3 addresses >> all your comments so far and also includes a fix for the timeout problem >> when there is no usbnet remote plugged in. > > Okay, I just uploaded a v4 - this just addresses a few things that I > noticed from the debug output of the run on the 1100.
When I got a moment I want to move this project over to git (still on SF), and then start using reitveld for code reviews. @@ -599,15 +610,11 @@ int init_concord() if ((err = FindRemote(hid_info))) { hid_info.pid = 0; -#ifdef WIN32 if ((err = FindUsbLanRemote())) { return LC_ERROR_CONNECT; } - rmt = new CRemoteZ_TCP; -#else - return LC_ERROR_CONNECT; -#endif + rmt = new CRemoteZ_USB; } I know I said CRemoteZ_USB, but now that I see it, I think it's equally confusing to someone trying to read the code, since they're *all* USB. USBNET? - if (of) - delete of; I haven't looked, but is there a sane place to delete this? Do we want to provide a function to release it like we do with some other internal data structures? + // usbnet seems to need a reset; not sure why zwave-hid's do not + if (noreset || (is_z_remote() && !is_usbnet_remote())) Not only do that not need it, sending them the reset command (and there is one) doesn't actually *do* anything. It's really weird. + data_alloc = true; Can you add a comment here, something like: // Sometimes alloc will be used to point at a data blob inside of // a data structure (of XML) that we don't allocate here, so we need // to track whether we allocated the memory so we don't free it if we didn't // allocate it + uint8_t num_regions; /* usbnet only from here down */ Can you move that comment to be above that line of code rather than next to it? I'm not a fan of comments on the same line in general, but in this case, it applies to a block, not a given line, so it bothers me even more. - 0, // cookie + 0x1, /* hack to make config test pass */ // cookie That check is wonky. My guess was always that we check for cookie==0 to make sure it's an arch that has SOMETHING filled in. But we almost certainly shouldn't report LC_ERROR_INVALID_CONFIG there, more like LC_ERROR_UNSUPPORTED. Not that that should go in this patch, but I'm just bitching outloud here. I don't see any change in here to handle the problem with resets on zwave-hid remotes, did I miss something? -- Phil Dibowitz p...@ipom.com Open Source software and tech docs Insanity Palace of Metallica http://www.phildev.net/ http://www.ipom.com/ "Be who you are and say what you feel, because those who mind don't matter and those who matter don't mind." - Dr. Seuss
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ This SF email is sponsosred by: Try Windows Azure free for 90 days Click Here http://p.sf.net/sfu/sfd2d-msazure
_______________________________________________ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel