Simple serial message not consistent (sometimes jumbled up)

Question

I have the following code, it works (most of the time) - yet it's not 100% reliable and now and then will send through a double message on one line or 2 subsequent alpha characters:

const byte interruptPin0 = CONTROLLINO_IN0;
const byte interruptPin1 = CONTROLLINO_IN1;
int counter1 = 0;
int counter2 = 0;
unsigned long previousMillis = 0;
unsigned long interval = 2000;
int a = 60;
char buffer1[256];
char buffer2[256];
void setup()
{
 pinMode(interruptPin0, INPUT_PULLUP);
 pinMode(interruptPin1, INPUT_PULLUP);
 attachInterrupt(digitalPinToInterrupt(interruptPin0), pin1Fired, RISING);
 attachInterrupt(digitalPinToInterrupt(interruptPin1), pin2Fired, RISING);
 Serial.begin(9600);
}
void loop()
{
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis > interval) {
 previousMillis = currentMillis;
 sprintf(buffer1,"A%d",counter1);
 Serial.println(buffer1);
 sprintf(buffer2,"D%d",counter2);
 Serial.println(buffer2);
 counter1 = 0;
 counter2 = 0;
 }
}
void pin1Fired()
{
 counter1 = counter1 + 1;
}
void pin2Fired()
{
 counter2 = counter2 + 1;
}

Result:

enter image description here

This later causes a problem when my code on the other end of the serial comms expects [LETTER][NUMBER].

What have I missed here? I'm relatively new to C for Arduino so quite possibly took a bad approach here?

I've tried to avoid sending serial comms from the interrupt, I read that was risky - hence me tracking the interrupt counts separately.

Can also confirm baud rate matches.

EDIT

To confirm MCU resets I have printed in setup - note: no interrupts firing, still the same issue:

enter image description here

• Juraj: add some printing to setup(). something like Serial.println("START");to see if the MCU doesn't reset for some reason.
• JᴀʏMᴇᴇ: Thanks @Juraj - see edit.
• Juraj: which version of Arduino AVR boards package do you use? The newer versions have newer avr-gcc compiler version and it sometimes has strange problems. Try the Arduino AVR boards package 1.6.21. It has the well-tried version of avr-gcc.
• JᴀʏMᴇᴇ: Having to use Controllino's own board manager @Juraj
• JᴀʏMᴇᴇ: It seems to compile/upload fine with 1.6.21, I'll let this run and see if this has solved it.

Answer 1

My guess is that an interrupt is occurring during your reading or writing of the counter variables.

Since the variables are bigger than the native size of the registers in the CPU multiple instructions are required to work with them, and an interrupt can occur right in the middle of that work which changes the values it's working on. That can cause a mess (though I wouldn't expect it to make that kind of mess, TBH).

You should wrap all accesses to the counters that aren't in the interrupts inside a critical section. This is a segment of the code where interrupts are not allowed to happen. For obvious reasons you want to keep that code as short as possible.

Also in the code you have given there is no need for your buffers and sprintfs. You can just use multiple Serial.prints to achieve the same result with a much smaller code/memory footprint.

Here's an example:

void loop() {
 unsigned long currentMillis = millis();
 if (currentMillis - previousMillis > interval) {
 previousMillis = currentMillis;
 // Copy and reset the counter values in a critical section
 noInterrupts();
 int nowCounter1 = counter1;
 counter1 = 0;
 int nowCounter2 = counter2;
 counter2 = 0;
 interrupts();
 Serial.print("A");
 Serial.println(nowCounter1);
 Serial.print("D");
 Serial.println(nowCounter2);
 }
}

Also your counter variables should be volatile since they are accessed from both the interrupts and the main loop:

volatile int counter1 = 0;
volatile int counter2 = 0;

• JᴀʏMᴇᴇ: Thanks for your reply! That makes sense, I do recall seeing 'noInterrupts' and should have known to use it. I've copied the loop example in your answer but it's still happening. Just received A0A0
• Majenko: What board are you using?
• JᴀʏMᴇᴇ: Controllino mini
• Majenko: I find it impossible to work out what USB chip they are using in there. It could be a pile of junk for all we know.

Answer 2

This is not an answer, but showing a way to make your code more clean.

Use the critical sections as proposed by Majenko.

You can rewrite

sprintf(buffer1,"A%d",counter1);
Serial.println(buffer1);
sprintf(buffer2,"D%d",counter2);
Serial.println(buffer2);

as

sprintf(buffer, "A%d\r\nD%d", counter1, counter2);
Serial.println(buffer);

Also, an unsigned long int can be maximum around 10 characters (2^32), so including the 'A', 'D', extra new line and 0,円 a single buffer of around 30 characters will suffice, instead of 2 buffers of 256 bytes (which takes 25% of an Arduino UNO's SRAM).

Also you can rewrite

counter1 = counter1 + 1;

as

counter1++;

which increases a variable with 1.

Also counter1 and counter2 should never be negative so an unsigned type is better.

• Michel Keijzers: @EdgarBonet Thanks for that comment, I updated my answer accordingly.

Answer 3

This isn't meant to be a solution; it's more of a workaround.

To avoid repeated letters, you could use code to make sure that each alpha character isn't the same as the one before, and if so, delete the current character before printing.

Then to tackle doubling, if the string has an even number of characters, split the string in half, and if both halves are equal, remove one of them from the string.

This isn't exactly what you wanted but it's more of a worst-case scenario kind of thing.

Answer 4

Very frustrating time trying to diagnose this.

All credit to Controllino for their support when looking into whether it was a hardware/code issue (it wasn't!).

The problem turned out to be the recipient of these messages. The performance of the PC the Controllino was plugged into was really bad. I noticed what you've seen in the screenshots I've shared every time I'd open a new app or dragged a window.

The serial monitor in the Arduino IDE would lock up for a few seconds and so would miss the serial messages (and have to catch up).