[chirp_devel] Issue #7409 patch

[Dan Smith]

> # HG changeset patch
> # User Rick DeWitt <aa0rd at yahoo.com>
> # Date 1576163136 28800
> #      Thu Dec 12 07:05:36 2019 -0800
> # Node ID 62a26da6eaddf9d563f9a2e2753aec6674a04ed9
> # Parent  b5589aa94c1e6a424d0f713017f68d39caa29be9
> [ts590] Resolving issue #7409
> Add match_model to satisfy File > New csv generation. Improve auto-baud. Unique class ID.

This is too much change, especially for a regression fix.

> diff -r b5589aa94c1e -r 62a26da6eadd chirp/drivers/ts590.py
> --- a/chirp/drivers/ts590.py	Thu Dec 05 21:14:35 2019 +1100
> +++ b/chirp/drivers/ts590.py	Thu Dec 12 07:05:36 2019 -0800
> @@ -19,7 +19,6 @@
>  import logging
>  import re
>  import math
> -import threading

This is gardening, should be consolidated to just a patch of cleanups.

>  from chirp import chirp_common, directory, memmap
>  from chirp import bitwise, errors, util
>  from chirp.settings import RadioSettingGroup, RadioSetting, \
> @@ -121,12 +120,10 @@
>    u8   ex099;
>  } exset[2];
>  
> -  char mdl_name[9];     // appended model name, first 9 chars
> -

This changes the image format. Maybe I missed this before, but is this not part of the actual radio image?

>  """
>  
> +MEMSIZE = 0x0bf8    # Img file size withoud metadata

This is probably not needed (see below). Also "without" is misspelled.

>  STIMEOUT = 2
> -LOCK = threading.Lock()

Chuck this in the gardening patch.

>  BAUD = 0    # Initial baud rate
>  MEMSEL = 0  # Default Menu A
>  BEEPVOL = 4     # Default beep volume
> @@ -163,7 +160,6 @@
>      #       If rsplen = 0 then do not read after write
>  
>      start = time.time()
> -    #   LOCK.acquire()

...and this.

>      stx = cmd       # preserve cmd for response check
>      stx = stx + exts + ";"    # append arguments
>      ser.write(stx)
> @@ -176,7 +172,6 @@
>          result = ser.read(rsplen)
>          LOG.debug("RADIO->PC [%s]" % result)
>          result = result[:-1]        # remove terminator
> -    #   LOCK.release()

...and this.

>      return result.strip()
>  
>  
> @@ -193,15 +188,16 @@
>      radio.pipe.timeout = STIMEOUT
>  
>      for bd in bauds:
> +        junk = radio.pipe.read(16)
>          radio.pipe.baudrate = bd
>          BAUD = bd
>          radio.pipe.write(";")
>          radio.pipe.write(";")
> -        resp = radio.pipe.read(4)
> +        resp = radio.pipe.read(16)
>          radio.pipe.write("ID;")
> -        resp = radio.pipe.read(6)
> -
> -        if resp == radio.ID:           # Good comms
> +        resp = radio.pipe.read(16)
> +        LOG.debug("Radio sent ID: %s for baud %i" % (resp, BAUD))
> +        if resp.find(radio.ID) >= 0:           # Good comms
>              resp = command(radio.pipe, "AI0", 0, W8L)
>              return
>          elif resp in RADIO_IDS.keys():

This change to the detection routine should be in its own patch, with its own justification so we can discuss it. In the py3 branch I was messing with my D700 and the changes you made to this code made it nearly impossible to use one of the regular live models in a single session because of all the delimiter stuff mixed in. I made a couple small tweaks there, which I should probably bring over to the main branch and then we can discuss if the above is really what we want.

> @@ -423,8 +419,7 @@
>          nc += 1
>          status.cur = nc
>          radio.status_fn(status)
> -    setts += radio.MODEL.ljust(9)
> -    # Now set the inidial menu selection back
> +    # Now set the initial menu selection back

Gardening.

>      result0 = command(radio.pipe, "MF", 0, W8L, "%1i" % MEMSEL)
>      # And the original Beep Volume
>      result0 = command(radio.pipe, "EX0050000%2i" % BEEPVOL, 0, W8L)
> @@ -562,10 +557,9 @@
>      return
>  
>  
> -# Bug #7409
> -# @directory.register
> -class TS590Radio(chirp_common.CloneModeRadio):
> -    """Kenwood TS-590"""
> + at directory.register
> +class TS590_CRadio(chirp_common.CloneModeRadio):
> +    """ Kenwood TS-590 using CAT """
>      VENDOR = "Kenwood"
>      MODEL = "TS-590SG_CloneMode"
>      ID = "ID023;"
> @@ -1614,7 +1608,6 @@
>  
>      def set_settings(self, settings):
>          _settings = self._memobj.settings
> -        _mem = self._memobj

Gardening.

>          for element in settings:
>              if not isinstance(element, RadioSetting):
>                  self.set_settings(element)
> @@ -1647,9 +1640,18 @@
>                      LOG.debug(element.get_name())
>                      raise
>  
> +    @classmethod
> +    def match_model(cls, fdata, fyle):
> +        """ Included to prevent 'File > New' error """
> +        # Test the file data size
> +        if len(fdata) == MEMSIZE:
> +            return True
> +        else:
> +            return False

Since your driver is very new, I think you can and should just return False here, and rely on the metadata-based model selection. Wouldn't that solve the problem? We shouldn't be adding entirely new drivers that depend on match_model guessing. Returning False here will cause the directory to never match a file against this unless the metadata specifically says it's for this model.

> +

This adds whitespace, which probably isn't what you want.

>  
>  @directory.register
> -class TS590SRadio(TS590Radio):
> +class TS590S_CRadio(TS590_CRadio):

Why are you changing this? This will cause us to have some old images that reference radio class names that are no longer valid. The directory currently looks at vendor/model for its work, but radio class is in there for the future. Unless there's an important reason to do this, I'd rather we not.

So in summary, if you could please strip out just the changes required to fix the matching bug into one patch (which includes the change to register the driver again), that would be good. Then a patch to do the gardening you want. If you want to hold off on the baud changes I'll bring my changes over to the default branch and then you can see what, if anything, you want to tweak on top of that.

Okay? Thanks for figuring out and working on the problem.

--Dan