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