Skip to content
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

IC-R6 Initial Driver #1082

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

IC-R6 Initial Driver #1082

wants to merge 9 commits into from

Conversation

jbradsha
Copy link

Icom IC-R6 Initial version

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. New drivers and radio models will affect the Python3 test matrix. You should regenerate this file with tox -emakesupported and include it in your commit.
  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
Copy link
Owner

kk7ds commented Jul 28, 2024

Your commit is titled something that won't make sense for people when it gets sent to the mailing list. Please change that to something like "Add Icom IC-R6 driver" per item 5 above. Also, please include a test image per item 6.

Thanks!

Copy link
Owner

@kk7ds kk7ds left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume based on the mailing list thread that you've figured this out, but your test image needs to go in tests/images :)

Thanks for all your work on this!

chirp/drivers/icr6.py Outdated Show resolved Hide resolved
u8 freq0; // Freq: low byte
u8 freq1; // Freq: mid byte
u8 freq_flags:6, // Related to multiplier (step size)
freq2:2; // Freq: high bits - 18 bits total = step count
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd really prefer that you use a u24 here instead of all the bit-shifting. I think you can just mask out the flag bits yourself once you grab the value. I'll do some thinking on how I could make this define-able in the bitwise here...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the to-do list, I need to figure out the endians, radio uses both.

unknown4e:1,
tmode:3; // TSQL/DTCS Setting: Index to "TONE_MODES"
u8 offset_l; // Offset - low value byte
u8 offset_h; // Offset - high value byte
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change this to a u16 please? The point of bitwise is to minimize the assembly and masking of bits in the code as much as possible.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check the endian, the radio has both types (see canceller_freq for example)

ctone:6; // TSQL Index. Valid range: 0 to 49
u8 unknown8;
u8 canceller_freq_h; // Canceller training freq index - high 8 bits
u8 canceller_freq_l:1, // - LSB
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just make this a u16 and then you won't need to combine freq_h with freq_l in your code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check the endian, the radio has both types (see offset for example)

chirp/drivers/icr6.py Outdated Show resolved Hide resolved
chirp/drivers/icr6.py Outdated Show resolved Hide resolved
flow = 1.0 * (_pses[kx].start3 * 65536 * 256 +
_pses[kx].start2 * 65536 +
_pses[kx].start1 * 256 +
_pses[kx].start0) / 3e6
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This almost certainly means you should be using a u32. Why not?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to figure out the endian, kept on the "to do" list for now.

chirp/drivers/icr6.py Outdated Show resolved Hide resolved
chirp/drivers/icr6.py Outdated Show resolved Hide resolved
chirp/drivers/icr6.py Outdated Show resolved Hide resolved
@jbradsha jbradsha closed this Aug 10, 2024
@jbradsha
Copy link
Author

Closed - will create a new PR once most of the above is fixed (some queries around endian - radio uses both big and small endian in different places).

@kk7ds
Copy link
Owner

kk7ds commented Aug 10, 2024

Please don't open a new one, just re-open this and push to it. That keeps the discussion history in tact and makes it easier to catch back up when you have another version.

Thanks!

@jbradsha
Copy link
Author

Thanks Dan. Will upload fresh copy shortly.. just running it through tox again. Changed not all done (the different endians still need looked at. Not sure why Icom even did that, even if they used legacy firmware from another IC-Rx model would have expected it to be consistent) and the warp_bytes for names not working as expected so not added to the PR code.

@jbradsha jbradsha reopened this Aug 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants