-
Notifications
You must be signed in to change notification settings - Fork 274
Related to #11696 #1350
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
base: master
Are you sure you want to change the base?
Related to #11696 #1350
Conversation
Adds Baofeng BF-9700. Removes 'Scan' checkbox for BF-190x and BF-9700 not present in OEM software. Addresses a few E722 warnings (not 100% sure about them).
Adds Baofeng BF-9700 + Stun entries in Browser. Removes 'Scan' checkbox for BF-190x and BF-9700 not present in OEM software. Addresses a few E722 warnings.
Related to #11864. Fixes swapped Last and Main channels in Priority Transmit for RA89/P62/P2. Adds Mic Level for RA89. Adds freq. band reporting in 'Browser' tab.
|
There are two modules in this PR: h777.py and th_uv88.py th_uv88.py is related to #11864. |
Renumbering 'unkXX' in sequential order. Added 'Eliminate Squelch Tail When No Ctc/Dcs Signaling' entry to be seen in 'Browser'
|
You may not have seen the PR guidelines because of how you created this, but this needs some changes to be merge-able. Specifically: The new model needs to be in its own commit with a proper subject line and a Also, the comments here in the PR are not persisted in git history, so most of your changes here have no context about what they're doing. Please put your comments from above (but hopefully with more detail than "many changes") into the commit messages themselves so they stick with the change in the history. The bug system will not notice the reference to bug 11696 if it's in a comment here, it has to be in the commit message. So in summary, from the look of it, this could be squashed down to three commits, the new model, and one cleanup commit per other driver. If you don't know how to do that I can try to do it for you this time. Thanks! |
|
Dan, you talk to me like I'm some kind of professional programmer, but in reality I am not and this is my first PR ever! |
Put two underscores back
|
Almost none of the contributors to CHIRP are professional programmers, FWIW :) And no, I'm talking to you like the obviously intelligent person that you are and trying to point you at the steps we all go through to get stuff into the tree in an organized way. These may seem foreign at first, but they're very doable. You mentioned (what I deduced was) this PR in a comment earlier today in a way that made it seem like you were expecting it to be merged. I had seen it marked as draft before and was still in a state needing to be cleaned up so I figured maybe I'd circle back here and nudge you towards the issues keeping it from being merged in case it wasn't obvious. I'll pull this down and reformat it the way it should be, but I'll want you to look it over again since I don't have any of these radios to test with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments of things that should change, and questions about the changes being made here so I can split and cleanup the the commit history as noted.
Thanks!
chirp/drivers/h777.py
Outdated
| #seekto 0x0250; | ||
| struct { | ||
| u8 stun_code[7]; | ||
| u8 unused1[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An array of 1 doesn't make sense, so this should just be u8 unused1; .. TBH, bitwise should be warning you about this, so I'll check and see why it's not.
| rs = RadioSetting("scan", "Scan", | ||
| RadioSettingValueBoolean(_settings.scan)) | ||
| basic.append(rs) | ||
| if self.PROGRAM_CMD == b'PROGRAM': |
There was a problem hiding this comment.
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 check something other than this PROGRAM_CMD, but I see you're mirroring another piece of code here that already does that. So maybe let's circle back and clean up both in a subsequent change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did as best as I could. Not the most elegant way, I know. Quick and dirty. I just wanted to get rid of "Scan" for radios that do not support it.
| self.driver = H777Radio(None) | ||
| self.testdata = bitwise.parse("lbcd foo[2];", | ||
| memmap.MemoryMap("\x00\x00")) | ||
| memmap.MemoryMapBytes("\x00\x00")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this to use MemoryMapBytes but this needs to take a byte string (so b"\x00\x00"). But, this little integrated test is not actually run anywhere, the original author was just using it (and I bet it doesn't even work anymore). I'll bytes-ify this string just for consistency, but this too could use a revisit.
| '4.0S', '4.5S', '5.0S'] | ||
| rx = RadioSettingValueList(options, current_index=_settings.voxDelay) | ||
| rset = RadioSetting("basicsettings.voxDelay", "VOX Delay", rx) | ||
| basic.append(rset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Help me out... this is just moved from below but otherwise unchanged? Is there some reason it was moved or just accidental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, purely for cosmetic reasons, so that Vox Level and Vox Delay appear in one place in GUI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay cool, so this change should be one commit on its own so it's clear that it's just moving (and not changing) anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also moves it into basic from advanced. I assume @KC9HI was mirroring the radio vs. software menu structure which is why this was down below in advanced and not with the other one. So I'll let him comment on whether or not he wants this moved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does. But, just FYI, in OEM software all these functions are in one, s.c. "Basic" window
| rx = RadioSettingValueBoolean(_settings.dualWait) | ||
| rset = RadioSetting("basicsettings.dualWait", | ||
| "Dual Wait/Standby", rx) | ||
| basic.append(rset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds the dual wait setting for all models, where it was restricted to only ones with LCDs before. Is this intentional? I have h777-based radios without displays that I'm sure don't have dual wait since they only have a way to select a single channel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h777 based radios in uv88 file?
| [(str(x + 1), x) for x in range(200)], _settings.dwchan) | ||
| rs = RadioSetting('basicsettings.dwchan', | ||
| 'Dual Wait Channel', rsv) | ||
| basic.append(rs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm guessing these non-display radios have a dedicated "always dual wait for channel X" function? Mine aren't in this list, which means they'll get a dual-wait setting in settings, but won't get this extra setting either. I think the logic should be "if LCD, always add dual-wait. If not LCD but dwchan in _settings then add dual-wait and dual-wait-chanel" right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the question above are still relevant to make sure you intended to change this in this way, however my confidence about whether it's correct or not is irrelevant since I don't know anything about these radios.
@KC9HI can you review the changes being made to this driver to make sure they jive with your understanding of them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, in Retevis P62/P2 and TYT TC29, when dual wait is activated, you tell the radio which other, permanently set channel to watch. Range 1 to 200. Dan, you did this piece when we spoke for the first time. See #11864
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, and I remember it being too wide a scope in terms of the models it affected, hence it being reverted and hence me asking for someone more familiar with these models to help review :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, it's just two models. Almost identical. One with display - Retevis RA89/TYT TH-UV99, the other one without - Retevis P62/P2/ TYT TC29.
I've double and triple checked all functions on my radios, everything is working as expected. I have a park of all of these. Found a few firmware bugs too along the way.
I keep filing on this driver and it would be extremely cool if @KC9HI could revisit it. I have a bunch of question to ask
| options = ["Main Channel", "Last Channel"] | ||
| else: | ||
| options = ["Main Channel", "Last Channel", | ||
| "Active Channel with Lockout Time"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure all h777-based radios without LCD support this extra setting? Or do you want to add that only for the subset of models you know have it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I just realized this is the second driver, so this comment makes no sense.
I'll look to Jim to review this part since I don't have any of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As before, I don't see h777 based radios in th_uv88 file. This setting is for Retevis P62, Retevis P2 and their father - TYT TC29. They have 3 options in OEM software. Third option activates extra "Active Channel with Lockout Time"
|
I cut my teeth on the uv5r.py driver which had Basic and Advanced tabs. I don't know of or if there is any published or otherwise standard for which settings go where. For the last couple of years I have just been putting settings that are included in the radio's menus in the Basic tab and putting any settings that only appear in software in the Advanced tab. It would be nice to always have common settings grouped together, but that doesn't always mesh with how the settings are arranged in the OEM software and can be difficult to achieve when the driver you are trying to add too may already support 10 or more radio models. For me it got to the point I was spending more time trying to arrange the settings to accomplish a goal that I felt very few would ever appreciate. So recently I have mostly been adding any settings that aren't already supported in their own group below the existing settings. I am good with anyone that wants to take on a project of reorganizing the settings into a more use-able arrangement. |
|
So, what's up with this PR? |
From about august Quansheng is shipping all UV-K5 variants with new controller chip and new firmware. Radios with previous controller chip are no longer being manufactured. Numbering of new firmware starts with 1.


Adds Baofeng BF-9700 + Stun entries in Browser. Removes 'Scan' checkbox for BF-190x and BF-9700 not present in OEM software. Addresses a few E722 warnings (not 100% sure about them).