[chirp_devel] [PATCH] [FT-70] New Model #5329 Increased second block size

[Dan Smith]

> The FT70 has a built in USB interface The Chirp driver is only intended to be used with this, no baud rate is set on it. Using a higher baud rate made a slight difference in speed, But initially thought this might now be causing other issues, so changed back to 38400. 

That is very surprising to me. Every other USB serial device I know of honors the baud rate as expected. Unless the radio is really mirroring whatever it can tell the USB serial bridge is set to, but that would be the first such device I've ever encountered.

> At 38400 or 15200 baud blocks were being returned with no data in them.

So the problem is with _reading_ from the radio, not writing to it? In almost all cases of flaky "it works on my system but not his" cases, it was writing to the radio that was the problem. This is usually because the radio has subtle timing requirements (or a small buffer with no flow control). So, odd thing #2.

> Investigated further with Fred and Mark
> We are losing blocks at the beginning in the Chirp yaesu_clone method.
> 
> [2018-03-19 21:35:41,483] chirp.drivers.yaesu_clone - DEBUG: Read 0/65920
> [2018-03-19 21:35:41,733] chirp.drivers.yaesu_clone - DEBUG: Read 0/65920
> [2018-03-19 21:35:41,986] chirp.drivers.yaesu_clone - DEBUG: Read 0/65920
> [2018-03-19 21:35:42,236] chirp.drivers.yaesu_clone - DEBUG: Read 0/65920
> 
> I thought the rest of the download was working, hence the puzzle of how are we short at the end. Which did not make sense if we were only losing blocks at the beginning...

These aren't blocks, they're chunks, and the yaesu_clone code assumes that each chunk of the whole block can and will be read. Sounds like that code should at _least_ assert that it read the length expected, because if it doesn't, it will silently return less data than the block requested. In reality, it should probably be smarter and plan to read the total number of bytes regardless of how many trips it takes, and have a overall timeout to catch the case where we don't hear anything from the radio.

> After increasing the size last night it worked and then we realised that blocks must be being returned with no data during the rest of the download. Slight confusion of the debug info as that shows the running total - not the bytes read for each block - errors will show as an unchanged value not as zero... Loaded it into a spreadsheet this morning and behold we are losing a dozen or so blocks...
> 
> Two examples
> 
> [2018-03-19 21:18:16,013] chirp.drivers.yaesu_clone - DEBUG: Read 49152/65920
> [2018-03-19 21:18:16,263] chirp.drivers.yaesu_clone - DEBUG: Read 49152/65920
> 
> [2018-03-19 21:18:16,489] chirp.drivers.yaesu_clone - DEBUG: Read 53248/65920
> [2018-03-19 21:18:16,739] chirp.drivers.yaesu_clone - DEBUG: Read 53248/65920
> 
> etc. Note the read figure has not changed, so as you said that block read did not return any data.

Yeah, so what I'm saying about changing the behavior of _chunk_read() above would apply I think.

> I submitted a patch to increase the size - for the time being - but it seems that yaesu_clone needs looking at.

Okay, so I had this backwards because I thought you were talking about uploading not downloading. So I understand why that being larger makes a difference, but it'd be wrong for anyone that doesn't stall at the beginning. I guess the same stupidness in _chunk_read() makes that not completely hang for those people, but...not the right solution :)

> chirp sets the time out to .25 secs I changed it to 2 secs and seems to work.

Yes, and some drivers have to change this because of their radios (although it's always on write AFAIK). You could do that, but I think making the change I described above to _chunk_read() would be better as it would allow you to keep a low timeout (which is desirable) and also not return a short block if you happen to hit a stall.

Something like this completely untested change:

diff -r 61ba9c815170 chirp/drivers/yaesu_clone.py
--- a/chirp/drivers/yaesu_clone.py      Thu Mar 15 23:41:07 2018 +0000
+++ b/chirp/drivers/yaesu_clone.py      Wed Mar 21 16:05:01 2018 -0700
@@ -43,14 +43,24 @@


 def _chunk_read(pipe, count, status_fn):
+    timer = time.time()
     block = 32
     data = ""
-    for _i in range(0, count, block):
-        data += pipe.read(block)
-        if data:
+    while len(data) < count:
+        # Don't read past the end of our block if we're not on a 32-byte
+        # boundary
+        chunk_size = min(block, count - len(data))
+        chunk = pipe.read(chunk_size)
+        if chunk:
+            timer = time.time()
+            data += chunk
             if data[0] == chr(CMD_ACK):
                 data = data[1:]  # Chew an echo'd ack if using a 2-pin cable
                 # LOG.debug("Chewed an ack")
+        if time.time() - timer > 2:
+            # It's been two seconds since we last saw data from the radio,
+            # so it's time to give up.
+            raise errors.RadioError("Timed out reading from radio")
         status = chirp_common.Status()
         status.msg = "Cloning from radio"
         status.max = count

--Dan