Re: [concordance-devel] [PATCH] Support the Harmony Touch (v12)
No kidding! You should give him double his money back!
On March 6, 2015 4:31:15 PM CST, Scott Talbert wrote:
>On Mon, 29 Dec 2014, 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()) {
?
>>>
>>> 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;
> -}
Er. 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...
>
>Are you ever going to review this? Going 3+ months between patch
>reviews
>is unacceptable.
>
>--
>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://li
Re: [concordance-devel] [PATCH] Support the Harmony Touch (v12)
On Mon, 29 Dec 2014, 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()) {
>>>
>>> ?
>>
>> 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;
-}
>>>
>>> Er. 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...
Are you ever going to review this? Going 3+ months between patch reviews
is unacceptable.
--
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
