[chirp_devel] [Patch][ft4] Tone and upload fixes #4787

[Dan Smith]

> # HG changeset patch
> # User DanClemmensen <DanClemmensen at gmail.com>
> # Date 1550894107 28800
> #      Fri Feb 22 19:55:07 2019 -0800
> # Node ID 41efaf746855eaa768603b5d29ee2d0d29ac4a5d
> # Parent  2513b6da29c39aa92e8df3c7468949d479e8a984
> bug fixes for ft4.py.
> 1) 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.

> 2) radio version is now checked on upload in addition to download
> 3) version check is more tolerant of different version formats
> 4) 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':
> +            break
> +        else:
> +            response += b
> +            i += 1
> +            if 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