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

Add many more settings to TH-D7G #665

Closed
wants to merge 1 commit into from
Closed

Conversation

W8BSD
Copy link
Contributor

@W8BSD W8BSD commented Jun 21, 2023

A large number of settings were missing, including dedicated memories like call memories and programmable scan memories.

There was also a fairly large number of options missing from TH-D7 (and TM-D700).

This is tested on TH-D7G, but should really be tested on TH-D7 and TM-D700

@W8BSD
Copy link
Contributor Author

W8BSD commented Jun 21, 2023

So this isn't well tested enough to merge I think, but I'm not sure I've done everything "right", and I'm not sure how testing on TH-D7 and TM-D700 could be arranged.

@W8BSD W8BSD force-pushed the TH-D7G-Overhaul branch 5 times, most recently from 803c291 to cbf2126 Compare June 21, 2023 22:23
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.

Thanks for all this work. I have very little time to review this today and then I'm out for the next four days. But, I skimmed and gave you some initial feedback. When I get back, I can test on a D700 if you want.

chirp/drivers/kenwood_live.py Show resolved Hide resolved
except:
raise errors.RadioError("Programmable VFO must be Start-End " +
"format (ie: 144-147)")
return "%05d,%05d" % (start, end)
Copy link
Owner

Choose a reason for hiding this comment

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

Er, this format is different than what you say it needs to be above in the error and in your splitting. I feel like it would be better to expose this as two settings, an upper and lower, like the other radios do...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When modifying on the radio, it's presented as "XXX-XXXMHz", and you can change between the two fields. Since the two values are set together with the same command, I wanted it to stay together since it seems that every time you change a setting, all the settings that have been changed are also updated, so I didn't want weird ordering "stuff" to happen.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, so you'll get all the settings at once, always. So I think you can handle it in a stable way even if they're separate. But yeah, let me think on this after I play with it on a D700.

chirp/drivers/kenwood_live.py Show resolved Hide resolved
if result == "N" or result == "E":
mem = chirp_common.Memory()
if self._has_name:
if name is not None:
Copy link
Owner

Choose a reason for hiding this comment

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

You're using this wrong. The name property is owned by the user and is only the alpha tag. If you can't have an alpha tag on these then you should set it to the empty string and make it immutable. What you're looking for is "special channels". You need to expose valid_special_chans in your RadioFeatures. You still need to give them a virtual mem.number like you are here, but set the name (like "Call 1") as mem.extd_number. That tells the UI that it's special and not part of the regular number memories. Check out some of the other drivers in the tree that expose those, and maybe open up an image file (in tests/images) of one to get an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will do... it was very hard to get it working this way, so I suspect I was doing this wrong. :D

("SQ 0", "Band A Squelch"),
("SQ 1", "Band B Squelch")],
'aprs': [("TXD", "Transmit Delay")],
}
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if it makes sense to pull the D7 out to another file at this point? You've added a lot to this file, which was already too big. You can just import kenwood_live and do the same thing, but keep all this verbose settings stuff to one file. What do you think? If you do, it'd be great if you could make one commit that splits the existing driver to a new file (unchanged) and then another commit to make your changes here, just so it's still easy to review the delta. I know that's more work, but hopefully you'll agree it's worthwhile. This file has just grown organically beyond what it really should have in it :)

Maybe move the D700 in there as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I was already considering it... at the same time, I'll make an abstract base class that they all share and hopefully come up with something better than the _SETTINGS_OPTIONS{}/_XXX_OPTIONS override mechanism that's currently in use.

The maidenhead conversion stuff doesn't really belong here either, it was just the easiest way to get lat/lon data into a single text box. The radio is set in XX°YY.YY′ format, but having four boxes all controlling the same set command felt more awkward to me.

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, cool. This definitely needs some cleanup, we just need to be careful not to disturb all the drivers under this that will be hard to find testers for. I have a D700, D710G, D72, D74, and TM271 I can use for verification and I think @KC9HI has an F6A, IIRC.

- Revert timeout increase, the issue with TXH was a bad cable
- Moved Channel Mode display into Main group

New/Modified Settings:
                                 TH-D7  TH-D7G  TM-D700
APRS Icon                          X      X       X
Auto Reply Message                        X
Automatic Power Off                X      X       X
Automatic Simplex Check            X      X       X
Balance                            X      X
Band                               X      X       X
Battery Save                       X      X
Call Memory 1-2                    X      X       X
CALL Key Set Up                           X
Callsign Colour                    X      X       X
DTMF Memory/Name 0-9               X      X       X
DTMF Pause Duration                X      X       X
Enable Tune When Locked            X      X
GPS Unit                           X      X       X
Group Code                         X      X       X
Lamp                               X      X       X
Message Colour                     X      X       X
Message Groups                            X
Mile/Kilometer                            X
My Position 1-3                    X      X
Packet Mode                        X      X       X
Position Ambiguity                        X
Program Scan Memories L0-L9, U0-U9 X      X       X
Programmable VFO Range             X      X
Reception Restriction Distance     X      X       X
RSV Colour                         X      X       X
Scan Resume                        X      X       X
SkyCommand Tone Frequency          X      X       X
Squelch                            X      X
SSTV Callsign                      X      X       X
SSTV Message                       X      X       X
SSTV RSV                           X      X       X
Status Text 1-3                    X      X       X
Status Transmit Rate                      X
Timezone                                  X
Tone Alert                                X
Transmit Delay                            X
Transmit Inhibit                   X      X
TX Hold                            X      X
Tx Hold for 1750                          X
VC Shutter                         X      X       X

This is tested on TH-D7G, but should really be tested on TH-D7 and
TM-D700
@W8BSD
Copy link
Contributor Author

W8BSD commented Jun 21, 2023

When I get back, I can test on a D700 if you want.

It's great that you have one! I looked for one at Hamvention this year and the only one in the flea market was priced at $525 and my upper limit was $350 with an expected price of around $300, so I didn't even bother trying to haggle him down.

Finding a TH-D7A is pretty tough too it seems.

@kk7ds
Copy link
Owner

kk7ds commented Jun 22, 2023

Yeah, I have two of them, even! I'll poke at one with this when I'm back. Thanks!

@kk7ds
Copy link
Owner

kk7ds commented Jun 22, 2023

I quickly broke out the D700 to validate your split PR so we can merge that and let you get underway with your fixes. While I did that I quickly tested this one and got:

Traceback (most recent call last):
  File "/Users/dan/Documents/chirp.git/chirp/wxui/radiothread.py", line 72, in dispatch
    self.result = getattr(radio, self.fn)(*self.args, **self.kwargs)
  File "/Users/dan/Documents/chirp.git/chirp/drivers/kenwood_live.py", line 333, in get_memory
    mem = self._parse_mem_spec(spec, read_cmd)
TypeError: TMD700Radio._parse_mem_spec() takes 2 positional arguments but 3 were given

I haven't looked at why, but should be an easy fix. Just recording for posterity while I have it out.

@W8BSD
Copy link
Contributor Author

W8BSD commented Jun 23, 2023

Closed in favour of #674

@W8BSD W8BSD closed this Jun 23, 2023
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