[chirp_devel] [PATCH] [New Model] Support for the BTECH Mobile Radios, update 1 for #3015

[Dan Smith]

> This patch includes the following changes:
> 
> Changes related to developer mailing list discussions
> - Less intensive strategy to get big chunks of data instead the former more
>   fine grained but resource intensive one
> - Removed our software serial timeout in favor of the system serial timeout
> - Removed the reference of the second magic for the alpha radio
> - Renamed the _do_magic() def
> - Restructured the "super nested" code to a less nested one
> - Removed the "not needed" time.sleep() before the read of just one ACK, in
>   favor of a serial system timeout
> - Removed/changed some of the LOG.xxx()
> - Removed the exception that goes to the UI about maxtime
> - Renamed the base class as BTech (from btech)
> 
> Bug fixes
> - Fixed the bug found by minuteman1120 about the duplex not working in some
>   cases
> - New "split" algorithm for the duplex bug above
> - Fixed a bug with the scode about 0x*F reseted to 0x*0
> 
> New
> - Added support for the QYT KT-UV980 Radio

This is a lot to digest in a single patch. It'd be great to separate
some of the changes so we don't have to block some of the
critical/obvious stuff on the other that needs some discussion.

> +def _clean_buffer(radio):
> +    """Cleaning the read serial buffer"""
> +    radio.pipe.setTimeout(0.1)
> +
> +    try:
> +        dump = radio.pipe.read(100)
> +
> +    except Exception:
> +        raise errors.RadioError("Unknown error cleaning the serial buffer")

This won't really flush the inbound reliably. You need to loop until you
read at least once with nothing returned (i.e. hit the timeout). If
there are 101 bytes in the pipe, this will not read them all out.
Similarly, if there are ten bytes and the first read returns five of
those, you won't clear the rest. Remember that read() does not have to
return all the bytes you asked for.

>  def _rawrecv(radio, amount):
> -    """Raw read from the radio device, new approach, this time a byte at
> -    a time as the original driver, the receive data has to be atomic"""
> +    """Raw read from the radio device, less intensive way"""
> +
>      data = ""
>  
>      try:
> -        tdiff = 0
> -        start = time.time()
> -        maxtime = amount * 0.020
> -
> -        while len(data) < amount and tdiff < maxtime:
> -            d = radio.pipe.read(1)
> -            if len(d) == 1:
> -                data += d
> -
> -            # Delta time
> -            tdiff = time.time() - start
> -
> -            # DEBUG
> -            if debug is True:
> -                LOG.debug("time diff %.04f maxtime %.04f, data: %d" %
> -                          (tdiff, maxtime, len(data)))
> +        # Getting the data with a dynamic timeout in the serial deppending on
> +        # the amount of data we need
> +        timeout = amount * 0.06

This really doesn't make any sense: the value of timeout doesn't need to
be greater if you're reading more data. The select() call that pyserial
is making doesn't know how much data you're asking for, it only reports
whether there is *any* data to be read from the descriptor. Further,
pyserial cuts down the timeout each time through the read loop, if it
has to go again. That means that if you're a little late on the first
read loop, your timeout is cut down even further in the second and
subsequent loops. Eventually that will end up cutting your timeout down
to very small (i.e. less than a byte interval) value, which will make
this unreliable. It might be good to review the implementation of
pyserial's read code:

https://github.com/pyserial/pyserial/blob/master/serial/serialposix.py#L470-L516

This is why you need to be processing incoming data into a queue if
you're having trouble with the timing bits. Relying on the timeouts like
you are is only going to work in lucky situations where you start your
read with enough timeout remaining to process the full block.

> +def _start_clone_mode(radio, status):
> +    """Put the radio in clone mode and get the ident string
> +    using multiple tries"""
>  
>      # how many tries
> -    tries = 5
> +    tries = 3
>  
>      # prep the data to show in the UI
>      status.cur = 0
>      status.msg = "Identifying the radio..."
> -    status.max = len(radio._magic) * tries
> +    status.max = tries * len(radio._magic)
>      radio.status_fn(status)
>      mc = 0
> +    radio.pipe.setTimeout(0.6)
> +
> +    def send_magic_word(m, magic):

Can you break this out to module level instead of defining it here? It's
a little long and deep as a closure -- I didn't realize it was separate
during my first read through the patch.

> +        """Send the magic word to the radio, catching the answer"""
> +
> +        # cleaning the buffer and set timeout
> +        _clean_buffer(radio)

You do the buffer purge here...

>  def _do_ident(radio, status):
> @@ -366,28 +352,21 @@
>      #  set the serial discipline
>      radio.pipe.setBaudrate(9600)
>      radio.pipe.setParity("N")
> -    radio.pipe.setTimeout(0.005)
> -    # 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)
> -        raise errors.RadioError(msg)
> +
> +    # cleaning the buffers
> +    _clean_buffer(radio)

But you've already done it here, just a line before you call the above
method which does it again:

>      # do the magic trick
> -    if _do_magic(radio, status) is False:
> +    if _start_clone_mode(radio, status) is False:

Once should be enough if it's properly implemented :)

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

s/sort/short/

>          raise errors.RadioError(msg)
>  
>      # check if ident is OK
> @@ -402,7 +381,9 @@
>          msg = "Incorrect model ID, got this:\n\n"
>          msg += util.hexprint(ident)
>          LOG.debug(msg)
> -        raise errors.RadioError("Radio identification failed.")
> +        error = "Radio identification failed, this is not the correct model, "
> +        error += "or it's a not supported variant yet."
> +        raise errors.RadioError()

Did you mean to include error in this RadioError() ?

> +    # set a delay based on the OS
> +    if sys.platform in ["win32", "cygwin"]:
> +        # we are in windows
> +        delay = 0.015
> +    else:
> +        # we are in linux/mac
> +        delay = 0.033

Where does this come from? Nowhere else in the CHIRP source do we have
to adjust timing like this based on the platform. I strongly object to
this without real justification.

> +        _send(radio, _make_frame("X", addr, TX_BLOCK_SIZE, d), delay)

This just makes _send() delay by 15 or 33ms *after* the write, which
makes no sense. If you need to delay, do it here before you do the read.

>          # receiving the response
>          ack = _rawrecv(radio, 1)

Jim, I get the impression that you are largely working on the memory
decode bits and Pavel is working on the cloning part? If so, it'd be
cool if you could just send your smaller tweaks (if possible) as
separate patches so we can get them landed and fixed for people while we
discuss the cloning bits separately.

Pavel, does your internet connection prevent you from sending patches to
the list for some reason?

Thanks!

--Dan