[chirp_devel] Kenwood TM-D710G

[Dan Smith]

Hi Rick,

Sorry for the delayed response on this. It's big and so I was waiting for a slot where I could look at it in at least some detail. I definitely appreciate your work on this and your patience.

> New driver for Kenwood TM-D710G clone mode, with Py3 compliance.
> This driver uses the future/bytes() method and MemoryMapBytes call.

Have you actually run this under py3? AFAICT, it can't work, but see below. It is *syntactically* compliant though.

Also, this seems to fail several tests even on python2:

>   Kenwood TM-D710G_Clon Detect      PASSED: All tests
>   Kenwood TM-D710G_Clon Settings    PASSED: All tests
>   Kenwood TM-D710G_Clon Clone       PASSED: All tests
>   Kenwood TM-D710G_Clon Edges       FAILED: Field `name' is `!"#', expected ` !"#'
>   Kenwood TM-D710G_Clon BruteForce CRASHED: 159.8 is not in list
>   Kenwood TM-D710G_Clon CopyAll     FAILED: <88>: Field `mode' is `FM', expected `AM'
>   Kenwood TM-D710G_Clon CopyAll     FAILED: <89>: Field `mode' is `FM', expected `AM'
>   Kenwood TM-D710G_Clon CopyAll     FAILED: <90>: Field `mode' is `FM', expected `AM'
>   Kenwood TM-D710G_Clon CopyAll     FAILED: <91>: Field `mode' is `FM', expected `AM'
>   Kenwood TM-D710G_Clon CopyAll     FAILED: <92>: Field `mode' is `FM', expected `AM'
>   Kenwood TM-D710G_Clon CopyAll     FAILED: <93>: Field `mode' is `FM', expected `AM'
>   Kenwood TM-D710G_Clon Banks      SKIPPED: Banks not supported

Did they run for you?

> +svers = sys.version_info[0]      # saved for debugging
> +HAS_FUTURE = True
> +if svers < 3:      # Don't import bytes if vers >= 3
> +    try:                         # PY3 compliance
> +        from builtins import bytes
> +    except ImportError:
> +        HAS_FUTURE = False

There's no need to do this. You can assume future is in the python3 build because it's in requirements.txt. So just try to import it and bail if it's not there. No need to behave differently on python3 (like the tk8180 example).

Also, the reason we're doing the import guard behavior for python2 is really just because of the MacOS build being basically frozen in time until we get the new python3-based build going. Otherwise, everything else should have it.

> +def _dly(tdly):
> +    """ Pause for tdly (float) secs """
> +    ts = time.time()
> +    while (time.time() - ts) < tdly:
> +        xx = 0      # NOP

This is not a reasonable way to wait. It will cause chirp to burn 100% CPU until the delay expires. If you need to pause, use time.sleep() which yields to the operating system.

> +    return

This return is not needed.

> +def command(ser, cmd, rsplen, w8t=0.01):
> +    """Send cmd to radio via ser"""
> +    # cmd is output string with possible terminator
> +    # rsplen is expected response char count, NOT incl prefix and term
> +    #       If rsplen = 0 then do not read after write
> +    ser.write(cmd)

Here, cmd needs to be a bytes() but you're passing in a string in many places. I was able to poke this to prove the point by just trying to download using this driver on py3, which gives me:

> Traceback (most recent call last):
>   File "/home/dan/dev/py3chirp/chirp/drivers/tmd710g.py", line 642, in sync_in
>     _connect_radio(self)
>   File "/home/dan/dev/py3chirp/chirp/drivers/tmd710g.py", line 412, in _connect_radio
>     radio.pipe.write(TERM)
>   File "/usr/lib/python3/dist-packages/serial/serialposix.py", line 518, in write
>     d = to_bytes(data)
>   File "/usr/lib/python3/dist-packages/serial/serialutil.py", line 58, in to_bytes
>     raise TypeError('unicode strings are not supported, please encode to bytes: %r' % (seq,))
> TypeError: unicode strings are not supported, please encode to bytes: '\r'
> ERROR: Failed to clone: Unexpected error communicating with the radio

Because cmd is a string (TERM in this case).

> +    LOG.debug("PC->RADIO [%s]" % cmd)
> +    _dly(w8t)
> +    result = ""

You start with result being a string here...

> +    if rsplen > 0:  # read response
> +        result = ser.read(rsplen)
> +        LOG.debug("RADIO->PC [%s]" % result)
> +    return result

...but here it's a bytes() if rsplen was nonzero. That's going to make your code that calls this hard to get right. You should always return a str() or bytes() here, but I'd recommend the latter if the input cmd is going to be a bytes.

> +def _connect_radio(radio):
> +    """Determine baud rate and verify radio on-line"""
> +    global BAUD        # Declaration allows modification
> +    bauds = [57600, 38400, 19200, 9600]
> +    if BAUD > 0:
> +        bauds.insert(0, BAUD)    # Make the detected one first
> +
> +    for bd in bauds:
> +        radio.pipe.baudrate = bd
> +        BAUD = bd
> +        # Flush the input buffer
> +        radio.pipe.timeout = 0.005
> +        junk = radio.pipe.read(256)
> +        radio.pipe.timeout = STIMEOUT
> +
> +        LOG.debug("Trying %i baud ID..." % BAUD)
> +        radio.pipe.write(TERM)

TERM is a str(), so this will fail because write() expects a bytes().

> +        radio.pipe.read(25)
> +        resp = command(radio.pipe, bytes("ID" + TERM), 24, W8S)
> +        if resp.find("?") >= 0:     # repeat it

This is just a nit, but you should use "b'?' in resp" here to be more pythonic.

Also, why are you reimplementing this here instead of using kenwood_live.get_id() ? It's okay to import that module to use that routine here if you need it, even though you're not making a live radio. If it's different in some subtle way, then I guess it's okay, but if it's not it'd be better to use the common version.

> +# Use a base class to allow for HAS_FUTURE registration at EOF
> +class KenwoodTMx710Radio(chirp_common.CloneModeRadio):
> +    """ Base class for TMD-710G """
> +    VENDOR = "Kenwood"
> +    MODEL = "TM-x710"
> +    ID = "TM-D710G"
> +    _upper = 999         # Number of normal chans
> +
> +    _num_blocks = 2
> +    _packet_size = 261
> +    _block_addr = [0, 0x100, 0x200]       # starting addr, each block
> +    _num_packets = [0x7f, 0x0fe, 0x200]   # num packets per block

So I don't have to refer back to your previous patch to compare, can you confirm that you're reading the full radio memory (minus maybe the APRS log) into the image that we discussed before? It looks like you've changed this code, so I assume you did, but I didn't dig in deep enough to figure it out specifically.

Thanks for moving the special memories later, I think that part looks good. Obviously the tests on python2 need to pass before we can put this in. I'd really also like to try to resolve the python3 bits before we put it into the tree to avoid adding one to the number of drivers that require conversion. If it becomes too onerous then I guess we can put it in when it passes on python2, but with an eye to fix it in short order for python3.

--Dan