Feb. 13,14 Code Review 1

Code Review was done via SkyPe.
The comments have been organized by topics, not time.


cMake compile of hg + 1103 code

[10:37:33 PM] Carolyn Oates: for cmake you copied everything from arch/ecos/lms unchanged into modules'/lms?

[10:37:40 PM] Alois Zoitl: nope

[10:37:51 PM] Alois Zoitl: I moved it to modules/lms/lms_hw

[10:38:04 PM] Alois Zoitl: I thought a little seperation might be nice

[10:38:09 PM] Carolyn Oates: No cmake...txt changes

[10:38:28 PM] Alois Zoitl: yes added subdirectory and added additional cmake.txt there

[11:07:23 PM] Carolyn Oates: lms_main goes in plain lms dir?

[11:11:20 PM] Carolyn Oates: filenames should all be lower case?

[11:11:27 PM] Alois Zoitl: yes and yes

[11:11:46 PM] Carolyn Oates: So I need to rename Buttons to buttons

[11:22:01 PM] Carolyn Oates: Do the .h files need to be listed in the CMakeLists.txt file?

[11:22:30 PM] Alois Zoitl: I'm not sure. However for building distributins afterwards it would be better I gues

[11:27:10 PM] Carolyn Oates: Does your code have #include "../../arch/ecos/lms/buttons.h" in it?

[11:27:38 PM] Alois Zoitl: no changed it to "lms_hw/buttons.h"

2/14 note: avr_ctl, buttons, sensors now start with lowercase letter and end with cpp
 Other files must stay .c files

////
follow the coding rules
10:15:31 PM] Alois Zoitl: How about having a baseclass for all sensor port fbs e.g. CSensorPortFB : public CFunctionBlock{...};
[10:15:43 PM] Alois Zoitl: and put there the port management


[10:26:17 PM] Alois Zoitl: please don't start functions with uppercase letters


////


2/14 note: LED on/off and initialization are ready inline functions in the FB.
Need to change lms_main to use these functions.

[11:44:04 PM] Alois Zoitl: I'll be gone for today. Had a look on the i2c files in ecos they are somewhat different. so we should currently stay with the ones we have.

10:23:46 PM] Carolyn Oates: LMS_LTLED_INIT and LMS_RS485 are init functions

[10:24:06 PM] Carolyn Oates: LMS_LT_LED_ON /OFF are led fns that do something

[10:25:30 PM] Carolyn Oates: The LMS_ functions should go in an LED.h except LMS_RS485... its initialized the Port in general.

[10:26:24 PM] Carolyn Oates: so lms_RsvSensorPort  and LMS_RS485_PORT_INIT() belong together

[10:26:45 PM] Alois Zoitl: there should also be a unrsv function

10:28:58 PM] Alois Zoitl: it is just needed to support the an INIT-

[10:30:42 PM] Alois Zoitl: no it means that you want to deinitialize the FB

[10:30:48 PM] Alois Zoitl: i.e., QI false

[11:39:46 PM] Carolyn Oates: isnt this backwards?

[11:39:48 PM] Carolyn Oates:   if (pa_nInputPortPin < NXT_SENSOR_PORTS_MAX)

    return PORT_NUMBER_UNDEFINED; // Port doesn't exist

[11:40:43 PM] Alois Zoitl: yep

Open Question: 
2/14 latest comment - avr_ctl content is ok.

Anything else in avr code other than sensor code that is:

[10:16:44 PM] Alois Zoitl: currently your separation feels for me not very natural as it seperates things that belong together
[10:16:48 PM] Alois Zoitl: e.g., avr code
[10:16:56 PM] Alois Zoitl: and isolates other stuff




What is the event sent to the application?
[10:35:52 PM] Alois Zoitl: there is the nice clean recsource feature we used in the demo with the students   


[10:55:52 PM] Carolyn Oates: Off the top of your head - any idea what misc.h an miscxx.c are?
[10:58:12 PM] Carolyn Oates: Asked without looking... Having looked I think its Alex's and Dominik's debug via usb print.


[11:05:57 PM] Carolyn Oates: Did you compile with or without boot blinky?
[11:06:18 PM] Alois Zoitl: without but that should't be an issue afik
[11:06:51 PM] Carolyn Oates: Does it compile files not needed?