======FEEDBACK: c4eng/ptb0======
=====Issue 1: Improperly setting wiringPi GPIO pins per device=====
At the top of the code, you had to indicate which wiringPi GPIO pin you plugged each unique device into.
We had 4 separately addressable components:
* red LED (REDLED)
* green LED (GRNLED)
* button (BUTTON)
* buzzer (SPEAKER)
Which means each one needs to be set to a unique wiringPi GPIO. You paired the REDLED with the BUTTON, and the GRNLED with the SPEAKER. Needless to say, this would cause some issues (not least of which: the REDLED and the BUTTON operate in opposing modes.
The REDLED is an OUTPUT device.
The BUTTON is an INPUT device.
//////////////////////////////////////////////////////////////////////////////
//
// Set asset property to the appropriate wiringPi pin
//
// TODO: Replace # with the actual wiringPi pin numbers that devices are
// connected
//
#define REDLED 26
#define GRNLED 6
#define BUTTON 26
#define SPEAKER 6
This, coupled with issue 2, likely derailed the LEDs properly working.
=====Issue 2: Not setting all LEDs wiringPi GPIO pins to OUTPUT mode=====
I remember you had raised this issue, and there are two common responses:
- the pin is in INPUT mode (sometimes causing a ghost glow)
- the actual LED just happens to be dimmer
The impression was that you were getting functionality out of the LED, just that it was dimmer. But, upon looking at the submitted code, you never actually set the GRNLED to OUTPUT mode, so it was probably doing the ghost glow thing:
//////////////////////////////////////////////////////////////////////////
//
// Initialize output device wiringPi pins to OUTPUT mode
//
// TODO: expand section to include all output devices
//
pinMode (REDLED, OUTPUT);
Make sure you read the comments, they are there to provide explanation, sometimes even leave breadcrumbs on what you have to do.
=====Issue 3: redundant/conflicting code=====
Inside the **while(1)** loop, you correctly checked the BUTTON state and reacted to it with an **if()** statement:
state = digitalRead (BUTTON);
if (state == LOW)
{
. . .
}
else
{
. . .
}
This single if/else is where the bulk of your actions needed to go. As the button can have only two states (LOW or HIGH), that dictated the pivot point as per the specifications of the project. Whichever state corresponded to the button going LOW, you’d put those steps in the **if()**, since that is what was checking for a LOW state.
Correspondingly, if the button state were **HIGH**, those steps would be placed inside the **else**.
One issue is what you put in this first **if()/else**:
if (state == LOW)
{
digitalWrite (REDLED, LOW);
digitalWrite (SPEAKER, LOW);
change = 200;
}
else
{
digitalWrite (GRNLED, HIGH);
digitalWrite (SPEAKER, HIGH);
change = 0;
}
Which by itself, isn’t actually wrong, just incomplete, and indeed could have worked, but: because you never set up the proper modes of the individual components, this would not have likely worked. The previous issues would have lead to this not working.
Also, since there’s only two states to the button, you need to address what you want to happen when each state occurs. When LOW, you set the REDLED LOW and the SPEAKER LOW. The GRNLED should have been set to the opposite state of HIGH here. But you did not.
Similarly, when NOT LOW (ie HIGH), you’d do the opposite: set the REDLED HIGH, GRNLED LOW, and SPEAKER HIGH.
One thing further predicated on correctness is what state activates each component. Here, I was just running with the assumption that REDLED of LOW turned the REDLED off, and SPEAKER state of LOW turned the SPEAKER on. If this isn’t the case, the proper states would need to be adjusted to allow for correct coordination and operation.
Finally, the whole **change = ** lines were a fragment from the demo, to cause a change in tone, but since any tone here would be a single tone (ie the SPEAKER is intended to be off in half the operating states), it isn’t strictly needed. All it does is provide some offset that is added to the base frequency, causing an audible change. Not wrong to keep it, but short of any creative use, largely vestigial.
The rest of the **if()** statements were redundant, also likely causing conflicts. You also didn’t need to read the button state again, as that would be accomplished in the next iteration of the loop:
if (state == HIGH)
{
digitalWrite (REDLED, HIGH);
digitalWrite (SPEAKER, HIGH);
change = 200;
}
else
{
digitalWrite (GRNLED, LOW);
digitalWrite (SPEAKER, LOW);
change = 0;
}
state = digitalRead (SPEAKER);
if (state == LOW)
{
digitalWrite (REDLED, LOW);
digitalWrite (BUTTON, LOW);
change = 200;
}
else
{
digitalWrite (REDLED, HIGH);
digitalWrite (BUTTON, HIGH);
change = 0;
}
if (state == HIGH)
{
digitalWrite (GRNLED, HIGH);
digitalWrite (BUTTON, HIGH);
change = 200;
}
else
{
digitalWrite (GRNLED, LOW);
digitalWrite (BUTTON, LOW);
change = 0;
}
If things had worked, these further **if()** statements and their conflicting operations would have nulled out any progress made.
Again, there are only TWO states to a BUTTON press: LOW and HIGH. You had those covered in the very first **if()/else**.
Finally, you left the bulk of the demo code in place providing further complications (as it is adjusting both SPEAKER and LED):
//////////////////////////////////////////////////////////////////////
//
// Make some noise, sustain it, then turn it off
// Modulate the red LED
//
softToneWrite (SPEAKER, TONE+change);
digitalWrite (REDLED, HIGH);
delay (1000);
softToneWrite (SPEAKER, 0); // turn off speaker when done
digitalWrite (REDLED, LOW);
delay (500);
The only thing you needed here at the end of the loop was some sort of **delay()** (and just one).