[chirp_devel] [PATCH] [New Model] Support for the BTECH Mobile Radios, update 1 for #3015
Jim Unroe
Sun Mar 27 16:41:49 PDT 2016
On Sun, Mar 27, 2016 at 5:42 PM, Dan Smith via chirp_devel
<chirp_devel at intrepid.danplanet.com> wrote:
>> 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
> _______________________________________________
> chirp_devel mailing list
> chirp_devel at intrepid.danplanet.com
> http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
> Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
More information about the chirp_devel
mailing list