Code review from v5 first, then replies below that.

+void delete_file()
+{
+ if (of)
+   delete of;
+}

Lets call that something like delete_opfile_obj or delete_operations_file -
the other one (delete_blob) is generic because you pass in various types of
blobs you may get back from the API.

+ /* In certain places, the data pointer is used to point to an area
+    inside of the xml memory.  In those cases, we don't want to delete
+    it.  Use the data_alloc variable to keep track of when we've actually
+    allocated unique memory to it, and in those cases, delete it.
+ */
+ if (data && data_alloc)

Please use the same comment style used everywhere else.

-   hid.mfg = "Logitech";
-   hid.prod = "Harmony 1000";
-   ri.flash_mfg = 0; // todo
-   ri.flash_id = 0;  // todo
+   ri.flash_mfg = 0x01; // Seems to be the same as regular zwave
+   ri.flash_id = 0x49;  // Seems to be the same as regular zwave
  } else {        // Not 1000
    ri.flash_mfg = 0x01;// todo
    ri.flash_id = 0x49; // todo

If they're the same, pull them out of the if-else, please.

+ if (IsUSBNet()) {
+   hid.mfg = ri.model->mfg;
+   hid.prod = ri.model->model;
+ }
+

Is this the same as the if hid.pid == 0 check above? Why not just do that
where it was being done before, in that if? I'm not opposed to changing that
conditional to IsUSBNet() though.

> 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?

Neither - nothing appears to happen, but you unplug the USB and it's just
using the new config. I don't know what's really happening under the hood.

> I already added a comment very similar to this in the destructor.  Do you
> think we need more than that?

Ah, that's fine.

> 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

Stupid SF tracker. I re-opened that bug as a code-review bug.

Testing:

885  (hid): works (minor delay on reboot, but that's OK)
895  (hybrid): works
1100 (usbnet): getinfo, gettime, settime all work[1]

[1] well getinfo worked, then I unplugged it, and thought I should try get/set
time, plugged it in and the usb subsystem crashed and I had to powercycle.
After reboot, I plugged it in, and it still thought it had it's IP and
wouldn't DHCP. I unplugged it, messed around with the menus, re-plugged it in
and everything worked as expected.

I have a feeling that config update will work with little or no tweaking -
I've emailed Ernst, the awesome guy who donated his old 1100 and asked for a
config update for his current one that I can use against this one.

-- 
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


Attachment: 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

Reply via email to