On Wed, Jul 21, 2010 at 07:38:15PM -0600, Stephen Warren wrote: > Attached is a patch to support the Harmony 700.
OK, this patch was smaller than I expected. It all looks good - just a few minor things I'd like to see fixed. > Note: The new prep_config/finish_config functions are technically > required to match the Windows software. However, during my testing, I > forgot to update congruity to call those functions, and everything > worked as expected. Should I just rip those new functions out? That > would significantly reduce the size of the patch, and remove the need to > release a new congruity version too. I have a suspicion we'll need this ability anyway, so lets keep them. Perhaps it's worth adding a comment in the code to this effect though? > +int prep_config() > +{ > + int err = 0; > + > + if (ri.architecture != 14) { > + return 0; > + } > + > + if ((err = rmt->PrepConfig())) { > + return LC_ERROR; > + } > + > + return 0; > +} When we prep/finish firmware we do the calls directly in libconcord.cpp. Ultimately what you do in CRemote::<whatever> is just low level stuff, and then we build on those here. So I'd probably say it's best to put that stuff here... However, I don't feel strongly, and if you want to push it down into the CRemote object, then, I'd ask you to make the {prep,finish}_firmware match. > @@ -117,7 +117,7 @@ int CRemote::GetIdentity(TRemoteInfo &ri > ri.architecture = rx_len < 6 ? 2 : rsp[5] >> 4; > ri.fw_type = rx_len < 6 ? 0 : rsp[5] & 0x0F; > ri.skin = rx_len < 6 ? 2 : rsp[6]; > - ri.protocol = rx_len < 7 ? 0 : rsp[7]; > + ri.protocol = rx_len < 7 ? 0 : ((rx_len < 8) ? rsp[7] : > ri.architecture); > ::wince:: Can you please break this up into a series of if's? The double-tertiary operator is just evil. > - if ((err = (ri.architecture == 2) > - // The old 745 stores the serial number in EEPROM > - ? ReadMiscByte(FLASH_EEPROM_ADDR, FLASH_SIZE, > - COMMAND_MISC_EEPROM, rsp) > - // All newer models store it in Flash > - : ReadFlash(FLASH_SERIAL_ADDR, 48, rsp, ri.protocol))) { > + switch (ri.arch->serial_location) { > + case SERIAL_LOCATION_EEPROM: > + err = ReadMiscByte(ri.arch->serial_address, SERIAL_SIZE, > + COMMAND_MISC_EEPROM, rsp); > + break; > + case SERIAL_LOCATION_FLASH: > + err = ReadFlash(ri.arch->serial_address, SERIAL_SIZE, rsp, > + ri.protocol); > + break; > + default: > + debug("Invalid TArchInfo\n"); > + return LC_ERROR_READ; > + } > + if (err) { Nice :) > -static const TArchInfo ArchList[11]={ +10 for reformatting this. -- 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: Digital signature
------------------------------------------------------------------------------ This SF.net email is sponsored by Sprint What will you do first with EVO, the first 4G phone? Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel