On Sat, 10 Mar 2012, Phil Dibowitz wrote: >> - printf(" Hardware Version: %i.%i\n", get_hw_ver_maj(), >> - get_hw_ver_min()); >> + printf(" Hardware Version: %i.%i.%i\n", get_hw_ver_maj(), >> + get_hw_ver_min(), get_hw_ver_mic()); > > > This seems unrelated - are you just doing this for... future compatibilit= > y?
Since I exposed the micro version that the 900 appears to have, I went ahead and exposed that through concordance. It isn't 100% necessary, though. I think this was my only change to concordance. If you'd prefer, I could leave this out and my changes would be isolated to libconcord. >> -#ifdef WIN32 >> if ((err =3D FindUsbLanRemote())) { >> return LC_ERROR_CONNECT; >> } > > I seem to remember this hanging horribly on non usblan remotes... This only gets called when a non-usblan remote cannot be found, so it won't come into play normally. It will get called (probably) when a non-usblan remote is rebooting (e.g., after a config update) but I think it only hangs for 5 seconds until the TCP connect attempt times out. >> @@ -635,8 +641,10 @@ int init_concord() >> int deinit_concord() >> { >> ShutdownUSB(); >> - if (of) >> - delete of; >> + // Do not delete the OperationFile here. It is used again in >> + // concordance after deinit_concord() is called. >> + //if (of) >> + //delete of; > > We do? For what? For figuring out how to make the final contact to the website, for example, in an update config. Here's an example: 1. Write new config to remote. 2. Reset remote (calls deinit_concord()). 3. Contact website to tell it that everything's good. Fails because the operation file has been deleted. >> @@ -113,6 +113,7 @@ int OperationFile::ReadZipFile(char *fil >> } else { >> data_size =3D dirent.st_size; >> data =3D new uint8_t[data_size]; >> + data_alloc =3D true; > > What is this? OperationFile was causing a glibc memory free error in its destructor because, in some cases, the "data" pointer is being set to point to something inside the xml structure. Basically, it ended up trying to free memory that it didn't allocate. I put this in to make sure it only tried to free memory it allocated. >> @@ -117,6 +118,13 @@ struct TRemoteInfo { >> bool valid_config; >> uint32_t config_bytes_used; >> uint32_t max_config_size; >> + uint8_t num_regions; >> + uint8_t *region_ids; >> + char **region_versions; > > I know what these are because I was trying to consider how we'd handle > multiple regions, but... > >> + uint32_t home_id; >> + uint8_t node_id; >> + char *tid; >> + char *xml_user_rf_setting; > > what are these? I'm not really entirely sure. But the Harmony Software sends them to the website, so I figured we should probably add them. I don't think they are technically required, though. The connectivity test and update configuration function seemed to work fine before I added them. >> public: >> CRemoteZ_TCP() {}; >> virtual ~CRemoteZ_TCP() {}; >> + int UpdateConfig(const uint32_t len, const uint8_t *wr, >> + lc_callback cb, void *cb_arg, uint32_t cb_stage); >> + int GetTime(const TRemoteInfo &ri, THarmonyTime &ht); >> + int SetTime(const TRemoteInfo &ri, const THarmonyTime &ht, >> + lc_callback cb=3DNULL, void *cb_arg=3DNULL, uint32_t >> cb_stage=3DNULL= > ); >> + int IsUSBNet() {return true;} > > I'm starting to feel like the CRemoteZ_TCP class should be renamed CRemot= > eZ_USB. Heh, probably. :) >> int CRemoteZ_TCP::Write(uint8_t typ, uint8_t cmd, uint32_t len, >> uint8_t *data) >> { >> - if (len > 60) { >> + if (len > 1033) { > > Explanation? The usblan remotes don't seem to be packet-size limited like the regular zwave remotes are. 1033 seems to be the largest packet that we currently need to send (the COMMAND_WRITE_UPDATE_DATA can send up to a 1024 byte data packet, plus the various header bytes, equals 1033). >> +int CRemoteZ_TCP::TCPSendAndCheck(uint8_t cmd, uint32_t len, uint8_t *= > data) >> +{ > > I'm wondering if the TCPSendAndCheck can be abstracted into the > CRemoteZ_Base. They seam nearly identical, the only issues I see is that = > the > default Write for HID is UDPWrite and so we force TCPWrite but the USB ca= > se > uses it's default (and only) Write() function. Also, you don't check for = > the > ACK - should you? Well, there isn't really an ACK for us to check since for usblan remotes, there isn't really an ACK. Unlike the regular zwave remotes that seem to have the two types of messages "UDP" and "TCP", the usblan remotes seems to have just one type, I guess because you already have a real TCP connection with the remote, there was no need to try and fake it out. The main problem with combining regular zwave stuff with usblan stuff is that the response byte ordering differs, so we would have to do different checks for each. It could be done, but may not be worth the effort? >> + /* write data */ >> + debug("UPDATE_DATA"); >> + uint32_t pkt_len; >> + int tlen =3D len; >> + int bytes_written =3D 0; >> + uint8_t *wr_ptr =3D const_cast<uint8_t*>(wr); >> + uint8_t tmp_pkt[1033]; >> + tmp_pkt[0] =3D 0x03; // 3 parameters >> + tmp_pkt[1] =3D 0x01; // 1st parameter, 1 byte >> + tmp_pkt[2] =3D REGION_USER_CONFIG; >> + tmp_pkt[3] =3D 0xC2; // 2nd parameter, 1024 bytes >> + tmp_pkt[1028] =3D 0x04; // 3rd parameter, 4 bytes > > This all still assumes we only need to write to one region, if I read it > correctly (which is probably true for the 900, but I'm fairly certain not= > true > for most zwave/usb remotes). Well, the existing regular zwave code only writes to region 4 "USER CONFIG" as well. There isn't any code for updating firmware or anything like that. >> @@ -428,10 +660,10 @@ int CRemoteZ_Base::GetIdentity(TRemoteIn >> const unsigned int hw =3D GetWord(pl.p[7]); >> ri.hw_ver_major =3D hw >> 8; // ??? >> ri.hw_ver_minor =3D (hw >> 4) & 0x0F; // ??? >> - // hw_ver_micro =3D hw & 0x0F // ??? >> + ri.hw_ver_micro =3D hw & 0x0F; // ??? > > Oh, *that's* why you did that. Heh. > >> //ri.hw_ver_major =3D hw >> 4; >> //ri.hw_ver_minor =3D hw & 0x0F; > > I wonder what's up with this. Dunno, I left that as I found it. :) >> + >> +int GetXMLUserRFSetting(char **data) > > Can you explain how this stuff goes together? Either in the docs/ directo= > ry or > at as part of the patch description or in comments, or some combination t= > hereof? Yes. This is another area where the Harmony software was grabbing some data and I figured we should probably grab that data, too, so we can send it to the website. I don't know if it is actually required, though. Scott ------------------------------------------------------------------------------ Virtualization & Cloud Management Using Capacity Planning Cloud computing makes use of virtualization - but cloud computing also focuses on allowing computing to be delivered as a service. http://www.accelacomm.com/jaw/sfnl/114/51521223/ _______________________________________________ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel