Re: [tuxdroid-user] Daemon to USB connection

[neimad <ror...@gm...>]

"David Bourgeois" <da...@ja...> writes:

> Hi Damien and the others,
>
> I'm finally looking at the daemon more seriously as I'm adding the
> sleep  and ID stuff now. There are a couple things I'd like to
> discuss.
>
> First, I'm writing some code in the daemon and as I'm still a beginner
> at  linux programming, I would appreciate any kind of feedback
> regarding my  commits, would it be the style, the kind of functions I
> use, the  structure, anything I could do better is welcome to point
> out.

Well, since you asked... In the recent firmware commits:

  * Several lines exceed the 80 column limit (as stated in the coding
    style guidelines) quite a bit (100+).

  * There's a *big* "if else if else if ..." on command[0] which could
    be replaced by a switch.

  * Some types have all uppercase names, but this should be reserved
    for macro constants only. Besides, the naming of some enums types
    and their values could be more consistent.

That's from the top of my head, I'll post more as I read the code.

> For the current code, it seems that by removing the USB status thread
> at  revision 322, the cmd_status_flag (which should probably have been
> a  mutex) is never reset when the usb_write_TuxDroid() function is
> called. To  understand what's happening, I need to describe how the
> USB/RF protocol  works.

That's something I'll have to check. When I removed the threads, I had
a strange feeling about the locking that I wasn't able to pinpoint at
that time, so I may have overlooked something...

Right now, what happens is that select() has a timeout of 50000 =C2=B5s,
and when it times out, it calls update_tux_status():

        ...
        r =3D select(fdmax + 1, &rset, NULL, NULL, &timeout);
        if (r =3D=3D 0)
        {
            if (!update_tux_status())
        ...

This triggers the following calls:

    update_tux_status()
      \_ usb_get_status_TuxDroid()
          \_ update_system_status_table()
               =3D> cmd_status_flag =3D 0

Two things worth of note:

  (1) Something I intended to fix but forgot about: currently, if
      select() always has data to process, the timeout may well never
      happen. We have to ensure that update_tux_status() is called
      every 50000 =C2=B5s, *no matter what*.

  (2) I got the 50000 =C2=B5s from the pre-r322 code. Not sure how it fits
      the 2ms you mention below.

That's just after a quick look, I'll have to read the code and my
commit more thoroughly.

> Each 2ms, a frame is sent from the RF module(I) (in the dongle) to the
> USB  chip. This frame contains a few RF and dongle status (in which
> the RF ACK  is), the microphone sound data and 4 bytes of raw status
> from tux. Here's  what's happening when sending some raw data to tux:

[...]

> I don't necessarily want to revert the thread and to improve the
> communication I think we should divide usb_write_TuxDroid() in 2
> functions, one that would send the raw, the other that would wait for
> the  ack

I'd rather avoid using threads, as it eludes portability problems (see
the NSLU2 problem). We can do just the same with a state machine using
select().

> 1. either they're called alternately and we can't send a raw if the
> ack is  not received (as of now)
> 2. either we can send a second command while waiting for the ack of
> the  first one; we can't send more than 2 commands while waiting for
> the ack  otherwise it's possible that it will overwrite the previous
> command.
>
> I'd like to implement 2. and also add a buffer for the incoming
> commands  (from the API or anything else).

Solution 2 would indeed be more optimized, but is it necessary ? I
mean, do we have performance problems, could we do with 2 commands
flying things we can't do know ?

> As of now the API sends one command to the daemon, waits for the ACK
> and  store it in a variable which is not used by the applications,
> this  explains why the daemon is still working. The ACK is always
> TIMEOUT but  that doesn't matter for the application.
>
> Any thought about this before I start working on it? (Yes I know I
> have to  add the protocols on the wiki ;-) )

> Finally, I find the file structure quite strange, there's only main.c
> in  the root folder, then all files are under /libs and all names
> start with  USBDaemon. I don't want to change this right now but any
> idea on what a  good structure and naming conventions would be?

Agreed, we'll have to shuffle things around quite a bit ;-)

	Damien