Apprentice coder

In my "quest to code" so I can stop pestering people on the forum to help me so much :blush: I have been trying to follow Grumpy Mikes tutorial at

http://www.thebox.myzen.co.uk/Tutorial/Arrays.html

I did not have a seven segment display so I hooked six LED's up to digital pins 3 to 8 on an Uno and progressed through the tutorial until I successfully got to the stage of running this code.

  int timer = 100;
  int pins [6] = { 3, 4, 5, 6, 7, 8 };
  boolean led [6] = { HIGH, LOW, LOW , LOW, LOW , HIGH };
  
  void setup () 
  
  { 
    for(int i = 3; i < 9; i++) pinMode(i, OUTPUT);
  }
  
   void loop ()
  
  {
    for(int i=0; i<7; i++) digitalWrite(pins[i], led[i]);
  }

The next stage of the tutorial expands the code to use char, a mask and a bitwise AND operation. So obviously I would need to incorporate this code into the previously posted code

char led = B10000001;
int mask = 1;

for(int i=0; i<7; i++)
{
  if((mask & led) == 0) digitalWrite(pins[i], LOW);
 else digitalWrite(pins[i], HIGH);
  mask = mask << 1;
}

I tried to put char led = B10000001; and int mask = 1; at the top of the code prior to setup and the if else and mask code into loop which did compile but did not light LED's 1 and 6 as hoped. I realise that this is a roundabout way to light two LED's but as I said at the start I am doing this to learn how to code and feel that this is an important coding technique to understand and use.
Thank you for taking the time to read this and hopefully explain what I am doing wrong,

Pedro.

Your led array as posted has only six elements, but your bit pattern spans eight.
Post your actual code instead of the second snippet, please.

Here is what I tried AWOL

int timer = 100;
int pins [6] = { 3, 4, 5, 6, 7, 8 };
char led = B10000001;
int mask = 1;

 

 void setup () 
  
  { 
    for(int i = 3; i < 9; i++) pinMode(i, OUTPUT);
  }
  
  
 void loop ()
 
 {

for(int i=0; i<7; i++)
{


 { if((mask & led) == 0) 
  digitalWrite(pins[i], LOW); 
  else digitalWrite(pins[i], HIGH);

 mask = mask << 1;
  
 }
}
}

I see what you mean re the discrepancy between six array elements and a byte for the bit pattern :grin:

The Tools + Auto Format menu item needs to become your friend.

You need to learn where { and } are needed (and where they are not, but are a good idea), and where they are not.

The code after an if or else statement should be in { and }. The if statement itself does not need to be.

The code for a for or if statement should be placed on different lines from the statement.

    for(int i = 3; i < 9; i++) pinMode(i, OUTPUT);

should be:

    for(int i = 3; i < 9; i++)
    {
      pinMode(i, OUTPUT);
    }

You can add a Serial.begin() call to setup() and Serial.print() statements to loop() to see what is happening.

Thank you for your response PaulS. I see what you are saying regarding the curly braces and using the serial monitor and point taken, but if I change the code in setup to how you suggest I get errors
"expected initialiser before"for"
"expected constructor, destructor or type conversion before < token"
"expected constructor, destructor or type conversion before ++ token"

Also the initial code that I posted (the code that did work)

     int timer = 100;
  int pins [6] = { 3, 4, 5, 6, 7, 8 };
  boolean led [6] = { HIGH, LOW, LOW , LOW, LOW , HIGH };
  
  void setup () 
  
  { 
    for(int i = 3; i < 9; i++) pinMode(i, OUTPUT);
  }
  
   void loop ()
  
  {
    for(int i=0; i<7; i++) digitalWrite(pins[i], led[i]);
  }

also had this for loop inside the curly braces and I am not doubting your expertise :blush: but I do not understand how your suggestion can help with the problem that I am having. Am I missing something Pedro.

No problems with that code here

Binary sketch size: 934 bytes (of a 30,720 byte maximum)

That is to say, the compiler is OK with it.
I still have issues about the size of the arrays (or the loop lengths, whichever way you look at it)

AWOL yes that compiled ok for me too. With regard to the "I still have issues about the size of the arrays (or the loop lengths, whichever way you look at it)" I added two more LED's (now eight) and changed the code to

int timer = 100;
int pins [8] = { 
  3, 4, 5, 6, 7, 8, 9, 10 };
char led = B10000001;
int mask = 1;



void setup () 

{
for(int i = 3; i < 11; i++)
  pinMode(i, OUTPUT);
}


void loop ()

{

  for(int i=0; i<9; i++)
  {


    { 
      if((mask & led) == 0) 
        digitalWrite(pins[i], LOW); 
      else digitalWrite(pins[i], HIGH);


      mask = mask << 1;


    }
  }
}

but still no luck. I suppose what I am asking is does the format of the code appear to be correct.The " mask = mask << 1;" at the end of the code doesn't quite look right to my very inexperienced eye 8)

Thanks for your help Pedro.

for(int i=0; i<9; i++)

Still unhappy.

How about the debug prints?

You might want to move "mask" and its initialsation into "loop ()"

I don't mean to sound unhappy, just frustrated :grin: But suffice to say AWOL, that edit you just did suggesting
"You might want to move "mask" and its initialsation into "loop ()" did the trick. I tried the dedug print but something just flashed across the screen at light speed and I couldn't remember enough about the serial monitor function to make it stay still until I could read it. So thanks very much for your help and that gives me something to work with. Also thanks to PaulS for his suggestions and help. You fellas are so cluey 8)

Pedro147:
did the trick.

The latest code you posted is still running off the end of the array. It might run and look OK, but it's not right yet.

I don't mean to sound unhappy, just frustrated

No, it is me that is still unhappy with the loop I quoted.

All is good that ends well AWOL. Thanks again for your help

PeterH:

Pedro147:
did the trick.

The latest code you posted is still running off the end of the array. It might run and look OK, but it's not right yet.

Can you offer any suggestions as to how I can fix this problem, because I prefer not to labour under any misapprehensions that the code is correct. Thanks PeterH.

If you've declared an array, then it is best to use it everywhere.
At the moment you're not using it in "setup()".

Also, if you make the array of the correct type, "byte", you can use the size of the array to control your loops, so you should never step off the end.#define N_ELEMENTS(array) (sizeof(array)/sizeof(array[0])).

(In fact, this method works for arrays of any datatype)

Pedro147:

for(int i=0; i<9; i++)

You're processing elements 0 .. 8 of an array that contains elements 0 .. 7.

Best to make your 'for' loop index be the same as the index of the array being processed in the loop, and define the loop in the same way that you defined the array. The best way is to work out the size of the array as AWOL shows you. If the array is explicitly sized when it is declared, another option is to specify the size using a constant and use that same constant as the bound of the for loop. Here you've defined them separately and they have ended up different. One of the fundamental principles of good design is Don't Repeat Yourself (DRY). If you're defining two things that need to match then you should look for ways to eliminate one of the definitions.

Maybe I didn't look back far enough:
did you initialize mask to 1 so you have a bit to shift across?

mask = 1;
for (whatever){
anding operation;
mask = mask<<1;
}

AWOL so as I understand it, you are suggesting that I

  1. use the array that I have declared in setup. How do I do this please.
  2. set the array to data type byte. So do I just change it from int to byte where I have declared it.
  3. define array by -

AWOL:
#define N_ELEMENTS(array) (sizeof(array)/sizeof(array[0])).

So do I just place this definition at the start of the code and then somehow use "(sizeof(array)/sizeof(array[0]))" in the for loops where I was using int i
I apologise for asking so many questions but with all the suggestions and advice that awaited me when I arose this morning, I dont't know how to procede :blush:

In regard to PeterH's observation

PeterH:

Pedro147:

for(int i=0; i<9; i++)

You're processing elements 0 .. 8 of an array that contains elements 0 .. 7.

So I also changed i<9 to i<8

Also Crossroads - yes I did
mask = 1;
for (whatever){
anding operation;
mask = mask<<1;
}
didn't I ? 8)

Pedro147:
So I also changed i<9 to i<8

That would fix the problem, but leaves you using an approach which makes this sort of problem likely.

You have a magic number ( 8 ) in several places in your code and the code will go wrong if they do not all match. Better to define the value as a constant and use it in all the places that need it.

const int PIN_COUNT = 8;
int pins [PIN_COUNT] = { 3, 4, 5, 6, 7, 8, 9, 10 };
...
for(int i=0; i<PIN_COUNT; i++)

Even better is to have the code work the number out for itself, avoiding the chance that you used the wrong constant.

#define N_ELEMENTS(array) (sizeof(array)/sizeof(array[0]))
...

int pins [] = { 3, 4, 5, 6, 7, 8, 9, 10 };
...

for(int i=0; i<N_ELEMENTS(pins); i++)

Thanks for that suggestion PeterH. So just to clarify if I may, I changed the code and it still works the same [phew 8) ]

 int timer = 100; 
#define N_ELEMENTS(array) (sizeof(array)/sizeof(array[0]))
int pins [ ] = { 3, 4, 5, 6, 7, 8, 9, 10 };
char led = B10000001;

void setup () 
{
  Serial.begin(9600); 
 {
    for(int i = 3; i < 11; i++)
      pinMode(i, OUTPUT);
  }
}

void loop ()
{
  int mask = 1;
  for(int i=0; i<N_ELEMENTS(pins); i++)
  {
   { 
      if((mask & led) == 0) 
        digitalWrite(pins[i], LOW); 
      else digitalWrite(pins[i], HIGH);
      mask = mask << 1;
      //  Serial.print(i);
      //   delay(1000);
   }
  }
}

Is that all you suggest I do. Does the rest of the code seem ok, and thanks again for your help there is so much to learn
Pedro.

Pedro147:
Is that all you suggest I do. Does the rest of the code seem ok, and thanks again for your help there is so much to learn

I suggest you also change:

for(int i = 3; i < 11; i++)
      pinMode(i, OUTPUT);

to:

for(int i=0; i<N_ELEMENTS(pins); i++)
{
        pinMode(pins[i], OUTPUT); 
}

Also, get into the habit of using a code block enclosed by { } after every control expression (if, else, for, while, do) even when you only have a single statement in it. It makes it much less likely that you will accidentally end up with a control structure that looks right but is actually wrong.

I also recommend that you put each { and } on a separate line with matching pairs indented by the same amount, and the contents indented one extra level. It makes it much easier to visually scan the structure of your code to confirm it matches your intention.