[chirp_devel] [PATCH 1 of 3] [TG-UV2+] adding missing log messages in set_settings()
# HG changeset patch # User Ran Katz rankatz@gmail.com # Date 1660469089 -10800 # Sun Aug 14 12:24:49 2022 +0300 # Node ID d0caf6c542c5cbe33cfb69e9edd978f4fdbad8e1 # Parent 0a7f0562b8c59ec96ee4f9724818694094a1f7c5 [TG-UV2+] adding missing log messages in set_settings()
diff --git a/chirp/drivers/tg_uv2p.py b/chirp/drivers/tg_uv2p.py --- a/chirp/drivers/tg_uv2p.py +++ b/chirp/drivers/tg_uv2p.py @@ -703,10 +703,13 @@ LOG.debug("using apply callback") element.run_apply_callback() elif setting == "beep_tone_disabled": + LOG.debug("Setting %s = %s" % (setting, not int(element.value))) setattr(obj, setting, not int(element.value)) elif setting == "busy_lockout": + LOG.debug("Setting %s = %s" % (setting, not int(element.value))) setattr(obj, setting, not int(element.value)) elif setting == "keyunlocked": + LOG.debug("Setting %s = %s" % (setting, not int(element.value))) setattr(obj, setting, not int(element.value)) elif element.value.get_mutable(): LOG.debug("Setting %s = %s" % (setting, element.value))
# HG changeset patch # User Ran Katz rankatz@gmail.com # Date 1660474607 -10800 # Sun Aug 14 13:56:47 2022 +0300 # Node ID 3eb8629bf6d76415605f6dc9004226c42d1bf55f # Parent d0caf6c542c5cbe33cfb69e9edd978f4fdbad8e1 [tg_uv2p] (revised) Inhibit keypad lock when in dual watch or crossmode. Fixes forth issue in #9939 Per Dan's suggestions, implemented with an exception (rather than a flag/basd on set_settings order)
diff --git a/chirp/drivers/tg_uv2p.py b/chirp/drivers/tg_uv2p.py --- a/chirp/drivers/tg_uv2p.py +++ b/chirp/drivers/tg_uv2p.py @@ -27,6 +27,7 @@ RadioSettingValueFloat, RadioSettingValueMap, RadioSettings from textwrap import dedent
+ LOG = logging.getLogger(__name__)
mem_format = """ @@ -703,14 +704,35 @@ LOG.debug("using apply callback") element.run_apply_callback() elif setting == "beep_tone_disabled": - LOG.debug("Setting %s = %s" % (setting, not int(element.value))) + LOG.debug("Setting %s = %s" % (setting, + not int(element.value))) setattr(obj, setting, not int(element.value)) elif setting == "busy_lockout": - LOG.debug("Setting %s = %s" % (setting, not int(element.value))) + LOG.debug("Setting %s = %s" % (setting, + not int(element.value))) setattr(obj, setting, not int(element.value)) elif setting == "keyunlocked": - LOG.debug("Setting %s = %s" % (setting, not int(element.value))) + if int(element.value): + if (getattr(obj, "rxmode") != 0x02): + LOG.debug("Setting %s = %s" % (setting, True)) + setattr(obj, setting, True) + raise errors.InvalidValueError( + "Keypad lock not allowed in " + "Dual-Watch or CrossMode") + LOG.debug("Setting %s = %s" % (setting, + not int(element.value))) setattr(obj, setting, not int(element.value)) + elif setting == "rxmode": + if int(element.value) != 2: + if not (getattr(obj, "keyunlocked")): + LOG.debug("Setting %s = %s" % (setting, + "Normal")) + setattr(obj, setting, 0x02) + raise errors.InvalidValueError( + "Dual-Watch or CrossMode can not be set " + "when keypad is locked") + LOG.debug("Setting %s = %s" % (setting, element.value)) + setattr(obj, setting, element.value) elif element.value.get_mutable(): LOG.debug("Setting %s = %s" % (setting, element.value)) setattr(obj, setting, element.value)
elif setting == "keyunlocked":
LOG.debug("Setting %s = %s" % (setting, not int(element.value)))
if int(element.value):if (getattr(obj, "rxmode") != 0x02):LOG.debug("Setting %s = %s" % (setting, True))setattr(obj, setting, True)raise errors.InvalidValueError("Keypad lock not allowed in ""Dual-Watch or CrossMode")LOG.debug("Setting %s = %s" % (setting,not int(element.value))) setattr(obj, setting, not int(element.value))
Since you're always raising, I think this is likely to be confusing because it's going to raise an exception to people who haven't changed that setting, and abort making the actual change they want, right?
Meaning, let's say I have a radio with the settings already set to the incompatible state (i.e. in dual-watch and keylock enabled). I open up the settings to make a change to something completely different like the backlight timer or something. When I do that, the UI is going to try to set all the settings, including the two incompatible ones that I didn't touch. I'll get an error and then potentially my backlight setting won't be changed if it is later in the list than the keylock one above (since the exception will abort our loop).
So I think you need some additional logic to check both stored values before you raise to avoid this breakage, Something like:
if setting_keylock: if is_dualwatch and now is_keylocked and setting_keylock.value == locked: raise Error
So basically, only raise the exception if they're changing keylock (or dual watch) to a bad combination. It looks like you're also asserting the keylock off before you raise, which would fix an existing broken radio, right? That probably makes sense to include in there regardless, but I think you should remove the explicit setting from this section:
elif setting == "rxmode":if int(element.value) != 2:if not (getattr(obj, "keyunlocked")):LOG.debug("Setting %s = %s" % (setting,"Normal"))setattr(obj, setting, 0x02)raise errors.InvalidValueError("Dual-Watch or CrossMode can not be set ""when keypad is locked")LOG.debug("Setting %s = %s" % (setting, element.value))setattr(obj, setting, element.value)
I was going to say that this will "fight" the other one and end up with both values changed in a radio with those flags conflicting in memory. However, it won't - only the first one to be encountered will do that, but which one will get flipped is not deterministic. I think flipping off the keylock is the better "fix the problem for them" approach.
--Dan
Hi Dan, Thanks again,
I will submit a patch shortly (running tests now) with the added "change" logic you proposed. I also removed the explicit "good" setting from both settings - as the radio is not really broken - it is functional but is locked in the not normal) mode you are in and cannot be unlocked from the keypad - only from chirp, so if the user has such a setting , maybe it is intentional? furthermore it will keep the UI sync'd with the actual setting.
Is there a way to raise an "informative" message ? (not an exception that will abort the set_settings loop) so that the user can be notified he has a "special" configuration?
Ran
On Sun, Aug 21, 2022 at 6:13 PM Dan Smith via chirp_devel < chirp_devel@intrepid.danplanet.com> wrote:
elif setting == "keyunlocked":
LOG.debug("Setting %s = %s" % (setting, notint(element.value)))
if int(element.value):if (getattr(obj, "rxmode") != 0x02):LOG.debug("Setting %s = %s" % (setting,True))
setattr(obj, setting, True)raise errors.InvalidValueError("Keypad lock not allowed in ""Dual-Watch or CrossMode")LOG.debug("Setting %s = %s" % (setting,notint(element.value)))
setattr(obj, setting, not int(element.value))Since you're always raising, I think this is likely to be confusing because it's going to raise an exception to people who haven't changed that setting, and abort making the actual change they want, right?
Meaning, let's say I have a radio with the settings already set to the incompatible state (i.e. in dual-watch and keylock enabled). I open up the settings to make a change to something completely different like the backlight timer or something. When I do that, the UI is going to try to set all the settings, including the two incompatible ones that I didn't touch. I'll get an error and then potentially my backlight setting won't be changed if it is later in the list than the keylock one above (since the exception will abort our loop).
So I think you need some additional logic to check both stored values before you raise to avoid this breakage, Something like:
if setting_keylock: if is_dualwatch and now is_keylocked and setting_keylock.value == locked: raise Error
So basically, only raise the exception if they're changing keylock (or dual watch) to a bad combination. It looks like you're also asserting the keylock off before you raise, which would fix an existing broken radio, right? That probably makes sense to include in there regardless, but I think you should remove the explicit setting from this section:
elif setting == "rxmode":if int(element.value) != 2:if not (getattr(obj, "keyunlocked")):LOG.debug("Setting %s = %s" % (setting,"Normal"))
setattr(obj, setting, 0x02)raise errors.InvalidValueError("Dual-Watch or CrossMode can not beset "
"when keypad is locked")LOG.debug("Setting %s = %s" % (setting,element.value))
setattr(obj, setting, element.value)I was going to say that this will "fight" the other one and end up with both values changed in a radio with those flags conflicting in memory. However, it won't - only the first one to be encountered will do that, but which one will get flipped is not deterministic. I think flipping off the keylock is the better "fix the problem for them" approach.
--Dan
chirp_devel mailing list chirp_devel@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
Is there a way to raise an "informative" message ? (not an exception that will abort the set_settings loop) so that the user can be notified he has a "special" configuration?
Nope, we'd have to refactor the contract quite a bit to do that I think, unfortunately.
--Dan
# HG changeset patch # User Ran Katz rankatz@gmail.com # Date 1660477131 -10800 # Sun Aug 14 14:38:51 2022 +0300 # Node ID 542344db60eb230eedc8962dc49175daa0ee3a53 # Parent 3eb8629bf6d76415605f6dc9004226c42d1bf55f [tg_uv2p] (revised) Dont set a priority channel if empty or Broadcast-FM. Fixes fifth issue in #9939 Per Dan's suggestion changed implementation to a validation function and an exception to notify user of wrong input
diff --git a/chirp/drivers/tg_uv2p.py b/chirp/drivers/tg_uv2p.py --- a/chirp/drivers/tg_uv2p.py +++ b/chirp/drivers/tg_uv2p.py @@ -508,10 +508,16 @@ mem_vals.insert(0, 0xFF) user_options.insert(0, "Not Set") options_map = zip(user_options, mem_vals) - - rs = RadioSetting("priority_channel", "Priority Channel", - RadioSettingValueMap(options_map, - _settings.priority_channel)) + if _settings.priority_channel >= 200: + _priority_ch = 0xFF + else: + _priority_ch = _settings.priority_channel + rs = RadioSetting( + "priority_channel", + "Priority Channel \n" + "Note: Unused channels,\nor channels " + "in the\nbroadcast FM band,\nwill not be set", + RadioSettingValueMap(options_map, _priority_ch)) cfg_grp.append(rs)
# Step @@ -679,6 +685,17 @@
return group
+ def _validate_priority_ch(self, ch_num): + if ch_num == 0xFF: + return True + _mem, _bf, _nam = self._get_memobjs(ch_num) + if (_mem.freq.get_raw()[0] == "\xFF") or (_bf.band == "\x0F"): + return False + elif _bf.band == 0x00: + return False + else: + return True + def set_settings(self, settings): for element in settings: if not isinstance(element, RadioSetting): @@ -733,6 +750,18 @@ "when keypad is locked") LOG.debug("Setting %s = %s" % (setting, element.value)) setattr(obj, setting, element.value) + elif setting == "priority_channel": + _check = self._validate_priority_ch(int(element.value)) + if _check: + LOG.debug("Setting %s = %s" % (setting, + element.value)) + setattr(obj, setting, element.value) + else: + raise errors.InvalidValueError( + "Please select a valid priority channel:\n" + "A used memory channel which is not " + "in the Broadcast FM band (88-108MHz),\n" + "Or select 'Not Used'") elif element.value.get_mutable(): LOG.debug("Setting %s = %s" % (setting, element.value)) setattr(obj, setting, element.value)
participants (2)
-
Dan Smith -
Ran Katz