[chirp_devel] [PATCH 0 of 3] Some issues with rx_dtcs, I think
# HG changeset patch # User Dan Smith dsmith@danplanet.com # Date 1341019319 25200 # Node ID 6aff75e219ac3ce6953ac7d3d4768597a597529b # Parent e17c096f7da6e236ef3bfdd165564dda49ab043c Simplify some of the cross tone tests exceptions
I think that the existing ones were overly complicated _and_ not quite right. Specifically, checking for has_rx_dtcs is unnecessary and not appropriate, as tmode == "Cross" is really the only gating factor, and the txmode and rxmode values are what need to be checked.
If you apply this patch, along with the next two, you'll see that we have some logical inconsistencies with radios that support *->DTCS cross modes, but weren't changed to support rx_dtcs.
Marco, are these changes okay? Assuming the other drivers need to be fixed, of course...
diff -r e17c096f7da6 -r 6aff75e219ac tests/run_tests --- a/tests/run_tests Fri Jun 29 17:16:23 2012 -0700 +++ b/tests/run_tests Fri Jun 29 18:21:59 2012 -0700 @@ -179,18 +179,10 @@ ) ): continue - elif k == "dtcs" and not ( - (a.tmode == "DTCS" and not rf.has_rx_dtcs) or - (a.tmode == "Cross" and tx_mode == "DTCS") or - (a.tmode == "Cross" and rx_mode == "DTCS" and not rf.has_rx_dtcs) - ): + elif k == "dtcs" and ((a.tmode != "DTCS") or + (a.tmode == "Cross" and tx_mode != "DTCS")): continue - elif k == "rx_dtcs" and (not rf.has_rx_dtcs or - not ( - a.tmode == "DTCS" or - (a.tmode == "Cross" and rx_mode == "DTCS") - ) - ): + elif k == "rx_dtcs" and (a.tmode == "Cross" and rx_mode != "DTCS"): continue elif k == "offset" and not a.duplex: continue
I will go deep in the details tomorrow but I was confident to have written correct checks for all cases. For sure they could be written in a more concise way as I choose to write them in an explicit but easy to reread manner.
I think that first of all they should be coherent with the documentation on the site ... I will check once again ...
73 de IZ3GME Marco
On 30/06/2012 03:22, Dan Smith wrote:
# HG changeset patch # User Dan Smith dsmith@danplanet.com # Date 1341019319 25200 # Node ID 6aff75e219ac3ce6953ac7d3d4768597a597529b # Parent e17c096f7da6e236ef3bfdd165564dda49ab043c Simplify some of the cross tone tests exceptions
I think that the existing ones were overly complicated _and_ not quite right. Specifically, checking for has_rx_dtcs is unnecessary and not appropriate, as tmode == "Cross" is really the only gating factor, and the txmode and rxmode values are what need to be checked.
If you apply this patch, along with the next two, you'll see that we have some logical inconsistencies with radios that support *->DTCS cross modes, but weren't changed to support rx_dtcs.
Marco, are these changes okay? Assuming the other drivers need to be fixed, of course...
diff -r e17c096f7da6 -r 6aff75e219ac tests/run_tests --- a/tests/run_tests Fri Jun 29 17:16:23 2012 -0700 +++ b/tests/run_tests Fri Jun 29 18:21:59 2012 -0700 @@ -179,18 +179,10 @@ ) ): continue
elif k == "dtcs" and not (
(a.tmode == "DTCS" and not rf.has_rx_dtcs) or
(a.tmode == "Cross" and tx_mode == "DTCS") or
(a.tmode == "Cross" and rx_mode == "DTCS" and not rf.has_rx_dtcs)
):
elif k == "dtcs" and ((a.tmode != "DTCS") or
(a.tmode == "Cross" and tx_mode != "DTCS")): continue
elif k == "rx_dtcs" and (not rf.has_rx_dtcs or
not (
a.tmode == "DTCS" or
(a.tmode == "Cross" and rx_mode == "DTCS")
)
):
elif k == "rx_dtcs" and (a.tmode == "Cross" and rx_mode != "DTCS"): continue elif k == "offset" and not a.duplex: continue
chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
Hi Dan I checked the proposed patch starting from wiki page http://chirp.danplanet.com/projects/chirp/wiki/DevelopersToneModes and with special attention to the usage matrix I written at the end. I find that documentation looks correct and is expecially consistent in respect of tone and dtcs fields and flags usage.
Stated this, the actual patch 1 looks, to me, wrong on some usage cases (also after the corrections in patch 3).
The original code looks correct compared to the tone usage matrix, although it can be probably written in a more synthetic way. As it is it's easy to read being in positive logic and explicitly recall every use case. Btw the code is "symmetric" on tone and dtcs checks which simplify the readings; once we change the dtcs checks we should replicate to the tone checks.
I'll add some more comment in reply to patch 3 but please don't think I'm "refusing" your changes just because the original code is "mine" ;)
73 de IZ3GME Marco
On 30/06/2012 03:22, Dan Smith wrote:
# HG changeset patch # User Dan Smith dsmith@danplanet.com # Date 1341019319 25200 # Node ID 6aff75e219ac3ce6953ac7d3d4768597a597529b # Parent e17c096f7da6e236ef3bfdd165564dda49ab043c Simplify some of the cross tone tests exceptions
I think that the existing ones were overly complicated _and_ not quite right. Specifically, checking for has_rx_dtcs is unnecessary and not appropriate, as tmode == "Cross" is really the only gating factor, and the txmode and rxmode values are what need to be checked.
If you apply this patch, along with the next two, you'll see that we have some logical inconsistencies with radios that support *->DTCS cross modes, but weren't changed to support rx_dtcs.
Marco, are these changes okay? Assuming the other drivers need to be fixed, of course...
diff -r e17c096f7da6 -r 6aff75e219ac tests/run_tests --- a/tests/run_tests Fri Jun 29 17:16:23 2012 -0700 +++ b/tests/run_tests Fri Jun 29 18:21:59 2012 -0700 @@ -179,18 +179,10 @@ ) ): continue
elif k == "dtcs" and not (
(a.tmode == "DTCS" and not rf.has_rx_dtcs) or
(a.tmode == "Cross" and tx_mode == "DTCS") or
(a.tmode == "Cross" and rx_mode == "DTCS" and not rf.has_rx_dtcs)
):
elif k == "dtcs" and ((a.tmode != "DTCS") or
(a.tmode == "Cross" and tx_mode != "DTCS")): continue
elif k == "rx_dtcs" and (not rf.has_rx_dtcs or
not (
a.tmode == "DTCS" or
(a.tmode == "Cross" and rx_mode == "DTCS")
)
):
elif k == "rx_dtcs" and (a.tmode == "Cross" and rx_mode != "DTCS"): continue elif k == "offset" and not a.duplex: continue
chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
# HG changeset patch # User Dan Smith dsmith@danplanet.com # Date 1341019320 25200 # Node ID ad0be2483a7cb68e85c2ffdd597befcfcfdfca54 # Parent 6aff75e219ac3ce6953ac7d3d4768597a597529b Don't declare a bunch of cross tone modes by default
Make radio drivers do them explicitly if they support them
diff -r 6aff75e219ac -r ad0be2483a7c chirp/chirp_common.py --- a/chirp/chirp_common.py Fri Jun 29 18:21:59 2012 -0700 +++ b/chirp/chirp_common.py Fri Jun 29 18:22:00 2012 -0700 @@ -767,7 +767,7 @@ self.init("valid_name_length", 6, "The maximum number of characters in a memory's " + "alphanumeric tag") - self.init("valid_cross_modes", list(CROSS_MODES), + self.init("valid_cross_modes", [], "Supported tone cross modes") self.init("valid_dtcs_pols", ["NN", "RN", "NR", "RR"], "Supported DTCS polarities") @@ -824,7 +824,6 @@ msg = ValidationError("Cross tone mode %s not supported" % \ mem.cross_mode) msgs.append(msg) - if self.has_dtcs_polarity and \ mem.dtcs_polarity not in self.valid_dtcs_pols: msg = ValidationError("DTCS Polarity %s not supported" % \
# HG changeset patch # User Dan Smith dsmith@danplanet.com # Date 1341019320 25200 # Node ID 8a6ee39c727dc84dff1eebe2b4121ff4f17f7926 # Parent ad0be2483a7cb68e85c2ffdd597befcfcfdfca54 Add a test to look for contradictions in radio_features
diff -r ad0be2483a7c -r 8a6ee39c727d tests/run_tests --- a/tests/run_tests Fri Jun 29 18:22:00 2012 -0700 +++ b/tests/run_tests Fri Jun 29 18:22:00 2012 -0700 @@ -180,9 +180,9 @@ ): continue elif k == "dtcs" and ((a.tmode != "DTCS") or - (a.tmode == "Cross" and tx_mode != "DTCS")): + (a.tmode != "Cross" or tx_mode != "DTCS")): continue - elif k == "rx_dtcs" and (a.tmode == "Cross" and rx_mode != "DTCS"): + elif k == "rx_dtcs" and (a.tmode != "Cross" or rx_mode != "DTCS"): continue elif k == "offset" and not a.duplex: continue @@ -765,6 +765,36 @@
TESTS["Clone"] = TestCaseClone
+class TestFeatures(TestCase): + def _check_cross_modes(self): + rf = self._wrapper.do("get_features") + + if "Cross" in rf.valid_tmodes and not rf.valid_cross_modes: + raise TestFailedError("Radio claims Cross tone mode support, " + "but declares none") + if "Cross" not in rf.valid_tmodes and rf.valid_cross_modes: + raise TestFailedError("Radio claims no Cross tone mode support, " + "yet declares some") + + def _check_rx_dtcs(self): + rf = self._wrapper.do("get_features") + + for cross_mode in rf.valid_cross_modes: + if "->DTCS" in cross_mode and not rf.has_rx_dtcs: + raise TestFailedError("Radio has receive DTCS cross mode, " + "yet not has_rx_dtcs") + + def run(self): + self._check_cross_modes() + self._check_rx_dtcs() + + return [] + + def __str__(self): + return "Features" + +TESTS["Features"] = TestFeatures + class TestOutput: def __init__(self, output=None): if not output:
Hi ... here I am again :)
First of all _I like the new test!_ more, we should add also the needed check for tone features.
But according to the usage matrix the _check_rx_dtcs is wrong as the check shoud be:
if "DTCS->DTCS" in cross_mode and not rf.has_rx_dtcs:
being "DTCS->DTCS" the only case where has_rx_dtcs is mandatory.
73 de IZ3GME Marco
On 30/06/2012 03:22, Dan Smith wrote:
# HG changeset patch # User Dan Smith dsmith@danplanet.com # Date 1341019320 25200 # Node ID 8a6ee39c727dc84dff1eebe2b4121ff4f17f7926 # Parent ad0be2483a7cb68e85c2ffdd597befcfcfdfca54 Add a test to look for contradictions in radio_features
diff -r ad0be2483a7c -r 8a6ee39c727d tests/run_tests --- a/tests/run_tests Fri Jun 29 18:22:00 2012 -0700 +++ b/tests/run_tests Fri Jun 29 18:22:00 2012 -0700 @@ -180,9 +180,9 @@ ): continue elif k == "dtcs" and ((a.tmode != "DTCS") or
(a.tmode == "Cross" and tx_mode != "DTCS")):
(a.tmode != "Cross" or tx_mode != "DTCS")): continue
elif k == "rx_dtcs" and (a.tmode == "Cross" and rx_mode != "DTCS"):
elif k == "rx_dtcs" and (a.tmode != "Cross" or rx_mode != "DTCS"): continue elif k == "offset" and not a.duplex: continue
@@ -765,6 +765,36 @@
TESTS["Clone"] = TestCaseClone
+class TestFeatures(TestCase):
- def _check_cross_modes(self):
rf = self._wrapper.do("get_features")
if "Cross" in rf.valid_tmodes and not rf.valid_cross_modes:
raise TestFailedError("Radio claims Cross tone mode support, "
"but declares none")
if "Cross" not in rf.valid_tmodes and rf.valid_cross_modes:
raise TestFailedError("Radio claims no Cross tone mode support, "
"yet declares some")
- def _check_rx_dtcs(self):
rf = self._wrapper.do("get_features")
for cross_mode in rf.valid_cross_modes:
if "->DTCS" in cross_mode and not rf.has_rx_dtcs:
raise TestFailedError("Radio has receive DTCS cross mode, "
"yet not has_rx_dtcs")
- def run(self):
self._check_cross_modes()
self._check_rx_dtcs()
return []
- def __str__(self):
return "Features"
+TESTS["Features"] = TestFeatures
- class TestOutput: def __init__(self, output=None): if not output:
chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
But according to the usage matrix the _check_rx_dtcs is wrong as the check shoud be:
if "DTCS->DTCS" in cross_mode and not rf.has_rx_dtcs:
being "DTCS->DTCS" the only case where has_rx_dtcs is mandatory.
Ah, perhaps here is where the confusion is I think that any mode with ->DTCS should use rx_dtcs, not just DTCS->DTCS. Otherwise, it's hard for the user to understand why he uses "DTCS Code" for TX and "RX DTCS Code" for RX in the DTCS->DTCS case and "DTCS Code" for TX or RX in all the other cases. Similarly, I think it requires too much logic to be embedded in every driver to remember this.
Instead, it's much easier (I think) to say "if cross_mode.endswith("->DTCS"), then use rx_dtcs as the code".
I guess I didn't pay attention to the rx_dtcs changes to the wiki when you made them, but the "if it has rx_dtcs do X otherwise do Y" logic is too confusing.
What do you think?
I guess I didn't pay attention to the rx_dtcs changes to the wiki when you made them, but the "if it has rx_dtcs do X otherwise do Y" logic is too confusing.
Well ... it's exactly what we have for tones: "if radio has_ctone use X else use Y" Consistency between tones and dtcs is vital from my point of view.
From the user perspective is not that confusing: if I see only dtcs column it will be used for all if I see dtcs and rx_dtcs columns the first will be used for tx and the last for rx
Seriously, the only confusing thing I see is that we have "DTCS" mode (with a name schema similar to "Tone") and it means "DTCS SQL", if I would change something I'd rename "DTCS" to "DTCS SQL" and add a "DTCS" with the meaning of tx only.
Your move ...
73 de IZ3GME Marco
Well ... it's exactly what we have for tones: "if radio has_ctone use X else use Y" Consistency between tones and dtcs is vital from my point of view.
Well, it's not exactly the same scenario, but fair point.
Seriously, the only confusing thing I see is that we have "DTCS" mode (with a name schema similar to "Tone") and it means "DTCS SQL", if I would change something I'd rename "DTCS" to "DTCS SQL" and add a "DTCS" with the meaning of tx only.
Your move ...
Hmm, I'm just not sure. I've lost a bit of context from a few days ago when I was working on the thuvf1 driver, so I need to go back and review that, and then think on it a bit I suppose.
Hmm, I'm just not sure. I've lost a bit of context from a few days ago when I was working on the thuvf1 driver, so I need to go back and review that, and then think on it a bit I suppose.
May I suggest you to forget for a second the code and review all starting from the documentation page?
Please note as the dtcs documentation has been obtained simply swapping the pairs rtone - dtcs ctone - rx_dtcs has_ctone - has_rx_dtcs sub-audible tone - DTCS code
I should have some time to live discuss this in chat on next monday (in the morning Oregon time) if you want, just let me known ...
I promise in the we I'll try to find the time to have a look at thuvf1 implementation ;)
73 de IZ3GME Marco
participants (3)
-
Dan Smith
-
IZ3GME Marco
-
Marco IZ3GME