[chirp_devel] Kenwood TM-D710/G driver
Dan Smith
Thu Apr 16 16:38:34 PDT 2020
> Woo-Hoo!
> Finally; the Kenwood TM-D710 /D710G combined driver patch file is attached, with the manifest and test images.
I've been wondering if you were still working on this -- glad to hear you are.
> Should be Py3 syntax compliant using bytes() and HAS_FUTURE global.
> It took forever due to having to test D710 model by email > Load Module....
Note that it fails the settings test on python3. I haven't looked into why, but it seems like a bytes/str issue:
> def _pswd_vfy(setting, obj, atrb):
> """ Verify password is 1-6 chars, numbers 1-5 """
> str1 = str(setting.value).strip() # initial string
> str2 = filter(lambda c: c in '12345', str1) # valid chars
> if str1 != str2:
> # Two lines due to python 73 char limit
> sx = "Bad characters in Password"
> > raise errors.RadioError(sx)
> E chirp.errors.RadioError: Bad characters in Password
If it's easier I can just try to figure this out later.
> +def _dbg_dump(stb, kn=32, cptn="DBG", ax=False):
> + """ Debugging tool to print up to kn hex values of byte array stb """
> + # ax: flag to append ascii
> + global DMPKNT
> + sx = ""
> + stz = stb.decode("latin-1") # bytes to string
> + kx = len(stz)
> + if kx > kn:
> + kx = kn
> + for ix in range(kx):
> + sx += "%02x " % ord(stz[ix])
> + if ax: # append as ascii
> + sx += " ["
> + for ix in range(kx):
> + if ord(stz[ix]) > 31 and ord(stz[ix]) < 127:
> + sx += "%s" % stz[ix]
> + else:
> + sx += "."
> + sx += "]"
> + if DMPKNT > 0:
> + if DMP & 1:
> + LOG.warning("%s: %s" % (cptn, sx))
> + if DMP & 2:
> + LOG.debug("%s: %s" % (cptn, sx))
> + if DMP & 4:
> + DMPKNT -= 1
> + return
First off, you might want to look at util.hexprint(), which will do the hex dumping for you. Second, this really isn't the way to do this -- using a global. If you think it's important to log this much, just do it at debug level. If debug logging is turned on, you'll see it, and if it's not, you won't. That's the whole point of the level-based logger. Just doing this will cause you to see it in the terminal:
$ CHIRP_DEBUG=y ./chirpw
or set it in your environment on Windows.
> +def _connect_radio(radio):
> + """Determine baud rate and verify radio on-line"""
> + global BAUD # Declaration allows modification
> + bauds = [57600, 38400, 19200, 9600]
> + xid = radio.MODEL.split("_")[0]
> +
> + if BAUD > 0:
> + bauds.insert(0, BAUD) # Make the detected one first
Why can't you use kenwood_live.get_id() for this?
> +def _read_mem(radio):
> + """ Load the memory map """
> + global DMPKNT # to allow mod
> + status = chirp_common.Status()
> + status.cur = 0
> + val = 0
> + for mx in range(0, radio._num_blocks):
> + val += radio._num_packets[mx]
> + status.max = val
> + status.msg = "Reading %i packets" % val
> + radio.status_fn(status)
> + DMPKNT = 25
You're setting this immediately, even though it's global. So you're just using this to communicate with the dump function through a global right? That makes it non-reentrant, which isn't okay. Please, just log debug info at debug level and use that to control what we do or don't see. Can you explain what you're trying to do with this signaling so maybe I can help suggest something more conventional?
> +
> + data = bytes()
> +
> + cmc = "0M PROGRAM" + TERM
> + resp0 = _command(radio.pipe, cmc, 3, W8S)
> + if radio.SHORT == "G": # D710G
> + radio.pipe.baudrate = 57600 # PROG mode is always 57.6
> + LOG.debug("Switching to 57600 baud download.")
> + junk = radio.pipe.read(1) # trailing byte
> + for blkn in range(0, radio._num_blocks):
> + for bkx in range(0, radio._num_packets[blkn]):
> + cmc = "R" + _make_address(radio.SHORT,
> + radio._block_addr[blkn], bkx, 0, 0)
> + resp0 = _command(radio.pipe, cmc,
> + radio._packet_size[blkn], W8S)
> + if len(resp0) < radio._packet_size[blkn]:
> + junk = _command(radio.pipe, "E", 0, W8S)
> + sx = "Block %x %x read error, %i bytes." % \
> + (blkn, bkx, len(resp0))
> + raise errors.RadioError(sx)
This error message (and several others) will be pretty cryptic for the user. Can you just log.error() all the details and then raise a message like "Error reading block from radio" ?
> + return ""
This return never gets run, because it's after a raise.
> + if blkn == 0 and bkx == 0: # 1st packet of 1st block
> + mht = resp0[5:9] # Magic Header Thingy after cmd echo
> + data += mht[0:1]
> + data += chr(255) + chr(255) + chr(255)
Why are you adding things to the data we're returning, other than exactly what we got from the radio? Every Kenwood I've implemented clone mode for has had a very straightforward "give me 0x40 bytes of memory at address 0x1234" protocol, which seems to be the case here. I'm not sure where all this complexity here comes from. The resulting chirp image should be basically the same as what the MCP software stores (aside from some header info), assuming it's like all the other Kenwood OEM software I've used. Have you compared your image to that to make sure you're faithfully representing the radio's memory layout?
> + data += resp0[9:]
> + else:
> + data += resp0[5:] # skip cmd echo
> + _update_status(radio, status) # UI Update
> + # Exit Prog mode, no TERM
> + resp = _command(radio.pipe, "E", 0, W8S)
> + else: # D710
> + junk = radio.pipe.read(16) # flushit
> + for bkx in range(0, 0x09c):
> + if bkx != 0x07f: # Skip block 7f !!??
> + cmc = "R" + chr(bkx) + chr(0) + chr(0)
> + resp0 = _command(radio.pipe, cmc, 260, W8S)
> + junk = _command(radio.pipe, ACK, 1, W8S)
> + if len(resp0) < 260:
> + junk = _command(radio.pipe, "E", 2, W8S)
> + sx = "Block %x read error, %i bytes." % \
> + (bkx, len(resp0))
> + raise errors.RadioError(sx)
> + return ""
> + if bkx == 0: # 1st packet of 1st block
> + mht = resp0[4:7] # [57 00 00 00] 03 4b 01 ff ff ...
> + data = resp0[5:6] # Store as 4b 4b 01 ff ff ff ...
> + data += resp0[5:]
Why are you doing this? Why not just:
data = resp0[5:]
? Aren't you repeating the [5] byte in what you're adding into data?
> + else:
> + data += resp0[4:] # skip cmd echo
> + _update_status(radio, status) # UI Update
> + DMPKNT = 10 # Reset dump count to do these
> + cmc = "R" + chr(0x0fe) + chr(0x0f0) + chr(0x010)
> + resp0 = _command(radio.pipe, cmc, 0x014, W8S)
> + data += resp0[4:]
> + junk = _command(radio.pipe, ACK, 1, W8S)
> + _update_status(radio, status)
> + cmc = "R" + chr(0x0ff) + chr(0) + chr(0x090)
> + resp0 = _command(radio.pipe, cmc, 0x094, W8S)
> + data += resp0[4:]
> + junk = _command(radio.pipe, ACK, 1, W8S)
> + _update_status(radio, status)
> + # Exit Prog mode, no TERM
> + resp = _command(radio.pipe, "E", 2, W8S) # Rtns 06 0d
> + radio.pipe.baudrate = BAUD
This function is basically two completely separate functions with a giant if..else in the middle. I would rather just have two functions, one for the 710 and one for the 710G. You can put it on the actual class for each if it makes it easier. Even better if you can reduce them down further so only the difference in behavior is conditional.
> diff -r 37379cfd09d9 -r 7b2152434e12 tools/cpep8.manifest
> --- a/tools/cpep8.manifest Mon Mar 02 20:28:45 2020 -0600
> +++ b/tools/cpep8.manifest Tue Apr 14 11:15:22 2020 -0700
> @@ -82,6 +82,7 @@
> ./chirp/drivers/thuv1f.py
> ./chirp/drivers/tk8102.py
> ./chirp/drivers/tk8180.py
> +./chirp/drivers/tmvd710.py
Note you typo'd this, but I corrected it locally and it passed the style check.
I'm really concerned about the clone routines getting a faithful copy of the radio's memory, as I've mentioned before. The complexity in those routines being somewhat suspect based on what I know of other models, as well as the thing I mentioned above about returning data that isn't actually in the radio (or the duplicated byte) are things I really want to make sure we get right. Other stuff in the actual driver that won't affect the stability of the file format we can work on later. However, if we're not pulling out the memory exactly, then we'll have images which could corrupt the radio if we push them in later after changing/fixing the clone routines.
--Dan
More information about the chirp_devel
mailing list