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
