New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial support for Icom IC-7400 #986
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty solid other than a couple of comments. I pinged Eric to have a look (wasn't sure if you had done that). Can you also squash these into a single commit (or I can before I merge). No need to have the non-style-compliant iterations in the git history.
Thanks!
chirp/drivers/icomciv.py
Outdated
| 331, 332, 343, 346, 351, 356, 364, 365, 371, 411, 412, 413, 423, | ||
| 431, 432, 445, 446, 452, 454, 455, 462, 464, 465, 466, 503, 506, | ||
| 516, 523, 526, 532, 546, 565, 606, 612, 624, 627, 631, 632, 654, | ||
| 662, 664, 703, 712, 723, 731, 732, 734, 743, 754 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these different from chirp_common.TONES and chirp_common.DTCS_CODES?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, my values are more specific and come directly from the IC-7400 manual (I cross-referenced them on the IC-746PRO manual too, and they are the same). The defaults from chirp_common contain more values, but those are not supported by the device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, the typical way this works is to copy and modify the base list so it's more obvious what's going on. Here's an example:
https://github.com/kk7ds/chirp/blob/master/chirp/drivers/alinco.py#L237
Not required to change if you have a preference, but it generally makes it easier when someone complains that some tone is "missing when I use the XX driver". Anyone not familiar with the radio can find the list in the driver, see that tone is removed and know it's not supported.
chirp/drivers/icomciv.py
Outdated
| self._rf.has_name = True | ||
| self._rf.has_tuning_step = False | ||
| self._rf.valid_modes = [ | ||
| "LSB", "USB", "AM", "CW", "RTTY", "FM", "N/A", "CWR", "RTTYR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't actually put "N/A" in here because it breaks the UI, UX, and inter-radio compatibility. Normally I'd say put the actual list you index somewhere else and filter what you provide in RadioFeatures, but that's hard here because of how automated this is. Can we just change N/A to USB or some sensible default? If the radio should never have that mode set then we'll never see it, and actually setting USB should choose the earlier one anyway during a .index() operation. If we ever got a bad frame with this value, I think USB is a reasonable fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll update that with USB. The outcome of repeating USB sounds good!
As I'm also trying to squash the commits, I'm not aware of a way of doing that without creating a new git history in a different branch. If I create a new branch with a single commit, you will also have to handle a new PR, right? If you are ok with that and we settle for keeping my DTCS/tones I can go on and try with a new overall commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, just squash them and git push -f. Don't create a new PR :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you have trouble with that, you can just stack more in here and I'll squash before merge.
|
Suggestions implmented. The tones/dtcs codes were indeed the same as in chirp_common. Everything now squashed in one commit (obviously with a force push! how could I not think of that?). |
|
Nice, thanks. I'll merge this for the next one. Looks like there wasn't a bug for this one, so I filed it: https://www.chirpmyradio.com/issues/11246 I'll tweak your commit message to reference it (per item 5 on the checklist above) before I do. |
This should also work seamlessly for IC-746PRO, as it is using the same address for the CI-V protocol. As with other existing icomciv.py based implementations, I did not add general radio settings: this implementation only allows for setting memories. Fixes #11246
This should also work seamlessly for IC-746PRO, as it is using the same address for the CI-V protocol.
As with other existing icomciv.py based implementations, I did not add general radio settings: this implementation only allows for setting memories.
(Probably) related to #2157
I meant to mention WA1RKT for possibly trying out this on the IC-746PRO, but I'm not able to find him on GH. If this PR lands decently I can drop him an email.
CHIRP PR Checklist
The following must be true before PRs can be merged:
Fixes #1234orRelated to #1234so that the ticket system links the commit to the issue.tests/images(except for thin aliases where the driver is sufficiently tested already).Please also follow these guidelines:
six,future, etc).NEEDS_COMPAT_SERIAL=Falseand useMemoryMapBytes.tox -emakesupportedand include it in your commit.