icf520: Add support for Icom IC-F520, IC-F521, IC-F620, and IC-F621

[N7XSD]

Fixes #11490
Fixes #6505

CHIRP PR Guidelines

The following must be true before PRs can be merged:

1. All tests must be passing. The "PR Checks" job is speculative and failure doesn't always indicate a critial problem, but generally it needs to pass as well.
2. Commits should be rebased (or simply rebase-able in the web UI) on current master. Do not put merge commits in a PR.
3. Commits in a single PR should be related. Squash intermediate commits into logical units (i.e. "fix tests" commits need not survive on their own). Keep cleanup commits separate from functional changes.
4. Major new features or bug fixes should reference a CHIRP issue in the commit message. Do this with the pattern Fixes #1234 or Related to #1234 so that the ticket system links the commit to the issue.
5. Please write a reasonable commit message, especially if making some change that isn't totally obvious (such as adding a new model, adding a feature, etc). The first line of every commit is emailed to the users' list after each build. It should be short, but meaningful for regular users (examples: "thd74: Fixed tone decoding" or "uv5r: Added settings support").
6. New drivers should be accompanied by a test image in tests/images (except for thin aliases where the driver is sufficiently tested already). All new drivers must use MemoryMapBytes.
7. All files must be GPLv3 licensed or contain no license verbiage. No additional restrictions can be placed on the usage (i.e. such as noncommercial).
8. Do not add new py2-compatibility code (No new uses of six, future, etc).

[kk7ds]

I don't have time for an in-depth review of this, but here are a few things from a skim. Thanks! I think I have an F621 somewhere, I might be able to try this.

[kk7ds]

Can you do this in sync_in() or maybe in process_mmap()? Doing it as part of save feels wrong. If this is needed for your code to work, doesn't this require a save and re-open for this to take effect anyway?

Also, doing it in process_mmap() will mean that it runs on load, so that if someone opens a newer image with an older chirp someday, that chirp will flatten things the way it needs.

Also-also, you may or may not want to implement this kind of radio as banks. The bank model and editor really doesn't work the way it needs to for these kind of commercial radios with dynamic zones or tables. In the TK-8180 driver (which has a similar dynamic split) I decided to make it expose sub-devices for each of the zones, which I think works better.

[N7XSD]

Yes. I moved the code to process_mmap().

I'll have a look at the TK-8180 driver. For now, treating the radio as having a single bank works.

I still hope to write a bank model... someday.

[kk7ds]

Please don't do it this way. This ALIASES= mechanism is from long ago, before we had the metadata trailer on the files and thus couldn't tell these apart. Now these should just be registered subclasses.

Also, if these really all use the same model info, is there some other way we can tell the difference between them? From the look of your code I can download any of these radios using any of the aliases. We should try to thwart that, even if we have to do something nasty like checking in process_mmap() and raising an error.

I'm guessing that also means that valid_bands covers all the union of all the variants instead of properly enforcing the limits for a given variant, right? That should be properly set on each subclass and enforced so you can't put VHF channels into a UHF radio, etc.

[N7XSD]

I'll remove the ALIASES= thing.

Frankly, I don't know if they use the same model info. There are a lot of bits sent by the radio I haven't mapped out.

decode_model() from icf.py looks useful but the IC-F520, etc. have a different header (?) size. With only two radios (IC-F521 and IC-F621-2), I'm not confident I could create a reliable test.

Yes, valid_bands covers the union of all the variants. (Icom's CS-F500 software doesn't even do that much checking.) I'm reluctant to base model checking on the frequencies in a download since a radio mis-programmed by CS-F500 could leave a radio un-programable by chirp. For a while I was using MODEL = "IC-F500 series". But decided to see what would happen with aliases (which I didn't know were deprecated.)

When one of these radios is programmed with a bad frequency, the display flashes when that channel is selected. The rest of the channels are still usable. Not much of an error message but it's something.

I'll spend some more time looking at the header.

[kk7ds]

Frankly, I don't know if they use the same model info. There are a lot of bits sent by the radio I haven't mapped out.

In that case let's please leave the others unavailable (we can put a module in an issue so someone can test and let us know). People get super mad when they see a radio is "supported", but it, and find out that it's never been tested :)

decode_model() from icf.py looks useful but the IC-F520, etc. have a different header (?) size. With only two radios (IC-F521 and IC-F621-2), I'm not confident I could create a reliable test.

Okay, but those two use the same model_info? There must be other bits in the image that tell the software which actual model it is.

Yes, valid_bands covers the union of all the variants. (Icom's CS-F500 software doesn't even do that much checking.) I'm reluctant to base model checking on the frequencies in a download since a radio mis-programmed by CS-F500 could leave a radio un-programable by chirp. For a while I was using MODEL = "IC-F500 series". But decided to see what would happen with aliases (which I didn't know were deprecated.)

When one of these radios is programmed with a bad frequency, the display flashes when that channel is selected. The rest of the channels are still usable. Not much of an error message but it's something.

Okay, but CHIRP works differently from the factory software and we should have one model per variant with a consistent set of parameters. Even if the radio "handles" invalid frequencies, CHIRP should try its best to mirror what the radio actually supports.

[N7XSD]

Got it! I'll drop all references to should work variants and find a way to distinguish between the IC-F521 and the IC-F621-2.

Thanks for the explanation. This helps me a lot.

[kk7ds]

LMK when I should look at this again. It's got a bunch of unsquashed commits and merges and it's not based on current master so I assume you're still working on it :)

[N7XSD]

Still not getting git and github. I'll probably just delete everything (except icf520.py) and create a fresh copy of the repository... again.

Is there a work flow you recommend to keep me out of trouble in the future?

[kk7ds]

You're doing fine, you just have to learn how to get git to bend to your will. I can squash and rebase it for you if it's a struggle. Should I look at the code again as it stands now? If so, I can try to do that tomorrow.

[N7XSD]

Yes, please. I decided to limit the driver to the IC-F621-2. I can come back to the IC-F521 later.

The Real World is taking up a lot of my time lately. This way, I can contribute something useful instead of letting it sit all lonesome on my hard disk. :-)

[kk7ds]

This is okay enough to include now. I'm going to snip out the extra log message and rebase/squash as mentioned. Thanks!

[kk7ds]

This is different from inhibit_tx above? Kenwood uses "lockout" to mean "skip during scan".. same here maybe?

[kk7ds]

Might want to look at RadioSettingValueMap which works like the list variant, but lets you just provide a mapping of values to choices and it handles the rest.

[kk7ds]

Also check out MemSetting which takes a symbolic path into the memory object and can set boolean, integer, string, and map types directly so you don't have to manually do everything in set_settings() yourself.