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