Strange issue adding one extra line of code..[SOLVED]

Hello,

I am building a magnetic stripe reader and im writing the code step by step because the tutorials ive encountered require a lot of tweaking…

Well, so far so good, and I’ve had a lot of success so far.
I can read binary swipe data just fine and on my way to converting to ascii I am encountering what I think is a stupid problem.

While debugging an issue, I found that adding a single Serial.println(“GotHere”); statement to one of my functions breaks the code. With out the println statement the arduino successfully vomits out the byte data stored on my magnetic card in the serial monitor. However when i simply add the print line, nothing shows up in the serial monitor after I swipe the card…

THIS CODE WORKS!!! You can skip this unless you want a general overview.

int cld1Pin = 5;            // Card status pin
int rdtPin = 2;             // Data pin
int reading = 0;            // Reading status
int buffer[400];   // Buffer for data
int i = 0;         // Buffer counter
int bit = 0;       // global bit

void setup() {
  Serial.begin(9600); 
  
  // The interrupts are key to reliable
  // reading of the clock and data feed
  attachInterrupt(0, changeBit, CHANGE);
  attachInterrupt(1, writeBit, FALLING);
  
}
 
// Flips the global bit
void changeBit(){
  if (bit == 0) {
    bit = 1;
  } else {
    bit = 0;
  }
}
 
// Writes the bit to the buffer
void writeBit(){
  buffer[i] = bit;
  i++;
}



void loop(){
  // Active when card present
  while(digitalRead(cld1Pin) == LOW){
    reading = 1;
  }     
 
  // Active when read is complete
  // Reset the buffer
  if(reading == 1) {      
    sendData();
  }
  
}

// prints the buffer
void sendData(){
  Serial.println("Raw: ");
  for (int j = 0; j < i; j = j + 1) {
    Serial.print(buffer[j]);
  }
  Serial.println(" ");
  reset();
}

void reset() {
  reading = 0;
  i = 0;
  detachInterrupt(0);
  detachInterrupt(1);
  attachInterrupt(0, changeBit, CHANGE);
  attachInterrupt(1, writeBit, FALLING);
}

HERE IS THE PROBLEM–>

In the sendData() function when I change it from:

// prints the buffer
void sendData(){
  Serial.println("Raw: ");
  for (int j = 0; j < i; j = j + 1) {
    Serial.print(buffer[j]);
  }
  Serial.println(" ");
  reset();
}

TO:

// prints the buffer
void sendData(){
  Serial.println("Raw: ");
  for (int j = 0; j < i; j = j + 1) {
    Serial.print(buffer[j]);
  }
  Serial.println(" ");
  Serial.println("gotHere");       //<--- JUST ADDED THAT NOW IT BREAKS
  reset();
}

Im running Arduino 018 IDE On

Fedora Core 12
(Linux dev 2.6.31.5-127.fc12.x86_64 #1 SMP Sat Nov 7 21:11:14 EST 2009 x86_64 x86_64 x86_64 GNU/Linux)

I’m using Arduino NG (rev.c)

I really really really hope this is a stupid mistake!

Extra Background info:
I started out using this instructable: http://www.instructables.com/id/Arduino-magnetic-stripe-decoder/
(So far so good, but couldnt get the code to work, Hence the reason I had to do tweaking and debugging)

In reset(), why do you detach and re-attach the same interrupt handlers?

The variable i has a really poor name for a global variable. It is used throughout the code, including the ISRs, but it is not declared volatile. Why not?

The buffer variable is taking up 800 bytes. That is a significant portion of SRAM. You haven't indicated WHICH Arduino you have, but they vary from 1000 bytes of SRAM to 8000 bytes. If you only have 1000 bytes, you are running out of memory.

Edit: I see that you have an NG, which appears to have a 168 chip, which is the 1000 byte SRAM model. You are definitely running out of memory.

@PaulS

Thanks alot man. I'm fairly new with arduino and I really appreciate your help! I did some research on your other comments and will makes the necessary changes!
I have an UNO coming in the mail too!

I am actually also building one of these, although I have had different problems (reading the incoming data fast enough). Would you mind posting all of your code somewhere? I'd like to see how you are doing things to give me some insight to my project.

Well you could halve your memory usage right away by declaring your buffer as byte rather than int (2 bytes).

But if I read your code correctly, you are only storing 1 or 0 in each int of your buffer. That is 16x more (8x more if you go to bytes) than you need.

A lower-memory approach would be to declare a smaller buffer, and use bit manipulations to store individual bits. You would need to create read and write functions that, given a direction to store or retrieve bit x in the array, find the appropriate bit.

I.e., if your buffer is an array of bytes, then I think bit x in the array = bitRead( buffer[x / 8], x % 8 )

You can also simplify your "changeBit" function to bit = bit ^ 1

Bit manipulation is your friend!

I would make “bit” volatile and smaller, eg.

volatile byte bit = 0;       // global bit

You can toggle it like this:

void changeBit(){
bit ^= 1;  // xor with 1
}

How many bits are you expecting? In this code here:

// Writes the bit to the buffer
void writeBit(){
  buffer[i] = bit;
  i++;
}

What if you get 401 bits? You have overwritten something else. If you expect 10 bits, then 400 is far too high.

To allow for overflow it should be something like:

byte buffer [200];  // say

...

// Writes the bit to the buffer
void writeBit(){
  if (i < sizeof buffer)
    buffer[i++] = bit;
}

But why have a second interrupt anyway? Why not read the status of the data on the clock pulse? So instead of the interrupt on the changing edge (which relies heavily on not missing one), just do this:

// Writes the bit to the buffer
void writeBit(){
  if (i < sizeof buffer)
    buffer[i++] = digitalRead (rdtPin);
}

I just caught this, Im only using one interrupt and I also trimmed the buffer down to 255 the card reader works great now! :grin:

EDIT: BTW thanks for everything everybody. This forum is great, sorry if I broke a few newbee post rules, but I’ll catch on :stuck_out_tongue: