Add Quansheng UV-K5 driver #667
Conversation
There was a problem hiding this comment.
I'm feeling pretty frustrated that so many of the things I've pointed out several times (in #610 and #623) are still not addressed here. The comments are still in those PRs to reference. I've also offered to help and you said you weren't interested. Please let me know if that has changed.
Things here that break the data model (like exposing the VFO channels as regular memories) need to be fixed before we merge. User-visible things that are wrong (like the typos in the radio instructions) need to be fixed before we merge. The nolimits variant class needs to be removed before we merge. I already asked you to remove the DRIVER_VERSION thing before we merge as it doesn't make sense once it's in the tree. If you're not interested in addressing these things, please let me know and I'll do them for you.
I'm willing to let some of the code cleanliness things that don't affect functionality wait until after this first round, although they're really easy to clean up.
Final thing: the commit title (the first line of the commit message) is what goes out to the mailing list. What you have now doesn't mention what is actually being added, so it needs to be something like "Add Quansheng UV-K5 support" so that it will make sense to people in the build announce email. You can just fix it (and all the other things) and force-push here, no need to create a new PR.
chirp/drivers/uvk5.py
Outdated
| (util.hexprint(header), len(header))) | ||
| raise errors.RadioError("Bad response header") | ||
|
|
||
| return False |
There was a problem hiding this comment.
Again, return after raise makes no sense, this is dead code. There are several other examples below, so please fix them.
chirp/drivers/uvk5.py
Outdated
| hellopacket = b"\x14\x05\x04\x00\x6a\x39\x57\x64" | ||
|
|
||
| tries = 5 | ||
| while (True): |
There was a problem hiding this comment.
These parens around the conditions are not necessary and are are anti-conventional python. Please remove this and the others below.
chirp/drivers/uvk5.py
Outdated
| def get_prompts(x=None): | ||
| rp = chirp_common.RadioPrompts() | ||
| rp.experimental = \ | ||
| ('This is an experimental driver for the Quanscheng UV-K5. ' |
There was a problem hiding this comment.
"Quansheng" is still misspelled and the other typos I mentioned below are still there.
chirp/drivers/uvk5.py
Outdated
| @directory.register | ||
| class UVK5Radio_nolimit(UVK5Radio): | ||
| VENDOR = "Quansheng" | ||
| MODEL = "UV-K5 (modified firmware)" |
There was a problem hiding this comment.
As I mentioned, I don't want to do this. And no, we can't do it temporarily because it will cause people to generate images that aren't usable if/when we remove this. You said you'd have a way to detect the modified version. If that implementation is still pending, I think we should leave the support for the modified firmware as a PoC module in an issue until we can handle it properly.
There was a problem hiding this comment.
Just consider it a separate radio model. There is currently no way to detect a radio with modified firmware, so the user has to make a decision which version he wants to use.
Currently of the people i know more than half is using the modified firmware, so this is an important use case. Chirp is currently the only software which can program these radios, so removing support will break it for a lot of people.
So how do you propose handling it? Breaking chirp support for >50% of people is not a good idea. Neither is having some (possibly rouge) driver on some website, that people will download and use without thinking.
There was a problem hiding this comment.
But it's not a separate radio model. Not any more than the fact that the Icom ID-5100 has had three major iterations of firmware (from the manufacturer) since its release, and we're able to support them without making the user pick different models, and without causing a user's image to be unusable after they upgrade their firmware.
To be clear, I do not want to exclude the people using the modified firmware. I just want to find a way to make this work for them all. It sounds like embedding a version or sentinel in the modified firmware is on the table, so I think that's what we should aim for long-term. Hopefully enabling chirp to do the right thing will be a strong incentive to get that done.
The way I see this, there are three viable options (excluding not supporting the modified firmware):
- Let chirp operate in wide-open mode, effectively assuming modified firmware. People with OEM firmware will be able to set out of band frequencies, and the radio will not fail, but also not honor them.
- Let chirp operate by default in the stricter set of ranges, and have a "virtual" setting that lets them opt into the wider set, effectively confirming that they have the modified firmware.
- Let chirp operate by default on the wider set of ranges, and raise a warning (not error) for every frequency that the OEM software doesn't support. This is how Kenwood commercial software works, FWIW, but may be annoying for users programming lots of channels.
Obviously I know that the first option is preferable for those running the modified firmware, but without very concrete data that it would represent the majority of users, I think most will expect the OEM limits in place.
If we go for the second option, we can basically remove it some time after the version sentinel gets added to the modified firmware, so that those users no longer need to tick the box.
My major concern with the first option is the support load. I think that if the radio blindly allows entry of these frequencies, users are going to report bugs against chirp because it's "not programming the radio properly." If the radio is exposing a wide range of supported bands* then users will try to use them, as chirp is able to expand some radios just by writing a different band edge to memory. If they do that and don't get the expected result (or an error) they will assume it's a bug and someone has to handle those.
As an example, I just tried this driver with my OEM-firmware UV-K5. I added a new memory of 900MHz in chirp. No warnings or errors, and it uploaded to the radio just fine. The radio shows that new memory as 470.0MHz. If I didn't know the internals (and clicked past the info box at download time) I would be super confused and assume that chirp is broken. To me, that makes the first option a non-starter, but it does make the other two options viable because if someone ticks that box, we won't hurt their radio.
@KC9HI you do as much user support and triaging of these sorts of issues as anyone, what are your thoughts?
PS: * I note that we're not pushing this to the chirp next build directories, I need to fix that!
chirp/drivers/uvk5.py
Outdated
| return mem | ||
|
|
||
| if number > 199: | ||
| mem.name = VFO_CHANNEL_NAMES[number-200] |
There was a problem hiding this comment.
Again, these are special channels and should be exposed as such, not as numbered channels. The name field is not yours to use as a label, that's what extd_number is for.
There was a problem hiding this comment.
Didn't see the extd_number functionality. Looking into it.
There was a problem hiding this comment.
Changed to use extd_number, however still not sure what to do with the name field?
- leave it blank and immutable
- put a description in it and make it immutable
- put a description in it and make it immutable, change extd_number to VFO-n (where n=1..14)
The first option is bad, because the user doesn't see all of the label, and there is no way to make the memory number field wider.
The second option is better because the description in the channel number doesn't fit. And i'd like to use the names that the vendor software uses, for example F4(174M-350M)A
The third option is best, because it informs the user that these are VFOs, and also shows the named from the vendor software in the name field.
Pick any version you want, and i will do it (but please try to pick the better-looking and more understandable for the user option).
There was a problem hiding this comment.
Changed to use extd_number, however still not sure what to do with the name field?
* leave it blank and immutable * put a description in it and make it immutable * put a description in it and make it immutable, change extd_number to VFO-n (where n=1..14)The first option is bad, because the user doesn't see all of the label, and there is no way to make the memory number field wider.
The second option is better because the description in the channel number doesn't fit. And i'd like to use the names that the vendor software uses, for example F4(174M-350M)A
The third option is best, because it informs the user that these are VFOs, and also shows the named from the vendor software in the name field.
Pick any version you want, and i will do it (but please try to pick the better-looking and more understandable for the user option).
The first option is the only viable one. Not only because you do not own the name field and cannot use it for random things, but also because that's the behavior of the other several hundred drivers here. Consistency wins. Please have some respect for the fact that this is just one driver of many hundreds currently supported in the code base.
I can work on resolving the issue with the display in some way, but in the mean time you should make the extd_number small enough that it fits. If you just drop the "-ENDFREQ" part of the name it should fit fine.
There was a problem hiding this comment.
Thanks for all these changes and your patience. This looks mostly good to me as it is, except for the following three things:
- The modified firmware class (and that whole discussion)
- The ValidationWarning -> ValidationError thing
- The special channel name width (and use of name)
I'm really tempted to try to fix these myself real quick so we can merge this tonight and make some progress, but I also don't want to yank this away from you. I suspect you're already end-of-day, but if you happen to see this and are okay with me resolving the above issues to my satisfaction and then we iterate from there, let me know and I'll try to do that. I don't think any of my proposed resolutions will tie our hands going forward (as would releasing with the modified class).
chirp/drivers/uvk5.py
Outdated
| @@ -47,7 +45,8 @@ | |||
| # might be useful for someone debugging some obscure memory issue | |||
| DEBUG_SHOW_MEMORY_ACTIONS = False | |||
|
|
|||
| DRIVER_VERSION = "Quansheng UV-K5 driver v20230621 (c) Jacek Lipkowski SQ5BPF" | |||
| # TODO: remove the driver version when it's in mainline chirp | |||
There was a problem hiding this comment.
You know that the day after I merge this it will be published automatically right? Apologies if that's not clear, but that's why I've asked you to remove it, because as soon as this merges it will be "in mainline chirp" and just hours later, it will be a binary build.
| "F7(470M-600M)A": 212, | ||
| "F7(470M-600M)B": 213 | ||
| } | ||
|
|
||
| VFO_CHANNEL_NAMES = ["F1(50M-76M)A", "F1(50M-76M)B", |
There was a problem hiding this comment.
Is this just here so you can get the name from the index? If so, the convention is:
SPECIALS = { .. } # like you have it
SPECIAL_NAMES = {v: k for k, v in SPECIALS.items()}
No need to change it right away, but that will make it only one list of names you have to update instead of two.
| elif mem.duplex == '+': | ||
| txfreq = mem.freq + mem.offset | ||
| else: | ||
| txfreq = mem.freq |
There was a problem hiding this comment.
I thought we had this in a common helper, but apparently only the opposite direction:
https://github.com/kk7ds/chirp/blob/master/chirp/chirp_common.py#L1749
So this is fine, but if you want to move this into chirp_common along with the above sometime that would be appreciated. We probably do this in too many places.
chirp/drivers/uvk5.py
Outdated
| if band is False: | ||
| msg = "Transmit frequency %.4fMHz is not supported by this radio" \ | ||
| % (txfreq/1000000.0) | ||
| msgs.append(chirp_common.ValidationWarning(msg)) |
There was a problem hiding this comment.
This needs to be a ValidationError so that it tells the UI not to push on anyway, assuming it's really a problem. If the radio will let the channel be set and just not transmit there, then it can be a warning. However, the receive frequency below needs to be a ValidationError (modulo the modified-or-not) question.
chirp/drivers/uvk5.py
Outdated
| @directory.register | ||
| class UVK5Radio_nolimit(UVK5Radio): | ||
| VENDOR = "Quansheng" | ||
| MODEL = "UV-K5 (modified firmware)" |
There was a problem hiding this comment.
But it's not a separate radio model. Not any more than the fact that the Icom ID-5100 has had three major iterations of firmware (from the manufacturer) since its release, and we're able to support them without making the user pick different models, and without causing a user's image to be unusable after they upgrade their firmware.
To be clear, I do not want to exclude the people using the modified firmware. I just want to find a way to make this work for them all. It sounds like embedding a version or sentinel in the modified firmware is on the table, so I think that's what we should aim for long-term. Hopefully enabling chirp to do the right thing will be a strong incentive to get that done.
The way I see this, there are three viable options (excluding not supporting the modified firmware):
- Let chirp operate in wide-open mode, effectively assuming modified firmware. People with OEM firmware will be able to set out of band frequencies, and the radio will not fail, but also not honor them.
- Let chirp operate by default in the stricter set of ranges, and have a "virtual" setting that lets them opt into the wider set, effectively confirming that they have the modified firmware.
- Let chirp operate by default on the wider set of ranges, and raise a warning (not error) for every frequency that the OEM software doesn't support. This is how Kenwood commercial software works, FWIW, but may be annoying for users programming lots of channels.
Obviously I know that the first option is preferable for those running the modified firmware, but without very concrete data that it would represent the majority of users, I think most will expect the OEM limits in place.
If we go for the second option, we can basically remove it some time after the version sentinel gets added to the modified firmware, so that those users no longer need to tick the box.
My major concern with the first option is the support load. I think that if the radio blindly allows entry of these frequencies, users are going to report bugs against chirp because it's "not programming the radio properly." If the radio is exposing a wide range of supported bands* then users will try to use them, as chirp is able to expand some radios just by writing a different band edge to memory. If they do that and don't get the expected result (or an error) they will assume it's a bug and someone has to handle those.
As an example, I just tried this driver with my OEM-firmware UV-K5. I added a new memory of 900MHz in chirp. No warnings or errors, and it uploaded to the radio just fine. The radio shows that new memory as 470.0MHz. If I didn't know the internals (and clicked past the info box at download time) I would be super confused and assume that chirp is broken. To me, that makes the first option a non-starter, but it does make the other two options viable because if someone ticks that box, we won't hurt their radio.
@KC9HI you do as much user support and triaging of these sorts of issues as anyone, what are your thoughts?
PS: * I note that we're not pushing this to the chirp next build directories, I need to fix that!
|
It would seem to me that going with option one will generate unnecessary bug reports. I don't know how the UV-K5 handles out-of-band frequencies, but the Radtel RT470 that I recently added won't display a channel that frequency that in not within one of the supported bands. I can see UV-K5 owners that are not using the modified firmware programming memories with frequencies that CHIRP happily lets them enter but are not supported by their radio's firmware. After they are uploaded to the radio they are in some way unusable (missing, won't RX/TX, etc). How would someone trying to support this issue, not familiar with the UV-K5 firmware situation, know that this situation is due to CHIRP's expanded support for 3rd party firmware and that this result is a normal (and I guess expected) for someone running the factory firmware. And how do you explain to the user that if an option doesn't work, it's not a bug? It isn't an option meany for your radio so stop using it. Option two seems to be the better way to go. CHIRP support would be in line with a new radio out-of-the-box. This is the way I would imagine that the configuration of a majority of CHIRP users programming a UV-K5. Those that are the adventurous type that are willing to use a 3rd party firmware can then opt-in by using whatever mechanism is created enable the expanded support. |
It was sort of buried in my tome above, but I tested this. If I put in 900MHz in CHIRP, the radio shows 470.0MHz in that channel. That, IMHO, is worse than it being blank or something like that because it looked like it worked but chirp used the wrong multiplier or something like that. It's good that the radio doesn't choke (like many would) but I think users will interpret it as a bug. |
This work was all done by @sq5bpf and is exactly like the last commit authored by him, which was 4d902e0 in PR 667. I just squashed this, rebased, and fixed the conflicts (all in chirp/locale). Fixes #10478
| "F6(400M-470M)A": 210, | ||
| "F6(400M-470M)B": 211, | ||
| "F7(470M-600M)A": 212, | ||
| "F7(470M-600M)B": 213 |
There was a problem hiding this comment.
I mentioned this to you elsewhere, but I think these names are too long for comfort. This doesn't need to change immediately, but I think it would be better to shorten these to just the start of the range. We have a bunch of examples of this (mostly in the HF radios which have more VFO slots than FM types). Here's an example:
https://github.com/kk7ds/chirp/blob/master/chirp/drivers/ft817.py#L236
kk7ds
force-pushed
the
master
branch
2 times, most recently
from
494df34
to
d4c50ef
Compare
This removes the modified firmware subclass for the UV-K5 and adds a virtual setting that allows relaxing the band checking at runtime for users with modified firmware. It does that by exposing the wider range in the static valid_bands list, and instead enforcing the OEM limits in validate_memory(). If the user toggles the virtual setting, then validate_memory() starts (or stops) honoring the wider set of limits. Note that if the modified firmware adds some sentinel we can use to detect it, we can simply remove this toggle and detect it from the memory or clone conversation itself. Further note that we can persist this setting in image metadata to make it sticky across images (although this patch does not do that). Related to #10478
This was for use during development but is removed for final inclusion in CHIRP itself. Related to #10478
This makes us automatically size the row labels after building the memory editor in case the driver has very long special memory labels. Related to #10478
|
Okay, @sq5bpf I pushed changes to your branch in hopes that this will accelerate getting something landed and we can iterate from there. This is purely to try to help reduce your delta and help get it landed, so I hope it's clear that's the goal here. Here's what I did:
I kept all of my changes as individual commits on top of the squashed version of your last commit so it's clear what I changed, and I only changed what I needed to for those items. There's still stuff to clean up but I didn't want to muddy up the view so it's clear. I tested this briefly (at least to cover my changes) with my OEM radio here. My plan would be to follow this up with making the modified firmware flag sticky per image if you think that will be appreciated. If you like option 3 above (just always warn for every edit) better then we can discuss that and potentially make that change (although I think this is the best option for now). Hopefully we can remove that toggle later with a sentinel from the modified firmware. So, I'm going to go ahead and merge this with those changes, which I hope you'll see as better than waiting another couple days before it can be included in a build. |
CHIRP PR Checklist
The following must be true before PRs can be merged:
tests/images(except for thin aliases where the driver is sufficiently tested already).Please also follow these guidelines:
six,future, etc).NEEDS_COMPAT_SERIAL=Falseand useMemoryMapBytes.tox -emakesupportedand include it in your commit.