-
Notifications
You must be signed in to change notification settings - Fork 199
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
base: master
Are you sure you want to change the base?
IC-R6 Initial Driver #1082
Conversation
Icom IC-R6 Initial version
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! |
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.
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!
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 |
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.
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...
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.
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 |
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.
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.
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.
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 |
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.
Just make this a u16 and then you won't need to combine freq_h
with freq_l
in your code.
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.
I need to check the endian, the radio has both types (see offset for example)
flow = 1.0 * (_pses[kx].start3 * 65536 * 256 + | ||
_pses[kx].start2 * 65536 + | ||
_pses[kx].start1 * 256 + | ||
_pses[kx].start0) / 3e6 |
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.
This almost certainly means you should be using a u32. Why not?
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.
Need to figure out the endian, kept on the "to do" list for now.
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). |
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! |
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. |
Icom IC-R6
Use updated icf.warp_byte_size() instead of bit-level operations
Add files via upload
Icom IC-R6 Initial version
CHIRP PR Guidelines
The following must be true before PRs can be merged:
Fixes #1234
orRelated to #1234
so that the ticket system links the commit to the issue.tests/images
(except for thin aliases where the driver is sufficiently tested already). All new drivers must useMemoryMapBytes
. New drivers and radio models will affect the Python3 test matrix. You should regenerate this file withtox -emakesupported
and include it in your commit.six
,future
, etc).