>                     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

_______________________________________________
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