[chirp_devel] New driver for Kenwood TM-D710G in true clone mode

[Dan Smith]

> +def upd8_stat(self, stat, stp=1):
> +def mak_addr(bk1, bka, sbn, knt):

This kind of thing really bugs me. This should be "update_status" and "stop". Saving a few characters is not worth the loss of readability in 2019.

> +def _read_mem(radio):
> +    """ Load the memory map """
> +    # UI progress
> +    status = chirp_common.Status()
> +    status.cur = 0
> +    val = 0
> +    for mx in range(0, radio._blks["nblks"]):
> +        val += radio._blks["numpkts"][mx]
> +    status.max = val
> +    status.msg = "Reading %i Blocks" % val
> +    radio.status_fn(status)
> +
> +    data = ""
> +    rdcnt = 261
> +    cmc = "0M PROGRAM" + TERM
> +    resp0 = command(radio.pipe, cmc, 3, W8S)
> +    radio.pipe.baudrate = 57600     # PROG mode is always 57.6
> +    junk = radio.pipe.read(16)     # trailing byte

Because of the comment, I assume you're not really expecting 16 bytes, but rather just one. Correct? If so, I think you should just read the one and validate that you got it. If this fails to read anything due to a race, you'll just proceed and try to read from the radio and be out of sync. This is the kind of thing that tends to introduce "works on linux, not windows" sorts of bugs, not because of any real incompatibility, but just because of small timing differences.

> +    blk0 = 0
> +    for bkx in range(0, 0x07f):
> +        cmc = "R" + mak_addr(blk0, bkx, 0, 0)
> +        resp0 = command(radio.pipe, cmc, rdcnt, W8S)
> +        if len(resp0) < rdcnt:

Actually, if you hit the above, you might be out of sync such that you're reading data off-by-one and creating a bad image internally, which will then get sent to the radio if someone tries to upload it later.

> +            junk = command(radio.pipe, "E", 0, W8S)
> +            raise errors.RadioError("Packet %i read error, %i bytes."
> +                                    % (bkx, len(resp0)))

Most everywhere else we say "block" for this instead of "packet" so unless there's a good reason, it would be good to stick to the convention.

> +def _write_mem(radio):
> +    """ Send MW commands for each channel """
> +    # UI progress
> +    status = chirp_common.Status()
> +    status.cur = 0
> +    val = 0
> +    for mx in range(0, radio._blks["nblks"]):
> +        val += radio._blks["numpkts"][mx]
> +    status.max = val
> +    status.msg = "Writing %i Blocks" % val
> +    radio.status_fn(status)
> +
> +    imgadr = 0
> +    resp0 = command(radio.pipe, "0M PROGRAM" + TERM, 3, W8S)
> +    radio.pipe.baudrate = 57600
> +    # ?? raise.error if resp0 != "0M 0d" ??
> +    junk = radio.pipe.read(16)

Looks like in _write_mem() you had the right idea :)

> +    # Read magic header thingy, save it
> +    blk0 = radio._blks["pktadr"][0]
> +    cmc = "R" + mak_addr(blk0, 0, 0, 4)
> +    resp0 = command(radio.pipe, cmc, 16, W8S)
> +    mht = resp0[5:]
> +    # LOG.warning("Magic Header Thingy: %02x %02x %02x %02x"
> +    #     % (ord(mht[0:1]), ord(mht[1:2]), ord(mht[2:3]), ord(mht[3:4])))

Please either log this (at debug) or remove this commented out code. When reading without syntax highlighting it's very easy to miss that code is commented out and wonder why something isn't happening. If you think it's useful, it's fine to log it at debug level (although probably s/thingy//).

> + at directory.register
> +class TMD710G_CRadio(chirp_common.CloneModeRadio):
> +    """ Kenwood TM-D710G VHF/UHF/GPS/APRS Radio """
> +    VENDOR = "Kenwood"
> +    MODEL = "TM-D710G_CloneMode"
> +    ID = "TM-D710G"
> +    _upper = 999         # Number of chans
> +    # Only reading first block, up to 7e for now

What does this mean? Are you not actually reading the whole radio? If so, you'll be creating an incompatibility later when you start reading the whole thing.

> +    _blks = {"nblks": 1,        # Number of addressed blocks
> +             "pktsz": 261,      # packet size
> +             "pktadr": [0, 0x100, 0x200],   # starting addr, each block
> +             "numpkts": [0x7f, 0x0fe, 0x200]}   # num packets per block

I'm not sure why this is a dict instead of just attrbutes on the class like the others. Why not just:

  class TMD710:
      ...
      _nblks = 1
      _pktsz = 261
      _pktadr = [0, 0x100, 0x200]
      _numpkts = [0x7f, 0x0fe, 0x200]

?

And, same as above, please spell these out closer to their proper words. There's no need to save the bytes and make the reader wonder if "pktsz" means "packet size" or "packets Z".

> +    @classmethod
> +    def get_prompts(cls):
> +        rp = chirp_common.RadioPrompts()
> +        rp.info = _(dedent("""
> +            This version implements the 'Clone' mode using the MCP
> +            data transfer method..

I'm not sure that "MCP data transfer method" means anything useful to the users. I think "Clone mode" is enough.

> +            The 2 Call and 10 VFO memory channels are displayed when
> +            the 'Special Chans' tab is toggled.

I'm going to make another plea to not put this in the instructions for the radio. This time, it's because this is already wrong for the py3 branch  because the new UI does not and will not have a "special chans" button. That blending of view and control is exactly why I don't want this in the radio instructions themselves. If you won't remove it, at least s/tab/button/ because it is a button, not a tab.

> +            For duplex 'Split' mode: enter the TX freq in Offset, and
> +            the TX step in the Properties > Other tab.

This too is generic chirp help and not driver specific, and mentions elements of the UI.

> +            Note: Window updates take a while, due to the 1000 channel
> +            memory size, so set the Memory Range values in the menu bar
> +            to your desired channels.

This too is wrong for the py3 branch, and is definitely very UI specific and not related to the driver itself.

> +            """))
> +        rp.pre_download = _(dedent("""\
> +            Follow these instructions to download the radio memory:
> +            1 - Connect your interface cable to the PC Port on the
> +            back of the 'TX/RX' unit. NOT the Com Port on the head.
> +            2 - Radio > Download from radio: Don't adjust any settings
> +            on the radio head!
> +            """))
> +        rp.pre_upload = _(dedent("""\
> +            Follow these instructions to upload the radio memory:
> +            1 - Connect your interface cable to the PC Port on the
> +            back of the 'TX/RX' unit. NOT the Com Port on the head.
> +            2 - Radio > Upload to radio: Don't adjust any settings
> +            on the radio head!
> +            """))

In both of these cases, the user isn't seeing this until they have already done Radio->Download right? So I'm not sure how this is applicable at the stage in which it's displayed. That, and it mentions UI elements again.

Maybe it's not clear, but the chirp/drivers modules are all intended to be completely generic API, and usable by a developer without the UI, or in a different way than intended. That clean separation is exactly why it is even _possible_ for us to be developing a new UI in parallel to the old one, and is a design decision that I very much want to keep.

> +    def sync_in(self):
> +        """Download from radio"""
> +        try:
> +            _connect_radio(self)
> +            data = _read_mem(self)
> +        except errors.RadioError:
> +            # Pass through any real errors we raise
> +            resp = command(self.pipe, "E", 0, W8S)     # Exit Program mode
> +            raise
> +        except Exception:
> +            # If anything unexpected happens, make sure we raise
> +            # a RadioError and log the problem
> +            LOG.exception('Unexpected error during download')
> +            raise errors.RadioError('Unexpected error communicating '
> +                                    'with the radio')
> +
> +        self._mmap = memmap.MemoryMap(data)

Please use MemoryMapBytes so that you're not creating a new driver that doesn't work on the py3 branch. Once you do, please check that it runs tests on that branch and fix up any issues you find. At some point, we'll need to stop accepting submissions that create new delta between the two branches and since you're a seasoned CHIRP developer, hopefully you'll help lead by example here :)

> +        self.process_mmap()
> +        return

This return is pointless, please remove.

> +    def sync_out(self):
> +        """Upload to radio"""
> +        try:
> +            _connect_radio(self)
> +            _write_mem(self)
> +        except Exception:
> +            # If anything unexpected happens, make sure we raise
> +            # a RadioError and log the problem
> +            LOG.exception('Unexpected error during upload')
> +            raise errors.RadioError('Unexpected error communicating '
> +                                    'with the radio')
> +        return

...this one too.

> +    def process_mmap(self):
> +        """Process the mem map into the mem object"""
> +        self._memobj = bitwise.parse(MEM_FORMAT, self._mmap)
> +        return

...and this one.

> +    def get_memory(self, number):
> +        """Convert raw channel data (_mem) into UI columns (mem)"""
> +        mem = chirp_common.Memory()
> +        mem.extra = RadioSettingGroup("extra", "Extra")
> +        if isinstance(number, str):
> +            mem.name = number   # Spcl chns 1st var
> +            mem.number = self.SPECIAL_MEMORIES[number]
> +            mem.extd_number = number    # Uses name as LOC

Please don't set extd_number for non-special channels. I'd have to go look, but I think this is already triggering some re-labeling in the regular UI that you don't want, and I think I check for this in the new UI to trigger it. Just only do this for specials.

> +            if mem.number < -12:

So in the new UI, special channels are always displayed and because of the way the grid works, it will be massively simpler if all radio drivers use MAX+N numbering for the specials. Since this radio appears to actually put them there, can you use their native number ordering in this so we don't have to convert this later?

> +    def set_memory(self, mem):
> +        """Convert UI column data (mem) into MEM_FORMAT memory (_mem)"""
> +        if mem.number < 0:      # Special chans
> +            # This line is required to prevent name column being blank
> +            mem.name = self.SPECIAL_MEMORIES_REV[mem.number]

This is not something we do anywhere else and is not appropriate, IMHO. Let the UI decide how to display this, don't fake it like it is actually set to this, that's the point of extd_number. I'm also not sure why you're changing anything on memory during set_memory() -- you're probably changing something the UI owns because it's passed by reference.

If you're trying to defeat the user's attempts to set the mem.name on a special channel, you need to make name immutable on the memory in get_memory().

> +    def get_settings(self):
> +        """Translate the MEM_FORMAT structs into settings in the UI"""

<snip>

> +        def my_mhz_val(setting, obj, atrb, ndx=-1):
> +            """ Callback to set freq back to Htz"""
> +            vx = float(str(setting.value))
> +            vx = int(vx * mhz1)

Please use chirp_common.to_MHz() and from_MHz()

> +        def my_bool(setting, obj, atrb, ndx=-1):
> +            """ Callback to properly set boolean """
> +            # set_settings is not setting [indexed] booleans???
> +            vx = 0
> +            if str(setting.value) == "True":

Why are you doing this? Stringifying a bool and comparing it to a string defeats the entire point of using a boolean type.

> +        def mak_str(chrx):
> +            """ Python wierdness: convert char array to string """
> +            #  chrx is char array
> +            stx = ""
> +            for sx in chrx:
> +                if int(sx) > 31 and int(sx) < 127:
> +                    stx += chr(sx)
> +            return stx
> 

What is this trying to do? Filter to ascii? Also, please s/mak/make/.

> +        def pswd_vfy(setting, obj, atrb):
> +            """ Verify password is 1-6 chars, numbers 1-5 """
> +            stx = str(setting.value)[0:6].strip()
> +            sty = ""
> +            for chx in stx:
> +                if ord(chx) < 49 or ord(chx) > 53:
> +                    raise errors.RadioError("Bad characters in Password")
> +            for ix in range(1, 6):       # append ff
> +                stx += chr(255)
> +            sty = stx[0:6]
> +            setattr(obj, atrb, sty)
> +            return

If you would spell out whatever "stx" and "sty" are it would be easier to follow what is going on here. Is the password just A-F, 1-5? If so, it may be far easier to just do this:

 
 sty = filter(lambda c: c in 'ABCDEF12345', stx[:6])
 sty.ljust(6, '\xff')
 if sty != stx:
     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

As discussed, just return False here and rely on the metadata always for new drivers. And no need to mention "the file->new error" every single time we add this. It's just the behavior that all new drivers should have.

That's all for now. Thanks for doing this work, it's clear that it was a massive undertaking. I've got one of these radios, so I'm happy to see this getting done. If it weren't for the concern above about not downloading all of the radio contents I would have tested this myself. On the next rev I will, based on the answer to that question.

--Dan