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 TYT TH8600 driver. Issue 4851. #824

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

Conversation

aaknitt
Copy link
Contributor

@aaknitt aaknitt commented Nov 27, 2023

CHIRP issue

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).
  • 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").
  • 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.

@aaknitt
Copy link
Contributor Author

aaknitt commented Nov 27, 2023

@kk7ds I'm not sure what to make of these tests not passing, can you advise?

@kk7ds
Copy link
Owner

kk7ds commented Nov 27, 2023

The support matrix needs re-generating by running tox -e makesupported and then commit the result. Looks like maybe you just hand-edited that file, but it's a generated one (from `tests/py3_driver_testers.txt' which is what you should actually modify).

The PR checks are failing because your file is too similar to th_uv88.py. Did you copy that file and make changes? If so, please just subclass that driver and change the necessary behaviors. If the parent driver needs some refactoring to make it so you can reimplement certain things, that's fine. We have way too many drivers in chirp that are created by copying a file, making a ton of tiny changes, and then putting it in tree. That means that bugs get copied around a log and fixing them (like the massive effort for python3 compatibility) is made much more difficult.

@aaknitt
Copy link
Contributor Author

aaknitt commented Dec 1, 2023

The support matrix needs re-generating by running tox -e makesupported and then commit the result. Looks like maybe you just hand-edited that file, but it's a generated one (from `tests/py3_driver_testers.txt' which is what you should actually modify).

Ok, I think I have that fixed now. The tests don't seem to be running in Github now, not sure why that is.

The PR checks are failing because your file is too similar to th_uv88.py. Did you copy that file and make changes?

There were elements from several different TYT radio drivers. The 8600 seemed to have similarities with several models but there were none that I found were a match in either memory map or supported functions. I may not have looked hard enough.

If so, please just subclass that driver and change the necessary behaviors. If the parent driver needs some refactoring to make it so you can reimplement certain things, that's fine.

I'm not going to have time to refactor this any time in the foreseeable future. I had some free time to work on this driver over the summer, but that's no longer the case.

We have way too many drivers in chirp that are created by copying a file, making a ton of tiny changes, and then putting it in tree. That means that bugs get copied around a log and fixing them (like the massive effort for python3 compatibility) is made much more difficult.

It would be helpful to have this type of background info & preferences on this page for new contributors to avoid rework.

@kk7ds
Copy link
Owner

kk7ds commented Dec 1, 2023

It would be helpful to have this type of background info & preferences on this page for new contributors to avoid rework.

Okay, seems like common good development practice to me, but I'll make it crystal clear on that page.

aaknitt and others added 2 commits September 22, 2024 12:07
This applies the recent change to all drivers to use current_index for
settings instead of current(value) for safety.
@kk7ds
Copy link
Owner

kk7ds commented Sep 22, 2024

I just rebased this and fixed some things that have shifted in the rest of the code base. I still have concerns over how much of this is identical to th_uv88, especially since there are lots of little changes which aren't significant which means it's actually more common than it appears. I'm not sure what to do about that at this point, but at the very least this is rebased and brought current.

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