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
