Re: [tuxdroid-user] Daemon to USB connection
Status: Beta
Brought to you by:
ks156
From: neimad <ror...@gm...> - 2007-06-02 07:59:22
|
"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 |