So sorry for missing this, it got lost in the noise. Always feel free to bug me.
On 08/22/2014 05:30 PM, Scott Talbert wrote:
> - printf(" Serial Number: %s\n\t%s\n\t%s\n", get_serial(1),
> - get_serial(2), get_serial(3));
> + if (strlen(mh_get_serial()) != 0)
> + printf(" Serial Number: %s\n", mh_get_serial());
> + else
> + printf(" Serial Number: %s\n\t%s\n\t%s\n", get_serial(1),
> + get_serial(2), get_serial(3));
How about:
if (is_mh_remote()) {
?
> +int mh_read_file(const char *filename, uint8_t *buffer, const uint32_t
> buflen,
> + uint32_t *data_read)
Is that 2 tabs and a space? We just nuked all our tabs! I think that should be
17 spaces. :)
> int err;
> - if ((err = usb_set_configuration(h_hid, 1))) {
> - debug("Failed to set device configuration: %d (%s)", err,
> - usb_strerror());
> - return err;
> - }
Errrrr. I'm not sure 1 is always the default on every device. This change
terrifies me.
> uint8_t msg_ack[MH_MAX_PACKET_SIZE] =
> - { 0xFF, 0x03, get_seq(seq), 0x02, 0x01, param, 0x01, pkts_to_send };
> + { 0xFF, 0x03, get_seq(seq), 0x02, 0x01, param, 0x01, 0x33 };
> + if (pkts_to_send < 0x33)
> + msg_ack[7] = pkts_to_send;
I suppose it's the same, but this logic seems backwards to me. I would do
something like:
uint8_t msg_ack[MH_MAX_PACKET_SIZE] =
{ 0xFF, 0x03, get_seq(seq), 0x02, 0x01, param, 0x01, pkts_to_send };
// cannot be > 0x33
if (pkts_to_send > 0x33)
msg_ack[7] = 0x33
Oh, I see why you did it the way you did - because of the ACK logic below. OK,
I don't actually care. In theory, if we are likely to send more than 50
packets in the common case, you should keep your logic, and if that's the
less-common scenario you should switch it - but again, I don't actually care,
just thinking out-loud.
> - * MH remotes do not support SetTime() operations, but we return
> + * Some MH remotes do not support SetTime() operations, but we return
> * success because some higher level operations (for example, update
> * configuration) call SetTime() and thus the whole operation would be
> * declared a failure, which we do not want.
> */
For this patch / now, I'm fine with this, but I wonder if we should instead
expose a _is_set_time_supported function that upper-level functions can call
to determine if this is a sensible thing to do.
By doing that, when a user asks to set time we can actually say "Sorry, your
device doesn't support that.
--
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: OpenPGP digital signature
------------------------------------------------------------------------------ Download BIRT iHub F-Type - The Free Enterprise-Grade BIRT Server from Actuate! Instantly Supercharge Your Business Reports and Dashboards with Interactivity, Sharing, Native Excel Exports, App Integration & more Get technology previously reserved for billion-dollar corporations, FREE http://pubads.g.doubleclick.net/gampad/clk?id=157005751&iu=/4140/ostg.clktrk
_______________________________________________ concordance-devel mailing list concordance-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/concordance-devel
