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

Reply via email to