Re: statemachine flaws/weaknesses

[Osma Suominen]

On Mon, 24 Feb 2003, Ladislav Michl wrote:

> 1) sm_block_no_retry _does_ resending of last message

Yes. That's actually the one and only reason I wrote the
expect_message() function, now that I think of it.

> 2) __sm_block_timeout will block at _most_ timeout amount of time.
>    select is used to see if a read will not block. this condition is
>    true when at least one character is waiting in input buffer...

Either I don't understand this or then I disagree. According to my
understanding the current __sm_block_timeout() first waits for up to 3
seconds for an acknowledge frame (up to twice, resending the message if
it isn't acknowledged at first), and then waits for up to the specified
timeout for a reply massige to arrive. However, I don't think that
simply one character in the input buffer is enough to break the loop,
because the statemachine has to change state to GN_SM_ResponseReceived,
which only happens when a complete message has been received (after
sm_incoming_function() is called). So unless I've misunderstood
something this is not a problem.

I think you've mixed up the timeout passed to the link.loop(), which is
currently hardcoded to 1 * 100ms, and the timeout used in the while()
loop in the __sm_block_timeout() function. These are different and in
effect the gn_sm_loop (and thus link.loop()) may be called several times
in the current implementation because its timeout is smaller. Now
whether this really is useful behaviour is another issue, but I don't
think it's directly harmful the way you say...

>       sec = t / 1000;
>       usec = (t % 1000) * 1000;
>       timeout.tv_sec = sec;
>       timeout.tv_usec = usec;
>       gettimeofday(&now, NULL);
>       timeradd(&now, &timeout, &next);
>       do {
>               state->link.loop(&timeout, state);
>               /* Some select() implementations will modify the timeval 
> structure */
>               timeout.tv_sec = sec;
>               timeout.tv_usec = usec;
>       } while (timercmp(&next, &now, >) && state->current_state != 
> GN_SM_ResponseReceived);

This is either totally wrong or then correct in some way that I can't
currently grasp :)

>       for (retry = 0; retry < 3; retry++) {
>               err = sm_block_no_retry(waitfor, t, data, state);

This should probably be sm_block_no_retry_timeout, other than that, no
objections...

> this way timeout is always what it ought to be. well, perhaps i missed
> some tweaks around GN_SM_ResponseReceived and GN_SM_WaitingForResponse,
> but above basically does the job. objections?

I certainl like the aspect that no_retry actually means what it says.
But I assume there was a reason for the current behaviour (retry in the
case of no ack). After all the statemachine code was recently rewritten
in this way in order to fix some serious bugs concerning retry
behaviour.

> also i'd like to see non timeout functions to die. suitable timeout is
> certainly different for mbus connection (9600), fbus (115200), atbus
> (19200) and is also phone specific. therefore timeout should me adjusted
> in driver code.

In this case all uses of these functions in all drivers would have to be
modified. Isn't there some way to have a reasonable default? Maybe the
default timeout could be set somewhere in link code, so it would at
least be connection type specific? Otherwise the drivers that support
multiple connections would need a lot of complicated heuristics to
determine a suitable timeout value.

-Osma

-- 
*** Osma Suominen *** address@hidden *** http://www.iki.fi/ozone/ ***