Array going wrong

I am working on a home light project and have it working, in a simple fashion so one switch controls multiple lights.

I have introduced another array that checks previous state, if there is no change then it doesn't drop into the routine that turns the light on or off. I have two reasons for doing this, first at the moment the routine is constantly pulling the pin high. The second and more import reason, I have created another project that controls the lights via a webpage. I would like to combine the switches and web control into one project. If I have the switches saying off constantly and the web saying on it will cause me problems.

With the previous state array introduced switches control the wrong light or not at all and it seems to be in a random state.

Sorry for the lack of code blocks I have checked the code compiles on my machine and copied again and included code blocks sorry for causing problems and thanks for taking a look. This project has been cobbled together and some of the ways C++ references arrays are completely different for me.

const int zone2[] = {8,13,0};
const int zone3[] = {11,0};
const int zone4[] = {7,0};
const int zone5[] = {9,0};
const int zone6[] = {12,0};

const int * zones[]={zone2,zone3,zone4,zone5,zone6};

int buttonState[] = {0,0};         // variable for reading the pushbutton status

int previousState[]={0,0,0,0,0};    // array for holding the previous state

void setup() 
{
  //initialize the output pins that will control lights
  pinMode(11, OUTPUT);//need to loop through all arrays setting up pins as output      
  pinMode(12, OUTPUT);
  pinMode(13,OUTPUT);
  
  pinMode(7, OUTPUT);//need to loop through all arrays setting up pins as output      
  pinMode(8, OUTPUT);
  pinMode(9,OUTPUT);
  
  // initialize the pushbutton pin as an input:
 //set all light switches to the same block ie pins 30 - 35
  
  byte i; 
  
  //this loop sets all the pins as inputs was 4
  for (i=30;i< 36;i++) {
    pinMode(i, INPUT);  
    digitalWrite(i,HIGH);  // this makes it connect to the internal resistor
  }
}

void loop()
{
  // read the state of the pushbutton value:
  byte myInput =2;

  // check if the pushbutton is pressed.
  // if it is, the buttonState is HIGH:  
    
    int ArrayCount;
    
    int arrayPosition;
    for (int z = 0; z < 5; ++z) 
      {
        buttonState[z] = digitalRead(z+30); 
	for (arrayPosition = 0;zones[z][arrayPosition] ; arrayPosition++)
          {
            if ((buttonState[z] == HIGH) && (previousState[z] == 0 )) {     
              // turn LED on:    
              digitalWrite(zones[z][arrayPosition],HIGH); 
              previousState[z] = 1;//I have added this line to handle previous state
              } 
            else if ((buttonState[z] == LOW) && (previousState[z] == 1)) {
            // turn LED off;
              digitalWrite(zones[z][arrayPosition],LOW);
              previousState[z] = 0;//I have added this line to handle previous state
              }
          } 
      }

}
 int buttonState[] = {0,0};         // variable for reading the pushbutton status

Valid indexes are 0 and 1.

    for (int z = 0; z < 5; ++z) 
      {
        buttonState[z] = digitalRead(z+30);

Why is your index going to 1 to 5?

marco_c:

    for (int z = 0; z < 5; ++z) 

{
        buttonState[z] = digitalRead(z+30);



Why is your index going to 1 to 5?

I see 0 to 4.

++z means the index is incremented before being used, so 1 to 5. z++ (the 'normal' way) gives 0 to 4.

marco_c:
++z means the index is incremented before being used, so 1 to 5. z++ (the 'normal' way) gives 0 to 4.

Read it again.

Marco, at the place of ++z is never used so z++ and ++z are the same.

As where
for(byte z = 0; ++z < 5; ) //1 to 4
and
for(byte z = 0; z++ < 5; ) //1 to 5
and
for(byte z = 0; z < 5; z++ ) //0 to 4
are not the same...

I couldn't get this code to compile. There seems to be a character - an opening bracket - missing from the listing, at the end of this line:

       if ((buttonState[z] == HIGH) && (previousState[z] == 0 ))

The error is shown as, "'else' without a previous 'if'."

Maybe that's an artifact of not putting the code inside code tags. In my experience, when I encounter code that won't compile, it turns out that the original poster has shown some obsolete or unused code by mistake. So, I'm not inclined to look at it further.

Is this the code that you're actually using? If it is, please put it inside code tags; if it's not, then please post your actual code.

hi sorry have recopied code in tags and compiled code to double check my fat fingers.

thanks

I recommend using logical constants, true and false, instead of 0 and 1. It will make it easier to read.
Edit - for this program, LOW and HIGH might be more descriptive. For example:

int previousState[]={LOW,LOW,LOW,LOW,LOW};    // array for holding the previous state
//...
else if ((buttonState[z] == LOW) && (previousState[z] == HIGH)) {

That seems sensible, didn't realize could do that. This project works fine when I am not trying to store the previous state and use it in the if statement. I thought dropping this array in wouldn't cause an issue!

	for (arrayPosition = 0; zones[z][arrayPosition] ; arrayPosition++)

Your exit condition looks weird. What is the purpose of that? zones[][] is integer type. Did you mean to compare it with something?

 for (arrayPosition = 0; zones[z][arrayPosition] ; arrayPosition++)

Your exit condition looks weird. What is the purpose of that? zones[][] is integer type. Did you mean to compare it with something?
[/quote]

that's there because of the way arrays are referenced in C++. Picked that up fromo a query I had on stack overflow.

aarg:


Your exit condition looks weird. What is the purpose of that? zones[][] is integer type. Did you mean to compare it with something?

If you look at the initialization of the zones[] arrays in post #1, all of the final elements are 0, which would terminate the for loop.

Take a look at what happens in this bit of code when the switch state changes:

 for (int z = 0; z < 5; ++z)
  {
    buttonState[z] = digitalRead(z + 30);
    for (arrayPosition = 0; zones[z][arrayPosition] ; arrayPosition++)
    {
      if ((buttonState[z] == HIGH) && (previousState[z] == 0 )) {
        // turn LED on:
        digitalWrite(zones[z][arrayPosition], HIGH);
        previousState[z] = 1;//I have added this line to handle previous state
      }
      else if ((buttonState[z] == LOW) && (previousState[z] == 1)) {
        // turn LED off;
        digitalWrite(zones[z][arrayPosition], LOW);
        previousState[z] = 0;//I have added this line to handle previous state
      }
    }
  }

The outer loop, indexed by z, steps through each zone. The inner loop, indexed by arrayPosition, steps through each point in a zone. Notice where previousState is updated: during the first iteration through the inner loop. The second iteration through the inner loop will find previousState already updated, it won't detect the change of state, and it won't operate the light.

Here's what I'd recommend:

  • Keep track of previousState in terms of HIGH and LOW, rather than 1 and 0, to facilitate comparing the values of the current and previous states.
  • If the two states aren't equal, there's been a change: execute the arrayPosition loop to iterate through the points in each zone, and set the pin state to the button state. And, update previousState.
  • If the two states are equal, there's been no change: skip the arrayPostion loop, and the update of previousState
    Maybe like this:
  for (int z = 0; z < 5; ++z)
  {
    buttonState[z] = digitalRead(z + 30);
    if (buttonState[z] != previousState[z]) {
      for (arrayPosition = 0; zones[z][arrayPosition] ; arrayPosition++)
      {
        // turn LED on:
        digitalWrite(zones[z][arrayPosition], buttonState[z]);
      }
      previousState[z] = buttonState[z];
    }
  }

Code is untested. As for whether that accounts for the symptoms you report, I can't say.

Then you should say this, for clarity:

for (arrayPosition = 0; zones[z][arrayPosition] != '\0'; arrayPosition++)

GCC is an optimizing compiler. You won't lose any efficiency doing this.

@aarg: why compare against the char '\0' when the array is an int?

econjack:
@aarg: why compare against the char '\0' when the array is an int?

Oh, drats, yes. Then != 0.

Or you could have something like:

const int endOfRecord = 0;
/...
const int zone2[] = {8,13,endOfRecord};
const int zone3[] = {11,endOfRecord};
//..
	for (arrayPosition = 0; zones[z][arrayPosition] != endOfRecord ; arrayPosition++)

I like the EOR concept. However, when I want to use the value as a symbolic constant (e.g., we care about the numeric value of the constant, not the variable that carries it), I usually use:

#define END_OF_RECORD   0
/...
const int zone2[] = {8,13, END_OF_RECORD};
const int zone3[] = {11, END_OF_RECORD};
//..
	for (arrayPosition = 0; zones[z][arrayPosition] != END_OF_RECORD ; arrayPosition++)

That tells me that I'm really not looking at a variable, but only at a value (i.e., rvalue). If we had a true symbolic debugger, I'd probably use #define a lot less, since your const approach does make an entry in the symbol table where #define does not. I also think using uppercase letters in this case reinforces the idea we're really not interested in a variable named endOfRecord, but only the value.

Clearly, this is just a matter of style more than substance.

Thank you @econjack, I appreciate the follow up. Just to clarify, the form is style. But the approach is substantive. I think you were referring only to your enhancements, so we agree, but in case anyone didn't get that, I should say:

Considering the original code, what would happen if the programmer finds that the value zero now has to be included in the data range? 0 could no longer be used as a terminator. Many lines would have to be changed and some might be missed. With this method, only one character needs to be changed:

#define END_OF_RECORD  0 becomes #define END_OF_RECORD  -1

Also with the original method, right out of the gate, a simple typo of omission like:

const int zone2[] = {8,13,0,0};

where the programmers finger slipped when typing

const int zone2[] = {8,13,10,0};

could easily lead to a sleepless night, hair loss and fingernails gnawed to the bone.

Thanks to everyone for taking the time to look.

I will make changes over the weekend and hopefully crack this. I have been looking around and I see people using #Define to debug but haven't seen a full example, does anyone have a good link for one.

How I miss the IDE that I can step through.

thanks
again