Party mode #167
Party mode #167
Conversation
… all 3 outputs. Works through a state machine.
| }state; | ||
|
|
||
| bool teststate = false; | ||
| state partystate = OFF; |
There was a problem hiding this comment.
Typically we'd make all the above variables "static"
| GREEN, | ||
| BLUE, | ||
| WHITE | ||
| }state; |
There was a problem hiding this comment.
Although technically fine, "state" is a little too generic. Consider "test_mode_state", or "test_mode_colour"
| }state; | ||
|
|
||
| bool teststate = false; | ||
| state partystate = OFF; |
There was a problem hiding this comment.
Again, technically fine, but typically use CamelCase or snake_case for variable names
| @@ -76,9 +92,48 @@ static void LightControllerStatus() | |||
| writeUSB(buf, len); | |||
| } | |||
|
|
|||
| static void updateLEDCtrl(int channel, unsigned int red, unsigned int green, unsigned int blue, int whiteOn) | |||
There was a problem hiding this comment.
Its much harder to spot changes if you move a chunk of code around like this. If you need to access a function from earlier in the file, the preferred method is to declare it at the top. E.g. see lines 50/51.
Really this is the previous authors mistake, because all static functions should already have been pre-declared.
There was a problem hiding this comment.
Moved it back and added declaration
| for (int channel = 0;channel < 3;channel++){ | ||
| updateLEDCtrl(channel,red,green,blue,whiteon); | ||
| }; | ||
| return true; |
There was a problem hiding this comment.
Why return anything if you always return true?
There was a problem hiding this comment.
Changed that to false in case someone wants to use the return value in the future
| updateLEDCtrl(channel,red,green,blue,whiteon); | ||
| }; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
A shorter way to write this:
if(OFF == partystate) {
partystate = RED;
for(int i = 0; i < 3; i++) {
updateLEDCtrl(i, 0xFF, 0, 0, false);
}
}
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This entire if statement is good, but should be moved to a new function. LightControllerLoop shouldn't be polluted too much (its a public function).
| if (teststate == true){ | ||
|
|
||
| // Turn on every colour for 2 second during the test | ||
| switch(partystate){ |
There was a problem hiding this comment.
Typically we add a "default" case. This catches if the state variable is wrong somehow. in this situation, I would go to teststate = false;\partystate = OFF; by default.
| int whiteon = false; | ||
| for (int channel = 0;channel < 3;channel++){ | ||
| updateLEDCtrl(channel,red,green,blue,whiteon); | ||
| }; |
There was a problem hiding this comment.
Reading how the rest of the function is written and used, I don't think it was intended to have side-effects, and this introduces one. I would remove this entire if statement.
There was a problem hiding this comment.
This is the only thing I've read by the end of the review that might cause the unit test failure by the way.
"PARTY" command added which cycles through all colors on all 3 ports to ease function testing the boards. Could also be used by production to see if the LED strips work properly.