[chirp_devel] [PATCH] [New Model] Support for the BTECH Mobile Radios, fixes issue #3015

[Dan Smith]

Hi Jim and Pavel,

Thanks for your work on this, I know it's in high demand and has been a
while coming. I have some concerns, detailed below:

> +def _rawrecv(radio, amount):
> +    # Get the header + basic sanitize
> +    hdr = _rawrecv(radio, 4)
> +    if len(hdr) != 4:
> +        msg = "Short header for block: 0x%04x" % addr
> +        LOG.error(msg)
> +        raise errors.RadioError(msg)
> +
> +    # receive and validate the header
> +    c, a, l = struct.unpack(">BHB", hdr)
> +    if a != addr or l != BLOCK_SIZE or c != ord("X"):
> +        msg = "Invalid answer for block 0x%04x:" % addr
> +        LOG.error(msg)
> +        LOG.debug("CMD: %s  ADDR: %04x  SIZE: %02x" % (c, a, l))
> +        raise errors.RadioError(msg)
> +
> +    # Get the data
> +    data = _rawrecv(radio, l)

This kind of fine-grained reading is hard on performance -- it would be
better if you can read larger chunks of data, buffer it and split it up
as you find frames.

> +def _do_magic(radio, status):
> +    """Try to put the radio in program mode and get the ident string
> +    it will make multiple tries"""

I think maybe renaming this at some point would be good.

> +
> +    # how many tries
> +    tries = 5
> +
> +    # prep the data to show in the UI
> +    status.cur = 0
> +    status.msg = "Identifying the radio..."
> +    status.max = len(radio._magic) * tries
> +    radio.status_fn(status)
> +    mc = 0
> +
> +    try:
> +        # do the magic
> +        for magic in radio._magic:
> +            # we try a few times
> +            for a in range(0, tries):

This is super deeply nested. I think this should be broken up into
easier-to-understand pieces.

> +                # Update the UI
> +                status.cur = (mc * tries) + a
> +                radio.status_fn(status)
> +
> +                # cleaning the serial buffer, try wrapped
> +                try:
> +                    radio.pipe.flushInput()
> +                except:
> +                    msg = "Error with a serial rx buffer flush at _do_magic"
> +                    LOG.error(msg)
> +                    raise errors.RadioError(msg)
> +
> +                # send the magic a byte at a time
> +                for byte in magic:
> +                    ack = _rawrecv(radio, 1)
> +                    _send(radio, byte)
> +
> +                # A explicit time delay, with a longer one for the UV-5001
> +                if "5001" in radio.MODEL:
> +                    time.sleep(0.5)
> +                else:
> +                    time.sleep(0.1)

I don't think this makes sense. Sleeping before a read (especially of
one byte) is kinda weird. Can't you just set your serial timeout to
cover both of these cases and then read the byte, handling the timeout
if it happens?

> +            # wait between tries for different MAGICs to allow the radio to
> +            # timeout, this is an experimental fature for the 5001 alpha that
> +            # has the same ident as the MINI8900, raise it if it don't work
> +            time.sleep(5)

Yikes, five seconds? I guess it seems like we should be able to avoid
this with different model subclasses.

> +def _do_ident(radio, status):
> +    """Put the radio in PROGRAM mode & identify it"""
> +    #  set the serial discipline
> +    radio.pipe.setBaudrate(9600)
> +    radio.pipe.setParity("N")
> +    radio.pipe.setTimeout(0.005)

I feel like a timeout this low at 9600 baud is probably not very useful.
The inter-byte timing of 9600N81 is about 1ms and your timeout here is
5ms. Other drivers that communicate at this (and faster) speeds are
using timeouts on the order of 100-250ms.

Is there some detail you can offer here?

> +    # cleaning the serial buffer, try wrapped
> +    try:
> +        radio.pipe.flushInput()
> +    except:
> +        msg = "Error with a serial rx buffer flush at _do_ident"
> +        LOG.error(msg)

This should be a LOG.exception() right? Presumably this means something
very strange happened. Also, why do you think this is necessary?

> +    # basic check for the ident
> +    if len(ident) != 49:
> +        msg = "Radio send a sort ident block, you need to increase maxtime."

This goes to the UI, right? What is maxtime? Does it refer to the value
in _rawrecv()?

> +class btech(chirp_common.CloneModeRadio, chirp_common.ExperimentalRadio):

This should be:

  class BTech(...

or something. Starting a class name with a lowercase letter is not
really in line with anyone's style.

> +    @classmethod
> +    def get_prompts(cls):
> +        rp = chirp_common.RadioPrompts()
> +        rp.experimental = \
> +            ('This driver is experimental and for personal use only.\n'

You can't make this requirement of the user based on the license. Please
remove it.

> +             '\n'
> +             'Please keep a copy of your memories with the original software '
> +             'if you treasure them, this is the first release and may contain'
> +             ' bugs.\n'
> +             '\n'
> +             'You will miss the setting tab, we are working on it. Your '
> +             'success/failure story is appreciated, visit the Chirp\'s '
> +             'website and drop us a comment or just say THANKS if it works '
> +             'for you.\n'

This is probably a little too verbose for the experimental warning.

> +# Note:
> +# the order in the lists in the _magic, IDENT and _fileid is important
> +# we put the most common units first, the policy is as follows:
> +
> +# - First latest (newer) units, as they will be the most common
> +# - Second the former latest version, and recursively...
> +# - At the end the pre-production units (pp) as this will be unique

This concerns me a bit. Is there any chance that the value you're using
for identification is actually just a mutable value and we'll be chasing
valid values forever?

I went ahead and applied this because I know that it's high-demand and
that the Jetstream work will likely depend on this for collaboration.
I've also put the test images into the tree.

I don't think this should go out to users with the experimental warning
the way it is, so I will fix that up myself.

I hope that there will be some further iteration on the timing stuff,
possibly with David's help from the Jetstream side.

Thanks!

--Dan