Missing buffers with serial communication

Question

I am currently facing a problem with Serial communication with my arduino MEGA 2560.

My project is to collect data from an encoder. There are 2 signals : ACP and ARP. For 1 rotation, there are 16 384 ACPs (counter) and 1 ARP (reference). I have to store the high level and length of EACH ACP and if there is an ARP. To receive these signals I created a little PCB with a dual line receiver (because At the output of the encoder I revceive differential signals).

To store these data, I use a buffer of bytes. I need 3 bytes to store both ACP length and high and 1 ARP (I store the data in an ISR, see code below) :

buff1[i] = char((arpNbr << 7) +  (acpPeriod >> 8));
i++;
buff1[i] = char(acpHigh);
i++;
buff1[i] = char(acpPeriod);
i++;

I use 2 circular buffers (when 1 is filled, I start wrinting into the other). As there are different speed rotation, the fastest one is 15 RPM (rotation per minute) so 1 ACP signal (1 period) last for 244 us and 4 s are needed for 1 rotation.

To collect the data, I directly use :

Serial.write(buff1, sizeof(buff1));

Here is the code I am using :

#include <eRCaGuy_Timer2_Counter.h> //To use timer in ISRs

//Constants
#define NO_DEFAULT_LED_PIN            40
#define DEFAULT_LED_PIN               42
#define WRITE_LED_PIN                 44
#define REC_IN_PROGRESS_LED_PIN       50
#define ACP_LED                       46
#define ARP_LED                       48
#define ACP_PIN                       20
#define ARP_PIN                       21
#define STOP_REC_BUTTON_PIN           28

#define SIZE                          800
#define BAUDSPEED                     230400   

//Variables    
boolean recordingEnded = false;
volatile boolean writing = false;
volatile unsigned short arpNbr = 0;
volatile unsigned short acpHigh = 0;
volatile unsigned short acpPeriod = 0;
volatile int i = 0;
volatile bool change_buffer = 0;

char buff1[3 * SIZE];
char buff2[3 * SIZE];

//Variables for interruption routines
volatile unsigned long acpHighUs = 0UL; volatile unsigned long acpPeriodUs0 = 0UL; volatile unsigned long acpPeriodUs1 = 0UL;

/**************************************************************************************/
/* Initialization                                                                     */
/**************************************************************************************/
void setup()
{
  Serial.begin(BAUDSPEED);

  //Setup LED pins
  pinMode(DEFAULT_LED_PIN, OUTPUT);
  pinMode(NO_DEFAULT_LED_PIN, OUTPUT);
  pinMode(SD_WRITE_LED_PIN, OUTPUT);
  pinMode(REC_IN_PROGRESS_LED_PIN, OUTPUT);
  pinMode(ACP_LED, OUTPUT);
  pinMode(ARP_LED, OUTPUT);

  //Setup ACP and ARP inputs
  pinMode(ACP_PIN, INPUT_PULLUP);
  pinMode(ARP_PIN, INPUT_PULLUP);

  //Setup the button
  pinMode(STOP_REC_BUTTON_PIN, INPUT_PULLUP);

  //Light ALL LEDs during initialisation
  digitalWrite(DEFAULT_LED_PIN, HIGH);
  digitalWrite(NO_DEFAULT_LED_PIN, HIGH);
  digitalWrite(SD_WRITE_LED_PIN, HIGH);
  digitalWrite(REC_IN_PROGRESS_LED_PIN, HIGH);
  digitalWrite(ACP_LED, HIGH);
  digitalWrite(ARP_LED, HIGH);
  delay(500);

  //Setup the timer
  timer2.setup();

  //Setup is done
  digitalWrite(NO_DEFAULT_LED_PIN, LOW);
  digitalWrite(SD_WRITE_LED_PIN, LOW);
  digitalWrite(ACP_LED, LOW);
  digitalWrite(ARP_LED, LOW);

  //Attach interruption at the end of the initialisation (recording starts...)
  attachInterrupt(digitalPinToInterrupt(ACP_PIN), acpRising, RISING);
  attachInterrupt(digitalPinToInterrupt(ARP_PIN), arpRising, RISING);
}

/**************************************************************************************/
/* Main loop                                                                          */
/**************************************************************************************/
void loop()
{
  //Check if the user stops the recording
  if (digitalRead(STOP_REC_BUTTON_PIN) == LOW && !recordingEnded )
  {
    noInterrupts();
    delay(500);

    //Switch off the recording LED
    digitalWrite(SD_WRITE_LED_PIN, LOW);
    digitalWrite(REC_IN_PROGRESS_LED_PIN, LOW);
    digitalWrite(ACP_LED, LOW);
    digitalWrite(ARP_LED, LOW);

    return;
  }

  //Check if the writing should be done
  if (writing && !recordingEnded)
  {
    digitalWrite(WRITE_LED_PIN, HIGH);

    if (change_buffer) {
       Serial.write(buff1, sizeof(buff1));
    }
    else {
       Serial.write(buff2, sizeof(buff2));
    }

    digitalWrite(WRITE_LED_PIN, LOW);
    writing = false;
  }
}

/**************************************************************************************/
/* This method is called at the beginning of each ACP.                                */
/**************************************************************************************/
void acpRising()
{
  digitalWrite(ACP_LED, HIGH);
  acpHighUs = timer2.get_count();
  acpPeriodUs1 = timer2.get_count() / 2 - acpPeriodUs0;
  acpPeriodUs0 = timer2.get_count() / 2;

  acpPeriod = acpPeriodUs1;

  attachInterrupt(digitalPinToInterrupt(ACP_PIN), acpFalling, FALLING);    
}

/**************************************************************************************/
/* This method is called at the end of each ACP.                                      */
/**************************************************************************************/
void acpFalling()
{
  digitalWrite(ACP_LED, LOW);
  acpHighUs = (timer2.get_count() - acpHighUs) / 2;
  acpHigh = (unsigned short)acpHighUs;

  if (change_buffer) {
    buff1[i] = char((arpNbr << 7) +  (acpPeriod >> 8));
    i++;
    buff1[i] = char(acpHigh & 0xFF);
    i++;
    buff1[i] = char(acpPeriod & 0xFF);
    i++;

  }
  else  {
    buff2[i] = char((arpNbr << 7) +  (acpPeriod >> 8));    
    i++;
    buff2[i] = char(acpHigh & 0xFF);
    i++;
    buff2[i] = char(acpPeriod & 0xFF);
    i++;
  }

  arpNbr = 0;

  if (i == 3 * SIZE) {
    i = 0;
    change_buffer = !change_buffer;
    writing = true;
  }

  attachInterrupt(digitalPinToInterrupt(ACP_PIN), acpRising, RISING);
}

/**************************************************************************************/
/* This method is called at the beginning of each ARP.                                */
/**************************************************************************************/
void arpRising()
{
  digitalWrite(ARP_LED, HIGH);
  arpNbr = 1;

  attachInterrupt(digitalPinToInterrupt(ARP_PIN), arpFalling, FALLING);
}

/**************************************************************************************/
/* This method is called at the end of each ARP.                                */
/**************************************************************************************/    
void arpFalling() {
  digitalWrite(ARP_LED, LOW);
  attachInterrupt(digitalPinToInterrupt(ARP_PIN), arpRising, RISING);
}

Problem :

I use CoolTerm software to collect my data into a .txt file and analyse it with Hexed.it.

It seems like after a certain period, like 6-10 minutes, I loose a part of 1 buffer. I checked this beacause as I wrote above, I need 3 bytes for information and in Hexed.it, I saw that I had only 2 bytes at once, and after, data was correct.

After analysing in depth, I saw that there was a part of a buffer and a few more (1-2) which were missing. I managed to see this because there was not 16384x3 bytes between 2 ARP (reference signal of the encoder).

I checked all timings to see if I sent the buffer before it was filled but everything is fine...

I also tried other methods like :

• Writing on a SD card;

• Changing buffer size;

• Changing baudrate;

• Using polling method instead of ISRs.

But nothing concluant ...

EDIT:

It seems like my problem came from CoolTerm software after correcting my code. I now use RealTerm and I made a 1 night recording duration and every seems to be perfect. I don't have any data lost.

Answer 1

Not a direct answer to your questions, but after reading your code I would like to give some comments:

1. You are not using “circular buffers”, you are double-buffering. If you used a circular buffer, you would not need two of them.
2. recordingEnded is always false and thus serves no purpose.
3. When writing and change_buffer are both true, you should transmit buff2, not buff1, as the ISR is now filling buff1.
4. There are too many globals. Variables that are used in a single function should be local to the function. If you need the value to be preserved between calls, qualify the variable as static.
5. The code that fills the buffers would be clearer, IMO, if you avoided the repetition. E.g.

char *buff = change_buffer ? buff1 : buff2;
buff[i++] = (arpNbr << 7) | (acpPeriod >> 8);
buff[i++] = acpHigh & 0xFF;
buff[i++] = acpPeriod & 0xFF;

Note that buf[i++] is idiomatic C/C++.

Now, regarding your specific question, you mentioned in a comment that “there were not 3x16384 bytes between 2 ARP”. This may well be a consequence of issue 3: you are not sending the bytes in the correct order, as at times (when Serial.write() runs ahead of the ISR) you may be sending old data.

---

Edit: to answer the comment

char *buff = change_buffer ? buff1 : buff2; won't take useless time?

Not really. Testing change_buffer is something you have to do either way. Assigning the buffer address to a local pointer is also done, under the hood, by the compiler. In the best case, it optimizes your code to something like:

register char *Z;
if (change_buffer)
  Z = buff1 + i;
else
  Z = buff2 + i;
Z[0] = (arpNbr << 7) | (acpPeriod >> 8);
Z[1] = acpHigh & 0xFF;
Z[2] = acpPeriod & 0xFF;
i += 3;

Using an explicit buff pointer gives a similar result, as the compiler will allocate this to the CPU's Z register. Actually, you may get shorter code, as the compiler may not entirely factor out the repetition.