> 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.
Yeah, it seems that CVS is kinda behind the times these days. :-) > + 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? Done, in v5. > - 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? I added a function so it can be deleted and I added an invocation in concordance when it is doing other cleanup. > + // 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. Odd, so do HID ZWAVE remotes automatically reset after you upload a config? Or how do you get them to take the new config otherwise? > + 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 I already added a comment very similar to this in the destructor. Do you think we need more than that? > + 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. Yep, done in v5. > - 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. Yeah, probably something that could be improved. The zwave remotes don't really use any of that stuff anyway. BTW, I know you wanted to move to that other bug report, but I seem to be unable to attach files to it. In the interim, I've posted v5 of the patch here: http://www.techie.net/~talbert/harmony_900_support_v5.patch Scott ------------------------------------------------------------------------------ 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
