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.

Sign up for GitHub

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

Jump to bottom

Q10h test #796

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Q10h test #796

wants to merge 2 commits into from

Conversation

mrtjr1
Copy link
Contributor

@mrtjr1 mrtjr1 commented Oct 27, 2023

CHIRP PR Checklist

The following must be true before PRs can be merged:

  • All tests must be passing.
  • Commits should be squashed into logical units.
  • Commits should be rebased (or simply rebase-able in the web UI) on current master. Do not put merge commits in a PR.
  • Commits in a single PR should be related.
  • Major new features or bug fixes should reference a CHIRP issue.
  • New drivers should be accompanied by a test image in tests/images (except for thin aliases where the driver is sufficiently tested already).
  • 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).

Please also follow these guidelines:

  • Keep cleanups in separate commits from functional changes.
  • 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).
  • Do not add new py2-compatibility code (No new uses of six, future, etc).
  • All new drivers should set NEEDS_COMPAT_SERIAL=False and 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.

kk7ds
kk7ds requested changes Oct 27, 2023
View reviewed changes
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 have some other concerns about some of this driver, but I'll hold off until we resolve this am_mode conversation.

Note that it would be much easier on me if you would submit a basic driver with just the cloning and trivial memory stuff implemented and then iterate from there to expand the functionality over time. Asking me to review 3K lines of code in one sitting is a big task, and it also means you don't get any feedback about what you're doing until you've done all that work, which also makes me feel bad about asking for changes. IMHO, it'd be a lot better for you, me, and the users if the basics were already merged and usable and we just needed to discuss and review smaller chunks to add support for mem.extra, settings, etc :)

chirp/drivers/kgq10h.py Show resolved Hide resolved
chirp/drivers/kgq10h.py Outdated
_mem.power = True

for setting in mem.extra:
setattr(_mem, setting.get_name(), setting.value)
Copy link
Owner

Choose a reason for hiding this comment

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

Right here, when we get to am_mode, you set it right back to 0 because the memory's am_mode hasn't yet been set. Since you set it to 0 here, then when the UI refreshes the memory after the set operation, your own get_memory() is returning mode=FM.

If you're doing the edits direct in the spreadsheet with "show extra", you can't set both fields to a consistent value in one operation, but you can in the properties dialog, which lets you make several changes that get sent to the driver all at once.

You can fix the driver to not be broken like this, but I think it's probably not ideal. I think you should probably not make am_mode be a tri-state thing including "Off" and instead just make it be ["RX", "RX+TX"]. That way the user selects mode=AM and am_mode separately, which is how it should be anyway. Since am_mode can't be stored when FM is selected anyway, if they choose anything for am_mode when mode is not AM, that's an error or something to ignore.

You could also only populate am_mode with "off" if mode is set to FM/NFM and populate it with the two choices only when mode=AM. That way there's nothing to select for am_mode until mode=AM.

Make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One concern with this approach. There is only a single setting in memory to determine AM/FM. It is the am_mode setting.
it has 3 values, Off, AM Rx, AM Rx+Tx. If am_mode is OFF, then FM mode is used. So when you say don't make am_mode a tri-state, it is exactly a tri-state setting. What I was trying to do is when FM is selected in the spreadsheet view - force am_mode to OFF like the radio expects. When AM is selected, I wanted to force am_mode to AM Rx. Then if the user wanted AM Rx+Tx they would choose that in the properties box. I don't know how to not use am_mode as a tri-state. What am I missing?

Copy link
Owner

@kk7ds kk7ds Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What you're describing is basically my primary suggestion above: if mode==FM, then am_mode is off regardless. No matter what they choose for am_mode in the properties or spreadsheet view, you ignore that if mode==FM. Then if mode==AM, you default to whichever am_mode makes sense, and then let them choose between those two sub-modes. It's the same behavior as was discussed on the list for the FT450, fwiw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am confused.. The code does exactly what we are talking about. But then after executing the IF / ELIF statement on MODE, and setting am_mode correctly,

    if ((mem.mode == "FM") or (mem.mode == "NFM")):
        _mem.iswide = int((mem.mode == "FM"))
        _mem.am_mode = 0
    elif mem.mode == "AM":
        _mem.am_mode = 1

the following code is run
for setting in mem.extra:
setattr(_mem, setting.get_name(), setting.value)

which then sets am_mode back to what it was previously.

If I remove the code
for setting in mem.extra:
setattr(_mem, setting.get_name(), setting.value)

then the mem.mode value is updated as I want it to be through the spreadsheet UI, but then I can't change any of the Extra settings and get them to take effect.

What am I missing that gets the 2 pieces of code to play nice together?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to the code and it seems to be doing what I want.
When mode != AM - the mem.extra am_mode is not shown in the properties and it is forced to 0.
When mode = AM, the mem.extra allows selection of AM Rx or AM Rx+Tx
Switching from FM/NFM to AM or AM to FM/NFM must be done via the spreadsheet UI.
The properties diaglog box can only change AM modes when AM is selected via the spreadsheet UI mem.mode setting.

    for setting in mem.extra:
        if setting.get_name() != "am_mode":
            setattr(_mem, setting.get_name(), setting.value)
        else:
            if mem.mode != "AM":
                setattr(_mem, setting.get_name(), 0)
            elif int(setting.value) == 2:
                setattr(_mem, setting.get_name(), 2)

Although I think I need to add another elif to handle switching from value 2 back to default value 1.

            elif int(setting.value) == 1:
                setattr(_mem, setting.get_name(), 1)

Copy link
Contributor Author

@mrtjr1 mrtjr1 Nov 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

with the above changes - all now pass Tox which was previously failing for FM/NFM/AM mode switching.

style: OK (64.62=setup[27.91]+cmd[36.72] seconds)
unit: OK (41.86=setup[12.97]+cmd[28.89] seconds)
driver: OK (557.69=setup[13.22]+cmd[544.47] seconds)
congratulations :) (664.42 seconds)

@mrtjr1
Copy link
Contributor Author

mrtjr1 commented Oct 27, 2023 via email

@kk7ds
Copy link
Owner

kk7ds commented Oct 27, 2023

Yep, and that rearranging is what has me (very) concerned that something else is going on. I know we briefly discussed it before, but I think this is the first time I'm seeing the actual implementation of that. It surely looks to me like there has to be some endianess confusion going on or something and I'm not sure I think we should put something like this into the tree. But, get your other stuff figured out and then maybe we can deep dive on the massive rearrangement table thing to see if we can figure out what's going on.

@mrtjr1
Copy link
Contributor Author

mrtjr1 commented Oct 27, 2023 via email

@mrtjr1 mrtjr1 force-pushed the q10h_test branch 4 times, most recently from 73c840b to 01e682d Compare November 1, 2023 21:45
@mrtjr1
q10h troubleshooting
c8232e3 
Updates to resolve mem.mode and extra settings isuses switching between FM and AM modes

Update kgq10h.py

Update for some style fixes and to add ability to switch back to AM Rx mode from AM Rx+Tx

Force Wideband AM mode

Radio only supports wide AM mode.

remove currently unused upload map

Delete unused upload map

Update kgq10h.py

fix VFO AM Mode List
@mrtjr1
Copy link
Contributor Author

mrtjr1 commented Nov 1, 2023

Yep, and that rearranging is what has me (very) concerned that something else is going on. I know we briefly discussed it before, but I think this is the first time I'm seeing the actual implementation of that. It surely looks to me like there has to be some endianess confusion going on or something and I'm not sure I think we should put something like this into the tree. But, get your other stuff figured out and then maybe we can deep dive on the massive rearrangement table thing to see if we can figure out what's going on.

Here is what the data on the radio looks like compared to what it looks like once rearranged in chirp.
File on left is a straight 0x0000 to 0x8000 read from radio. On the right is what it looks like after rearrangment.
you can see in the original, data jumps around every 256 bytes. Sometimes in the middle of a piece of data (like Ch Name).
image

@kk7ds
Copy link
Owner

kk7ds commented Nov 1, 2023

Here is what the data on the radio looks like compared to what it looks like once rearranged in chirp. File on left is a straight 0x0000 to 0x8000 read from radio. On the right is what it looks like after rearrangment. you can see in the original, data jumps around every 256 bytes. Sometimes in the middle of a piece of data (like Ch Name).

Right, so "every 256 bytes" is exactly what I'd expect if you're using the wrong byte order in your address. If you switch your addressing to little-endian does it all line up without rearranging?

@mrtjr1
Copy link
Contributor Author

mrtjr1 commented Nov 1, 2023 •
edited
Loading

Here is what the data on the radio looks like compared to what it looks like once rearranged in chirp. File on left is a straight 0x0000 to 0x8000 read from radio. On the right is what it looks like after rearrangment. you can see in the original, data jumps around every 256 bytes. Sometimes in the middle of a piece of data (like Ch Name).

Right, so "every 256 bytes" is exactly what I'd expect if you're using the wrong byte order in your address. If you switch your addressing to little-endian does it all line up without rearranging?

Where would I do that? My commands directly copy what the CPS does. Using the same addressing format, I get the same data. I would be willing to try, but would your thoughts explain why the order goes from 300 to 400 and then 200 to 300, then 100 to 200 and then 0 to 100 followed by 700 to 800, then 600 to 700, then 500 to 600 then 400 to 500 etc?

@kk7ds
Copy link
Owner

kk7ds commented Nov 1, 2023

Where would I do that?

In your struct.pack, change > to <

My commands directly copy what the CPS does. Using the same addressing format, I get the same data. I would be willing to try, but would your thoughts explain why the order goes from 300 to 400 and then 200 to 300, then 100 to 200 and then 0 to 100 followed by 700 to 800, then 600 to 700, then 500 to 600 then 400 to 500 etc?

Yes. If the OEM software is pulling them in non-sequential order, they're likely byte-swapping before inserting into their memory map.

@mrtjr1
Copy link
Contributor Author

mrtjr1 commented Nov 1, 2023

Where would I do that?

In your struct.pack, change > to <

My commands directly copy what the CPS does. Using the same addressing format, I get the same data. I would be willing to try, but would your thoughts explain why the order goes from 300 to 400 and then 200 to 300, then 100 to 200 and then 0 to 100 followed by 700 to 800, then 600 to 700, then 500 to 600 then 400 to 500 etc?

Yes. If the OEM software is pulling them in non-sequential order, they're likely byte-swapping before inserting into their memory map.

Simply swapping the address endianness and not rearranging did not return a complete usable image. It did communicate with the radio, but it was pretty much the same data repeating over and over and just shifting in some bytes and shifting out others. It never got to any channel data and all 999 channels were populated with data and names in the radio.

        for i in range(start, end, blocksize):
            time.sleep(0.005)
            req = struct.pack('<HB', i, blocksize)
            self._write_record(CMD_RD, req)

I have seen this repeating pattern before when I was trying to see how much more memory previous wouxun radios had above 7FFF. Once above 7FFF, they just started repeating the data at 0 again.
Wouxun_KG-Q10H_20231101 little endianess download test.zip

downloading with big endianess without rearranging yielded the following:

        for i in range(start, end, blocksize):
            time.sleep(0.005)
            req = struct.pack('>HB', i, blocksize)
            self._write_record(CMD_RD, req)

Wouxun_KG-Q10H_20231101 big endianess download test.zip

You can see with the big endian download, all the data is there just interspersed on 256byte boundaries.
Did I do something wrong in the addressing when switching to little endian?
I know you think that may be a simpler explanation, but I am pretty confident that what I am doing is what they are doing and it is an attempt to cause issues with 3rd party programmers. My address commands are their address commands and the data sent and received is the same.
When I emulated the CPS command order (with big endianess addressing) and just filled the map with the data as received (no rearranging), It also generated a usable image. The problem with that was that there were gaps as the CPS does not read all 32K bytes. It only reads the settings, then jumps to the channel mem, then the ch names, then the valid bytes, etc. So if new settings ( like freq limits ) were discovered it would mess up the map and older images would be incompatible with the new memmap. By doing what I am doing, I have all 32768 bytes in every image and all I need to do is add to the memmap as things are discovered.

@mrtjr1
Copy link
Contributor Author

mrtjr1 commented Nov 2, 2023

I looked a bit deeper at the radio responses when sending the Little Endian Addresses - It does not appear that the radio is swapping the bytes before replying. Any address that is >= 0x8000 gets all 0xDD responses from the radio. All the other data is consistent with the data that is stored in Big Endian address format and it explains the repeating pattern shifted by 1 byte as seen in the image.

<style> </style>
Address Dec Address Hex Big End Address Hex Little End
0 0000 0000
64 0040 4000
128 0080 8000
192 00C0 C000
256 0100 0001
320 0140 4001
384 0180 8001
448 01C0 C001
512 0200 0002
576 0240 4002
640 0280 8002
704 02C0 C002
768 0300 0003
832 0340 4003
896 0380 8003
960 03C0 C003
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kk7ds kk7ds kk7ds requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mrtjr1 @kk7ds