tux-droid-user

[tuxdroid-user] Daemon to USB connection

[David B. <da...@ja...>]

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 a=
t  =

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.

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.

Each 2ms, a frame is sent from the RF module(I) (in the dongle) to the U=
SB  =

chip. This frame contains a few RF and dongle status (in which the RF AC=
K  =

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:

Sending a command - done by usb_write_TuxDroid():
(RF_ACK is NULL in each USB->PC frame prior to sending a command)
1. Write the command on the USB - usb_interrupt_write(...) (RF_ACK =3D N=
ULL)
2. The USB gives the 4 bytes raw command to the RF module (I) (RF_ACK =3D=
  =

WAITING)
3. The RF(I) sends it to the other RF module (II) (RF_ACK =3D WAITING)
4. The RF(II) sends back an acknowledge to the RF(I)
5. If RF(I) receives the ACK, then RF_ACK =3D OK
    else after a timeout, RF_ACK =3D KO

So basically the daemon sends a raw command, set the cmd_status_flag and=
  =

waits (loop and usleep) for the flag to be reset which means the RF_ACK =
 =

has been received and is either OK or KO, with a timeout of 150ms in cas=
e  =

the flag is never reset. It's the USB status thread which was supposed t=
o  =

poll the status regularly and reset the flag. As this thread is not ther=
e  =

anymore, I guess the flag is not reset during usb_write_TuxDroid and the=
  =

returned value is always ACK_CMD_TIMEOUT.

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 th=
e  =

ack
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).
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?

Cheers,
David

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

Re: [tuxdroid-user] Daemon to USB connection

[David B. <da...@ja...>]

On Fri, 01 Jun 2007 20:04:42 +0200, neimad <ror...@gm...> wrote:

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

Great, all recent commits are in my code, so I can only blame myself then  
:)
>
>   * Several lines exceed the 80 column limit (as stated in the coding
>     style guidelines) quite a bit (100+).

Yes, I'm sensible about that but still not very good about where to break  
the lines.
I still have problems with comments. What to do when I get a long comment  
this kind of line?
     event_timer = 2; /* wait 200ms for the pull-up to rise before next  
standalone behavior otherwise position switches signals are wrong */

vi seems to indent like this:
     event_timer = 2; /* wait 200ms for the pull-up to rise before next
                         standalone behavior otherwise position switches  
signals
                         are wrong */
but then i sometimes get this:
     event_timer = long_variable_that_spans_most_of_the_line; /* wait 200ms  
for
                                                                 the  
pull-up to
                                                                 rise before
                                                                 next  
standalone
                                                                 behavior
                                                                 otherwise
                                                                 position
                                                                 switches
                                                                 signals are
                                                                 wrong */

so I may prefer this
     event_timer = 2; /* wait 200ms for the pull-up to rise before next
     standalone behavior otherwise position switches signals are wrong */
or
     event_timer = 2; /* wait 200ms for the pull-up to rise before next
             standalone behavior otherwise position switches signals are
             wrong */

There's the same for function calls, should we do
     void my_long_function_call(parameter_1, parameter_2,  parameter_3,
                                parameter_4)
or
     void my_long_function_call(parameter_1, parameter_2,  parameter_3,
             parameter_4)

Any suggestion?

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

Switch are not optimized correctly most of the times. The last time made  
some experiments (GCC 3.4.6 iirc), switch were fine if all cases were  
growing sequentially, starting from 0 (case 0: ... case 1: ... case 2: ...  
) otherwise the assembly generated was pretty bad. That's a bit tricky but  
in the embedded world, a good if else sequence brings much more control on  
the assembly and leads to much smaller code.

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

Are you talking about these registers? if so, these are the #defines of  
memory-mapped I/O in avrlibc and some macro aliases to these register  
definitions. I think it's better to keep them uppercased as it's the  
common practice and they're at the end const pointers.
      CHARGER_INH_DDR |= CHARGER_INH_MK;
+    PCICR = 0;
+    EICRA = 0;
+    ADCSRA = 0;

For the enums, some examples are welcome.

> 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 µs,
> and when it times out, it calls update_tux_status():
>
>         ...
>         r = select(fdmax + 1, &rset, NULL, NULL, &timeout);
>         if (r == 0)
>         {
>             if (!update_tux_status())
>         ...
>
> This triggers the following calls:
>
>     update_tux_status()
>       \_ usb_get_status_TuxDroid()
>           \_ update_system_status_table()
>                => cmd_status_flag = 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 µs, *no matter what*.
>
>   (2) I got the 50000 µs from the pre-r322 code. Not sure how it fits
>       the 2ms you mention below.

That part is fine. Moreover there's only 1 status (4bytes) maximum that  
can be sent each 2ms and I think it's more 1 every 4ms maximum with the  
current ack scheme. Moreover I only generate 6 status (24 bytes) each  
100ms from tuxcore so there's no need to read them each 2 ms, each 50ms or  
100ms is good enough.

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

The problem was only in the usb_write_TuxDroid() function where there was  
a loop waiting for the thread to read the status, which doesn't occur  
anymore. Nothing bad as it didn't affect the end applications and it's now  
fixed.

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

pthreads should be part of uclibc but if I agree we better avoid them.

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

Well, it's more for the challenge, I like to build asynchronous stuff ;-)
The problem may occur if in the future we want to send more data to tux,  
like if we send raw IR codes for the IR emitter, we could send 100 bytes 2  
at a time (1st=header, 2nd=index, 3 and 4 are parameters) and that may  
take some more time (400ms if we do it correctly, more around 1200ms if we  
do it like now.) I'd also want to uplad the eeprom through the RF in order  
to change the configuration of tux easily, and that's 512 bytes in case we  
use them all.
But 1. can still do fine I think.

By the way, I saw your commit about changing the const local arrays into  
static const arrays. So that was the opportunity to understand what's the  
difference as I thought the C compiler was smart enough to output the same  
code in both situations. I don't know about GCC but some do and some  
don't. For those interested; here's the link:  
http://www.embedded.com/story/OEG20011129S0065 (local constant objects at  
the bottom)
I really like the articles of Dan Saks and there's one that explains why I  
used 'uint8_t const' instead of 'const uint8_t' as normally used:
http://www.dansaks.com/articles/1999-02%20const%20T%20vs%20T%20const.pdf

Thanks for that feedback, really appreciated!
David

Re: [tuxdroid-user] Daemon to USB connection

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

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

[...]
> so I may prefer this
>      event_timer = 2; /* wait 200ms for the pull-up to rise before next
>      standalone behavior otherwise position switches signals are wrong */
> or
>      event_timer = 2; /* wait 200ms for the pull-up to rise before next
>              standalone behavior otherwise position switches signals are
>              wrong */

I never ever comment statements like this, because it looks like shit,
really. I always put the comment on the line before the statement, so
I would write your example above as follows:

    /* wait 200ms for the pull-up to rise before next standalone
     * behavior otherwise position switches signals are wrong */
    event_timer = 2;

> There's the same for function calls, should we do
>      void my_long_function_call(parameter_1, parameter_2,  parameter_3,
>                                 parameter_4)
> or
>      void my_long_function_call(parameter_1, parameter_2,  parameter_3,
>              parameter_4)

Both styles are ok and I have used both. Depends on the coding style
conventions. (For the second one, it is advised to indent the second
part with two indentation levels.)

I prefer the first one though, as I find it a bit cleaner (and it is
how Emacs indents C by default ;-)).

>>   * There's a *big* "if else if else if ..." on command[0] which could
>>     be replaced by a switch.
>
> Switch are not optimized correctly most of the times. The last time made
> some experiments (GCC 3.4.6 iirc), switch were fine if all cases were
> growing sequentially, starting from 0 (case 0: ... case 1: ... case 2: ...
> ) otherwise the assembly generated was pretty bad. That's a bit tricky but
> in the embedded world, a good if else sequence brings much more control on
> the assembly and leads to much smaller code.

Hmm, ok then. If you have pointers on embedded-specific programming
practices, I'd be very interested (if only to avoid committing code
considered bad from an embedded pov).

>>   * 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.
>
> Are you talking about these registers? if so, these are the #defines of
> memory-mapped I/O in avrlibc and some macro aliases to these register
> definitions. I think it's better to keep them uppercased as it's the
> common practice and they're at the end const pointers.
>       CHARGER_INH_DDR |= CHARGER_INH_MK;
> +    PCICR = 0;
> +    EICRA = 0;
> +    ADCSRA = 0;
>
> For the enums, some examples are welcome.

In commit r335:

     typedef enum
     {
         USB_CONNECTION_CONNECT = 1,
         USB_CONNECTION_DISCONNECT = 2,
         USB_CONNECTION_ID_LOOKUP = 3,
         USB_CONNECTION_CHANGE_ID = 4,
         USB_CONNECTION_WAKEUP = 5,
         USB_CONNECTION_WIRELESS_CHANNEL = 6,
     } USB_ID_PARAM1;

The type is badly named: "USB_ID_PARAM1" is about its *use* whereas
its name should really be about what it *is*.

Thus I would rather have the following:

     typedef enum
     {
         USB_CONNECTION_CONNECT = 1,
         USB_CONNECTION_DISCONNECT = 2,
         USB_CONNECTION_ID_LOOKUP = 3,
         USB_CONNECTION_CHANGE_ID = 4,
         USB_CONNECTION_WAKEUP = 5,
         USB_CONNECTION_WIRELESS_CHANNEL = 6,
     } usb_connection_t;

The type is in lower case suffixed instead of all-uppercase, it is
suffixed with _t (a widespread convention) and it is consistent with
its values: usb_connection_t / USB_CONNECTION_xxx

[...]
> That part is fine. Moreover there's only 1 status (4bytes) maximum that
> can be sent each 2ms and I think it's more 1 every 4ms maximum with the
> current ack scheme. Moreover I only generate 6 status (24 bytes) each
> 100ms from tuxcore so there's no need to read them each 2 ms, each 50ms or
> 100ms is good enough.

Okay. We'll need detailed docs about this so that we don't slowly drift
away from the protocol.

[...]
>> 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 ?
>
> Well, it's more for the challenge, I like to build asynchronous stuff ;-)
> The problem may occur if in the future we want to send more data to tux,
> like if we send raw IR codes for the IR emitter, we could send 100 bytes 2
> at a time (1st=header, 2nd=index, 3 and 4 are parameters) and that may
> take some more time (400ms if we do it correctly, more around 1200ms if we
> do it like now.) I'd also want to uplad the eeprom through the RF in order
> to change the configuration of tux easily, and that's 512 bytes in case we
> use them all.
> But 1. can still do fine I think.

Your call. I don't mind implementing solution 2, just wanted to know
if there were "good" reasons (other than "let's do it 'cause it's
cool" ;)).

> By the way, I saw your commit about changing the const local arrays into
> static const arrays. So that was the opportunity to understand what's the
> difference as I thought the C compiler was smart enough to output the same
> code in both situations. I don't know about GCC but some do and some
> don't. For those interested; here's the link:
> http://www.embedded.com/story/OEG20011129S0065 (local constant objects at
> the bottom)
> I really like the articles of Dan Saks and there's one that explains why I
> used 'uint8_t const' instead of 'const uint8_t' as normally used:
> http://www.dansaks.com/articles/1999-02%20const%20T%20vs%20T%20const.pdf

Interesting point in favor of placing "const" to the right, but I'm so
used to placing it to the left I'd have a hard time adopting this way
of writing !

	Damien

Re: [tuxdroid-user] Daemon to USB connection

[David B. <da...@ja...>]

On Sat, 02 Jun 2007 10:01:06 +0200, neimad <ror...@gm...> wrote=
:

> [...] I always put the comment on the line before the statement, so
> I would write your example above as follows:
>
>     /* wait 200ms for the pull-up to rise before next standalone
>      * behavior otherwise position switches signals are wrong */
>     event_timer =3D 2;
>

Thanks, that's what I wanted to know, though I'm so used to it that I'll=
  =

have a hard time ... ;-)
What about defines, enums, etc., you'd put the comment/description of  =

define_1 on the right or on the line before?
enum my_enum {
     define_1,
     define_2,
}

>> There's the same for function calls, should we do
>>      void my_long_function_call(parameter_1, parameter_2,  parameter_=
3,
>>                                 parameter_4)
>> or
>>      void my_long_function_call(parameter_1, parameter_2,  parameter_=
3,
>>              parameter_4)
>
> Both styles are ok and I have used both. Depends on the coding style
> conventions. (For the second one, it is advised to indent the second
> part with two indentation levels.)
>
> I prefer the first one though, as I find it a bit cleaner (and it is
> how Emacs indents C by default ;-)).

vi defaults to the second, I think we'll never agree on this ;-)

> [...]
> Hmm, ok then. If you have pointers on embedded-specific programming
> practices, I'd be very interested (if only to avoid committing code
> considered bad from an embedded pov).

Looking into this again (to find you some pointers), I'm not sure my  =

previous statement was right, I'll have to do some tests again. This was=
  =

discussed on the avrlibc mailing list some time ago. But what I'm sure n=
ow  =

is that a swicth can be very efficient if all cases are incremental as i=
t  =

can use a lookup table, otherwise it generates a if-then-else sequence  =

which takes much more space. In i2c.c (tuxcore) I created the lookup tab=
le  =

manually as the compiler never wanted to do it, and I saved more than 10=
0  =

bytes doing so.
I'll let you know if I find something interesting.

> In commit r335:
>
>      typedef enum
>      {
>          USB_CONNECTION_CONNECT =3D 1,
>          USB_CONNECTION_DISCONNECT =3D 2,
>          USB_CONNECTION_ID_LOOKUP =3D 3,
>          USB_CONNECTION_CHANGE_ID =3D 4,
>          USB_CONNECTION_WAKEUP =3D 5,
>          USB_CONNECTION_WIRELESS_CHANNEL =3D 6,
>      } USB_ID_PARAM1;
>
> The type is badly named: "USB_ID_PARAM1" is about its *use* whereas
> its name should really be about what it *is*.

Well, it *is* the first parameter of the USB ID command which I later  =

renamed USB connection command and probably forgot about it here.

>
> Thus I would rather have the following:
>
>      typedef enum
>      {
>          USB_CONNECTION_CONNECT =3D 1,
>          USB_CONNECTION_DISCONNECT =3D 2,
>          USB_CONNECTION_ID_LOOKUP =3D 3,
>          USB_CONNECTION_CHANGE_ID =3D 4,
>          USB_CONNECTION_WAKEUP =3D 5,
>          USB_CONNECTION_WIRELESS_CHANNEL =3D 6,
>      } usb_connection_t;
>
> The type is in lower case suffixed instead of all-uppercase, it is
> suffixed with _t (a widespread convention) and it is consistent with
> its values: usb_connection_t / USB_CONNECTION_xxx

Yep, my fault, I was probably asleep at the time I wrote that. I'll fix =
 =

it, write it 100 times and go to sleep without eating ;-)

Thanks,
David

Re: [tuxdroid-user] Daemon to USB connection

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

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

[...]
> What about defines, enums, etc., you'd put the comment/description of
> define_1 on the right or on the line before?
> enum my_enum {
>     define_1,
>     define_2,
> }

For enums, I write the comment about the enum itself above it and the
comments describing each enum member (not always necessary, sometimes
the names speak for themselves) to the right of the member. Using dox
comments:

    /** My very nice enum */
    enum my_enum {
        define_1,  /**< Blah blah 1 */
        define_2,  /**< Blah blah 2 */
    };

>>> There's the same for function calls, should we do
>>>      void my_long_function_call(parameter_1, parameter_2,  parameter_3,
>>>                                 parameter_4)
>>> or
>>>      void my_long_function_call(parameter_1, parameter_2,  parameter_3,
>>>              parameter_4)
>>
>> Both styles are ok and I have used both. Depends on the coding style
>> conventions. (For the second one, it is advised to indent the second
>> part with two indentation levels.)
>>
>> I prefer the first one though, as I find it a bit cleaner (and it is
>> how Emacs indents C by default ;-)).
>
> vi defaults to the second, I think we'll never agree on this ;-)

So, which style to you choose ? (the second one, I bet) I'll set up my
Tuxdro=C3=AFd mode to use that style.

[...]
> Looking into this again (to find you some pointers), I'm not sure my
> previous statement was right, I'll have to do some tests again. This
> was  discussed on the avrlibc mailing list some time ago. But what I'm
> sure now  is that a swicth can be very efficient if all cases are
> incremental as it  can use a lookup table, otherwise it generates a
> if-then-else sequence  which takes much more space. In i2c.c (tuxcore)

Yeah, I have some memories about lookup tables being generated for
small switches on contiguous values. Never bothered to look at the
generated code, though.

> I created the lookup table  manually as the compiler never wanted to
> do it, and I saved more than 100  bytes doing so.
> I'll let you know if I find something interesting.

Okay, thanks.

[...]
> Yep, my fault, I was probably asleep at the time I wrote that. I'll
> fix  it, write it 100 times and go to sleep without eating ;-)

We'd rather add the things we discuss here to the coding style
guidelines when we're set :-)

	Damien

Re: [tuxdroid-user] Daemon to USB connection

[David B. <da...@ja...>]

On Sun, 03 Jun 2007 08:30:32 +0200, neimad <ror...@gm...> wrote:

>>>> There's the same for function calls, should we do
>>>>      void my_long_function_call(parameter_1, parameter_2,   
>>>> parameter_3,
>>>>                                 parameter_4)
>>>> or
>>>>      void my_long_function_call(parameter_1, parameter_2,   
>>>> parameter_3,
>>>>              parameter_4)
>>>
>>> Both styles are ok and I have used both. Depends on the coding style
>>> conventions. (For the second one, it is advised to indent the second
>>> part with two indentation levels.)
>>>
>>> I prefer the first one though, as I find it a bit cleaner (and it is
>>> how Emacs indents C by default ;-)).
>>
>> vi defaults to the second, I think we'll never agree on this ;-)
>
> So, which style to you choose ? (the second one, I bet) I'll set up my
> Tuxdroïd mode to use that style.

Just kidding ;-)
Most of the code is already inlining with the opening parenthesis so we  
can just keep on with that.

Re: [tuxdroid-user] Daemon to USB connection

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

More coding style stuff. This time from commit r357:

[...]
    switch (data[0])
    {
    case TUX_CONNECTION_DISCONNECT:
        if (disconnect_from_tux() >= 0)
            result[1] = TUX_CONNECTION_ACK;
        else
            result[1] = TUX_CONNECTION_NACK;
        break;
    case TUX_CONNECTION_CONNECT:
    {
        union_uint16_t id;
        id.b[1] = data[1];
        id.b[0] = data[2];
        printf("coucou\n");
        if (connect_to_tux(id.w) >= 0)
            result[1] = TUX_CONNECTION_ACK;
        else
            result[1] = TUX_CONNECTION_NACK;
        break;
    }
    case TUX_CONNECTION_ID_REQUEST:
[...]

I can't agree with the block indentation in TUX_CONNECTION_CONNECT:
the curly braces are at the same depth as the switch's braces, which
is badly readable.

I'd indent it as follows:

[...]
    switch (data[0])
    {
    case TUX_CONNECTION_DISCONNECT:
        if (disconnect_from_tux() >= 0)
            result[1] = TUX_CONNECTION_ACK;
        else
            result[1] = TUX_CONNECTION_NACK;
        break;
    case TUX_CONNECTION_CONNECT:
        {
            union_uint16_t id;

            id.b[1] = data[1];
            id.b[0] = data[2];

            printf("coucou\n");

            if (connect_to_tux(id.w) >= 0)
                result[1] = TUX_CONNECTION_ACK;
            else
                result[1] = TUX_CONNECTION_NACK;
        }
        break;

    case TUX_CONNECTION_ID_REQUEST:
[...]

(Note that I also spaced things out, most notably the variable
declaration, but that's another topic ;-))

The block is a sub-block of the switch()'s block and as such must be
indented. Yes, as a consequence the code within this case is indented
one depth level more than the code in other, block-less cases, but
that's a small price to pay for increased readability.

I also moved the closing brace *before* the break, as in my view all
the code in a case should appear before the break (unless, of course
there are several places where break appears) just like in the
block-less cases.

This last point is debatable, but I think the indentation level should
*really* be as I pointed out above :-)

	Damien, coding-style integrist

Re: [tuxdroid-user] Daemon to USB connection

[David B. <da...@ja...>]

On Fri, 08 Jun 2007 20:06:46 +0200, neimad <ror...@gm...> wrote=
:

>
> More coding style stuff. This time from commit r357:
>
> [...]
>     switch (data[0])
>     {
>     case TUX_CONNECTION_DISCONNECT:
>         if (disconnect_from_tux() >=3D 0)
>             result[1] =3D TUX_CONNECTION_ACK;
>         else
>             result[1] =3D TUX_CONNECTION_NACK;
>         break;
>     case TUX_CONNECTION_CONNECT:
>     {
>         union_uint16_t id;
>         id.b[1] =3D data[1];
>         id.b[0] =3D data[2];
>         printf("coucou\n");
>         if (connect_to_tux(id.w) >=3D 0)
>             result[1] =3D TUX_CONNECTION_ACK;
>         else
>             result[1] =3D TUX_CONNECTION_NACK;
>         break;
>     }
>     case TUX_CONNECTION_ID_REQUEST:
> [...]
>
> I can't agree with the block indentation in TUX_CONNECTION_CONNECT:
> the curly braces are at the same depth as the switch's braces, which
> is badly readable.
>
> I'd indent it as follows:
>
> [...]
>     switch (data[0])
>     {
>     case TUX_CONNECTION_DISCONNECT:
>         if (disconnect_from_tux() >=3D 0)
>             result[1] =3D TUX_CONNECTION_ACK;
>         else
>             result[1] =3D TUX_CONNECTION_NACK;
>         break;
>     case TUX_CONNECTION_CONNECT:
>         {
>             union_uint16_t id;
>
>             id.b[1] =3D data[1];
>             id.b[0] =3D data[2];
>
>             printf("coucou\n");
>
>             if (connect_to_tux(id.w) >=3D 0)
>                 result[1] =3D TUX_CONNECTION_ACK;
>             else
>                 result[1] =3D TUX_CONNECTION_NACK;
>         }
>         break;
>
>     case TUX_CONNECTION_ID_REQUEST:
> [...]
>
> (Note that I also spaced things out, most notably the variable
> declaration, but that's another topic ;-))
>
> The block is a sub-block of the switch()'s block and as such must be
> indented. Yes, as a consequence the code within this case is indented
> one depth level more than the code in other, block-less cases, but
> that's a small price to pay for increased readability.
>
> I also moved the closing brace *before* the break, as in my view all
> the code in a case should appear before the break (unless, of course
> there are several places where break appears) just like in the
> block-less cases.
>
> This last point is debatable, but I think the indentation level should=

> *really* be as I pointed out above :-)
>
> 	Damien, coding-style integrist

I was a bit surprised at the beginning, not being used to get switch and=
  =

case on the same level. I must admit I didn't know what to do with the  =

curly braces.
I changed in my code, it'll be soon on svn. Thanks for keeping attention=
  =

to my commits :-)

vi lovers can use that option to indent switch statements that way:
    :set cino=3D:0

David

Re: [tuxdroid-user] Daemon to USB connection

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

Yet more coding style, this time in the Python API...

_Continued lines_

In tuxapi_class.py, there are lots of continued lines indented (or
rather, *not* intended) like the following:

        # create and insert a frame to match
        data_to_match =3D (SOURCE_TUX,SS_DEFAULT,DATA_TP_RSP,\
        SUBDATA_TP_STATUS,DATA_STATUS,DATA_VALUE,0,0,0,0,0,\
        0,0,0,0,0)
        data_to_match_list.append(data_to_match)

Readability is very bad, and this is against the Python coding
style[1] pointed to by tuxisalive[2]:

    =C2=AB The preferred way of wrapping long lines is by using Python's
    implied line continuation inside parentheses, brackets and braces.
    If necessary, you can add an extra pair of parentheses around an
    expression, but sometimes using a backslash looks better. *Make
    sure to indent the continued line appropriately*. =C2=BB

So the code snippet above should be written like this:

        # create and insert a frame to match
        data_to_match =3D (SOURCE_TUX,SS_DEFAULT,DATA_TP_RSP, \
            SUBDATA_TP_STATUS,DATA_STATUS,DATA_VALUE,0,0,0,0,0, \
            0,0,0,0,0)
        data_to_match_list.append(data_to_match)


_Private methods and variables_

In tuxapi_class.py again, class TUXcmd has a big doc string listing all
functions available to the users of the class. This is not necessary
as the list can be found out using dir().

Some methods have a doc string saying "Not a user function". Their
name should start with "__" (double underscore) to make them really
private, which also makes such doc strings useless.

	Damien

[1] http://www.python.org/dev/peps/pep-0008/
[2] http://www.tuxisalive.com/documentation/how-to/guidelines-for-creating-=
and-packaging-an-application

Re: [tuxdroid-user] Daemon to USB connection

[David B. <da...@ja...>]

On Sat, 09 Jun 2007 07:56:27 +0200, neimad <ror...@gm...> wrote:

> Yet more coding style, this time in the Python API...
>
> _Continued lines_

Corrected now.

> _Private methods and variables_
>
> In tuxapi_class.py again, class TUXcmd has a big doc string listing all
> functions available to the users of the class. This is not necessary
> as the list can be found out using dir().
>
> Some methods have a doc string saying "Not a user function". Their
> name should start with "__" (double underscore) to make them really
> private, which also makes such doc strings useless.

I didn't change these as there are many things I'd like to review in the  
api before taking action. I'll start a new thread on tomorrow about this.

Thanks as usual ;-)
David

Re: [tuxdroid-user] Daemon to USB connection

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

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

[...]
> 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?

A few ideas:

  (1) I'd rename the daemon to "tuxd", to follow conventions in the
      Unix world (okay, "tuxdaemon" matches "tuxttsdaemon", but maybe
      that can be changed too ?). This is nitpicking.

  (2) I'd get rid of the "USBDaemon" prefix. It's long and annoying to
      type with its mixed upper and lowercase, even with filename
      completion.

      Having a prefix is generally a good idea, though: it avoids
      ending up with files in different directories but with the same
      name, which eventually leads to troubles. The project is quite
      small, so we don't have this problem yet.

  (3) Merge USBDaemon_usb_enum.[ch] and USBDaemon_usb_readWrite.[ch]
      into tuxd_usb.[ch] (for example). Both these files handle USB
      and are small enough to be merged.

  (4) Except for USB, logging, and maybe command_tux and pidfile,
      there's no point in keeping any of the libs/ source as pseudo
      libraries.

  (5) The current "main.c" is an empty shell: the daemon is actually
      implemented in tcp_server, command_tux and status_table. Hence
      these files should be moved to the daemon's toplevel directory,
      and main.c somehow merged with tcp_server.

This is a very rough outline, but I think it will lead to a more
sensible daemon file structure.

	Damien

Re: [tuxdroid-user] Daemon to USB connection

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

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

[...]
> 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).
> 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 ;-) )

Well, I'd like the daemon to use real *structures* instead of byte
arrays. At least for client communication (i.e., commands coming from
and replies to the API).

The byte arrays' contents is anonymous, needs the declaration of
symbolic index constants, makes the code bigger (and less readable)
than necessary.

On the Python side, sending data to a C program that expects structs
can be achieved by the use of marshaling. I'm not aware of the latest
and greatest ways to do this in Python, but 10+ years ago there were
already such facilities, so I guess it must be possible and even
easier today ;-)

	Damien

Re: [tuxdroid-user] Daemon to USB connection

[Vincent F. <vin...@gm...>]

2007/6/10, neimad <ror...@gm...>:>
> On the Python side, sending data to a C program that expects structs
> can be achieved by the use of marshaling. I'm not aware of the latest
> and greatest ways to do this in Python, but 10+ years ago there were
> already such facilities, so I guess it must be possible and even
> easier today ;-)

Hi!
You can create struct in python with pack. See an example in the
nfsapp python version (a script you put on your S60 phone or other
which work with p3nfs (http://www.koeniglich.de/p3nfs.html)) :
http://amalfi.dis.unina.it/~foggia/nfsapp.py
Here is the example:

from struct import pack, unpack
msg = pack('<HBBLL', subdirs, flags, pad, size, mtime)
< means little-endian
H:unsigned short;
B:unsigned byte;
L:unsigned long;

for more information:
import struct
help(struct)

I hope it will be useful to you.
Vincent Fretin

Re: [tuxdroid-user] Daemon to USB connection

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

"Vincent Fretin" <vin...@gm...> writes:

[...]
> from struct import pack, unpack
> msg = pack('<HBBLL', subdirs, flags, pad, size, mtime)

Yes, that's exactly what I meant by "marshalling", though I didn't
remember the API :-)

	Damien