On Sun, Mar 27, 2016 at 5:42 PM, Dan Smith via chirp_devel chirp_devel@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 = 0start = time.time()maxtime = amount * 0.020while len(data) < amount and tdiff < maxtime:d = radio.pipe.read(1)if len(d) == 1:data += d# Delta timetdiff = time.time() - start# DEBUGif 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 needtimeout = amount * 0.06This 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-...
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 windowsdelay = 0.015- else:
# we are in linux/macdelay = 0.033Where 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@intrepid.danplanet.com http://intrepid.danplanet.com/mailman/listinfo/chirp_devel Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers