"David Bourgeois" <[EMAIL PROTECTED]> 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
-------------------------------------------------------------------------
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
