For loop memory corruption

[Stonent]

I haven't actually done any C/C++/Arduino programming in about 6 months because I'd been taking non-programming classes at school and I guess I've forgotten how to correctly count backwards and looks like I'm corrupting my RAM in my MCU on my Arduino and for some reason I can't wrap my head around the logic to get it right.

What I'm doing is taking a 5x7 LED matrix and just moving the dot.
To light a LED, the Column pin needs to go low, and the Row pin needs to go high.
I'm using a tri-state setup to keep from lighting other leds unintentionally, so any pin not being used gets set to a high impedance input.

The arrays at the top are just mapping rows to actual pins. To make it simple, I'm taking pin 1 on the LED matrix and wiring to pin 1 on the arduino and so on.

Everything works fine until I go to the backwards section. I think it's corrupting the memory because each time it executes, it goes slower and slower, so I think it's overwriting the delayms variable, and sometimes the dot just goes into a random spot.

Code: [Select]

int msdelay = 10;
int ColTotal = 5;
int RowTotal = 7;

int col[] = {
  1,3,10,7,8}; //Pull Low
int row[] = {
  12,11,2,9,4,5,6}; // Pull High

void setup() {

  for (int i=0; i < 12; i++){
    pinMode(i, INPUT);
  }
}

void loop() {

 // This section moves the dot from L to R then goes to next line and repeats - This works fine
 
  for (int r=0; r < RowTotal; r++)
  {
    for (int c=0; c < ColTotal; c++)
    {
      pinMode(col[c], OUTPUT);
      pinMode(row[r], OUTPUT);
      digitalWrite (col[c], LOW);
      digitalWrite (row[r], HIGH);
      delay(msdelay);
      digitalWrite (row[r], LOW);
      delay(msdelay);
      pinMode(col[c], INPUT);
      pinMode(row[r], INPUT);
    }
  }

 // This section moves the dot from Top to Bottom then moves Right, also works fine

  for (int c=0; c < ColTotal; c++)
  {
    for (int r=0; r < RowTotal; r++)
    {
      pinMode(col[c], OUTPUT);
      pinMode(row[r], OUTPUT);
      digitalWrite (col[c], LOW);
      digitalWrite (row[r], HIGH);
      delay(msdelay);
      digitalWrite (row[r], LOW);
      delay(msdelay);
      pinMode(col[c], INPUT);
      pinMode(row[r], INPUT);
    }
  }

  // Reverse Section

// This section is supposed to start from bottom right and move the dot up
// It starts but skips the top row, after the 2nd or 3rd time the entire program loops the
// loops get progressively slower and occasionally the dot starts moving around on its own

  for (int c=ColTotal; c > 0; c--)
  {
    for (int r=RowTotal; r > 0; r--)
    {
      pinMode(col[c], OUTPUT);
      pinMode(row[r], OUTPUT);
      digitalWrite (col[c], LOW);
      digitalWrite (row[r], HIGH);
      delay(msdelay);
      digitalWrite (row[r], LOW);
      delay(msdelay);
      pinMode(col[c], INPUT);
      pinMode(row[r], INPUT);
    }
  }
}



[nitro2k01]

Clue: What is the highest index of an array of size n?

[Stonent]

Ah! N-1. Arrays start at 0 and not 1. Doh!

[Stonent]

Well whaddya know! It works now.

Thanks for the hint!

[nitro2k01]

However, don't forget to change the end condition as well. Your current loop would start one index too high and stop one index too high as well. You'd want to use >= instead of > for the end condition in order for the loop to reach element 0.

Code: [Select]

for (int c=ColTotal-1; c >= 0; c--)

[Stonent]

However, don't forget to change the end condition as well. Your current loop would start one index too high and stop one index too high as well. You'd want to use >= instead of > for the end condition in order for the loop to reach element 0.

Code: [Select]

for (int c=ColTotal-1; c >= 0; c--)

Yep, that's exactly what I did after realizing what my issue was.  I also need to go back to the forward loops and fix them so they match the style of the reverse loop so I don't confuse myself in the future because though they worked, I had written them thinking they started at 1, so I actually coded them incorrectly based on that assumption but the incorrect code was actually correct based on how the arrays worked.

Here's the program running (with an additonal section for the first part to reverse as well)

I used red wires for pins that needed to be high, and yellow for ones that needed to be low.

[electr_peter]

Change lines

Code: [Select]

for (int i=0; i < 12; i++){
    pinMode(i, INPUT);
}
to this code

Code: [Select]

for (int i=1; i <= 12; i++){
    pinMode(i, INPUT);
}
Initial code setup pins 0 through 11. I don't think that pin 0 exists on Arduino. Edited codes setups pins 1 through 12.
This change should not matter in practice, as Arduino setups pins to INPUT by default. But we are for better coding practices, aren't we?

[dannyf]

My 2 cents:

1) you want to comment more extensively, so that you know what the code is doing;
2) give the code better structure - make it more modular and easier to read;
3) I would reorganize the code a little bit.

I would start with a set of macros to turn on / off cols / rows:

Code: [Select]

//macro on a pin
#define PIN_SET(pin)  digitalWrite(pin, HIGH) //set a pin
#define PIN_CLR(pin)  digitalWrite(pin, LOW) //clear a pin

//macros on a col
#define COL_ON(pin) PIN_CLR(pin) //turn on a col - active low
#define COL_OFF(pin) PIN_SET(pin) //turn off a col - active high

//macros on a row
#define ROW_ON(pin) PIN_SET(pin) //turn on a row - active high
#define ROW_OFF(pin) PIN_CLR(pin) //turn off a row - active high

You can then build your routines on COL_ON/OFF() or ROW_ON/OFF(), coupled with a data structure.

For example, if you organize your display matrix into col:row (col in MSB and row in LSB, active high), the following would display an imagine on the matrix:

Code: [Select]

//display an image on the led (up to 8x8)
//dat: imagine to be displayed, col:row, active high)
//comment out the unused pins
void led_display(uint16_t col_row) {
  //process the cols first
  if (col_row & 0x8000) COL_ON(col[7]); else COL_OFF(col[7]);
  if (col_row & 0x4000) COL_ON(col[6]); else COL_OFF(col[6]);
  if (col_row & 0x2000) COL_ON(col[5]); else COL_OFF(col[5]);
  if (col_row & 0x1000) COL_ON(col[4]); else COL_OFF(col[4]);
  if (col_row & 0x0800) COL_ON(col[3]); else COL_OFF(col[3]);
  if (col_row & 0x0400) COL_ON(col[2]); else COL_OFF(col[2]);
  if (col_row & 0x0200) COL_ON(col[1]); else COL_OFF(col[1]);
  if (col_row & 0x0100) COL_ON(col[0]); else COL_OFF(col[0]);

  //process the rows then
  if (col_row & 0x0080) ROW_ON(row[7]); else ROW_OFF(row[7]);
  if (col_row & 0x0040) ROW_ON(row[6]); else ROW_OFF(row[6]);
  if (col_row & 0x0020) ROW_ON(row[5]); else ROW_OFF(row[5]);
  if (col_row & 0x0010) ROW_ON(row[4]); else ROW_OFF(row[4]);
  if (col_row & 0x0008) ROW_ON(row[3]); else ROW_OFF(row[3]);
  if (col_row & 0x0004) ROW_ON(row[2]); else ROW_OFF(row[2]);
  if (col_row & 0x0002) ROW_ON(row[1]); else ROW_OFF(row[1]);
  if (col_row & 0x0001) ROW_ON(row[0]); else ROW_OFF(row[0]);
}

Once you have that, you can rotate the dots all you want.

For example, the following sequence shifts dots horizontally:

Code: [Select]

  led_display(0x8001);
  led_display(0x4001);
  led_display(0x2001);
  led_display(0x1001);
  led_display(0x0801);
  led_display(0x0401);
  led_display(0x0201);
  led_display(0x0101);

The following rolls the dot diagnolly:

Code: [Select]

  led_display(0x8080);
  led_display(0x4040);
  led_display(0x2020);
  led_display(0x1010);
  ...

Something like that is a lot more portable, more readable and far easier to port to a new chip, or away from Arduino - you simply need to remap the PIN_ON()/OFF() macros.

[SL4P]

With the nested loops, you may like to move the outer loop assignments out of the inner loop - the code will run faster...

col loop {
  row loop {
    do col() thing
    do row() thing
  }
}

could be

col loop {
  row loop {
    do row() thing
  }
  do col() thing
}

[Jeroen3]

The fastest way to do a loop an almost every architecture is like this:

Code: [Select]

while(i--)Since there i-- is a single instruction and the implicit branch when i != 0 is a single instruction. Sometimes even better.

[andersm]

The fastest way to do a loop an almost every architecture is like this:

Code: [Select]

while(i--)Since there i-- is a single instruction and the implicit branch when i != 0 is a single instruction. Sometimes even better.

Any half-decent compiler will transform loops into their optimal form. Focus on writing clear code, and worry about micro-optimizations only if profiling and inspection shows there is an actual problem.

[dannyf]

Sometimes even better.

But does it matter?

[Stonent]

Change lines

Code: [Select]

for (int i=0; i < 12; i++){
    pinMode(i, INPUT);
}
to this code

Code: [Select]

for (int i=1; i <= 12; i++){
    pinMode(i, INPUT);
}
Initial code setup pins 0 through 11. I don't think that pin 0 exists on Arduino. Edited codes setups pins 1 through 12.
This change should not matter in practice, as Arduino setups pins to INPUT by default. But we are for better coding practices, aren't we?

Yes, I 100% understand the coding is functional but not good.  Now that I'm out of school for the summer I want to spend more time on programming again.

There is a pin0 and pin1, they are mapped to RX and TX on the serial IO pins. I started at pin 1 so I could map pin 1 to pin 1 on the matrix and go up from there. They can also be used as digital GPIO as well.

I like to use Arduino for playing around with ideas with the final intention of moving to AVR C.  I have a couple of ATMEGA48V chips I got cheap that I might want to use.  They're TQFP so I've been a little reluctant to use them since I don't really have the skills or the right kind of really thin solder or a flux pen yet.

I actually got distracted from fixing the ugly code by looking at all the LED matrices I had from a purchase several months back and started writing portions at the top where the matrix could be selected by uncommenting the code and keeping with the 1 to 1 pin-mapping. Though it did get messed up slightly on one of my matrices that had Pin 11 and 12 NC.

Regarding loops, I've always had a bit of a hard time getting C loops right.  I've used BASIC a lot more than C, in fact started using BASIC around age 5 or so.  I find FOR X=1 to 10 : Print X : NEXT X   a lot more intuitive way to count from 1 to 10, but I understand that the C way is more like current more usable languages.

[Stonent]

My 2 cents:

1) you want to comment more extensively, so that you know what the code is doing;
2) give the code better structure - make it more modular and easier to read;
3) I would reorganize the code a little bit.

I would start with a set of macros to turn on / off cols / rows:

Code: [Select]

//macro on a pin
#define PIN_SET(pin)  digitalWrite(pin, HIGH) //set a pin
#define PIN_CLR(pin)  digitalWrite(pin, LOW) //clear a pin

//macros on a col
#define COL_ON(pin) PIN_CLR(pin) //turn on a col - active low
#define COL_OFF(pin) PIN_SET(pin) //turn off a col - active high

//macros on a row
#define ROW_ON(pin) PIN_SET(pin) //turn on a row - active high
#define ROW_OFF(pin) PIN_CLR(pin) //turn off a row - active high

You can then build your routines on COL_ON/OFF() or ROW_ON/OFF(), coupled with a data structure.

For example, if you organize your display matrix into col:row (col in MSB and row in LSB, active high), the following would display an imagine on the matrix:

Code: [Select]

//display an image on the led (up to 8x8)
//dat: imagine to be displayed, col:row, active high)
//comment out the unused pins
void led_display(uint16_t col_row) {
  //process the cols first
  if (col_row & 0x8000) COL_ON(col[7]); else COL_OFF(col[7]);
  if (col_row & 0x4000) COL_ON(col[6]); else COL_OFF(col[6]);
  if (col_row & 0x2000) COL_ON(col[5]); else COL_OFF(col[5]);
  if (col_row & 0x1000) COL_ON(col[4]); else COL_OFF(col[4]);
  if (col_row & 0x0800) COL_ON(col[3]); else COL_OFF(col[3]);
  if (col_row & 0x0400) COL_ON(col[2]); else COL_OFF(col[2]);
  if (col_row & 0x0200) COL_ON(col[1]); else COL_OFF(col[1]);
  if (col_row & 0x0100) COL_ON(col[0]); else COL_OFF(col[0]);

  //process the rows then
  if (col_row & 0x0080) ROW_ON(row[7]); else ROW_OFF(row[7]);
  if (col_row & 0x0040) ROW_ON(row[6]); else ROW_OFF(row[6]);
  if (col_row & 0x0020) ROW_ON(row[5]); else ROW_OFF(row[5]);
  if (col_row & 0x0010) ROW_ON(row[4]); else ROW_OFF(row[4]);
  if (col_row & 0x0008) ROW_ON(row[3]); else ROW_OFF(row[3]);
  if (col_row & 0x0004) ROW_ON(row[2]); else ROW_OFF(row[2]);
  if (col_row & 0x0002) ROW_ON(row[1]); else ROW_OFF(row[1]);
  if (col_row & 0x0001) ROW_ON(row[0]); else ROW_OFF(row[0]);
}

Once you have that, you can rotate the dots all you want.

For example, the following sequence shifts dots horizontally:

Code: [Select]

  led_display(0x8001);
  led_display(0x4001);
  led_display(0x2001);
  led_display(0x1001);
  led_display(0x0801);
  led_display(0x0401);
  led_display(0x0201);
  led_display(0x0101);

The following rolls the dot diagnolly:

Code: [Select]

  led_display(0x8080);
  led_display(0x4040);
  led_display(0x2020);
  led_display(0x1010);
  ...

Something like that is a lot more portable, more readable and far easier to port to a new chip, or away from Arduino - you simply need to remap the PIN_ON()/OFF() macros.

That's some good stuff there!

I don't normally do line by line commenting, more so functional block commenting.  (I do a lot of windows batch files more than anything)