[chirp_devel] New FT-405D Driver

[Dan Smith]

Hi Rick,=

> Attached is the patch file for the new Yaesu FT-450D driver.
> Please let me know what fails if it doesn't pass the style check, I scrubbed it as well as I can with Windows and Notepad++ and I am trying to keep notes of what fails.

I need you to send me (or better, point me to) an image of the radio so I can run tests. Style checks appear to pass.

> Ditto for the patch file format.

(see below)

> # HG changeset patch
> # Parent  0cbedd969bad9e0635e58d138266add5edbf2779
> New Yaesu FT-450D Driver
> 
> Submitting new driver in support of issues #0951, #3427 and #3621

Can you just pick one of these and close the others as duplicates of the one you choose? Unfortunately, users don't search before they file :/

> user: Rick DeWitt <aa0rd at yahoo.com>
> branch 'default'
> added chirp/drivers/ft450d.py


It looks to me like you're just constructing the patch header by hand, which isn't the right way. Either use patchbomb to email or "hg export tip" to generate the proper patch.

> diff -r 0cbedd969bad -r c2f84de00484 chirp/drivers/ft450d.py
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/chirp/drivers/ft450d.py	Mon Jun 04 10:39:30 2018 -0700
> @@ -0,0 +1,1481 @@
> +# Copyright 2018 by Rick DeWitt (aa0rd at yahoo.com>
> +# Adapted from FT817 driver by Filippi Marco <iz3gme.marco at gmail.com>

Is there any of that driver you can re-use verbatim? If so we really should as this is a lot of stuff to copy and I assume quite a bit of it is similar, yeah?
> 
> + at directory.register
> +class FT450DRadio(yaesu_clone.YaesuCloneModeRadio,
> +                         chirp_common.ExperimentalRadio):

Indentation is wrong here. I dunno why the style checker didn't catch this but I did! :)

> +    @classmethod
> +    def get_prompts(cls):
> +        rp = chirp_common.RadioPrompts()
> +        rp.experimental = ("The FT-450D driver supports 'Special Channels' "
> +                           "and the 'Other' tab of memory properties.")

The rp.experimental field is for warning the user about stuff that might *not* work or be fully implemented. Doesn't sound like this is useful for that? If not, it should be omitted.

> +        rp.pre_download = _(dedent("""\
> +            1. Turn radio off.
> +            2. Connect cable to ACC jack.
> +            3. Press and hold in the [MODE &lt;] and [MODE &gt;] keys while
> +                 turning the radio on ("CLONE MODE" will appear on the
> +                 display).
> +            4. <b>After clicking OK</b>, press the [C.S.] key to
> +               send image."""))
> +        rp.pre_upload = _(dedent("""\
> +            1. Turn radio off.
> +            2. Connect cable to ACC jack.
> +            3. Press and hold in the [MODE &lt;] and [MODE &gt;] keys while
> +                 turning the radio on ("CLONE MODE" will appear on the
> +                 display).
> +            4. Click OK.
> +                ("Receiving" will appear on the LCD)."""))
> +        return rp
> +
> +    def _read(self, block, blocknum):
> +        # be very patient at first block
> +        if blocknum == 0:
> +            attempts = 60

This equates to 30 seconds to get the clone started, right? Seems like we should wait almost forever for the first block, given how difficult Yaesu makes it to get the clone started...

> +        else:
> +            attempts = 5
> +        for _i in range(0, attempts):
> +            data = self.pipe.read(block + 2)

What is block + 2? A leader byte and a checksum byte or something?

> +            if data:
> +                break
> +            time.sleep(0.5)
> +        if len(data) == block + 2 and data[0] == chr(blocknum):
> +            checksum = yaesu_clone.YaesuChecksum(1, block)
> +            if checksum.get_existing(data) != \
> +                    checksum.get_calculated(data):
> +                raise Exception("Checksum Failed [%02X<>%02X] block %02X" %
> +                                (checksum.get_existing(data),
> +                                 checksum.get_calculated(data), blocknum))
> +            # Chew away the block number and the checksum
> +            data = data[1:block + 1]
> +        else:
> +            raise Exception("Unable to read block %i expected %i got %i"
> +                            % (blocknum, block + 2, len(data)))

This should be one of the RadioError type exceptions, not a generic one. I think this will fail tests the way it is, unless it's not reachable in our current test cases.
> 
> +    def _clone_in(self):
> +        # Be very patient with the radio
> +        self.pipe.timeout = 2

This is _too_ patient :)

Can you knock this down to something lower and do the same graceful start as above?

> +        self.pipe.baudrate = self.BAUD_RATE
> +        self.pipe.bytesize = self.COM_BITS
> +        self.pipe.parity = self.COM_PRTY
> +        self.pipe.stopbits = self.COM_STOP
> +        self.pipe.rtscts = False
> +
> +        start = time.time()
> +
> +        data = ""
> +        blocks = 0
> +        status = chirp_common.Status()
> +        status.msg = _("Cloning from radio")
> +        nblocks = len(self._block_lengths) + 100

What is this +100?

> +        status.max = nblocks
> +        for block in self._block_lengths:
> +            if blocks == 8:
> +                # repeated read of 101 block same size (memory area)
> +                repeat = 101
> +            else:
> +                repeat = 1
> +            for _i in range(0, repeat):
> +                data += self._read(block, blocks)
> +                self.pipe.write(chr(CMD_ACK))
> +                blocks += 1
> +                status.cur = blocks
> +                self.status_fn(status)
> +
> +        status.msg = _("Clone completed, checking for spurious bytes")

"Spurious bytes" means you've gotten out of sync or something.

> +        self.status_fn(status)
> +        moredata = self.pipe.read(2)
> +        if moredata:
> +            raise Exception(
> +                    _("Radio sent data after the last awaited block."))

Does this happen often enough that you really need to check for it here, or was this just for debugging?
> 
> +    def process_mmap(self):
> +        self._memobj = bitwise.parse(self.MEM_FORMAT, self._mmap)
> +
> +    def get_features(self):
> +        rf = chirp_common.RadioFeatures()
> +        rf.has_bank = False
> +        rf.has_dtcs= False
> +        if MEM_GRP_LBL:
> +            rf.has_comment = True   # Used for Mem-Grp number
> +#        rf.has_nostep_tuning = True

If you don't mind, just remove these commented code lines. They're confusing to look at without syntax highlighting. If the thing isn't implemented yet, then don't put it in here. We can always revise this later to add stuff.
> 

> +    def get_settings(self):
> +        _settings = self._memobj.settings
> +        _beacon = self._memobj.beacontext
> +        gen = RadioSettingGroup("gen", "General")
> +        cw = RadioSettingGroup("cw", "CW")
> +        pnlcfg = RadioSettingGroup("pnlcfg", "Panel buttons")
> +        pnlset = RadioSettingGroup("pnlset", "Panel settings")
> +        voxdat = RadioSettingGroup("voxdat", "VOX and Data")
> +        mic = RadioSettingGroup("mic", "Microphone")
> +        mybands = RadioSettingGroup("mybands", "My Bands")
> +        mymodes = RadioSettingGroup("mymodes", "My Modes")
> +
> +        top = RadioSettings(gen,  cw, pnlcfg, pnlset, voxdat, mic,
> +                            mymodes, mybands)
> +
> +        def invert_me(setting, obj, atrb):
> +            """Callback: from inverted logic 1-bit booleans"""
> +            invb = not setting.value
> +            setattr(obj, atrb, invb)
> +            return
> +
> +# - - - Gen ---

It would be better if you just created helpers for these groups to break things up, instead of commenting off blocks. Just do this here:

  self._do_general_settings(gen)

and put all the code below in a so-titled method.

--Dan