Are you 100% sure you used a clean source tree? These are the only
differences between v11 and v12:
< + case 0xC129: /* Harmony Ultimate Hub */
---
+ case 0xC129: /* Harmony Hub */
< + { MFG_HAR, "Harmony Ultimate Hub", NULL },
---
+ { MFG_HAR, "Harmony Hub", NULL },
In any event, you don't really need the changes in v12 as they are not
functional. The changes I'm looking for you to test are in congruity.
Scott
On Sun, 11 Jan 2015, Cédric de Launois wrote:
Hi Scott,
I tried to apply your patch v12 but got two rejects (using a fresh source
clone). See files attached.
Patch v11 applies correctly.
Regards,
Cedric
2014-12-30 1:25 GMT+01:00 Scott Talbert <s...@techie.net>:
On Sat, 29 Nov 2014, Scott Talbert wrote:
> On Thu, 27 Nov 2014, Phil Dibowitz 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()) {
>>
>> ?
>
> Can't, because not all MH remotes use this weird serial number
format.
> The earlier ones use the more traditional one.
>
>>> +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. :)
>
> Ooops. I don't know how that snuck in there. I'll fix in the
next
> version.
>
>>> 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.
>
> Heh. I figured you might ask about this. So, for one thing,
this only
> affects the libusbhid code (which the person who was testing
the Harmony
> Touch happened to be using). HIDAPI doesn't ever call Set
Configuration
> and we seem to be working fine there. If it would make you
feel better,
> maybe we can make a Get Configuration call first and then Set
> Configuration if it isn't 1. Unfortunately, for the Touch
(and probably
> also at least the Link), calling Set Configuration when the
configuration
> is already set to 1 causes the device to reset.
>
>>> 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.
>
> Eh, it's hard to say whether we'll commonly send more or less
than 50
> packets. I think probably in this use case, we'll send less,
but I was
> thinking about another patch down the road to refactor the
UpdateConfig
> code to use this code, and I think in that case, we'd probably
be sending
>> 50 packets most of the time. So, eh, I think I'll just leave
this
> as-is.
>
>>> - * 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.
>
> Yeah, that's probably not a bad idea for the future.
Ping! It's been a month...
---------------------------------------------------------------------------
---
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media,
is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more.
Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
------------------------------------------------------------------------------
Dive into the World of Parallel Programming! The Go Parallel Website,
sponsored by Intel and developed in partnership with Slashdot Media, is your
hub for all things parallel software development, from weekly thought
leadership blogs to news, videos, case studies, tutorials and more. Take a
look and join the conversation now. http://goparallel.sourceforge.net
_______________________________________________
concordance-devel mailing list
concordance-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/concordance-devel
