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

Added driver for Baofeng UV-17 fixes #10865 #819

Merged
merged 3 commits into from
Nov 27, 2023
Merged

Conversation

vdwel
Copy link
Contributor

@vdwel vdwel commented Nov 23, 2023

This driver adds support for the Baofeng UV-17 radio with the blue background screen. It fixes issue #10865

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.

@vdwel vdwel force-pushed the UV-17 branch 2 times, most recently from 38dc90d to ae822ea Compare November 23, 2023 13:05
@vdwel
Copy link
Contributor Author

vdwel commented Nov 23, 2023

I somehow can't manage to reduce the overlapping code with the UV-17 Pro driver. Any suggestions?

@vdwel vdwel force-pushed the UV-17 branch 4 times, most recently from c608f21 to 944f273 Compare November 23, 2023 13:37
@vdwel vdwel marked this pull request as ready for review November 24, 2023 17:14
@kk7ds
Copy link
Owner

kk7ds commented Nov 24, 2023

Yeah, there's still a lot that is shared between them, so there's plenty of room to re-use code I think. Some of your closures like apply_code() can be brought out to module-level and used by both. Looks like your sync_in(), sync_out() and get_features() are basically identical between the two. There's no need to re-define those if they're the same as the parent of course. Also looks like a lot of get_memory() could be the same. You can refactor the pro driver to break out things like the mem.extra stuff into a separate helper and then just call that from both drivers to avoid replicating that.

@vdwel
Copy link
Contributor Author

vdwel commented Nov 24, 2023

Don't merge this one yet. It seems that some people are not able to connect to their radio, using this driver.. I am trying to figure out if the problem is the driver or that there is another UV-17 radio out there which I haven't heard of yet.

@vdwel
Copy link
Contributor Author

vdwel commented Nov 24, 2023

People were trying to use the Pro driver for this non-pro driver. :-) How do we fix the fact that it is all so confusing??

So don't put it on hold for merging..

I will try to fix the code reuse issue first... But I'm afraid that's going to be a challenge.. And I don't have infinite time..

@vdwel vdwel force-pushed the UV-17 branch 3 times, most recently from 649fcae to 023bfda Compare November 25, 2023 20:15
@vdwel
Copy link
Contributor Author

vdwel commented Nov 25, 2023

I have modified this driver to use a lot of code from the UV-17Pro driver. In order to do so, I also had to modify the UV-17Pro driver. The new version of the Pro driver needs to be merged before this one.

@kk7ds
Copy link
Owner

kk7ds commented Nov 26, 2023

We should just put both commits in this PR, so we can see it tested together. They're definitely related enough to be merged as a unit.

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, it's a holiday weekend here so I've been a bit tied up with other stuff.

chirp/drivers/baofeng_uv17Pro.py Outdated Show resolved Hide resolved
chirp/drivers/baofeng_uv17.py Outdated Show resolved Hide resolved
chirp/drivers/baofeng_uv17.py Outdated Show resolved Hide resolved
chirp/drivers/baofeng_uv17.py Outdated Show resolved Hide resolved

def set_memory(self, mem):
offset = 0
# skip 16 bytes at memory block boundary
Copy link
Owner

Choose a reason for hiding this comment

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

Does this comment refer to the conditional offsets below?

You can use #skip in your memory format to move forward a specific amount, if it helps. When I've written drivers that don't store memories all linearly, I generally try to define the format declaratively instead of doing stuff like this. Something like:

struct mem lowmems[252];
#skip 0x0x10;
struct mem highmems[506];
// ... etc

Your comment in get_memory() mentions that there's a 16-byte "page number". Not really sure what that means but you can also do something like this:

struct {
  struct mem memories[252];
  u8 page_stuff[16];
} mem_page[3];

The reason to define it like this instead of just papering over it the way you are doing here, is that these "page number" structures will be represented in the browser as memories with non-sensical data in them, which is not great.

At the very least, you should probably make this logic a helper so you don't have to repeat it in get_memory() and set_memory(), especially if you ever have to tweak it.

Oh and this makes me realize - you didn't implement get_raw_memory() here or for the pro one. You should do that, because it makes it much easier to iterate and fix things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Added get_raw_memory and removed the offset stuff..

chirp/drivers/baofeng_uv17.py Outdated Show resolved Hide resolved
@vdwel
Copy link
Contributor Author

vdwel commented Nov 27, 2023

Thanks for looking at the code!!

@vdwel vdwel force-pushed the UV-17 branch 2 times, most recently from 27010df to 2eb2842 Compare November 27, 2023 14:33
@kk7ds
Copy link
Owner

kk7ds commented Nov 27, 2023

Ah, looks like you squashed the refactor and new driver commits together. I had just brought the other commit in there. I know it's probably maddening to understand my preferences here but: commit history should be clean, methodical changes that build on each other and don't do and then undo things in one epoch of development. So, refactoring one driver in a commit so the next commit can make a change on its own is totally cool, and means that reviewing one commit alone is still easy.

So, if it's cool with you I'll split this back in two (commits) before I merge, since it's trivial to do so and then I promise to take my pill.

Also, I didn't notice before but you need to be adding these drivers to the test matrix as "python3 tested" which is why the existing two drivers show up on the front page with an asterisk. I'll do that for you while I'm splitting this.

vdwel and others added 3 commits November 27, 2023 15:04
Prepare this driver to be subclassed with better re-use.

Related to #10865
This driver adds support for the Baofeng UV-17 radio with the blue background screen. It fixes issue #10865
@kk7ds kk7ds merged commit 0a6397c into kk7ds:master Nov 27, 2023
@vdwel vdwel deleted the UV-17 branch November 28, 2023 21:46
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