[chirp_devel] [ts2000] Feature complete TS-2000 driver. Fixes #217
# HG changeset patch # User Charles Stewart chuckination@gmail.com # Date 1427604940 14400 # Sun Mar 29 00:55:40 2015 -0400 # Node ID 4e970afe0a97d3d5f52165a1705c848f5e670825 # Parent ee81cedd447b16eacca70ff6e8bff3552e4a5227 [ts2000] Feature complete TS-2000 driver. Fixes #217
Debugged Tom Hayward's initial TS-2000 driver and modified kenwood_live.py to support detecting it.
Thanks!
Tom, can you take a look over this before we put it into the tree? Since it affects more than just the TS2000 driver, we should try to avoid breaking others.
--Dan
I can verify that functionality with my TM-V71A is not affected by this patch.
Charles Stewart KK4TSJ kk4tsj@gmail.com ccstewart@gmail.com
On Sun, Mar 29, 2015 at 2:55 PM, Dan Smith dsmith@danplanet.com wrote:
# HG changeset patch # User Charles Stewart chuckination@gmail.com # Date 1427604940 14400 # Sun Mar 29 00:55:40 2015 -0400 # Node ID 4e970afe0a97d3d5f52165a1705c848f5e670825 # Parent ee81cedd447b16eacca70ff6e8bff3552e4a5227 [ts2000] Feature complete TS-2000 driver. Fixes #217
Debugged Tom Hayward's initial TS-2000 driver and modified kenwood_live.py to support detecting it.
Thanks!
Tom, can you take a look over this before we put it into the tree? Since it affects more than just the TS2000 driver, we should try to avoid breaking others.
--Dan
chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
...and I have way too many email addresses. Apologies if the FROM: differences are confusing in any way.
Charles Stewart chuckination@gmail.com ccstewart@gmail.com
On Sun, Mar 29, 2015 at 2:56 PM, Charles Stewart kk4tsj@gmail.com wrote:
I can verify that functionality with my TM-V71A is not affected by this patch.
Charles Stewart KK4TSJ kk4tsj@gmail.com ccstewart@gmail.com
On Sun, Mar 29, 2015 at 2:55 PM, Dan Smith dsmith@danplanet.com wrote:
# HG changeset patch # User Charles Stewart chuckination@gmail.com # Date 1427604940 14400 # Sun Mar 29 00:55:40 2015 -0400 # Node ID 4e970afe0a97d3d5f52165a1705c848f5e670825 # Parent ee81cedd447b16eacca70ff6e8bff3552e4a5227 [ts2000] Feature complete TS-2000 driver. Fixes #217
Debugged Tom Hayward's initial TS-2000 driver and modified kenwood_live.py to support detecting it.
Thanks!
Tom, can you take a look over this before we put it into the tree? Since it affects more than just the TS2000 driver, we should try to avoid breaking others.
--Dan
chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
On Sun, Mar 29, 2015 at 11:55 AM, Dan Smith dsmith@danplanet.com wrote:
# HG changeset patch # User Charles Stewart chuckination@gmail.com # Date 1427604940 14400 # Sun Mar 29 00:55:40 2015 -0400 # Node ID 4e970afe0a97d3d5f52165a1705c848f5e670825 # Parent ee81cedd447b16eacca70ff6e8bff3552e4a5227 [ts2000] Feature complete TS-2000 driver. Fixes #217
Debugged Tom Hayward's initial TS-2000 driver and modified kenwood_live.py to support detecting it.
Thanks!
Tom, can you take a look over this before we put it into the tree? Since it affects more than just the TS2000 driver, we should try to avoid breaking others.
I'm really struggling to read code this morning, but I'll do my best.
The use of "curj" is odd. Some code from the patch: + command_delimiters = [("\r"," "), (";","")] + for i in bauds: + curj = 0 + for j in command_delimiters: + curj += 1 + LOG.info("Trying ID at baud %i with delimiter check #%i" % (i, curj))
In Python, we usually do this with enumerate() if you really need the index, like this: command_delimiters = [("\r"," "), (";","")] for i in bauds: for curj, j in enumerate(command_delimiters): LOG.info("Trying ID at baud %i with delimiter check #%i" % (i, curj))
However, it looks like you only use the integer index during logging, so why not make the logs easier to read by logging the actual delimiter: command_delimiters = [("\r"," "), (";","")] for i in bauds: for j in command_delimiters: LOG.info("Trying ID at baud %i with delimiter %s" % (i, j))
Going one more step, you could make the code easier to read with a descriptive variable name: command_delimiters = [("\r"," "), (";","")] for i in bauds: for delimiter in command_delimiters: LOG.info("Trying ID at baud %i with delimiter %s" % (i, delimiter))
You changed a couple double line-breaks between top-level functions to single line-breaks. According to PEP8, these really should be double line-breaks. Please revert this. https://www.python.org/dev/peps/pep-0008/#blank-lines
Why change __memcache to _memcache? This appears to be only a style change and not affect the behavior of the TS-2000 driver. If this is true, I'd prefer if you chose one of these alternatives: 1) Leave __memcache the way it is, or 2) Send style changes in a separate patch.
Here's the list of errors from run_all_tests.sh: chirp/drivers/ts2000.py:19:80: E501 line too long (80 > 79 characters) chirp/drivers/ts2000.py:87:80: E501 line too long (84 > 79 characters) chirp/drivers/ts2000.py:156:80: E501 line too long (80 > 79 characters) chirp/drivers/ts2000.py:201:80: E501 line too long (93 > 79 characters) chirp/drivers/ts2000.py:235:80: E501 line too long (89 > 79 characters) chirp/drivers/ts2000.py:236:80: E501 line too long (92 > 79 characters) chirp/drivers/ts2000.py:237:80: E501 line too long (90 > 79 characters) chirp/drivers/ts2000.py:278:80: E501 line too long (89 > 79 characters) chirp/drivers/ts2000.py:279:80: E501 line too long (92 > 79 characters) chirp/drivers/ts2000.py:280:80: E501 line too long (90 > 79 characters) FAIL: Please use <80 columns in source files Checking for PEP8 regressions... ./chirp/drivers/kenwood_live.py:80:1: E302 expected 2 blank lines, found 1 ./chirp/drivers/kenwood_live.py:88:32: E231 missing whitespace after ',' ./chirp/drivers/kenwood_live.py:88:43: E231 missing whitespace after ',' ./chirp/drivers/kenwood_live.py:95:80: E501 line too long (81 > 79 characters) ./chirp/drivers/kenwood_live.py:113:1: E302 expected 2 blank lines, found 1 ./chirp/drivers/kenwood_live.py:124:1: E302 expected 2 blank lines, found 1
Thanks for working on this driver! Lots of people have asked for it.
Tom KD7LXL
The __memcache to _memcache change was necessary for the TS2000Radio class to be able to access the KenwoodLiveRadio superclass's member variable for the subclass's get_memory(), set_memory(), and erase_memory() methods and cannot be reverted without impacting functioning of the driver. The double underscore is analogous to java/C++ private and the single underscore is analogous to protected, and it's part of the python black magic that appears stylistic but isn't. I really hate that particular bit of the language because it does nothing but cause problems, but there's no such thing as a language without warts.
Roger on the PEP-8 points. I printed the index of the delimiters instead of themselves since the KenwoodLive uses line breaks, but looks like I could have used repr(delim) instead and it wouldn't have trashed the logs with the carriage return. That's why I initially printed the index instead of the delimiter itself. Thanks for the tip! I'll send style changes in another patch when I have an opportunity to make the clean ups.
Charles Stewart chuckination@gmail.com ccstewart@gmail.com
On Mon, Mar 30, 2015 at 2:13 PM, Tom Hayward tom@tomh.us wrote:
On Sun, Mar 29, 2015 at 11:55 AM, Dan Smith dsmith@danplanet.com wrote:
# HG changeset patch # User Charles Stewart chuckination@gmail.com # Date 1427604940 14400 # Sun Mar 29 00:55:40 2015 -0400 # Node ID 4e970afe0a97d3d5f52165a1705c848f5e670825 # Parent ee81cedd447b16eacca70ff6e8bff3552e4a5227 [ts2000] Feature complete TS-2000 driver. Fixes #217
Debugged Tom Hayward's initial TS-2000 driver and modified kenwood_live.py to support detecting it.
Thanks!
Tom, can you take a look over this before we put it into the tree? Since it affects more than just the TS2000 driver, we should try to avoid breaking others.
I'm really struggling to read code this morning, but I'll do my best.
The use of "curj" is odd. Some code from the patch:
- command_delimiters = [("\r"," "), (";","")]
- for i in bauds:
curj = 0for j in command_delimiters:curj += 1LOG.info("Trying ID at baud %i with delimiter check #%i"% (i, curj))
In Python, we usually do this with enumerate() if you really need the index, like this: command_delimiters = [("\r"," "), (";","")] for i in bauds: for curj, j in enumerate(command_delimiters): LOG.info("Trying ID at baud %i with delimiter check #%i" % (i, curj))
However, it looks like you only use the integer index during logging, so why not make the logs easier to read by logging the actual delimiter: command_delimiters = [("\r"," "), (";","")] for i in bauds: for j in command_delimiters: LOG.info("Trying ID at baud %i with delimiter %s" % (i, j))
Going one more step, you could make the code easier to read with a descriptive variable name: command_delimiters = [("\r"," "), (";","")] for i in bauds: for delimiter in command_delimiters: LOG.info("Trying ID at baud %i with delimiter %s" % (i, delimiter))
You changed a couple double line-breaks between top-level functions to single line-breaks. According to PEP8, these really should be double line-breaks. Please revert this. https://www.python.org/dev/peps/pep-0008/#blank-lines
Why change __memcache to _memcache? This appears to be only a style change and not affect the behavior of the TS-2000 driver. If this is true, I'd prefer if you chose one of these alternatives:
- Leave __memcache the way it is, or
- Send style changes in a separate patch.
Here's the list of errors from run_all_tests.sh: chirp/drivers/ts2000.py:19:80: E501 line too long (80 > 79 characters) chirp/drivers/ts2000.py:87:80: E501 line too long (84 > 79 characters) chirp/drivers/ts2000.py:156:80: E501 line too long (80 > 79 characters) chirp/drivers/ts2000.py:201:80: E501 line too long (93 > 79 characters) chirp/drivers/ts2000.py:235:80: E501 line too long (89 > 79 characters) chirp/drivers/ts2000.py:236:80: E501 line too long (92 > 79 characters) chirp/drivers/ts2000.py:237:80: E501 line too long (90 > 79 characters) chirp/drivers/ts2000.py:278:80: E501 line too long (89 > 79 characters) chirp/drivers/ts2000.py:279:80: E501 line too long (92 > 79 characters) chirp/drivers/ts2000.py:280:80: E501 line too long (90 > 79 characters) FAIL: Please use <80 columns in source files Checking for PEP8 regressions... ./chirp/drivers/kenwood_live.py:80:1: E302 expected 2 blank lines, found 1 ./chirp/drivers/kenwood_live.py:88:32: E231 missing whitespace after ',' ./chirp/drivers/kenwood_live.py:88:43: E231 missing whitespace after ',' ./chirp/drivers/kenwood_live.py:95:80: E501 line too long (81 > 79 characters) ./chirp/drivers/kenwood_live.py:113:1: E302 expected 2 blank lines, found 1 ./chirp/drivers/kenwood_live.py:124:1: E302 expected 2 blank lines, found 1
Thanks for working on this driver! Lots of people have asked for it.
Tom KD7LXL _______________________________________________ chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
On Tue, Mar 31, 2015 at 9:26 PM, Chucks chuckination@gmail.com wrote:
The __memcache to _memcache change was necessary for the TS2000Radio class to be able to access the KenwoodLiveRadio superclass's member variable for the subclass's get_memory(), set_memory(), and erase_memory() methods and cannot be reverted without impacting functioning of the driver. The double underscore is analogous to java/C++ private and the single underscore is analogous to protected, and it's part of the python black magic that appears stylistic but isn't. I really hate that particular bit of the language because it does nothing but cause problems, but there's no such thing as a language without warts.
Okay, that makes sense. I didn't realize you were accessing this from the subclass.
P.S. It's not black magic, just name mangling. You should be able to find it at TS2000Radio._KenwoodLiveRadio__memcache, but please don't try this :-)
Tom KD7LXL
Wrapped up the PEP-8 cleanups as much as possible. There was one where it complained about under indenting, but that's nit picking to the extreme. Caught a few minor issues as well in the process, too.
Charles Stewart chuckination@gmail.com ccstewart@gmail.com
On Wed, Apr 1, 2015 at 1:14 AM, Tom Hayward tom@tomh.us wrote:
On Tue, Mar 31, 2015 at 9:26 PM, Chucks chuckination@gmail.com wrote:
The __memcache to _memcache change was necessary for the TS2000Radio
class
to be able to access the KenwoodLiveRadio superclass's member variable
for
the subclass's get_memory(), set_memory(), and erase_memory() methods and cannot be reverted without impacting functioning of the driver. The
double
underscore is analogous to java/C++ private and the single underscore is analogous to protected, and it's part of the python black magic that
appears
stylistic but isn't. I really hate that particular bit of the language because it does nothing but cause problems, but there's no such thing as
a
language without warts.
Okay, that makes sense. I didn't realize you were accessing this from the subclass.
P.S. It's not black magic, just name mangling. You should be able to find it at TS2000Radio._KenwoodLiveRadio__memcache, but please don't try this :-)
Tom KD7LXL _______________________________________________ chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
Hi Charles,
Wrapped up the PEP-8 cleanups as much as possible. There was one where it complained about under indenting, but that's nit picking to the extreme. Caught a few minor issues as well in the process, too.
Was this supposed to be a patch against your last patch? If so, I can't get this to apply cleanly against that.
Either way, until your patch makes it into the tree, you should be sending patches rebased on current tip each time. A patch against a patch requires me to manually merge them to pass the checks.
Could you maybe send a clean patch against current tip that adds the driver with everything passing the checks?
Really looking forward to having this in the tree, as I know it's desired by a lot of folks. Thanks!
--Dan
Was meant to, yes. I'll reroll an omnibus patch from tip when I get home tonight. On Apr 6, 2015 3:43 PM, "Dan Smith" dsmith@danplanet.com wrote:
Hi Charles,
Wrapped up the PEP-8 cleanups as much as possible. There was one where it complained about under indenting, but that's nit picking to the extreme. Caught a few minor issues as well in the process, too.
Was this supposed to be a patch against your last patch? If so, I can't get this to apply cleanly against that.
Either way, until your patch makes it into the tree, you should be sending patches rebased on current tip each time. A patch against a patch requires me to manually merge them to pass the checks.
Could you maybe send a clean patch against current tip that adds the driver with everything passing the checks?
Really looking forward to having this in the tree, as I know it's desired by a lot of folks. Thanks!
--Dan
chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
That should be a cleaner patch. I got rejected hunks when winding through the hg generate patches, too. Thanks for being patient with a mercurial rookie!
Charles Stewart chuckination@gmail.com ccstewart@gmail.com
On Mon, Apr 6, 2015 at 4:32 PM, Dan Smith dsmith@danplanet.com wrote:
Was meant to, yes. I'll reroll an omnibus patch from tip when I get home tonight.
Okay, thanks!
--Dan
chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
participants (4)
-
Charles Stewart -
Chucks -
Dan Smith -
Tom Hayward