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

Add Baofeng BF-F8HP-PRO #1072

Merged
merged 1 commit into from
Sep 27, 2024
Merged

Add Baofeng BF-F8HP-PRO #1072

merged 1 commit into from
Sep 27, 2024

Conversation

KC9HI
Copy link
Contributor

@KC9HI KC9HI commented Jun 22, 2024

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

@KC9HI
Copy link
Contributor Author

KC9HI commented Jun 22, 2024

@kk7ds When it is convenient, would you take a look at this to see if you can help me figure out what is wrong. It won't pass the 'drivers' test. It appears to have problems with the Baofeng UV-17 and Baofeng UV-13Pro radios that I don't think are even a part of the driver that I am editing.

Thanks,
Jim

kk7ds
kk7ds reviewed Jun 24, 2024
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.

The problem stems from the fact that the UV17 driver (and all its descendants) inherit from the UV17Pro. That's why the errors show that the UV17 models are failing, and are running your new pro code changes (around the parameterized MEM_FORMAT.

I'll push a fix for those issues on top in a sec. I'd normally like that to be in the patch itself, but I'll keep it separate in order to make it clear what I've changed, so please do look at that. Also, I'm going to add another commit on top to clean up the extra settings methods you're adding here and move them to the F8HP subclass. If you were to look at a python model browser, you'd see that all the subclasses of UV17Pro, UV17, etc all get those f8hp methods, even though they're not really used. That can be quite confusing to people, and especially code inspection tools. The right way to do that is to override the method in the subclass, call the parent's version first (which is what super() does) and then add on the extra stuff you want. Hope that makes sense when you see it, but if not, let me know and I'll help clarify.

I've been working on the GA510v2 driver, which is based on the UV17 one, so the lineage from the UV17Pro driver is getting really deep at this point and thus we really need to be careful about cleanliness here. A lot of what you've changed here breaks my patches (not your fault, you haven't seen them yet) but I'll try to get those rebased on top of this and hopefully it won't be too bad.

So I'll push those revisions in a sec, please confirm they look okay to you before we merge this.

Thanks!

chirp/drivers/baofeng_uv17Pro.py Outdated
@@ -521,6 +575,35 @@ def get_settings_pro_dtmf(self, dtmfe, _mem):
val]))
dtmfe.append(rs)

def get_settings_f8hp_pro_dtmf(self, dtmfe, _mem):
Copy link
Owner

Choose a reason for hiding this comment

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

This should be defined on the F8HP class only since it's only used there.

chirp/drivers/baofeng_uv17Pro.py Outdated
@@ -968,6 +1112,8 @@ def get_settings(self):
dtmfe = RadioSettingGroup("dtmfe", "DTMF Encode Settings")
self.get_settings_common_dtmf(dtmfe, _mem)
self.get_settings_pro_dtmf(dtmfe, _mem)
if self.MODEL == "BF-F8HP_Pro":
self.get_settings_f8hp_pro_dtmf(dtmfe, _mem)
Copy link
Owner

Choose a reason for hiding this comment

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

It would be much better to just override get_settings_f8hp_pro_dtmf() in the F8HP subclass and add the extra stuff there. I know this is the pattern you're used to, but in this case, this stuff is starting to get a bit messy as a result, so it'd be better to move that there. Using super() will let you call the parent implementation and then add on more so it's all localized down there.

chirp/drivers/baofeng_uv17Pro.py Outdated
@@ -775,6 +862,58 @@ def apply_Key2short(setting, obj):
RadioSettingValueBoolean(_mem.settings.fmenable))
basic.append(rs)

def get_settings_f8hp_pro_basic(self, basic, _mem):
Copy link
Owner

Choose a reason for hiding this comment

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

This too.

@kk7ds
Copy link
Owner

kk7ds commented Jul 16, 2024

This pretty much rebased on current master without any trouble. It's been a while since I made the changes so I'm not sure if that's surprising or not. I know I had done some work on this already. If you can test it and report what doesn't work I can try to address it.

@KC9HI
Copy link
Contributor Author

KC9HI commented Jul 18, 2024

@kk7ds Sorry for the delay. I finally got a chance to test. All that I can see that I needed to change was the 'if self.MODEL == "BF-F8HP_Pro"' to use "BF-F8HP-PRO" and all seems to be working OK.

@kk7ds
Copy link
Owner

kk7ds commented Jul 28, 2024

Ah, yep, sorry. When I read this a bit ago, I thought you maybe were going to push up the fix, but I can, if you're willing to test again.

And... what are you thinking about time-wise for merging this?

@kk7ds kk7ds changed the title test radio driver only Add Baofeng BF-F8HP-PRO Sep 26, 2024
@kk7ds kk7ds merged commit fdc91bf into kk7ds:master Sep 27, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kk7ds kk7ds kk7ds left review comments

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
@KC9HI @kk7ds