On Fri, 01 Jun 2007 20:04:42 +0200, neimad <[EMAIL PROTECTED]> 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
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
tux-droid-user mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tux-droid-user
