Author Topic: For loop memory corruption  (Read 4014 times)

0 Members and 1 Guest are viewing this topic.

Offline Stonent

  • Super Contributor
  • ***
  • Posts: 3824
  • Country: us
For loop memory corruption
« on: May 17, 2014, 06:56:09 pm »
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);
    }
  }
}

The larger the government, the smaller the citizen.
 

Offline nitro2k01

  • Frequent Contributor
  • **
  • Posts: 844
  • Country: 00
Re: For loop memory corruption
« Reply #1 on: May 17, 2014, 07:05:29 pm »
Clue: What is the highest index of an array of size n?
Whoa! How the hell did Dave know that Bob is my uncle? Amazing!
 

Offline Stonent

  • Super Contributor
  • ***
  • Posts: 3824
  • Country: us
Re: For loop memory corruption
« Reply #2 on: May 17, 2014, 07:13:17 pm »
Ah! N-1. Arrays start at 0 and not 1. Doh!
The larger the government, the smaller the citizen.
 

Offline Stonent

  • Super Contributor
  • ***
  • Posts: 3824
  • Country: us
Re: For loop memory corruption
« Reply #3 on: May 17, 2014, 07:15:51 pm »
Well whaddya know! It works now.

Thanks for the hint!
The larger the government, the smaller the citizen.
 

Offline nitro2k01

  • Frequent Contributor
  • **
  • Posts: 844
  • Country: 00
Re: For loop memory corruption
« Reply #4 on: May 17, 2014, 07:23:32 pm »
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--)
Whoa! How the hell did Dave know that Bob is my uncle? Amazing!
 

Offline Stonent

  • Super Contributor
  • ***
  • Posts: 3824
  • Country: us
Re: For loop memory corruption
« Reply #5 on: May 17, 2014, 07:31:25 pm »
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.



« Last Edit: May 17, 2014, 07:36:27 pm by Stonent »
The larger the government, the smaller the citizen.
 

Offline electr_peter

  • Supporter
  • ****
  • Posts: 936
  • Country: lt
Re: For loop memory corruption
« Reply #6 on: May 17, 2014, 10:39:27 pm »
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?
 

Offline dannyf

  • Super Contributor
  • ***
  • Posts: 8229
  • Country: 00
Re: For loop memory corruption
« Reply #7 on: May 17, 2014, 10:51:17 pm »
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.
================================
https://dannyelectronics.wordpress.com/
 

Offline SL4P

  • Super Contributor
  • ***
  • Posts: 2130
  • Country: au
  • There's more value if you figure it out yourself!
Re: For loop memory corruption
« Reply #8 on: May 18, 2014, 01:56:24 am »
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
}
Don't ask a question if you aren't willing to listen to the answer.
 

Offline Jeroen3

  • Super Contributor
  • ***
  • Posts: 3403
  • Country: nl
  • Embedded Engineer
    • jeroen3.nl
Re: For loop memory corruption
« Reply #9 on: May 18, 2014, 11:09:08 am »
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.
 

Offline andersm

  • Super Contributor
  • ***
  • Posts: 1119
  • Country: fi
Re: For loop memory corruption
« Reply #10 on: May 18, 2014, 12:52:47 pm »
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.

Offline dannyf

  • Super Contributor
  • ***
  • Posts: 8229
  • Country: 00
Re: For loop memory corruption
« Reply #11 on: May 18, 2014, 01:03:43 pm »
Quote
Sometimes even better.

But does it matter?
================================
https://dannyelectronics.wordpress.com/
 

Offline Stonent

  • Super Contributor
  • ***
  • Posts: 3824
  • Country: us
Re: For loop memory corruption
« Reply #12 on: May 18, 2014, 07:59:26 pm »
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.
« Last Edit: May 18, 2014, 08:04:09 pm by Stonent »
The larger the government, the smaller the citizen.
 

Offline Stonent

  • Super Contributor
  • ***
  • Posts: 3824
  • Country: us
Re: For loop memory corruption
« Reply #13 on: May 18, 2014, 08:06:25 pm »
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)
The larger the government, the smaller the citizen.
 


Share me

Digg  Facebook  SlashDot  Delicious  Technorati  Twitter  Google  Yahoo
Smf