[chirp_devel] [Patch][ft4] Tone and upload fixes #4787
# HG changeset patch # User DanClemmensen DanClemmensen@gmail.com # Date 1550894107 28800 # Fri Feb 22 19:55:07 2019 -0800 # Node ID 41efaf746855eaa768603b5d29ee2d0d29ac4a5d # Parent 2513b6da29c39aa92e8df3c7468949d479e8a984 bug fixes for ft4.py.
- upload to radio now works (tip and py3)
Hmm, did it not before? Or only on one? I certainly assumed that it worked for the default branch when I applied it initially.
- radio version is now checked on upload in addition to download
- version check is more tolerant of different version formats
- Tone handling is now correct and tested
The bug reference needs to be in the patch itself, not just in the email subject. If you use patchbomb then these will be the same automatically.
Also, this is a lot of change for a single patch, and even beyond that, there is a ton of "gardening" in here which makes it super hard to figure out what _functional_ changes you're making. If we had to bisect a regression and landed on this patch it would be hard to tell what in here caused the problem. The whole point of mq is to make it easy to maintain a stack of patches, and patchbomb can send a whole stack in one operation.
//miscellaneous params. One 4-block group. (could be treated as 4 separate.) -//"SMI": "Set Mode Index" of the radio keypad function used to set a parameter. +//"SMI": "Set Mode Index" of the FT-4 radio keypad function to set parameter. +//"SMI numbers on the FT-65 are difrferent but the names an mem are the same.
You're adding a misspelled "different" in this comment :)
+def variable_len_resp(pipe):
- """
- when length of expected reply is not known, read byte at a time
- until the ack character is found.
- """
- response = b""
- i = 0
- toolong = 256 # arbitrary
- while True:
b = pipe.read(1)if b == b'\x06':breakelse:response += bi += 1if i > toolong:LOG.debug("Response too long. got" + util.hexprint(response))raise errors.RadioError("Response too long.")
This is not a very useful error for the user to see, and people will open bugs with this as the content. Maybe this would be better as something like "Radio sent too much data while reading command response" ? Also, remember that the UI is displaying this to a user, so asserting that this is the end of a sentence may not be valid :)
Also, I think you likely want to LOG.debug() the command you sent so you know what caused the long response right?
- if id_response != expected_id:
if id_response[0:8] != expected_id[0:8]:msg = "ID mismatch. Expected" + util.hexprint(expected_id)msg += ", Received:" + util.hexprint(id_response)LOG.debug(msg)raise errors.RadioError("Incorrect ID.")
Same here, I think this should probably be a teensy bit more wordy, just so it's clear that you're talking to the radio but got back an identifier you didn't expect.
@@ -401,12 +444,53 @@ chirp_common.PowerLevel("Mid", watts=2.5), chirp_common.PowerLevel("Low", watts=0.5)] STEPS = [0, 5.0, 6.25, 10.0, 12.5, 15.0, 20.0, 25.0, 50.0, 100.0] -TONE_MODES = ["", "Tone", "TSQL", "DTCS", "DTCS-R", "TSQL-R", "Cross"] -CROSS_MODES = ["DTCS->", "DTCS->DTCS"] # only the extras we need -# The radio and the code support the additional cross modes, but -# they are redundant with the extended tone modes, and they cause -# the "BruteForce" unit test to fail. -# CROSS_MODES += ["Tone->Tone", "->DTCS", "->Tone", "DTCS->DTCS", "Tone->"] +# Yaesu sql_type field codes +SQL_TYPE = ["off", "R-TONE", "T-TONE", "TSQL", "REV-TN", "DCS", "PAGER"]
+# map a CHIRP tone mode to a FT-4 sql and which if any code to set to 0. +# The keys are provided to RadioFeatures as a list +MODES_TONE = {
- "": ("off", None),
- "Tone": ("T-TONE", None), # chirp means "xmit, not rcv"
- "TSQL": ("TSQL", None), # chirp means "xmit and rcv, x==r"
- "DTCS": ("DCS", None), # chirp means "xmt and rcv
- "TSQL-R": ("REV-TN", None), # not documented. reverse R-Tone
None of these have a value for the second item in the tuple. Am I missing why this is there or maybe left-over from previous debugging? Can we just make the values of this dict be the first item in the tuples?
- "Cross": () # not used in lookup
This should be removed I think, especially since it is a different-sized tuple than the others and likely to surprise the caller just as much as a KeyError.
+# map a CHIRP Cross type if the CHIRP sql type is "cross". The keys +# are provided to RadioFeatures as a list. +MODES_CROSS = {
- "DTCS->": ("DCS", "rx_dcs"),
- "->DTCS": ("DCS", "tx_dcs"),
- "DTCS->DTCS": ("DCS", None),
- "->Tone": ("R-TONE", None),
This cross-mode means "require a tone on receive but don't send one". R-TONE for the yaesu means the opposite, right?
+# Map the radio image tone/dtcs settings back to the CHIRP settings +RADIO_TMODES = [
("(None)", ["", ""]), # sql_type= 0. off("(None)", ["Cross", "->Tone"]), # sql_type= 1. R-TONE("(None)", ["Tone", ""]), # sql_type= 2. T-TONE("(None)", None, "tx_ctcss", "rx_ctcss", [ # sql_type= 3. TSQL["", None], # x==0, r==0 : not valid["TSQL", ""], # x==0["Tone", ""], # r==0["Cross", "Tone->Tone"], # x!=r["TSQL", ""] # x==r]),("(None)", ["TSQL-R", ""]), # sql_type= 4. REV TN("(None)", None, "tx_dcs", "rx_dcs", [ # sql_type= 5.DCS["", None], # x==0, r==0 : not valid["Cross", "->DTCS"], # x==0["Cross", "DTCS->"], # r==0["Cross", "DTCS->DTCS"], # x!=r["DTCS", ""] # x==r]),("PAGER", ["", None]) # sql_type= 6. handled as a CHIRP "extra"]
It would be nice if you could define one data structure to translate forward and back and just use that, as this seems fairly duplicative and error-prone (i.e. this has to exactly represent the two forward maps above). Since you've already had to edit these manually to correct bugs I'm concerned about the maintainability of this in the future :)
So, I know this is a lot to split apart, but I'd definitely prefer it if you could do that. If not, removing any gardening from the patch that you can in order to keep it as on-topic as possible would be good, and include the bug reference in the patch itself. Maybe it would be easyish to split the tone fixes away from the clone changes since they're in different parts of the patch?
Thanks for following up with the users on the bug and working to get the tone decoding right!
--Dan
I knew this patch was too big. I was hoping you would give me one freebie before I started in with actual maintenance-style patches. :-)
I'll try to be good from now on.
Some of the data structure issues you see are in fact leftovers, and I have already made changes. When I originally coded the tone stuff, I had convinced myself that there was no way to create a true two-way mapping. In fact there is not (because the UI cannot handle the values "none" for the tones, but the radio requires this value). However, I now know that the unit tests will forgive you if you fail to return the same tone the UI supplies in those circumstances. I will re-work the code.
About those multiple patches: Is it acceptable to send a patch and then send a second patch (and third...) before the first one becomes part of the daily build?
OK. I will start from what is in your current repository (i.e., the initial ft4.py) and make isolated incremental changes that cumulatively will reach my current working file. Patches: 1) whitespace, spelling, other non-code changes 2) re-arrangement 3) through N): incremental fixups, each addressing a specific issue.
For the py3 upload: I only realized that I had failed to test this after I submitted the initial build. I had tested it earlier before is consolidated my py2 and py3 code, and I overlooked the fact that the unit tests don't test the serial code, which of course is blatantly obvious in retrospect. As penance, I'm going to write that emulator. As you said earlier, emulators cannot do a valid timing test for those drivers that have timing sensitivities, but this particular protocol does not have a timing issue.
About patchbomb: I tried to set up to send patches directly to your smtp server by following your directions, but my system refused because it could not negotiate a compatible secure protocol.
On Mon, Feb 25, 2019 at 11:55 AM Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
# HG changeset patch # User DanClemmensen DanClemmensen@gmail.com # Date 1550894107 28800 # Fri Feb 22 19:55:07 2019 -0800 # Node ID 41efaf746855eaa768603b5d29ee2d0d29ac4a5d # Parent 2513b6da29c39aa92e8df3c7468949d479e8a984 bug fixes for ft4.py.
- upload to radio now works (tip and py3)
Hmm, did it not before? Or only on one? I certainly assumed that it worked for the default branch when I applied it initially.
- radio version is now checked on upload in addition to download
- version check is more tolerant of different version formats
- Tone handling is now correct and tested
The bug reference needs to be in the patch itself, not just in the email subject. If you use patchbomb then these will be the same automatically.
Also, this is a lot of change for a single patch, and even beyond that, there is a ton of "gardening" in here which makes it super hard to figure out what _functional_ changes you're making. If we had to bisect a regression and landed on this patch it would be hard to tell what in here caused the problem. The whole point of mq is to make it easy to maintain a stack of patches, and patchbomb can send a whole stack in one operation.
//miscellaneous params. One 4-block group. (could be treated as 4
separate.)
-//"SMI": "Set Mode Index" of the radio keypad function used to set a
parameter.
+//"SMI": "Set Mode Index" of the FT-4 radio keypad function to set
parameter.
+//"SMI numbers on the FT-65 are difrferent but the names an mem are the
same.
You're adding a misspelled "different" in this comment :)
+def variable_len_resp(pipe):
- """
- when length of expected reply is not known, read byte at a time
- until the ack character is found.
- """
- response = b""
- i = 0
- toolong = 256 # arbitrary
- while True:
b = pipe.read(1)if b == b'\x06':breakelse:response += bi += 1if i > toolong:LOG.debug("Response too long. got" +util.hexprint(response))
raise errors.RadioError("Response too long.")This is not a very useful error for the user to see, and people will open bugs with this as the content. Maybe this would be better as something like "Radio sent too much data while reading command response" ? Also, remember that the UI is displaying this to a user, so asserting that this is the end of a sentence may not be valid :)
Also, I think you likely want to LOG.debug() the command you sent so you know what caused the long response right?
- if id_response != expected_id:
if id_response[0:8] != expected_id[0:8]:msg = "ID mismatch. Expected" + util.hexprint(expected_id)msg += ", Received:" + util.hexprint(id_response)LOG.debug(msg)raise errors.RadioError("Incorrect ID.")Same here, I think this should probably be a teensy bit more wordy, just so it's clear that you're talking to the radio but got back an identifier you didn't expect.
@@ -401,12 +444,53 @@ chirp_common.PowerLevel("Mid", watts=2.5), chirp_common.PowerLevel("Low", watts=0.5)] STEPS = [0, 5.0, 6.25, 10.0, 12.5, 15.0, 20.0, 25.0, 50.0, 100.0] -TONE_MODES = ["", "Tone", "TSQL", "DTCS", "DTCS-R", "TSQL-R",
"Cross"]
-CROSS_MODES = ["DTCS->", "DTCS->DTCS"] # only the extras we need -# The radio and the code support the additional cross modes, but -# they are redundant with the extended tone modes, and they cause -# the "BruteForce" unit test to fail. -# CROSS_MODES += ["Tone->Tone", "->DTCS", "->Tone", "DTCS->DTCS",
"Tone->"]
+# Yaesu sql_type field codes +SQL_TYPE = ["off", "R-TONE", "T-TONE", "TSQL", "REV-TN", "DCS",
"PAGER"]
+# map a CHIRP tone mode to a FT-4 sql and which if any code to set to
+# The keys are provided to RadioFeatures as a list +MODES_TONE = {
- "": ("off", None),
- "Tone": ("T-TONE", None), # chirp means "xmit, not rcv"
- "TSQL": ("TSQL", None), # chirp means "xmit and rcv,
x==r"
- "DTCS": ("DCS", None), # chirp means "xmt and rcv
- "TSQL-R": ("REV-TN", None), # not documented. reverse R-Tone
None of these have a value for the second item in the tuple. Am I missing why this is there or maybe left-over from previous debugging? Can we just make the values of this dict be the first item in the tuples?
- "Cross": () # not used in lookup
This should be removed I think, especially since it is a different-sized tuple than the others and likely to surprise the caller just as much as a KeyError.
+# map a CHIRP Cross type if the CHIRP sql type is "cross". The keys +# are provided to RadioFeatures as a list. +MODES_CROSS = {
- "DTCS->": ("DCS", "rx_dcs"),
- "->DTCS": ("DCS", "tx_dcs"),
- "DTCS->DTCS": ("DCS", None),
- "->Tone": ("R-TONE", None),
This cross-mode means "require a tone on receive but don't send one". R-TONE for the yaesu means the opposite, right?
+# Map the radio image tone/dtcs settings back to the CHIRP settings +RADIO_TMODES = [
("(None)", ["", ""]), # sql_type= 0. off("(None)", ["Cross", "->Tone"]), # sql_type= 1. R-TONE("(None)", ["Tone", ""]), # sql_type= 2. T-TONE("(None)", None, "tx_ctcss", "rx_ctcss", [ # sql_type= 3.TSQL
["", None], # x==0, r==0 : not valid["TSQL", ""], # x==0["Tone", ""], # r==0["Cross", "Tone->Tone"], # x!=r["TSQL", ""] # x==r]),("(None)", ["TSQL-R", ""]), # sql_type= 4. REV TN("(None)", None, "tx_dcs", "rx_dcs", [ # sql_type= 5.DCS["", None], # x==0, r==0 : not valid["Cross", "->DTCS"], # x==0["Cross", "DTCS->"], # r==0["Cross", "DTCS->DTCS"], # x!=r["DTCS", ""] # x==r]),("PAGER", ["", None]) # sql_type= 6. handled as a CHIRP "extra"]It would be nice if you could define one data structure to translate forward and back and just use that, as this seems fairly duplicative and error-prone (i.e. this has to exactly represent the two forward maps above). Since you've already had to edit these manually to correct bugs I'm concerned about the maintainability of this in the future :)
So, I know this is a lot to split apart, but I'd definitely prefer it if you could do that. If not, removing any gardening from the patch that you can in order to keep it as on-topic as possible would be good, and include the bug reference in the patch itself. Maybe it would be easyish to split the tone fixes away from the clone changes since they're in different parts of the patch?
Thanks for following up with the users on the bug and working to get the tone decoding right!
--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
I knew this patch was too big. I was hoping you would give me one freebie before I started in with actual maintenance-style patches. :-)
I'll try to be good from now on.
Heh, thanks :)
Some of the data structure issues you see are in fact leftovers, and I have already made changes. When I originally coded the tone stuff, I had convinced myself that there was no way to create a true two-way mapping. In fact there is not (because the UI cannot handle the values "none" for the tones, but the radio requires this value). However, I now know that the unit tests will forgive you if you fail to return the same tone the UI supplies in those circumstances. I will re-work the code.
Okay, if the re-work comes after a big last-freebie version that's okay, I would just like it to be cleaned up in the tree at some point before we lose context, you stop paying attention to it daily, etc.
About those multiple patches: Is it acceptable to send a patch and then send a second patch (and third...) before the first one becomes part of the daily build?
Yeah, if you send them as a stack with patchbomb, it's clear that they're supposed to be applied in a specific order. If you don't, then just make sure to call out the order somehow. Queueing up multiple is fine, but I'd prefer if they weren't like "change thing, okay nevermind change it like this, okay that was dumb, now it works" kind of thing. If they really build on each other, then by all means.
OK. I will start from what is in your current repository (i.e., the initial ft4.py) and make isolated incremental changes that cumulatively will reach my current working file. Patches:
- whitespace, spelling, other non-code changes
- re-arrangement
- through N): incremental fixups, each addressing a specific issue.
This would be great thanks. So just to be clear you're doing that for this patch or just in the future?
For the py3 upload: I only realized that I had failed to test this after I submitted the initial build. I had tested it earlier before is consolidated my py2 and py3 code, and I overlooked the fact that the unit tests don't test the serial code, which of course is blatantly obvious in retrospect.
That's fine. I was worried maybe it worked _only_ on py2. It's not a requirement to be working on py3 yet, so that's totally cool.
As penance, I'm going to write that emulator. As you said earlier, emulators cannot do a valid timing test for those drivers that have timing sensitivities, but this particular protocol does not have a timing issue.
Cool, I wrote one for icf-based drivers here in the py3 tree if you want to look at it:
https://chirp.danplanet.com/projects/chirp/repository/revisions/py3/entry/te...
It's not beautiful, but it's something. I haven't ported all the test cleanups from the py3 branch over to default yet, so maybe just doing that emulator in a patch for py3 for now would be good and we'll get it in default when we merge?
About patchbomb: I tried to set up to send patches directly to your smtp server by following your directions, but my system refused because it could not negotiate a compatible secure protocol.
Hmm, okay, it worked for me last time I tried it. Maybe contact me off-list with the details of the failure and we can try to figure out if the hangup is work-around-able?
Thanks!
--Dan
- OK. I will start from what is in your current repository (i.e., the initial ft4.py) and make isolated incremental changes that cumulatively will reach my current working file.
*>*> Patches: *>*> 1) whitespace, spelling, other non-code changes *>*> 2) re-arrangement *>
*> 3) through N): incremental fixups, each addressing a specific issue.>This would be great thanks. So just to be clear you're doing that for this patch or just in the future?*
Sigh. I guess I'd better start from the very first one: the "patch" (i.e. the whole file) named add_ft4. You should therefore ignore my most recent (i.e, only other) patch.
This will take awhile, so I may submit some intermediates one at a time. If I get ambitious I will submit multiple patches in a set. So, my proposed methodology is to pull a completely fresh repository, then begin a cycle of creating and soft-committing incremental patches, and then producing the multi-patch single submittal. Is it acceptable to continue to use issue #4787 at least until I catch up?
On Mon, Feb 25, 2019 at 12:40 PM Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
That's fine. I was worried maybe it worked _only_ on py2. It's not a
requirement to be working on py3 yet, so that's totally cool.
Re-reading myself, I meant "only on py3" above :)
--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
- through N): incremental fixups, each addressing a specific issue.
This would be great thanks. So just to be clear you're doing that for this patch or just in the future?
Sigh. I guess I'd better start from the very first one: the "patch" (i.e. the whole file) named add_ft4. You should therefore ignore my most recent (i.e, only other) patch. This will take awhile, so I may submit some intermediates one at a time. If I get ambitious I will submit multiple patches in a set. So, my proposed methodology is to pull a completely fresh repository, then begin a cycle of creating and soft-committing incremental patches, and then producing the multi-patch single submittal.
Okay, well, like I said, if you want to split this one into just maybe two pieces, even that would help.
Sorry for being a stickler, but really appreciate you spending a little time making it cleaner. Whoever has to read history and support this driver in five years (even if it's you) will appreciate it too :)
Is it acceptable to continue to use issue #4787 at least until I catch up?
Yep, there doesn't need to be a 1:1 issue:patch ratio :)
--Dan
Dan, you MUST be the "stickler". You have kept this project going for many years, and somebody must provide the guidance for such a project or it will begin to bitrot. I even knew this big patch was a bad idea. It won't hurt me to do it right.
On Mon, Feb 25, 2019 at 1:38 PM Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
- through N): incremental fixups, each addressing a specific issue.
This would be great thanks. So just to be clear you're doing that for
this patch or just in the future?
Sigh. I guess I'd better start from the very first one: the "patch"
(i.e. the whole file) named add_ft4. You should therefore ignore my most recent (i.e, only other) patch.
This will take awhile, so I may submit some intermediates one at a time.
If I get ambitious I will submit multiple patches in a set. So, my proposed
methodology is to pull a completely fresh repository, then begin a cycle
of creating and soft-committing incremental patches, and then producing the
multi-patch single submittal.
Okay, well, like I said, if you want to split this one into just maybe two pieces, even that would help.
Sorry for being a stickler, but really appreciate you spending a little time making it cleaner. Whoever has to read history and support this driver in five years (even if it's you) will appreciate it too :)
Is it acceptable to continue to use issue #4787 at least until I catch
up?
Yep, there doesn't need to be a 1:1 issue:patch ratio :)
--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 (2)
-
Dan Clemmensen -
Dan Smith