[Hamlib-developer] Proposed patch for tranceive hanging

[Chuck H. <n2...@am...>]

Short description:

Here's a patch to fix my tranceive problem a a bit more.  (More like a re-write
of a small piece of the icom frame handling).  Feel free to use it, ignore it,
use pieces of it, ask me to change and/or extend it, or whatever.

---

Long description:

I went to try to fix my problems with tranceive hanging in icom_decode, and
also saw the comment in read_icom_frame:
         * However, an automate with a state model would be more efficient..

In doing this I was almost ready to write a new routine, then I noticed
read_string was close to what I wanted:
1. It didn't do any reads without a select (which solved my tranceive problem)
2. It could read until a given set of stop characters (as in: FI or COL)
3. It only printed out the complete frame with dump_hex rather than the start
   followed by each additional character.
However, it didn't support one thing I needed:
1. Support for 00 as a character in the block.

So I copied it and created a new routine "read_block_until" (feel free to
change the name) for reading a block (that can include 00) up until a set of
stop characters (which may or may not include 00).  Then I changed
read_icom_block and read_icom_frame to call this new routine.
(see docs at top of read_block_until for more information about it)

I tested it, and it seems to work fine here with my R7000.

Feel free use it, parts of it, change it, or whatever.

Since I didn't know what format you wanted it in, I attached a patch in 
"diff -urN" format.
I also attached copies of the the new read_block_until and modified
read_icom_frame and read_icom_block to make them easier to read and/or do
something else with them.
(tried just sticking them in the message, but my email program decided to word
 wrap them, which would have made it difficult to do anything but read them)

If you want anything different, let me know.

---

A few other things:

1. read_icom_block and read_icom_frame are very similar now.  I didn't know
   what you had planned for each of them.

2. I didn't add any additional checking for collisions or timeouts after
   receiving at least one character.  Timeouts before the first character are
   handled as before.

   Example collision/timeout processing:

   rxlen = read_icom_block(
   or 
   rxlen = read_icom_frame(

   if(rxlen <= 0)
   {
        /* normal error handling including timeouts before the first character*/
        /* should never be 0 */
   } else {
      select(rxbuffer[rxlen-1])
      {
         case FI:  /* Normal frame */
         case COL: /* Collision */
         default:  /* timeout after receiving at least one character */
      }

   If you would like me to work on this, let me know.  Also, I didn't notice an
   error message for a collision.  Didn't know if you wanted a new one, or fit
   it in one of the others.

   Note: The docs I have say the radio sends the collision/jammer code 5 times,
   waits for no more incoming data and then resends the frame.  Just a word of
   warning for anyone trying to implement collision recovery.

   BTW With a bit a playing I did cause a collision.   In the trace, I saw
   multiple calls to icom_decode in a row:

icom_decode: tranceive cmd unsupported 0x05
sa_sigioaction: activity detected
icom: icom_decode called
RX 1 characters
0000     fc     .
icom_decode: CI-V 0x8 called for 0xe0!

   Now, this error comes from the fact that it's trying to look at parts of the
   buffer that are not initialized.  With my current xterm buffer size, I
   couldn't scroll back far enough to see which routine got the collision first.

   I'll come up with a better trace if need be.

3. Just a note to anyone who does want to try to implement resend on
   error/collisions:

   It would be nice if there was a way to turn it off.

   A collision could easily be caused by someone turning the knob while the
   software is trying to do something to the radio.  In that case, my
   software would prefer to know that the knob was turned rather than the
   command was done correctly so it doesn't lose any knob turns.  I understand
   that many other pieces of software would prefer their commands to work and
   don't want to have to worry about handling the errors.

   (Well, the error handling in my software is fairly simple right now.  :)
    It just does a get after every set, and uses the last successful get as the
    number to check the new calculated value against when deciding to an
    update. 
    Also, I have implemented a semi-atomic update that does a get, checks to see
    if the value is the same as the old value, and if it is, then it does a set
    followed by another get.  This helps catch any knob changes that it might
    miss due to delays and such)

4. The old read_icom_frame didn't didn't worry about the max rxbuffer length. 
   This is a good thing to check just in case the radio/ci-v bus decided to
   send lots of garbage.  Since this new version does check, I looked at a
   bunch of the callers.
   Most of them call it with a buffer of size of 16, however icom_decode calls
   it with a buffer size of 32.  This should probably either be passed as an
   argument to read_icom_frame, a #define'ed constant, or at least consistent. 
   For now, I set it to 16 in read_icom_frame.  This will cause problems if
   icom_decode_frame gets a frame between 17 and 32 characters (Not seen in
   my testing)

5. Because I needed a character array with FI and COL in it in two different
   routines and couldn't figure out how to use a #define to create it, I
   defined it as a "static const char" in "icom/frame.c" (not inside of
   either of the routines, but not EXTERN either).
   If you have any better ideas of what you want to do with it, go ahead and
   change it or let me know.

6. In testtrn I added a printf of the idle loop counter so as to better show
   that it's working.  Otherwise you can't tell if something is hung in
   tranceive handing routines without waiting to see if the program exits
   after 120 seconds.  Feel free to ignore/change this if you don't want it.

7. read_block_until uses read rather than fread.  I don't know if it was
   changed to use fread in the past for a reason, but sharing the same
   standard i/o file descriptor between "normal" and signal handler contexts
   makes me nervous.  Someone else may have a better idea of what is allowed.
   You also could add buffering directly to read_block_until, but to do that
   you would need:
   a. A place to store characters read, but beyond the end of the current frame.
   b. You might want to think a bit more about how you want to share a ci-v bus
      between multiple items.  With this, right now, non-tranceive stuff
      probably works because it doesn't read anything it doesn't handle.
      return i;

8. If you don't like memchr (I needed a routine to that treated 00 as a normal
   char), it could easily be replaced by a for loop.