Add bankable Yaesu presets for FT-nD (n=1,2,3) radios.#1445
Add bankable Yaesu presets for FT-nD (n=1,2,3) radios.#1445Darieb wants to merge 11 commits intokk7ds:masterfrom
Conversation
|
Please don't close and open new PRs for one set of work. It fragments history, conversation, and review in multiple places which is very unhelpful... |
kk7ds
left a comment
There was a problem hiding this comment.
Please remove all the changes that are not necessary for the declared feature to avoid confusing the history with all the "gardening" going on here. It makes it massively difficult to review for correctness. Also, please stop using variable names like _d. The existence of them in the old code makes it doubly hard to review the new code with them to make sure we're not breaking anything.
chirp/drivers/ft1d.py
Outdated
| [" ", ] + \ | ||
| [chr(x) for x in range(ord("a"), ord("z") + 1)] + \ | ||
| list(".,:;*#_-/&()@!?^ ") + list("\x00" * 100) | ||
| A2R = ''.join(CHARSET).ljust(256, '.') |
chirp/drivers/ft1d.py
Outdated
| self.slotloc(memory.number, memory.extd_number) | ||
| # Enforce no-changes-allowed to Presets specials | ||
| if regtype == 'Presets': | ||
| raise errors.RadioError('Cannot change presets.') |
There was a problem hiding this comment.
These should have everything set to immutable to prevent this. Also, as you probably noted, this doesn't get reported in the UI because things like this have to be checked in validate_memory().
chirp/drivers/ft1d.py
Outdated
| mem.duplex = _mem[4] | ||
| mem.offset = _mem[5] | ||
| mem.comment = _mem[6] | ||
| # Can't make freq immutable, because tests try to change it |
There was a problem hiding this comment.
That means we need to teach the tests about this. The tests are there to enforce behavior not be worked around. If the behavior we want to enforce changes, then we need to teach the tests.
There was a problem hiding this comment.
OK. I've put the immutable back in.... the test_drivers.py::TestCaseEdges_... consistently fails. I suspect the test may need to check for mutability before trying to set a value? ATM, I've chosen to make all the fields available in Memory to be immutable for the presets.
driver_report.html shows:
driver_report.html
[gw3] darwin -- Python 3.14.2 /Users/declan/chirp/.tox/driver/bin/python
self = <tests.TestCaseEdges_Yaesu_FT-1D_R testMethod=test_get_set_specials>
name = 'test_get_set_specials', a = (), k = {}
@functools.wraps(attr)
def wrapper(self, name, *a, **k):
# This is a hacky super() replacement
return getattr(parents[0], name)(self, *a, **k)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
tests/init.py:43:
tests/test_edges.py:140: in test_get_set_specials
m1 = self.radio.get_memory(name)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
chirp/drivers/ft1d.py:1260: in get_memory
self._get_mem_extra(mem, False)
chirp/drivers/ft1d.py:1300: in _get_mem_extra
mem.extra = RadioSettingGroup('Extra', 'extra')
^^^^^^^^^
self = <Memory WX01(1110): freq=162550000,name='WX1PA7',vfo=0,rtone=88.5,ctone=88.5,dtcs=23,rx_dtcs=23,tmode='',cross_mode='T..., 'immutable', 'mode', 'name', 'number', 'offset', 'power', 'rtone', 'rx_dtcs', 'skip', 'tmode', 'tuning_step', 'vfo']>
name = 'extra', val = <chirp.settings.RadioSettingGroup object at 0x10e9b6b50>
def __setattr__(self, name, val):
if not hasattr(self, name):
raise ValueError("No such attribute `%s'" % name)
if name in self.immutable:
raise ImmutableValueError("Field %s is not " % name +
"mutable on this memory")
E chirp.chirp_common.ImmutableValueError: Field extra is not mutable on this memory
chirp/chirp_common.py:412: ImmutableValueError
------------------------------ Captured log call -------------------------------
DEBUG chirp.drivers.ft1d:ft1d.py:1380 New mode FM, disabling AMS
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 0.500000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 0.500000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 0.540000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 0.540000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 1.800000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 1.800000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 50.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 50.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 88.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 88.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 95.750000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 95.750000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 144.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 144.005000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 174.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 174.005000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 222.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 222.005000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 430.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 430.005000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 470.000000
DEBUG chirp.chirp_common:chirp_common.py:1798 Chose step 5.0 for 470.005000
There was a problem hiding this comment.
Making all Memory fields immutable is A Bad Thing. it gets me the open-file error mentioned in the next conversation.
There was a problem hiding this comment.
I agree that making all fields is a bad thing, but I also think Yaesu putting a bunch of immutable channels in their radios is kind of questionable :)
There are lots of drivers that mark freq as immutable, FWIW, so it's totally a legit thing to do. If there's a test failing for you that's not failing for other drivers it might just be because of other logic being used to select the test frequency/channel. If you have already pushed up changes here that have the test failing, I'll have a look where I can poke it and see about a solution.
chirp/drivers/ft1d.py
Outdated
| mem.tuning_step = STEPS[_mem.tune_step] | ||
| mem.power = self._decode_power_level(_mem) | ||
| self._get_mem_extra(mem, _mem) | ||
| _b = _mem.digmode == 1 |
There was a problem hiding this comment.
Is there some reason this change is related? If not, it should be in a separate commit (and probably a separate PR given the complexity here to make review easier). And also, please explain what it's fixing because it doesn't look like it changes anything to me.
There was a problem hiding this comment.
I had to make the _get_mem_extra different to handle the presets, which don't have a _mem structure. I would have preferred to make an empty extra field for presets
It appears that if one memory has an extra field, all of them are expected to have one. If I try to make
extra = []
only for presets, the GUI comes up, but fails without further interaction with Yaesu driver when I select a file from the Open Recent menu:
ERROR: <function ChirpMain.open_file at 0x10eb9ada0> raised unexpected exception
Traceback (most recent call last):
File "/Users/declan/chirp/chirp/wxui/common.py", line 656, in run_safe
return fn(*args, **kwargs)
File "/Users/declan/chirp/chirp/wxui/main.py", line 593, in open_file
editorset = ChirpEditorSet(radio, filename, self._editors)
File "/Users/declan/chirp/chirp/wxui/main.py", line 205, in __init__
edit.refresh()
~~~~~~~~~~~~^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 1701, in refresh
self.refresh_memory(i, lazy=True)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 1602, in refresh_memory
executor(get_cb, 'get_memory', number)
~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/common.py", line 304, in do_radio
cb(job)
~~^^^^^ERROR: <function ChirpMain.open_file at 0x10eb9ada0> raised unexpected exception
Traceback (most recent call last):
File "/Users/declan/chirp/chirp/wxui/common.py", line 656, in run_safe
return fn(*args, **kwargs)
File "/Users/declan/chirp/chirp/wxui/main.py", line 593, in open_file
editorset = ChirpEditorSet(radio, filename, self._editors)
File "/Users/declan/chirp/chirp/wxui/main.py", line 205, in __init__
edit.refresh()
~~~~~~~~~~~~^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 1701, in refresh
self.refresh_memory(i, lazy=True)
~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 1602, in refresh_memory
executor(get_cb, 'get_memory', number)
~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/common.py", line 304, in do_radio
cb(job)
~~^^^^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 1598, in get_cb
executor(extra_cb, 'get_memory_extra', job.result)
~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 1598, in get_cb
executor(extra_cb, 'get_memory_extra', job.result)
~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/common.py", line 304, in do_radio
cb(job)
~~^^^^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 1581, in extra_cb
self._refresh_memory(number, job.result, orig_mem=orig_mem)
~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 1536, in _refresh_memory
self._grid.SetCellValue(row, col, col_def.render_value(memory))
~~~~~~~~~~~~~~~~~~~~^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 245, in render_value
return self._render_value(memory, self.value(memory))
~~~~~~~~~~^^^^^^^^
File "/Users/declan/chirp/chirp/wxui/memedit.py", line 233, in value
return getattr(memory, parent)[child].value
~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^
TypeError: list indices must be integers or slices, not str
There was a problem hiding this comment.
Yes, they all have to be identical (or rather, have the same "shape", the same settings with the same schema). However, I don't see how this answer addresses my question of what is changing here. What is the _b = ... doing?
chirp/drivers/ft1d.py
Outdated
| return mem | ||
|
|
||
| def _get_mem_extra(self, mem, _mem): | ||
| def _get_mem_extra(self, mem: chirp_common.Memory, _d: bool): |
There was a problem hiding this comment.
It's called _b above (which is not a good variable name) and _d here? Please make these consistent and not obscure.
chirp/drivers/ft1d.py
Outdated
| _mem, flag, ndx, num, regtype, ename = self.slotloc(mem.number, | ||
| mem.extd_number) | ||
| if mem.empty: | ||
| def set_memory(self, memory: chirp_common.Memory) -> None: |
There was a problem hiding this comment.
Please don't change mem to memory, both "at all" but as a part of the special change. This makes every line below look changed by this patch even though it's not, which means it's very hard to tell what and why things have changed as a part of this feature.
There was a problem hiding this comment.
Pro tip here... I'm going through the diff and seeing everything you're changing to make sure it's okay. Small changes that I can easily glance at and judge as "okay" are perfect. A whole bunch of green and red that makes me go "wow that's a lot of change, what is actually changing?" is bad. Changing the name of a variable (which doesn't have any actual effect), potentially mixed with things that do have effect makes it hard to reason about. If you want to change a variable name, make that a separate PR with a justification and we can argue about it where I know all that is changing is a name.
chirp/drivers/ft1d.py
Outdated
| def _get_special_indices(self, name: str): | ||
| ''' Find type of special memory "name" and index into that memory ''' | ||
| _n = self.MAX_MEM_SLOT | ||
| for _x in SPECIALS: |
There was a problem hiding this comment.
Please choose better variable names. It also looks like you could probably iterate with a tuple to make this clearer. Like:
for memtype, channels in SPECIALS:
There was a problem hiding this comment.
Yes, you are right. It's more clear with such a tuple iteration. The reason it's not just finding the match in the dictionary is because the driver needs both the index into the specific special AND the corresponding index into the CHIRP memories. I might have tried a comprehension instead, but couldn't figure it out. I was playing with my old FORTRAN building blocks.
chirp/drivers/ft1d.py
Outdated
| _n = self.MAX_MEM_SLOT | ||
| for _x in SPECIALS: | ||
| try: | ||
| ndx = _x[1].index(name) |
There was a problem hiding this comment.
...aren't you looking for an exact match here? Why .index(name)?
Also, why are you iterating this list? Looks SPECIALS should be a dict? This whole function is just implementing SPECIALS[name], if it was...
For that matter, this would even be more obvious:
special_info = dict(SPECIALS)
return special_info[name]
chirp/drivers/ft1d.py
Outdated
| array = _x[0] | ||
| break | ||
| except Exception: | ||
| _n += len(_x[1]) |
There was a problem hiding this comment.
Catching Exception here is far too broad. This bit of obscure logic/match against two obscure non-obvious variable names with no comment is totally not going to be maintainable in the future. I'm hoping you can just convert this to a dict lookup and remove this whole function.
|
I expect the get_set_specials will fail, as describe in other comments. I think I've addressed most of(all?) your other comments. Thanks for looking at it. Most of the 'gardening' is type-hinting, which I use to help me make the correct choices in fields-to-be-sent and -returned. Also, in one case with several instance, I finally 'discovered' that you'd already built a |
|
The PR check (check_commit.sh) Failed with an error: What image? and I've only changed one driver, which works for me locally. How to diagnose and fix this, please? |
|
I've now tried asking copilot for help with the PR checks failure. It showed me how to run the ./tools/check_commit.sh origin/master by hand, but it gives me no more information than the link in "PR Checks" expansion above. From copilot explanations: I certainly did not change any of the tests/images files. Furthermore, I tried (by hand) the offending lines in It looks to me like the test command is incorrect. I think it wants to mean "if there's a changed test image and a changed python source' fail. I don't know what the existing one is doing, but it should be Running tests by hand show that the original version is always true. (No, I did not try exhaustively.) Copilot does come up with several options for me to try to fix this in my PR, but I think the script is incorrect.
The latter suggestion makes more sense to me. But how to re-submit two sets of changes as separate pull requests? |
|
OK. I was wrong about the test command. it's essentially saying if the strings But I've changed no image, so |
|
NVM the PR problem. and my rants in the previous two comments. I think I've solved the |
|
Yeah, so the test that is poking at specials just assumes it can tweak the frequency and doesn't consider the possibility of them being fixed like other tests do about other types of memories. So just adding this is fine: diff --git a/tests/test_edges.py b/tests/test_edges.py
index d41970a0..1e51f487 100644
--- a/tests/test_edges.py
+++ b/tests/test_edges.py
@@ -142,6 +142,9 @@ class TestCaseEdges(base.DriverTest):
# radios have empty in the immutable set.
if m1.empty:
m1.empty = False
+ if 'freq' in m1.immutable:
+ # Can't modify frequency, so skip rest of test
+ continue
self.assertIsInstance(m1.number, int,
('Special memory %s number %r is not an ' |
|
I did put the I also added an entry to . They're each in separate commits; the second is not necessary, but sure keeps my finally... FINALLY, I figured out to do a BTW, WRT read-only memory channels: the Yaesu scanning has a (settable) dwell time when scanning. Thus, a constantly-broadcasting station such as weather will not totally dominate the scan; it will hit the dwell time limit and go on scanning. Of course, the scan will hit the broadcast on the next go-around. |
31843b9 to
2312ce1
Compare
Yaesu FT-1D, FT2D, FT3D: adds preset receive memories to banks system.
Adds feature #10630 to Yaesu FT-1D, FT-2D, FT-3D (Add Yaesu preset receive memories to bank system)
Related to Bug #5167 (Yaesu FT2DR memory banks problem. Was CLOSED with a workaround.)
Yaesu defines some 100 read- and receive-only frequency memories for several radios (for weather, marine and SWL.)
These can only be referenced in the memory-banks system. During a memory-banks scan, the radio dwells on a signal for only a few seconds before moving on, so broadcast channels won't totally capture the scan.
The presets SHOW in CHIRP's memory tab, but are immutable. They are SELECTABLE in the banks tab.
In addition, the SKIP and PMS special memories are also now SELECTABLE in the banks tab, as they are in the radios.
[Yaesu FT-4, FT-70, FTM-3200 and FTM-5000 have weather channel(s) only. This driver does not support the presets or specials for those radios.]