On Tue, 5 Feb 2013, Phil Dibowitz wrote:
> The MH patch:
>
> if ((err = rmt->UpdateConfig(of->GetDataSize(), of->GetData(),
> cb, cb_arg, cb_stage)))
> return LC_ERROR_WRITE;
> + } else if (is_mh_remote()) {
> + if ((err = rmt->UpdateConfig(of->GetDataSize(), of->GetData(),
> + cb, cb_arg, cb_stage, of->GetXmlSize(),
> + of->GetXml())))
> + return LC_ERROR_WRITE;
>
> You changed the virtual's signature, so can't this be combined into one
> section of if?
Yep, done.
> + for (int i=0; i<5; i++) {
> + if ((err = HID_WriteReport(msg_one))) {
> + debug("Failed to write to remote");
> + return 1;
> + }
> + }
>
> Why do we write msg 1 5 times?
Because that's what the Logitech sw does and it seems to be required if I
recall correctly. Yes, it's weird. :-) I added a comment.
> + while(!(err = HID_ReadReport(rsp))) {
> + // Ignore 1st two bits on 2nd type for length.
> + int len = rsp[1] & 0x3F;
>
> You mean "1st two bits on 2nd byte" ?
Yep, disconnect between brain and keyboard.
> + // Skip 1st two bytes, read up to packet length.
> + for (int i=2; i<len+2; i++) {
>
> Do you mean len-2?
No, the first two bytes are not included in "len" so we read a full "len"
of bytes from 2 -> len+2. Comment added to clarify.
> +int CRemoteMH::SetTime(const TRemoteInfo &ri, const THarmonyTime &ht,
> + lc_callback cb, void *cb_arg, uint32_t cb_stage)
> +{
> + return 0;
> +}
>
> Why isn't this LC_ERROR_UNSUPP?
I think it was because SetTime() gets called by some of the higher level
operations (update config, firwmare) and if it returns non-success, it
causes the higher level operations to report failure. I'll add a comment.
> + if (pkt_count == 50) {
> + if ((err = HID_ReadReport(rsp, MH_TIMEOUT))) {
> + debug("Failed to read from remote");
> + return 1;
> + }
> + print_packet(rsp);
>
> print_packet calls debug, so it'll get compiled out, but it's not obvious to
> the reader. Can we call it debug_print_packet or something?
Yep, makes sense. Done.
> + /* 2nd parameter is the number of packets remaining,
> + plus one */
> + if (pkts_to_send < 50)
> + ack_rsp[7] = pkts_to_send + 1;
>
> And if it's more than 50? You don't fill it in at all?
No, it's initialized to 0x33 before the loop:
uint8_t ack_rsp[MH_MAX_PACKET_SIZE] =
{ 0xFF, 0x03, 0x00, 0x02, 0x01, 0x05, 0x01, 0x33 };
> + const uint8_t finish_msg[MH_MAX_PACKET_SIZE] = {
>
> Jesus, these guys aren't playing around...
What do you mean?
> + debug("msg_5");
> + print_packet(rsp);
> +
> + cb(LC_CB_STAGE_FINALIZE_UPDATE, cb_count++, 3, 4,
> + LC_CB_COUNTER_TYPE_STEPS, arg);
> +
> + /* write msg 7 */
>
> Huh? what happened to 6?
In this particular case, "msg 7" refers to the message having 0x07 as the
second byte. Unfortunately I was not very creative in naming messages.
Updated patch attached to bug report.
------------------------------------------------------------------------------
Free Next-Gen Firewall Hardware Offer
Buy your Sophos next-gen firewall before the end March 2013
and get the hardware for free! Learn more.
http://p.sf.net/sfu/sophos-d2d-feb
_______________________________________________
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
