Add new RadioSettingValue Classes for Hex and DTMF Values - #1558
TrimbleSoftware wants to merge 18 commits into
Conversation
kk7ds
left a comment
There was a problem hiding this comment.
Hey, this is a great idea, thanks for thinking about "the system" instead of just a single driver. Obviously there's a lot more bespoke validation in a lot of places than would be ideal, so standardizing these sorts of things is very good. We can probably clean up some of that from a bunch of drivers once this is in.
That, said I think this exact implementation mixes too much of the view and the model, but it's definitely on the right track. If you look lower in settings.py you'll see MemSetting, which I really want to see more people using instead of the sort of old way of manually applying settings to memory object values (I need to write some docs about that). That method can basically walk a tree of setting values and set them on the memory object, pretty much doing the entire set_settings() function on its own in a generic way for most drivers. However, it needs the value models to behave like it expects, and for something like the Hex value you have here - it needs to behave more like an integer than a string (because that's what it is).
We already have a hex-enforcing UI element in the radio browser, but.. can I write you the equivalent for a RadioSetting and we re-swizzle this to let the UI do its job of controlling the way the data is viewed/entered and make your model classes here just do the model part?
Again, thanks very much for working on this as a holistic solution!
| elif isinstance(value, settings.RadioSettingValueHex): | ||
| editor = self._get_editor_str(element, value) | ||
| elif isinstance(value, settings.RadioSettingValueDTMF): | ||
| editor = self._get_editor_str(element, value) |
There was a problem hiding this comment.
isinstance() actually takes a tuple of valid classes, so you could define things_that_edit_like_strings = (RadioSettingValueString, RadioSettingValueHex, ...)
(just a comment)
There was a problem hiding this comment.
That's a good catch. It would consolidate the code somewhat.
| try: | ||
| if str(value).strip(): | ||
| value = int(value, 16) | ||
| except Exception: |
There was a problem hiding this comment.
This should catch the actual things we expect (which is probably just ValueError right?)
There was a problem hiding this comment.
Either-Or. But ValueError is probably more approiate.
There was a problem hiding this comment.
It's always best to catch the actual things you expect to avoid squashing something else that happened into "you entered an invalid digit". It's easy to always catch Exception, but it's not usually best. Some projects have static checkers that won't let you catch Exception unless you're also handling the expected ones separately.
| self._minlength = minlength | ||
| self._maxlength = maxlength | ||
| self._min = minval | ||
| self._max = maxval |
There was a problem hiding this comment.
It seems a bit strange to have a min/maxlength and a min/maxval as they sorta overlap but not quite. Isn't it enough to have just maxval? Also, min/maxlength - do they include the prefix or not? You're basically controlling the display/view of the data in here (which is the model) and overriding the UI's job.
For hex, it makes more sense to me to make this subclass from IntegerValue and only override the UI element for editing/display since the underlying thing is still just an integer in memory. Then, the prefix part is just an artifact of the display and we can just enforce the min/maxval like normal, right? Like, the UI could enforce that the 0x prefix never leaves the editor field, or is displayed in a different color or something. Having the SettingValue here (the model) keep forcing it back in will make it harder to do that.
I think if you just subclass the integer class for a hex one, with no other difference, then the UI can know whether or not to use a Hex editor class or not.
There was a problem hiding this comment.
When I wrote this I was debating that very thing. Then I thought of the use-case where you might need a 4 digit hex number between 0xF and 0xFFFF for example. Then you would need both min and max value as well as min and max length to validate against. In this case it would avoid having to input 0x000F to get 0xF. After the input it would be displayed as 0x000F.
Internally it is treated as a binary integer and is only formatted as a string on output. It would be great if the UI knew how and when to do that to display the value in an industry standard Hex notation.
The "0x" prefix is specifically excluded from the min and max length validation values.
That is probably a good idea to subclass the IntegerValue and override the needed functionality.
There was a problem hiding this comment.
Yeah, the UI is best positioned to handle the display and input part. Let it reject invalid keystrokes but not obsess over the length until the user tries to save the value, then reformat to four characters with prefix, etc.
| return self._step | ||
|
|
||
|
|
||
| class RadioSettingValueDTMF(RadioSettingValue): |
There was a problem hiding this comment.
Isn't this just a string with a CHARSET = CHARSET_DTMF?
I think using that will be almost exactly the same except it won't have "DTMF" in the error messages. Could we just subclass StringValue and pass our CHARSET to it. Could maybe even get similar error behavior by wrapping set_value(). Something like:
class RadioSettingValueDTMF(RadioSettingValueString):
def __init__(self, minlength, maxlength, current):
super().__init__(minlength, laxlength, current, charset=CHARSET_DTMF)
def set_value(self, value):
try:
super.set_value(value)
except InvalidValueError as e:
raise InvalidValueError('%s (DTMF values must be digits: %s)' % (e, CHARSET_DTMF)
There was a problem hiding this comment.
That may work to simplify the code. I'd still want to normalize the output (like accept lowercase input, but convert it to uppercase for display consistancey, but here again a model vs. view argument...).
There was a problem hiding this comment.
Yep, let the UI handle that part. The RadioSettingValue is just the carrier of the actual value, which is always an integer and thus has no min/maxlength.
So maybe give me a bit (possibly later today) and I'll see if I can cook up the UI bit and trim the display bits of this part down to give you something to play with.
I use and like |
Well, the point of Added this to at least encourage people to use it for new code. |
|
Here's something to try with the Hex value. I added a setting to FakeLiveRadio so you can "download" from that and have something to poke at. It keeps the required 0x prefix and limits keystrokes to just hex values. Let me know what you think and I can work on the DTMF one next. |
I tried out the your I noticed these glitches:
This is gelled enough now for me to ask you to work on the DTMF editor too... Thanks for entertaining this kind of a fix for CHIRP, Fred |
|
Sorry for the delay here, busy week. I'll circle back next week and work on those things and on DTMF. Thanks! |
I took a WAG at fixing the HexText by hooking the keyboard and mouse focus events and selection just the right amount of the hex value. Seems to work OK on the fake live driver. One problem that still exists is that HexText will not allow a blank/null value. 0x00 is not the same as a NULL value. Usually radios allow a null hex value (all 0xFF values). Also took another WAG at adding the DTMFText editor as a subclass of string with the charset fixed and the padchar removed. It had some of the same focus problems as the HexText and were fixed the same way. Also mem.extra is something I have no clue an adding these new editors to. |
|
I haven't forgotten about this I just know it's not super critical and have had a lot else going on. Thanks for your patience :) |
kk7ds
left a comment
There was a problem hiding this comment.
Thanks for the updates, this mostly works the way I expect. Lemme have the "lock" on this for a bit if I can and I'll try to make the cleanups and changes I've identified here and squash things down appropriately.
| # Arrow keys and navigation allowed anywhere | ||
| pos = self._text.GetSelection()[1] # get the right pos | ||
|
|
||
| if key in self.SHIFTED_TO_INGNORE and event.ShiftDown(): |
There was a problem hiding this comment.
I'm trying to understand the logic here. What you want to do is only allow shift to be held for the letter digits right? I think it would be more straightforward to put this down in the clause that handles those instead of this pre-filter before any of the other logic. Also, we can use chr(key).isdigit() to handle that and make it a little more obvious what's going on I think.
There was a problem hiding this comment.
On my Win11 dev laptop, the top row of number keys 1,2,3, etc. returns the same key code whether the shift key is pressed of not. So with out that conditional it was allowing the shifted value of the key to be entered into the field. For example it the Shift+4 was typed a '$' would be entered into the field. Which is not an acceptable value for Hex or DTMF text. That is why there is separate ignore lists for Hex and DTMF. (DTMF includes # and *, Hex does not)
I'm sure there would be a simpler way to handle this logic...
| self._text.SetSelection(2, -1) # move beyond the "0x" | ||
|
|
||
| def _on_focus(self, event): | ||
| self._text.Enable(True) |
There was a problem hiding this comment.
This would un-disable a field if we had it disabled for some reason. Is this left over from experimentation or here for some reason?
There was a problem hiding this comment.
This is an attempt to keep the "0x" part of the HexText from being selected when the editor get focus. Otherwise that prefix gets selected and is subject to overtyping when editing the int value.
| wx.CallAfter(self._select_text) | ||
| event.Skip() | ||
|
|
||
| def _on_char(self, event): |
There was a problem hiding this comment.
I think it's probably better to do this in KEY_DOWN if we can to avoid catching both of those. Did you try changing the key in the event by chance before you went this route? I can give it a shot.
| return | ||
|
|
||
|
|
||
| class DTMFText(HexText): |
There was a problem hiding this comment.
I'll also try to refactor the base behaviors to include an optional prefix and let Hex and DTMF inherit from that so we don't imply that DTMF is a special case of Hex. And, I think we can avoid a bunch of the duplication below if we make it general.
Who knows, maybe we'll need an OctalText at some point.
There was a problem hiding this comment.
OctalText, maybe... :)
What I could use now is a way for a driver to ask the UI to prompt for a radio programming password. The UI would the have to pass the collected value back to the driver so the value could be used in the enter_programming() phase of the handshake. So something like a PasswordText editor field or a call to wx.PasswordEntryDialog could be used to prompt for the password value. I'm not sure how the UI could then pass that value to the driver.
|
|
||
| class HexText(wx.propgrid.PGTextCtrlEditor): | ||
| HEX_CODES = [ord(x) for x in '0123456789abcdefABCDEF'] | ||
| SHIFTED_TO_INGNORE = [ord(x) for x in '1234567890'] |
You've got the Conn! |
|
Sorry, I've let this languish too long. If you can fix the typos and squash let's just not hold this up any further. If you could drop comments on the obvious things (like the |
Add new RadioSettingValue Classes and for Hex and DTMF Values
The inspiration for this enhancement comes from a recent driver change that added a Hex value column to the memory grid and there were problems getting the value validation methods to work properly without causing unhandled exceptions.
This addition to the CHIRP UI functionality allows a consistent and easy approach to entering, editing, formatting and validating Hex and DTMF values. These new input fields work just like all the other RadioSettingValueX classes and accept values to assist in validation of input and formatting current values. GUI feedback is provided for values that don't pass validation rules and values.
It has always been a chore to add validation and formatting code when adding Hex or DTMF values to a CHIRP driver for mem.exrta columns or settings. Especially when using a string input then having to format, parse and validate the input.
There are many drivers that have had to deal with the rigors of entering, formatting and validation Hex and DTMF values. Each one has always had to come up with some novel way to handle this reoccurring need. And as such, the result and presentation of this type of information can and will differ.
The new added Classes are:
RadioSettingValueHex
Only accepts input of valid hex values (0-9, A-F) in either upper or lower case or with a 0X or 0x prefix. Both min and max values and min and max length and step can be specified. All output is consistently formatted with a 0x prefix and all hex digits are converted to uppercase.
This new input field can be used for memory columns or settings with Hex values like FHSS Codes or APRS values and the like. Use it anywhere it makes sense to prompt for a value in Hex notation.
RadioSettingValueDTMF
Only accepts input of valid DTMF character values (0-9, A-D, #,*) in either upper of lower case. Both a min and max length can be specified. All output is consistently formatted with all DTMF characters converted to uppercase.
This new input field can be used for memory columns or settings with DTMF values like PTT IDs or DTMF code values. Use it anywhere it makes sense to prompt for a literal DTMF code string value.
Usage:
The new Classes have to be imported to used:
And a validated mem.extra column can be created with:
For a validated Setting value here is a somewhat more complex example that prompts for a series of 15 DTMF values: