Serial data corrupt on ONE computer....

I wrote this application to log vehicle data to a SQL database so that I can analyze my driving habits. The arduino makes a few measurements, and sends them across the USB serial connection in a comma separated, semi-human readable format. I have a VB app that sits on the computer and either logs it to a sql database or caches it in a text file when the sql server is unavailable.

Everything is working great on my development computer - I have another micro generating a sample waveform and it measures it perfectly. But, when I plug it into the laptop that I had intended to use on the road, a FEW of the values are corrupt, but the formatting and all of the delimiters are fine. I know it's not the VB end of things - I can open it in hyperterminal, and it's corrupt there as well. I've tried changing the baud rate, etc, but the results are the same - fine on the desktop, first two values are corrupt on the laptop. And even the WAY that they are corrupt is consistent - should be sending a value of 300, and it will alternate between a few different values - 0.00, 2500000 and 1300000 for example. Not just any random values, particular ones. It's as if the buffer is getting full on the laptop and affecting the calculations on the arduino, but why on one, and why does the baud rate have no effect? Any ideas???

Good Output:
S39928,300.61,0.00,436,5,322,7,413,2,
S59900,300.61,0.00,439,5,355,7,422,3,

Bad Output:
S86688,0.00,0.00,436,5,322,7,413,2,
S91820,1875000.00,0.00,439,5,355,7,422,3,

Code is a little messy, but here it is:

unsigned long InjOpenTime;
unsigned long InjOpenLast;
unsigned long InjCloseTime;
unsigned long InjOpenTotal;
//unsigned long InjCloseTotal;
unsigned long SampleStartTime;
unsigned long SampleStartLast;
unsigned long SampleIntervalLength;
float SampleMillis;
float SampleMinutes;
float SampleHours;
float SampleDistance;
float SampleDistanceOld;
float DistanceChange;

long InjLoopTime;
long InjSampleTime;
long InjSampleLast;
long InjDuty;
long InjCycleOpenTotal;
float RPMs;
long VssPulseCount;
long VssMicro;
long VssLastMicro;
long VssTime;
float MPH;
int distance;
int distanceOld;
int InjCloseCount;
int InjCloseCountOld;
int DeltaInjCloseCount;
boolean VssState;
boolean VssLastState;
boolean Test;



void setup() {
  Serial.begin(57600);
  pinMode(2, INPUT);
  pinMode(3, INPUT);
  pinMode(15, INPUT);
  
  attachInterrupt(0, InjOpen, RISING); // Pin 2 interrupt
  attachInterrupt(1, InjClose, FALLING); // Pin 3 interrupt
}

void loop() {
    SampleStartTime = micros();
    SampleDistance = distance;
    InjSampleTime = InjOpenTotal;
    InjDuty = InjSampleTime - InjSampleLast;
    InjSampleLast = InjSampleTime;
        
    SampleIntervalLength = SampleStartTime - SampleStartLast;
    SampleStartLast = SampleStartTime;
    
    DistanceChange = SampleDistance-SampleDistanceOld;
    SampleDistanceOld = SampleDistance;
    
    SampleMillis = SampleIntervalLength / 1000;
    SampleMinutes = SampleIntervalLength / 60000;
    SampleHours = SampleMinutes/60;
   
    MPH = DistanceChange*.66 * 5280/SampleIntervalLength;
    
    DeltaInjCloseCount = InjCloseCount - InjCloseCountOld;
    if (DeltaInjCloseCount < 2)
    {
    // Not Running..
    InjLoopTime = 400002;
    }
    InjCloseCountOld = InjCloseCount;
    
  if (InjLoopTime > 400000)
  {
    RPMs = 0;
    InjLoopTime = 400000;
  }
  else
  {
    RPMs = InjLoopTime;
    RPMs = 1/RPMs * 60000000;
  }
  
  
  
  Serial.print("S"); 
    Serial.print(InjDuty); // Inj pW
    Serial.print(",");
    Serial.print(RPMs);  // RPM
    Serial.print(",");
    Serial.print(MPH); // MPH
    Serial.print(",");
    Serial.print(analogRead(5)); // AccelX
    Serial.print(",");
    Serial.print("5"); // AccelY
    Serial.print(",");
    Serial.print(analogRead(4)); // AccelZ
    Serial.print(",");
    Serial.print("7"); // CoolantTemp
    Serial.print(",");
    Serial.print(analogRead(5)); // AirTemp
    Serial.print(",");
    Serial.print(DeltaInjCloseCount); // InjEvents
    Serial.println(",");

    delay(1000); // Sampling period
  
}

void InjOpen()
{ 
  InjOpenTime= micros();
}

void InjClose()
{
 InjCloseTime = micros();
 InjCycleOpenTotal =  InjCloseTime - InjOpenTime;
 InjOpenTotal += InjCycleOpenTotal; 
 
 InjLoopTime = InjOpenTime - InjOpenLast;
 InjOpenLast = InjOpenTime;
 
 //VssState = digitalRead(15);
 //if(VssState != VssLastState)
 //{
 //  distance++; // We just went .66 feet
 //  VssLastState = VssState;
 //}
 InjCloseCount +=1;
}

Moderator edit:
</mark> <mark>[code]</mark> <mark>

</mark> <mark>[/code]</mark> <mark>
tags added.

Bad Output:
S86688,0.00,0.00,436,5,322,7,413,2,

Why is that line "bad"?

You have two ISRs defined. In those ISRs and in loop(), you reference the same variables, and yet there is nary a volatile keyword in sight. Why would that be?

2nd parameter should have a value - in this case, it should be around 300 (RPM). Interrupts capture a running total of how long the injectors have been open (and number of times opened) since the dawn of time (or power, in this case). The main loop captures a snapshot of those values every x milliseconds (1000, in this case), and compare the current values with the last sample. The difference is how long / how many times it opened during the sample period. Values are as follows:

Total injector on-time during this sample period,
RPM,
MPH (placeholder for now),
X Axis acceleration,
Y Axis acceleration,
Z Axis acceleration,
Coolant temp placeholder,
Air temperature,
Count of injector events this sample period.

PaulS:
You have two ISRs defined. In those ISRs and in loop(), you reference the same variables, and yet there is nary a volatile keyword in sight. Why would that be?

You are correct. Pin two and three are both being used as interrupts for the injector signal, but on opposite edges. The injector should never open and close quick enough to actually cause a problem, but I hadn't considered asynchronous read access of the main loop as being an issue. I understand that this could be an issue if the values change significantly. FWIW, I've logged literally hours of data and this doesn't seem to be an issue on the desktop.

On a side note, while this might be my first post, I've been reading this forum for a while, and I've noticed a certain tone in a number of your responses. I don't know for certain what your intended tone/attitude is, but keep in mind the audience that you are dealing with. I would be suprised to learn that the majority of users on this forum are experienced, professional programmers. I myself was never formally educated on any high level language, and I take a great deal of pride in my self-inscribed abilities. I appreciate your input, but I still feel that you could have presented it in a format that doesn't carry the risk of sounding condescending.

"You have two ISRs defined. In those ISRs and in loop(), you reference the same variables. You should be using the volatile keyword when declaring them. This prevents the compiler from optimizing them under the assumption that they can't change on their own." would have worked just fine.

Thanks

...would have worked just fine.

I have no way of knowing why you didn't use the volatile keyword. Perhaps you were aware of it, and had good reasons for not using it. I offered you a change to explain them. If you weren't aware of the volatile keyword, you could google it, or ask here, and we could offer (more) advice.

I don't think I was being condescending. At least, that wasn't my intent.

PaulS:

...would have worked just fine.

I have no way of knowing why you didn't use the volatile keyword. Perhaps you were aware of it, and had good reasons for not using it. I offered you a change to explain them. If you weren't aware of the volatile keyword, you could google it, or ask here, and we could offer (more) advice.

I don't think I was being condescending. At least, that wasn't my intent.

I assumed it was condescending. My apologies.

Edit: Back to the topic, I'll change that later today and see what happens. I have a feeling that the InjLoopTime variable is having issues because of this - since it's being divided by a constant, and the result is a very large number, I'm guessing that it's being changed, and becoming a single digit number - that's why it's being sent as either a very large, granular value, or zero.

I changed the variables that are modified in ISRs to volatile, and it seems to have fixed the issue. It makes sense that the timing might be different between two different computers, but I still don't understand exactly why it would only cause a problem with one computer. Oh well....

So here's an update - I don't think it was the volatile keyword all along - I was powering the test function generator from a PC while running the datalogger. When they were both plugged into the same computer, it would work fine. I transferred JUST the datalogger to a laptop running off of batteries, and it would start to have corruption issues. When I moved the signal generator to the laptop as well, everything works fine again, so I'm apparently picking up noise on the ground connection even though the laptop is un-grounded. Weird, but at least I have the source.

I don't know what your circuit is, but the symptoms you're giving make me suspect earthing issues. Perhaps you could post the circuit?

I'm unclear now whether the problem is that the data is getting garbled during transmission, or is corrupted in the first place. Perhaps a test sketch which produces dummy data would tell you which was the case?

How is the Arduino powered in the car? It'll be very vulnerable to any electrical noise on the power supply or any of its inputs, so you'd need to buffer them to ensure reliable operation.

The hardware setup was taken directly from MPGuino, so I know it's pretty solid. I have one arduino putting a test pulse train out on pin 2 - 2ms on, 98ms off, for example (same as 2ms of fuel at 1200 RPMs, since the injector fires every other cycle). Pin 2 on the test signal generator is linked to a 22k resistor to pin 2+3 on the target, with a 5.6v zener to ground (for protection from injector flyback,etc). Ground pin on both boards are linked with a straight jumper, about 4 inches long.

In the car it seems to work fine, so I'm not concerned, but when I have the test generator powered by USB on my PC and the target board powered by USB on a laptop (not even plugged in), it gives weird results.

Again, it works fine in the car, it's just strange.