Go Down

Topic: Help needed to convert some code. (Read 3309 times) previous topic - next topic

flapjackboy

#15
Apr 16, 2010, 04:20 pm Last Edit: Apr 16, 2010, 04:34 pm by flapjackboy Reason: 1
Quote
Not every if statement needs an else. If the else is not doing anything, get rid of it.


These do. If you read through the function's logic you'll see that they are all dependent on one another. There was yet another if...else in there, but I managed to refactor them into one. Unfortunately, that was the only one I have been able to refactor without breaking the logic. If you can find a way to reorganise it and have it perform the same function, I would appreciate the help.

Quote
The else; look like errors. At least, errors waiting to happen.


The original code compiles with no errors, so I transposed it almost verbatim, editing for the differences in syntax between C and Arduino. If the Arduino compiler throws a hissy fit, it's due to its interpretation of the C language, not my initial code.
Quote

It's an array, not a function.


Good catch, thanks.

Quote
Not much point initiailising the flag, if you're going to overwrite it:


Unless of course, you need it resetting at the beginning of each loop.

Groove

#16
Apr 16, 2010, 04:27 pm Last Edit: Apr 16, 2010, 04:41 pm by GrooveFlotilla Reason: 1
The "else;"s do nothing - the fact that there's a semicolon after them signals that.
They are redundant, as is the "return;".

Quote
Unless of course, you need it resetting at the beginning of each loop.

But you overwrite it at the beginning of every loop!

Just because a bit of code compiles without errors doesn't mean it doesn't harbour potential nasties.
Most languages allow you to shoot yourself in the foot, but C hands you the gun.
If you cut out redundant code, you leave fewer dark corners for the bugs to hide.
Per Arduino ad Astra

flapjackboy

#17
Apr 16, 2010, 04:41 pm Last Edit: Apr 16, 2010, 04:44 pm by flapjackboy Reason: 1
Quote
The "else;"s do nothing - the fact that there's a semicolon after them signals that.
They are redundant, as is the "return;".


So they are, just test compiled the original code without them and it returned no errors.

Quote
But you overwrite it at the beginning of every loop!


D'oh!

Just goes to show how useful it is having an objective pair of eyes go over your code, lol.

flapjackboy

#18
Apr 16, 2010, 05:37 pm Last Edit: Apr 16, 2010, 06:17 pm by flapjackboy Reason: 1
Code: [Select]

void CheckInputs(boolean screen = true);  //passes a true value to screen
                                         //and sets CheckInputs to perform
                                         //the initial input check.

boolean inputflag = false;                //the initial input error check
boolean exitflag = false;
int x;
const int buttonPins[] = {2, 3, 4, 5, 6};

void setup()
{
   Serial.begin(9600);
}

void loop()
{
 
 CheckInputs();
 
 Serial.println("Initial Check Complete");
 Serial.println("Enter number of questions");
 x = Serial.read();
 
 for (int i = 0; i < x; i++)
 {
   do
   {
     CheckInputs(false);
     
     if (digitalRead(buttonPins[6]));
     {
       for (int a= 2; a <=6; a++)
         {
           digitalWrite(buttonPins[a], LOW);
         }
       inputflag = false;
       exitflag = true;
     } while (exitflag == false);
     
   while (digitalRead(buttonPins[6]));
     
   exitflag = false;
   }
   
   
 }
)

void CheckInputs(boolean screen)
{
 int i;
 for (i = 2; i <=6; i++)
 {
   boolean inputstatus = digitalRead(buttonPins[i]);
   
   if (inputstatus)
   {
     Serial.println("Input " i " depressed");
     inputflag = true;
   }
   else
   {
     if (i != 6)
       if (!inputflag)
       {
         Serial.println("Contestant " i-1 " buzzed!");
         inputflag = true;
       }
   }
 }
 
 if (!inputflag && screen)
 {
   Serial.println("No inputs depressed");
 }
 
}


OK, so this is how far I've got so far.

gbulmer

#19
Apr 16, 2010, 06:16 pm Last Edit: Apr 16, 2010, 06:30 pm by gbulmer Reason: 1
This doesn't compile, right?

It has errors around lines like:
Code: [Select]
Serial.println("Input " i " depressed");
which are not legal C or C++.

This would need to become
Code: [Select]
Serial.print("Input ");
Serial.print( i );
Serial.println(" depressed");


Also
Code: [Select]
 for (i = 2; i <=6; i++)
 {
   boolean inputstatus = digitalRead(buttonPins[i]);
is likely wrong, because the only valid indexes for buttonPins[] is 0 to 4.

HTH
GB

flapjackboy

#20
Apr 16, 2010, 07:09 pm Last Edit: Apr 16, 2010, 07:23 pm by flapjackboy Reason: 1
I think I've got it, got a clean compile with this:

Code: [Select]

void CheckInputs(boolean screen = true);  //passes a true value to screen
                                           //and sets CheckInputs to perform
                                           //the initial input check.

boolean inputflag = false;                  //the initial input error check
boolean exitflag = false;
int x;
const int buttonPins[] = {2, 3, 4, 5, 6};

void setup()
{
   Serial.begin(9600);
}

void loop()
{
 
 CheckInputs();
 
 Serial.println("Initial Check Complete");
 Serial.println("Enter number of questions");
 x = Serial.read();
 
 for (int i = 0; i < x; i++)
 {
   do
   {
     CheckInputs(false);

     if (digitalRead(buttonPins[4]));
     {
       for (int a= 0; a <=4; a++)
         {
           digitalWrite(buttonPins[a], LOW);
         }
       inputflag = false;
       exitflag = true;
     } while (exitflag == false);
   }
   while (digitalRead(buttonPins[4]));

   exitflag = false;
   
   
   
 }
}

void CheckInputs(boolean screen)
{
 int i;
 for (i = 0; i <=4; i++)
 {
   boolean inputstatus = digitalRead(buttonPins[i]);
   
   if (inputstatus)
   {
     Serial.print("Input ");
       Serial.print(i+1);
       Serial.println(" depressed");
     inputflag = true;
   }
   else
   {
     if (i != 4)
       if (!inputflag)
       {
         Serial.print("Contestant ");
           Serial.print(i+1);
           Serial.println(" buzzed!");
         inputflag = true;
       }
   }
 }
 
 if (!inputflag && screen)
 {
   Serial.println("No inputs depressed");
 }
 
}


Hopefully, this will work.

EDIT: Nope, it just keeps looping through the initial check saying all inputs are pressed. :(

AWOL

Quote
Nope, it just keeps looping through the initial check saying all inputs are pressed


Pull-ups?
Pull-downs?
"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

flapjackboy

Well, for the contestant buttons, I'm using some handsets scavenged from an interactive DVD quiz game. These have 5 buttons, each with a different value resistor in series with it. The original control box used the different voltage potentials to distinguish which button had been pressed on each handset.

The only button I could get the Arduino to pick up when I tested it was the one with the 1K resistor.

PaulS

Code: [Select]
     if (digitalRead(buttonPins[4]))[glow];[/glow]


This isn't doing anything...

Code: [Select]
       exitflag = true;
     } while (exitflag == false);


Doesn't look like exitFlag will ever get to be false.

Code: [Select]
           digitalWrite(buttonPins[a], LOW);


Are the pins listed in buttonPins input or output? You are trying to use them as both.

You can't have it, with switches attached, both ways.

AWOL

How are the switches wired?

Probably the simplest way is to use the AVR's internal pull-ups, but this inverts the logic of an open switch (It will be HIGH when open, and LOW when closed), but this is simple to deal with in software.

Without pull-ups or pull-downs, the pins are floating and can appear random.

Have a look at the examples and tutorials.
"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

flapjackboy

#25
Apr 16, 2010, 09:01 pm Last Edit: Apr 16, 2010, 09:27 pm by flapjackboy Reason: 1
OK, I think that rather than try to shoehorn the old code into the Arduino, I'm going to have to rewrite this from scratch. stick with the code that works and use the K8055.

Thanks for all the help, but as the old adage goes, if it ain't broke, don't fix it.

AWOL

Quote
I think that rather than try to shoehorn the old code into the Arduino


I don't know about the other guys here, but I'm glad I didn't invest too much time in your day.
"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

gbulmer

#27
Apr 16, 2010, 09:53 pm Last Edit: Apr 16, 2010, 10:32 pm by gbulmer Reason: 1
Quote
Quote
I think that rather than try to shoehorn the old code into the Arduino



I don't know about the other guys here, but I'm glad I didn't invest too much time in your day.

I felt a bit annoyed that I'd spent my limited time on someone who gave up, when I might have helped someone who made some progress instead. But I know who I'll avoid helping in future, and ...

I'm now quite pleased that I have an example thread where a simple program was made so complex that it defeated the author who claimed they'd got it working on different hardware.

In future I can reference this thread, and say "Rather than start with that complex old thing, take confidence from it that you can do it again, but instead start with a simple program. You will make progress faster. You might learn something new. It may even turn out much better! Sadly, this person didn't do that, and ended up giving up!"

Many silver linings are embedded in clouds ;D

GB



AWOL

#28
Apr 16, 2010, 09:58 pm Last Edit: Apr 16, 2010, 09:59 pm by AWOL Reason: 1
gbulmer, had I a cap to doff, I would doff it at you!

I like your attitude - you are obviously far less cynical than I.   ;D
"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

gbulmer

#29
Apr 16, 2010, 10:00 pm Last Edit: Apr 16, 2010, 10:33 pm by gbulmer Reason: 1
AWOL - sorry I missed a bit and modified it.
I may have tarnished my attitude

GB

Go Up