Re: [concordance-devel] [PATCH] Support the Harmony Touch (v12)
On Fri, 3 Apr 2015, 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. Gotcha. Comment please. +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. Never saw one. 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. OK. Fuck it. ;) 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. File a wishlist for it? Fix the spacing, add the comment and I'll merge it. v13 sent. -- 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
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
Re: [concordance-devel] [PATCH] Support the Harmony Touch (v12)
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; - } 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
Re: [concordance-devel] [PATCH] Support the Harmony Touch (v12)
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; -} 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... -- 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
[concordance-devel] [PATCH] Support the Harmony Touch (v12)
Add the PID for the Touch to the list of MH PIDs. Add the Touch's Skin/Arch to the remote_info structs. Add support for handling the Touch's non-standard serial number, and provide a libconcord call to expose this information. Don't try to read the /cfg/usercfg on the Link/Touch. Remove set_configuration - doesn't seem to be needed and causes problems for Harmony Touch. Add new API methods so that MHGUI can perform a sync operation. Fix WriteFile() so that it reads and responds to the periodic acks. Implement SetTime() for the Touch. Signed-off-by: Scott Talbert s...@techie.net --- concordance/concordance.c| 7 +- libconcord/bindings/python/libconcord.py | 27 +++ libconcord/libconcord.cpp| 23 ++ libconcord/libconcord.h | 5 ++ libconcord/libusbhid.cpp | 5 -- libconcord/remote.h | 2 + libconcord/remote_info.h | 15 +++- libconcord/remote_mh.cpp | 126 +-- 8 files changed, 163 insertions(+), 47 deletions(-) diff --git a/concordance/concordance.c b/concordance/concordance.c index 4471697..6b66527 100644 --- a/concordance/concordance.c +++ b/concordance/concordance.c @@ -900,8 +900,11 @@ int print_version_info(struct options_t *options) printf( USB PID: %04X\n, get_usb_pid()); printf( USB Ver: %04X\n, get_usb_bcd()); -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)); } used = get_config_bytes_used(); diff --git a/libconcord/bindings/python/libconcord.py b/libconcord/bindings/python/libconcord.py index 718fc0e..491471a 100644 --- a/libconcord/bindings/python/libconcord.py +++ b/libconcord/bindings/python/libconcord.py @@ -973,3 +973,30 @@ mh_set_wifi_config = _create_func( _ret_lc_concord(), _in('config', POINTER(mh_wifi_config)) ) + +# const char *mh_get_serial(); +mh_get_serial = _create_func( +'mh_get_serial', +c_char_p +) + +#int mh_read_file(const char *filename, uint8_t *buffer, const uint32_t buflen, +# uint32_t *data_read); +mh_read_file = _create_func( +'mh_read_file', +_ret_lc_concord(), +_in('filename', c_char_p), +_in('buffer', POINTER(c_ubyte)), +_in('buflen', c_uint), +_out('data_read', c_uint) +) + +#int mh_write_file(const char *filename, uint8_t *buffer, +# const uint32_t buflen); +mh_write_file = _create_func( +'mh_write_file', +_ret_lc_concord(), +_in('filename', c_char_p), +_in('buffer', POINTER(c_ubyte)), +_in('buflen', c_uint) +) diff --git a/libconcord/libconcord.cpp b/libconcord/libconcord.cpp index be54ab5..c5f9121 100644 --- a/libconcord/libconcord.cpp +++ b/libconcord/libconcord.cpp @@ -229,6 +229,8 @@ int is_mh_pid(unsigned int pid) case 0xC124: /* Harmony 300 */ case 0xC125: /* Harmony 200 */ case 0xC126: /* Harmony Link */ +case 0xC129: /* Harmony Hub */ +case 0xC12B: /* Harmony Touch/Ultimate */ return 1; default: return 0; @@ -1942,6 +1944,27 @@ int mh_set_wifi_config(const struct mh_wifi_config *config) return err; } +const char *mh_get_serial() +{ +return ri.mh_serial.c_str(); +} + +int mh_read_file(const char *filename, uint8_t *buffer, const uint32_t buflen, +uint32_t *data_read) +{ +if (!is_mh_remote()) +return LC_ERROR; +return rmt-ReadFile(filename, buffer, buflen, data_read, 0x00, NULL, NULL, + 0); +} + +int mh_write_file(const char *filename, uint8_t *buffer, const uint32_t buflen) +{ +if (!is_mh_remote()) +return LC_ERROR; +return rmt-WriteFile(filename, buffer, buflen); +} + /* * PRIVATE-SHARED INTERNAL FUNCTIONS * These are functions used by the whole library but are NOT part of the API diff --git a/libconcord/libconcord.h b/libconcord/libconcord.h index 8e6f7ec..c0d2ba0 100644 --- a/libconcord/libconcord.h +++ b/libconcord/libconcord.h @@ -527,6 +527,11 @@ int mh_set_cfg_properties(const struct mh_cfg_properties *properties); int mh_get_wifi_networks(struct mh_wifi_networks *networks); int mh_get_wifi_config(struct mh_wifi_config *config); int mh_set_wifi_config(const struct mh_wifi_config *config); +const char *mh_get_serial(); +int mh_read_file(const char *filename, uint8_t *buffer, const uint32_t buflen, + uint32_t *data_read); +int mh_write_file(const char *filename, uint8_t *buffer, + const uint32_t buflen); #ifdef __cplusplus } diff --git a/libconcord/libusbhid.cpp b/libconcord/libusbhid.cpp index 915c11f..86ba786 100644 --- a/libconcord/libusbhid.cpp +++