Interrupt safe serial --- Guard against non-atomic variable changes from interrupt routines

[AnHardt]

Protect MarlinSerial against interrupts
by shielding the CRITICAL_SECTIONs

Now with a test if RX_BUFFER_SIZE is in the required range.
Code in peek() and read() is now optimized for readability and showing the similarity between the two.

Maybe a bit overprotected in checkRx() and store_char(), but now some days without detected errors from this source.

[Roxy-3D]

I'm sure #2994 is the better one.

I'm confused. Did you do two separate solutions? Can you explain what you are (or were) thinking?

Here is what I'm thinking: I don't like the idea that we are doing changes to the interrupt routines in a Release Candidate, but from what I've seen so far, it is very dangerous not do these changes. Please give us your best thinking on the subject!

If there are two separate solutions, can you give us an explanation about the pluses and minuses of each?

[thinkyhead]

Or this, just for neatness…

CRITICAL_SECTION_START;
int v = (rx_buffer.head == rx_buffer.tail) : -1 : rx_buffer.buffer[rx_buffer.tail];
CRITICAL_SECTION_END;
return v;

…which the compiler should optimize. But if not it can be prompted…

CRITICAL_SECTION_START;
uint8_t t = rx_buffer.tail;
int v = (rx_buffer.head == t) : -1 : rx_buffer.buffer[t];
CRITICAL_SECTION_END;
return v;

Anyway, no code seems to even call peek() so it's fairly a moot point.

[AnHardt]

The compiler is not allowed to optimize version 1 because rx_buffer.tail is volatile.
Version 2 looks nice but does not show well the simmitarity to read().

[thinkyhead]

In Marlin there shouldn't be any serial code being called from interrupt, so we may not see any effect from this. I also worry that this change might cause interrupts to be disabled so often during serial printing that it could cause a degradation in performance. But perhaps not! An interrupt that gets blocked by cli() should fire as soon as sei() is called, right?

[AnHardt]

All the routines in the MarlinSerial can be, and are, interrupted by SIGNAL(M_USARTx_RX_vect) and checkRx() called from the stepper interrupt.
In Marlin the 'hack' is called from the stepper interrupt. (https://github.com/MarlinFirmware/Marlin/blob/RC/Marlin/stepper.cpp#L650)
At 250000bd the stepper interrupt can take longer to finish than the distance between two received characters - so one gets lost.

Right. As soon we set sei() again, the highest priority pending interrupt is entered.
All these CRITICAL_SECTIONs are very short. Compared to a interrupt header (saving all the registers) - the delay is minimal. To be sure about that i replaced the potentially long lasting modulo division by an and (and restricted the buffer size to a power of two for that).
Compared to the huge delay the stepper interrupt causes, this is about nothing.

If you are scared by delays have a look on the next iteration (AnHardt#22) where i introduce a write buffer to eliminate busy waiting while writing. When sending lots of text i can prove a benefit. But when it matters, during speedy short moves, when we usually only send short 'ok's, my improvised benchmark (idle counter) is not exact enough, because it's completely dominated by the stepper interrupt. I've no doubt the positive effect is there, but i can't quantify it.

[AnHardt]

rx_buffer.tail = (unsigned int)(t + 1) & (RX_BUFFER_SIZE - 1);

Any reason for casting to 16 bit and back to 8 to fit in rx.buffer.tail. If the RX_BUFFER_SIZE is 256 we could omit the & (RX_BUFFER_SIZE - 1) part. 8 bit overflow would do exactly what we want. The aim is to do this calculation in 8 bit if possible

How about this variation without an else?

int MarlinSerial::read(void) {
  int v = -1;
  CRITICAL_SECTION_START;
  uint8_t t = rx_buffer.tail;
  if (rx_buffer.head != t) {
    v = rx_buffer.buffer[t];
    rx_buffer.tail = (uint8_t)(t + 1) & (RX_BUFFER_SIZE - 1);
  }
  CRITICAL_SECTION_END;
  return v;
}

[Roxy-3D]

If you are scared by delays have a look on the next iteration (AnHardt#22) where i introduce a write buffer to eliminate busy waiting while writing.

I really want an interrupt driven Tx buffer. But especially since it is broken out as its own patch it would be much safer to start the use of that over in MarlinDev. @AnHardt do you agree and are you OK with that thinking?

[AnHardt]

@Roxy-3DPrintBoard
Let's satisfy @thinkyhead up to here. Than i'll redo the tx-buffer based on this and maybe i'm able to send it to Dev.

[Roxy-3D]

My vote is against trying to save the 8 bit compare and branch overhead. But if we are going to do that... We need everything clearly warning about the buffer size assumption.

How about this variation without an else?

#define RX_BUFFER_SIZE 256  // The code is dependent upon RX_BUFFER_SIZE being 256.
                            // Do not change without changing MarlinSerial::read() code.

int MarlinSerial::read(void) {
  int v = -1;
  CRITICAL_SECTION_START;
  uint8_t t = rx_buffer.tail;
  if (rx_buffer.head != t) {
    v = rx_buffer.buffer[t];
    rx_buffer.tail = (uint8_t)(t + 1) & (RX_BUFFER_SIZE - 1);   // This code assumes the RX_BUFFER_SIZE is 256
                                                                // bytes big.
  }
  CRITICAL_SECTION_END;
  return v;
}

[AnHardt]

@Roxy-3DPrintBoard
Dear Roxy
(unsigned int)(rx_buffer.tail + 1) % RX_BUFFER_SIZE; works with any size buffer.
(uint8_t)(t + 1) & (RX_BUFFER_SIZE - 1); works with any buffer size that is a power of two <= 256.
(uint8_t)(t + 1); works only with 256.

This comment and check is already in the tx-buffer extension. I suppose that is safe enough.
https://github.com/AnHardt/Marlin/pull/22/files#diff-046a3d48c0a2ee03ef0b6a4a5a7cbb8cR83

Even in the original Arduino HardwareSerial there is a warning about, to avoid not power of 2 size buffers. (https://github.com/MarlinFirmware/Marlin/blob/RC/ArduinoAddons/Arduino_1.6.x/hardware/marlin/avr/cores/arduino/HardwareSerial.h#L35)

[Roxy-3D]

OK. I like the way you did that! Nobody will get surprised.

-#define RX_BUFFER_SIZE 128
 -
 +// 256 is the max limit due to uint8_t head and tail. (...,16,32,64,128,256)
 +#ifndef RX_BUFFER_SIZE
 +  #define RX_BUFFER_SIZE 128
 +#endif
 +#ifndef TX_BUFFER_SIZE
 +  #define TX_BUFFER_SIZE 32
 +#endif
 +#if !((RX_BUFFER_SIZE == 256) ||(RX_BUFFER_SIZE == 128) ||(RX_BUFFER_SIZE == 64) ||(RX_BUFFER_SIZE == 32) ||(RX_BUFFER_SIZE == 16) ||(RX_BUFFER_SIZE == 8) ||(RX_BUFFER_SIZE == 4) ||(RX_BUFFER_SIZE == 2) ||(RX_BUFFER_SIZE == 0))
 +  #error RX_BUFFER_SIZE has to be a power of 2 or 0
 +#endif
 +#if !((TX_BUFFER_SIZE == 256) ||(TX_BUFFER_SIZE == 128) ||(TX_BUFFER_SIZE == 64) ||(TX_BUFFER_SIZE == 32) ||(TX_BUFFER_SIZE == 16) ||(TX_BUFFER_SIZE == 8) ||(TX_BUFFER_SIZE == 4) ||(TX_BUFFER_SIZE == 2) ||(TX_BUFFER_SIZE == 0))
 +  #error TX_BUFFER_SIZE has to be a power of 2 or 0
 +#endif

I wish we could get the -ii option working on GCC so that we could look at the optimizations it is doing. In the case of RX_BUFFER_SIZE set at 256, I bet the extra AND to mask the result is not done. But there isn't an easy way to check.

(uint8_t)(t + 1) & (RX_BUFFER_SIZE - 1);

[AnHardt]

Added test, if RX_BUFFER_SIZE meets the requirements.
Code in peek() and read() is now optimized for readability and showing the similarity between the two.

Updated and rebased.

[Roxy-3D]

@thinkyhead I know we both agreed it did not make sense to keep MarlinDev and the RC in lock step. But the bug these changes fix is serious enough I'm thinking it makes sense to also put it over at MarlinDev in advance of all the other bug fixes.

[thinkyhead]

the bug these changes fix is serious enough I'm thinking it makes sense to also put it over at MarlinDev

Yes, please do. I've got a number of PRs piling up on the dev branch to keep track of bug-fixes that ought to get into dev. If this actually prevents loss of serial characters, that's pretty important.