Re: [Hamlib-developer] Proposed patch for tranceive hanging

[Stephane F. <f4...@fr...>]

On Sun, Mar 10, 2002, Chuck Hemker wrote:
> 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.

Short answer:

The icom frame handling has to be completly re-written.
And you're taking a good start.

> ---
> 
> 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..

The automate would be a kind of lexer. Maybe a bit overkill at 1200bauds.

> 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.

yep, this function written by Francois is really handy. It can even handle
some protocols that have optional acknowledge (Kenwood).

> 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)

No need for code duplication. I've added the missing parameter to
read_string. And since this function didn't exist in Hamlib-1.1.2, we
don't break binary compability.
Your patch works fine on my Ic-706MkIIG.

> Since I didn't know what format you wanted it in, I attached a patch in 
> "diff -urN" format.

very good. unified patches are the easiest to read.

> ---
> 
> 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.

the death to read_icom_block. It was a hack to read n bytes. Very
dangerous if you get some collision or transceive bytes in the way.

> 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.

RIG_EPROTO is the only one to fit. However, it's rather pointless to the
user. When having a collision, we should retry rs->retry times, and fail
with an error code. I was thinking of something like RIG_ERESTART, telling
that there was a temporary protocol issue. Do you have a better name in
mind?

And yes, if you'd like to work on this, you're more than welcome!
I don't have time to work on the backend, even though I have a clear idea
of what needs to be done.

>    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.

transceive is largely untested. The more traces, the easier to fix.
A complete rewrite may help also.

> 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.

To my mind, if a collision occurs, there's no way to give priority to
transceive event simply because TX and RX are tighed together, meaning
that data on the CI-V bus is jammed. The controller knows its command
failed (readback mismatch), chances are big the transceive event is lost,
i.e. the rig won't resend it. TBC.

>     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)

Stupid idea: could we have a collision callback? This way the application
can be notified when a transceive event may have been lost.

> 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)

a #define'ed constant or an argument to read_icom_frame would be a must.
Do you have a patch already?

> 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.

That's fine. Anyhow it would end up as a static const char[].

> 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.

Acutally, the loop is a kludge. Or to put it better, the application has
to be well written, i.e. check for errno == ERESTART, which means
"Interrupted system call should be restarted".

Maybe testtrn should be merged in rigctl..

> 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;

The fread was here just because I was doing a lot of 1-byte reads.
Wrong approach. IMO, read_string is the way to go.
Maybe the days of fread are counted. And that would be a good thing.
(Let me be the executioner :)
Buffered reads can save some syscalls, but is it worth it?

Now, talking about the ci-v bus, it's not that simple, I agree with you.
Just imagine the following cases:
* the controller remote controls the rig (basic we're doing already, but
	in ideal case where the following don't happen too much)
* transceive frames received now and then, while doing something else
* one controller and several rigs on the same bus (same serial port)
* multithreaded application (-> serialize Hamlib calls, message
	interleaving and multiple "open" cmds would be a nightmare)
* more than one controller on the bus (-> ignore the noise)



Stephane

PS: BTW, you must know the excellent Ekki's site at the following URL:
		http://www.plicht.de/ekki/civ/