-
-
Notifications
You must be signed in to change notification settings - Fork 19.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interrupt safe serial --- Guard against non-atomic variable changes from interrupt routines #2994
Interrupt safe serial --- Guard against non-atomic variable changes from interrupt routines #2994
Conversation
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? |
else { | ||
CRITICAL_SECTION_END; | ||
return c; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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().
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 |
All the routines in the MarlinSerial can be, and are, interrupted by Right. As soon we set sei() again, the highest priority pending interrupt is entered. 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. |
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 How about this variation without an else?
|
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? |
@Roxy-3DPrintBoard |
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.
|
@Roxy-3DPrintBoard This comment and check is already in the tx-buffer extension. I suppose that is safe enough. 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) |
OK. I like the way you did that! Nobody will get surprised.
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.
|
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.
dc75fd9
to
cb88fdd
Compare
Added test, if RX_BUFFER_SIZE meets the requirements. Updated and rebased. |
@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. |
Yes, please do. I've got a number of PRs piling up on the |
Interrupt safe serial --- Guard against non-atomic variable changes from interrupt routines
…Interrupt safe serial --- Guard against non-atomic variable changes from interrupt routines MarlinFirmware#2994
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.