Good day,
I am experimenting with serial communication and started building a small program that simulate a dispenser.
I still have many things to add to the project like inventory depletion etc....
I have come to a situation where I am unable to understand why one of the counter in the program is not working, specifically DispencingItem(). no matter the input of the end user the LED blink only once.
Why is the counter not blinking x time matching the UserItemCount?
void DispencingItem() {
for (int counter = 1; counter <= UserItemCount; counter++)
digitalWrite(GreenLedPin, HIGH);
delay(500);
digitalWrite(GreenLedPin, LOW);
delay(500);
}
This is the main program and we can find a call to the function at the very end.
String UserItem;
int UserItemCount;
String UserConfirmation;
String InvCheck;
int RedLedPin = 2;
int YellowLedPin = 3;
int WhiteLedPin = 4;
int GreenLedPin = 5;
void ClearScreen() {
for (int counter = 1; counter <= 15; counter++ ) {
Serial.println(" ");
}
}
void EndProgram() {
Serial.println ("Thank You for using Fabiolus dispenser machine");
for (int counter = 1; counter <= 10; counter++)
{
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
digitalWrite(GreenLedPin, HIGH);
delay(100);
digitalWrite(GreenLedPin, LOW);
delay(100);
}
}
void DispencingItem() {
for (int counter = 1; counter <= UserItemCount; counter++)
digitalWrite(GreenLedPin, HIGH);
delay(500);
digitalWrite(GreenLedPin, LOW);
delay(500);
}
void InvalidInput() {
for (int counter = 1; counter <= 5; counter++)
{
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
}
Serial.println ("Invalid Input, try again");
}
void NotenoughInv() {
for (int counter = 1; counter <= 5; counter++)
{
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
}
Serial.println ("Not enough inventory");
}
void InventoryProcess() {
int appleInventory = 10;
int bananaInventory = 10;
int bottleOfWaterInventory = 10;
for (int counter = 1; counter <= UserItemCount; counter++)
if (UserItemCount > appleInventory) {
// ClearScreen();
NotenoughInv();
InvCheck = "fail";
return InvCheck;
}
else if (UserItemCount <= appleInventory) {
ClearScreen();
InvCheck = "pass";
return InvCheck;
}
if (UserItemCount > bananaInventory) {
// ClearScreen();
NotenoughInv();
InvCheck = "fail";
return InvCheck;
}
else if (UserItemCount <= bananaInventory) {
ClearScreen();
InvCheck = "pass";
return InvCheck;
}
if (UserItemCount > bottleOfWaterInventory) {
//ClearScreen();
NotenoughInv();
InvCheck = "fail";
return InvCheck;
}
else if (UserItemCount <= bottleOfWaterInventory) {
ClearScreen();
InvCheck = "pass";
return InvCheck;
}
}
void PinModeOutput () {
pinMode(GreenLedPin, OUTPUT);
pinMode(YellowLedPin, OUTPUT);
pinMode(WhiteLedPin, OUTPUT);
pinMode(RedLedPin, OUTPUT);
}
void PinModeLow () {
digitalWrite(GreenLedPin, LOW);
digitalWrite(YellowLedPin, LOW);
digitalWrite(WhiteLedPin, LOW);
digitalWrite(RedLedPin, LOW);
}
void setup() {
// put your setup code here, to run once:
Serial.begin(9600);
PinModeOutput();
PinModeLow();
}
void loop() {
// put your main code here, to run repeatedly:
step1:
Serial.flush();
ClearScreen();
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.println(" input item of choice ");
Serial.println(" Choice are : ");
Serial.println(" apple ");
Serial.println(" banana ");
Serial.println(" bottle of water ");
Serial.println(" ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
UserItem = Serial.readStringUntil('\n');
UserItem.trim();
if (UserItem.equals("apple")) {
goto step2;
}
if (UserItem.equals("banana")) {
goto step2;
}
if (UserItem.equals("bottle of water")) {
goto step2;
}
else {
InvalidInput();
//Serial.println("Invalid Input");
goto step1;
}
step2:
ClearScreen();
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.print(" How many " );
Serial.print(UserItem);
Serial.println(" would you like? ");
Serial.println(" ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
UserItemCount = Serial.parseInt();
if (UserItemCount == 0) {
InvalidInput();
goto step2;
}
InventoryProcess();
if (InvCheck == "fail") {
goto step2;
}
if (InvCheck == "pass") {
goto step3;
}
step3:
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.print(" You need " );
Serial.print(UserItemCount);
Serial.print(" " );
Serial.println(UserItem);
Serial.println(" Are you sure? ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
String UserConfirm = Serial.readStringUntil('\n');
if (UserConfirm == "yes") {
DispencingItem();
delay(2000);
EndProgram();
} else if (UserConfirm == "no") {
goto step2;
} else {
InvalidInput();
ClearScreen();
goto step3;
}
}
for (int counter = 1; counter <= UserItemCount; counter++)
digitalWrite(GreenLedPin, HIGH);
delay(500);
digitalWrite(GreenLedPin, LOW);
delay(500);
I think you're missing a pair of these {}
Because your for loop is only executing digitalWrite(GreenLedPin, HIGH);.
You need to make a code block in order for the loop to execute the blink:
void DispencingItem() {
for (int counter = 1; counter <= UserItemCount; counter++) {
digitalWrite(GreenLedPin, HIGH);
delay(500);
digitalWrite(GreenLedPin, LOW);
delay(500);
}
}
Good day,
As I was writing back I saw your reply and yes I was missing a bracket....
noob experimenting at its best
here is the correction, if anyone wants to use this sketch or modify it.
String UserItem;
int UserItemCount;
String UserConfirmation;
String InvCheck;
int RedLedPin = 2;
int YellowLedPin = 3;
int WhiteLedPin = 4;
int GreenLedPin = 5;
void ClearScreen() {
for (int counter = 1; counter <= 15; counter++ ) {
Serial.println(" ");
}
}
void EndProgram() {
Serial.println ("Thank You for using Fabiolus dispenser machine");
for (int counter = 1; counter <= 10; counter++)
{
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
digitalWrite(GreenLedPin, HIGH);
delay(100);
digitalWrite(GreenLedPin, LOW);
delay(100);
}
}
void DispencingItem() {
Serial.println(UserItemCount);
for (int counter = 1; counter <= UserItemCount; counter++)
{
digitalWrite(GreenLedPin, HIGH);
delay(500);
digitalWrite(GreenLedPin, LOW);
delay(500);
}
}
void InvalidInput() {
for (int counter = 1; counter <= 5; counter++)
{
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
}
Serial.println ("Invalid Input, try again");
}
void NotenoughInv() {
for (int counter = 1; counter <= 5; counter++)
{
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
}
Serial.println ("Not enough inventory");
}
void InventoryProcess() {
int appleInventory = 10;
int bananaInventory = 10;
int bottleOfWaterInventory = 10;
for (int counter = 1; counter <= UserItemCount; counter++)
if (UserItemCount > appleInventory) {
// ClearScreen();
NotenoughInv();
InvCheck = "fail";
return InvCheck;
}
else if (UserItemCount <= appleInventory) {
ClearScreen();
InvCheck = "pass";
return InvCheck;
}
if (UserItemCount > bananaInventory) {
// ClearScreen();
NotenoughInv();
InvCheck = "fail";
return InvCheck;
}
else if (UserItemCount <= bananaInventory) {
ClearScreen();
InvCheck = "pass";
return InvCheck;
}
if (UserItemCount > bottleOfWaterInventory) {
//ClearScreen();
NotenoughInv();
InvCheck = "fail";
return InvCheck;
}
else if (UserItemCount <= bottleOfWaterInventory) {
ClearScreen();
InvCheck = "pass";
return InvCheck;
}
}
void PinModeOutput () {
pinMode(GreenLedPin, OUTPUT);
pinMode(YellowLedPin, OUTPUT);
pinMode(WhiteLedPin, OUTPUT);
pinMode(RedLedPin, OUTPUT);
}
void PinModeLow () {
digitalWrite(GreenLedPin, LOW);
digitalWrite(YellowLedPin, LOW);
digitalWrite(WhiteLedPin, LOW);
digitalWrite(RedLedPin, LOW);
}
void setup() {
// put your setup code here, to run once:
Serial.begin(9600);
PinModeOutput();
PinModeLow();
}
void loop() {
// put your main code here, to run repeatedly:
step1:
Serial.flush();
ClearScreen();
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.println(" input item of choice ");
Serial.println(" Choice are : ");
Serial.println(" apple ");
Serial.println(" banana ");
Serial.println(" bottle of water ");
Serial.println(" ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
UserItem = Serial.readStringUntil('\n');
UserItem.trim();
if (UserItem.equals("apple")) {
goto step2;
}
if (UserItem.equals("banana")) {
goto step2;
}
if (UserItem.equals("bottle of water")) {
goto step2;
}
else {
InvalidInput();
//Serial.println("Invalid Input");
goto step1;
}
step2:
ClearScreen();
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.print(" How many " );
Serial.print(UserItem);
Serial.println(" would you like? ");
Serial.println(" ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
UserItemCount = Serial.parseInt();
if (UserItemCount == 0) {
InvalidInput();
goto step2;
}
InventoryProcess();
if (InvCheck == "fail") {
goto step2;
}
if (InvCheck == "pass") {
goto step3;
}
step3:
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.print(" You need " );
Serial.print(UserItemCount);
Serial.print(" " );
Serial.println(UserItem);
Serial.println(" Are you sure? ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
String UserConfirm = Serial.readStringUntil('\n');
if (UserConfirm == "yes") {
DispencingItem();
delay(2000);
EndProgram();
} else if (UserConfirm == "no") {
goto step2;
} else {
InvalidInput();
ClearScreen();
goto step3;
}
}
Also, the use of goto is considered a very bad programming practice except in the most extreme cases. It can lead to very confusing an unmaintainable code. Can't remember the last time I used a goto...
You have basically implemented a state machine with goto. If you use a state variable and switch statement your code would be much more readable.
Ah, the 'dreaded' goto. Forgive me, I see no wrong in it!
If it is so bad, why do the makers continue to include it in c++?
Steve (running for cover)
steve1001:
Ah, the 'dreaded' goto. Forgive me, I see no wrong in it!
If it is so bad, why do the makers continue to include it in c++?
Steve (running for cover)
Removing it would break some legacy code. Also the philosophy of C/C++ is go hang yourself if you like. 
You are trying to return a global String variable from a function of type void (returns nothing).
You get a warning about that, or you should. Make sure your compiler warnings are turned up in the IDE preferences and pay attention to any issued during compilation. It is worth trying to make your code compile without warnings being issued, even if you program works.
As bad as goto is, this is what jumped at me. Did you make that up or get the idea from some (bad) source of learning?
a7
steve1001:
Ah, the 'dreaded' goto. Forgive me, I see no wrong in it!
If it is so bad, why do the makers continue to include it in c++?
Steve (running for cover)
It's not "wrong", just a bad practice. Any modern programming language has a rich set of constructs and control structures such that goto is rarely, if ever, needed. The only exception I can foresee is for exceptional conditions (fault handling, no pun intended). Even then most languages provide a construct for that (try/catch, for example).
I've been managing embedded systems for decades, some with millions of lines of code. I can't remember the last time I saw a goto in code. Indeed, our code scans would catch it, flag it, and require mitigation.
But, of course, this is an Arduino and OP is welcome to proceed with the program structure of his choice. If OP is interested in professional development in the field of software development then he would be well advised to structure his programs using constructs other than goto.
There is also an issue with the InventoryProcess() function. It always compares to apples and returns.
Haha, yes we have no bananas.
At least the inventory refills itself every time it is checked. 
a7
alto777:
Haha, yes we have no bananas.
At least the inventory refills itself every time it is checked.
a7
At least it's not comparing apples and oranges!
Looking at the ops code, I was kind of ok with 'if x then goto step1', and 'if y then goto step2', and it raised a smile. BUT, to 'goto step3' from within the step2 block (etc), was straying way way too far out of line for 2021.
I started programming on a Commodore 64, back in the 1980's. It was all gotos in those days, and there were no Do or While commands. I have had a fondness for goto ever since, and hate it when it gets (rightly) knocked!
Steve
steve1001:
Looking at the ops code, I was kind of ok with 'if x then goto step1', and 'if y then goto step2', and it raised a smile. BUT, to 'goto step3' from within the step2 block (etc), was straying way way too far out of line for 2021.
I started programming on a Commodore 64, back in the 1980's. It was all gotos in those days, and there were no Do or While commands. I have had a fondness for goto ever since, and hate it when it gets (rightly) knocked!
Steve
Back in the BASIC days. I remember those days!
Here is the same code using a state machine, with inventory bug fixed, and some clean up:
const int RedLedPin = 2;
const int YellowLedPin = 3;
const int WhiteLedPin = 4;
const int GreenLedPin = 5;
const int appleInventory = 5;
const int bananaInventory = 10;
const int bottleOfWaterInventory = 15;
enum state_enum
{
SELECT_STATE,
AMOUNT_STATE,
DISPENSE_STATE
} state;
String UserItem;
int UserItemCount;
void ClearScreen() {
for (int counter = 0; counter < 15; counter++ ) {
Serial.println(" ");
}
}
void EndProgram() {
Serial.println ("Thank You for using Fabiolus dispenser machine");
for (int counter = 0; counter < 10; counter++) {
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
digitalWrite(GreenLedPin, HIGH);
delay(100);
digitalWrite(GreenLedPin, LOW);
delay(100);
}
}
void DispencingItem() {
Serial.println(UserItemCount);
for (int counter = 0; counter < UserItemCount; counter++)
{
digitalWrite(GreenLedPin, HIGH);
delay(500);
digitalWrite(GreenLedPin, LOW);
delay(500);
}
}
void InvalidInput() {
for (int counter = 0; counter < 5; counter++)
{
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
}
Serial.println ("Invalid Input, try again");
}
void NotenoughInv() {
for (int counter = 0; counter < 5; counter++)
{
digitalWrite(RedLedPin, HIGH);
delay(100);
digitalWrite(RedLedPin, LOW);
delay(100);
}
Serial.println ("Not enough inventory");
}
bool InventoryProcess(String UserItem, int UserItemCount){
int inventory;
if (UserItem == "apple") inventory = appleInventory;
else if (UserItem == "banana") inventory = bananaInventory;
else if (UserItem == "bottle of water") inventory = bottleOfWaterInventory;
if (UserItemCount > inventory) {
NotenoughInv();
return false;
}
ClearScreen();
return true;
}
void PinModeOutput () {
pinMode(GreenLedPin, OUTPUT);
pinMode(YellowLedPin, OUTPUT);
pinMode(WhiteLedPin, OUTPUT);
pinMode(RedLedPin, OUTPUT);
}
void PinModeLow () {
digitalWrite(GreenLedPin, LOW);
digitalWrite(YellowLedPin, LOW);
digitalWrite(WhiteLedPin, LOW);
digitalWrite(RedLedPin, LOW);
}
void setup() {
// put your setup code here, to run once:
Serial.begin(9600);
PinModeOutput();
PinModeLow();
state = SELECT_STATE;
}
void loop() {
switch (state)
{
// put your main code here, to run repeatedly:
case SELECT_STATE:
Serial.flush();
ClearScreen();
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.println(" input item of choice ");
Serial.println(" Choice are : ");
Serial.println(" apple ");
Serial.println(" banana ");
Serial.println(" bottle of water ");
Serial.println(" ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
UserItem = Serial.readStringUntil('\n');
UserItem.trim();
if (UserItem == "apple" || UserItem == "banana" || UserItem == "bottle of water") {
state = AMOUNT_STATE;
}
else {
InvalidInput();
}
break;
case AMOUNT_STATE:
ClearScreen();
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.print(" How many " );
Serial.print(UserItem);
Serial.println(" would you like? ");
Serial.println(" ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
UserItemCount = Serial.parseInt();
if (UserItemCount <= 0) {
InvalidInput();
}
else {
if (InventoryProcess(UserItem, UserItemCount)) {
state = DISPENSE_STATE;
}
}
break;
case DISPENSE_STATE:
Serial.println("////////////////////////////////////////////////////");
Serial.println(" ");
Serial.print(" You need " );
Serial.print(UserItemCount);
Serial.print(" " );
Serial.println(UserItem);
Serial.println(" Are you sure? ");
Serial.println("////////////////////////////////////////////////////");
while (Serial.available() == 0); {}
String UserConfirm = Serial.readStringUntil('\n');
if (UserConfirm == "yes") {
DispencingItem();
delay(2000);
EndProgram();
state = SELECT_STATE;
} else if (UserConfirm == "no") {
state = AMOUNT_STATE;
} else {
InvalidInput();
ClearScreen();
}
break;
} // end switch
}
TheMemberFormerlyKnownAsAWOL:
Needs more F()
Yes. And no String objects.