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 subclass for Baofeng's 1900 series #700

Merged
merged 4 commits into from
Jul 4, 2023
Merged

Add subclass for Baofeng's 1900 series #700

merged 4 commits into from
Jul 4, 2023

Conversation

cetinajero
Copy link
Contributor

@cetinajero cetinajero commented Jul 3, 2023

Hello,

Currently Chirp doesn't support the Baofeng BF-1900 series, this PR was created to support them, including:

  • BF-1901(5W)
  • BF-1902(5W)
  • BF-1903(5W)
  • BF-1904(10W)

The PR updates the settings2 struct to reflect the scan mode options used by this models.

Baofeng decided to remove the programmable side key function and used that reserved address to add the "Search" scan mode option (having 3 scan options requires more than 1 memory bit).

My testing and debugging was based on the original Baofeng software (BF_1901_FH) and comparing back and forth.

Fixes: #8551

✌🏼

@kk7ds
Copy link
Owner

kk7ds commented Jul 3, 2023

Thanks for this, but it's very little delta from the h777 driver so it really needs to be a subclass for just the difference to avoid the duplication. We just need to refactor the base driver a tiny bit to support the difference. If you need help with this I could take a stab at it for you based on what you have here and then you can test and iterate from there.

@cetinajero
Copy link
Contributor Author

Let me take a shot at it, thank you Dan.

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.

Great, thanks very much! Here are some hints to get you started. Also, please include a test image in tests/images for the radio to exercise the new subclass. You're welcome to put it into the h777.py file, or keep your baofeng_1900.py file and just import h777 so you can subclass.

chirp/drivers/baofeng_1900.py Outdated Show resolved Hide resolved
chirp/drivers/baofeng_1900.py Outdated Show resolved Hide resolved
chirp/drivers/baofeng_1900.py Outdated Show resolved Hide resolved
chirp/drivers/baofeng_1900.py Outdated Show resolved Hide resolved
@cetinajero cetinajero changed the title Add driver for Baofeng's 1900 series Add subclass for Baofeng's 1900 series Jul 4, 2023
chirp/drivers/h777.py Outdated Show resolved Hide resolved
@cetinajero
Copy link
Contributor Author

Thanks for your guidance Dan, WIP yet.

Still pending tasks:

  • Alarms as lists
  • Search scan mode
  • Valid band range up to 520 MHz
  • Vox level up to 9

@kk7ds
Copy link
Owner

kk7ds commented Jul 4, 2023

Looks pretty close, that was fast :)

VENDOR = "Baofeng"
MODEL = "BF-1904"
ALIASES = []

Copy link
Owner

Choose a reason for hiding this comment

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

Do you want separate power levels for this? I think this is the 10W unit? Not a huge deal, but it should be easy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Different power levels but same subset of features... what do you suggest?

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 better to have different power levels exposed by that driver so that when importing memories it selects the closest one. You can just do the same thing you did for the VALID_BANDS but with a list of power levels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it

@kk7ds
Copy link
Owner

kk7ds commented Jul 4, 2023

Just need to fix the style things to be mergeable. Also, can you fix up the commit message? I think it still has content from a previous commit in it. Also put "Fixes: #8551" in the commit message so the bug system will close that bug and point to this when it merges. Looks great otherwise.

I can rebase for you when ready, if it helps.

@cetinajero
Copy link
Contributor Author

Yes, I'm running with tox -e style

Is there an auto fix style option?

I plan to add 3 total commits (other two for the scan list struct and alarm option list), is that ok?

@kk7ds
Copy link
Owner

kk7ds commented Jul 4, 2023

Yes, I'm running with tox -e style

Is there an auto fix style option?

No, but I can fix for you if needed.

I plan to add 3 total commits (other two for the scan list struct and alarm option list), is that ok?

Sure. However, I'm going to be unavailable for the next week starting tomorrow. How about we just get this merge-able now and you can follow up with the extra features later?

@cetinajero
Copy link
Contributor Author

Ok, almost there

@cetinajero
Copy link
Contributor Author

It should be mergeable by now Dan, let me know your thoughts.

@kk7ds
Copy link
Owner

kk7ds commented Jul 4, 2023

Looks good to me. Can I tweak your commit message for the first commit to reference the bug? Other than that I'm good.

@cetinajero
Copy link
Contributor Author

Oh yeah, I forgot that.

I can just amend it.

Fixes: #8551

```
python3 -mpip install tox
tox -e unit
tox -e makesupported
tox -e style
```
Also add POWER_LEVELS of 10 watts to BF1904, important to note that it only supports low and high as options.
The settings2 struct wasn't pointing to the right address, Baofeng decided to remove the programmable side key function and used that reserved address to add the "Search" scan mode option (having 3 scan options requires more than 1 memory bit).

This was debugged by using the original Baofeng software (BF_1901_FH) and comparing back and forth.
@kk7ds
Copy link
Owner

kk7ds commented Jul 4, 2023

Also, I assume we want to tell users with a BF-1902 to use BF-1901 right? There's mapping in the code that will update the website after merge so it says "BF-1902 (use BF-1901)".

@cetinajero
Copy link
Contributor Author

There's mapping in the code that will update the website after merge so it says "BF-1902 (use BF-1901)".

Yeah, that will be helpful, where can we add those relationships?

@kk7ds
Copy link
Owner

kk7ds commented Jul 4, 2023

I added it here, to model_alias_map.yaml. The build process will update the web page tomorrow when it creates the build.

@cetinajero
Copy link
Contributor Author

Awesome, thanks Dan!

It was great working on this

@kk7ds kk7ds merged commit 966fc66 into kk7ds:master Jul 4, 2023
@kk7ds
Copy link
Owner

kk7ds commented Jul 4, 2023

Excellent thanks very much!

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