Input: Adding arduino file

[angelinaazhu]

No description provided.

[BrianShTsoi]

Thanks for making the PR. My comments might be a bit overwhelming, I tend to write a lot when it comes to coding style. All the comments are just clean up since we know that the code functions just fine, and one can argue I am just being needlessly nitpicky. Feel free to disagree with my opinions, and don't worry if you don't have time to address all the comments before Sunday's meeting.

A few other notes that are outside of the code itself:

• The commit message (and the PR title) can be a tad bit more descriptive. maybe something like input: Add initial input.ino with jump/duck sensors and buttons
  • You don't have to fix this now (see point below about making new commit for the changes)
• Usually it's good practice to have some elaboration in your PR about what the changes do. Here you can edit your first comment and have a few sentences explaining what the code actually does.
• Since you don't have the hardware setup with you, you can't verify that the code is still functionally the same after modification, which is slightly unfortunate. So I would suggest when you address my comments, have those new changes in a new commit for now. On Sun when we confirm that the code still works as intended after the cleanup, then we can squash the commits and turn them into one commit.
• Before we merge we should run Alex's CI script to make sure the formats are correct. We'll worry about this later.

[BrianShTsoi]

Let's group all the pin consts together (ie. move SAMPLE_SIZE down to where the THRESHOLDs are.)

Also as mentioned last time, we probably should avoid having the pin number in the variable names. Also for the sake of consistency, maybe all pin number variables should have PIN in their name? (if you think that makes the variable names too long that's fair too, but the point is it has to be consistent, so we either have them or we don't)

We can also remove the unused IR pin (and all the associated code for that). That was originally reserved for jumping but now we are using beam break.

Side note, when you rename the consts, be sure to use something like VSCode, which can provide you with find and replace functionality so you can rename the variable all at once instead of finding them manually.

[BrianShTsoi]

We can keep print_min_of_samples around in case we need to debug the IR distance sensor again. But we can remove the parts of the function that involve IR_PIN_0

[BrianShTsoi]

I tend to like to avoid comments and just make sure the code is readable. In this case I think most of these comments can be removed?

For pinMode(BEAM_PIN_D2, INPUT_PULLUP); //for jumping specifically, maybe this is an indicator that we should name the pin differently so that we know it is for jumping (and we already have JUMP_OUTPUT, so it makes sense to have something like JUMP_INPUT as well. up to you whether you want to still keep BEAM as part of the name. Of course if we have JUMP_INPUT, maybe it makes sense to also have DUCK_INPUT or something instead of just IR_PIN, for the sake of consistency)

[BrianShTsoi]

We can probably order the pinMode(...) statements according to the order of the pin consts declaration.

[BrianShTsoi]

I tend to do digitalRead(...) == HIGH or digitalRead(...) == LOW rather than using the return value of digitalRead(...) as a bool. A bit more readable and does not rely on the fact that HIGH and LOW are macros that evaluate to 1 and 0. But this is purely personal preference

[BrianShTsoi]

Same idea about comments. I think the code is descriptive enough that the comments are more or less just repeating what the code says.

[BrianShTsoi]

This is more concise.

Suggested change

	if(!digitalRead(BUTTON_DUCK_D4)){
	duck = true;
	} else {
	duck = false;
	}
	duck = digitalRead(BUTTON_DUCK_D4) == LOW;

[BrianShTsoi]

Same idea as above

[BrianShTsoi]

Same idea as above,

Suggested change

	if (min1 < IR_1_THRESHOLD && min2 < IR_2_THRESHOLD) {
	Serial.println("D"); //ducked
	duck = true;
	} else {
	duck = false;
	}
	duck = min1 < IR_1_THRESHOLD && min2 < IR_2_THRESHOLD;
	if (duck) {
	Serial.println("D");
	}

[BrianShTsoi]

Right now there is a lot of comments and branching. We can try to make it more concise.
Often times an extra variable can be a helpful way to document (and replace comments). Here we can make a new bool called foot_detected and assign it the value digitalRead(BEAM_PIN_D2). If you want to explain it further, you can put the comments before this line to explain how beam broken or not is related to foot being detected. And from then on your code just reason with the fact that foot is being detected or not, rather the beam. (I am beating the dead horse here but this is essentially a tiny form of abstraction.) This is also good for consistency as we already have the prev_foot_detected variable.

In general we want to avoid nested if statements/for loops whenever possible to improve readability. Here there is a lot of branching and assignments of variables, but it boils down to two things:

• if we previously detect foot but now we don't, we register this as a jump
• we update prev_foot_detected to prepare for the next loop

So to write all of this more concisely, (hopefully my logic is right lolll, not too confident since I don't get to test it)

Suggested change

	// put your main code here, to run repeatedly:
	if(digitalRead(BEAM_PIN_D2)){//reads HIGH means beam unbroken
	if(prev_foot_detected){//foot there before
	//foot not there anymore
	Serial.println("J");
	jump = true;
	} else {
	jump = false;
	}
	prev_foot_detected = false;
	// Serial.println("Beam Ok!");
	} else {//reads LOW means beam broken
	prev_foot_detected = true;
	//Serial.println("Beam Broken!");
	jump = false;
	}
	// LOW means beam is broken
	bool foot_detected = digitalRead(BEAM_PIN_D2) == LOW;
	// Keeping the debug prints for now
	// if (foot_detected) {
	// Serial.println("Beam Broken!");
	// } else {
	// Serial.println("Beam Ok!");
	// }
	bool jump = prev_foot_detected && !foot_detected;
	if (jump) {
	Serial.println("J");
	}
	prev_foot_detected = foot_detected;

[BrianShTsoi]

Can probably remove this