[chirp_devel] [Patch][ft4] Tone and upload fixes #4787
Dan Clemmensen
Mon Feb 25 12:26:19 PST 2019
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 at intrepid.danplanet.com> wrote:
> > # 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
>
> _______________________________________________
> chirp_devel mailing list
> chirp_devel at intrepid.danplanet.com
> http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
> Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20190225/0838e86d/attachment-0001.html
More information about the chirp_devel
mailing list