ArduinoCore-avr
ArduinoCore-avr copied to clipboard
arduino
HardwareSerial atomicity issues
I just realized that there might be an atomicity issue in HardwareSerial. So far, we've assumed that the RX buffer is filled by an ISR and emptied by the main loop and that the TX buffer is filled by the main loop and emptied by an ISR (in other words, the head and tail pointers are only written in one "thread" and read by the other). However, when writing to or reading from the serial port from inside an (other) ISR violates this assumption.
Recently, HardwareSerial was modified to not lock up when writing from inside an ISR, but right now bytes written inside an ISR could still be dropped (or bytes read could be read twice). Note that it can still happen that serial printing blocks for a while when the buffer is full, which is typically a bad idea inside an ISR.
The problem is that updating the head and tail pointers is not atomic - it's read - increment - write. If, inside the write function an interrupt triggers between the read and the write that also calls write, the change of the head pointer that happens inside the ISR will be overwritten by the main thread - effectively dropping the byte.
To fix this, the increment should happen with (all) interrupts disabled, incurring a small overhead and interrupt latency.
Do we think this is a problem? Or is serial printing from inside an ISR considered bad practice and to be used for debugging only and is dropping a byte here or there acceptable?
I would try to make it correct. That means doing things in an atomic manner. I really doubt that such a small latency really is going to hurt.
On a side note, not that I am expecting it to actually change anything... I wish a lot of things such as Serial were a fully separate library, which would allow easier hacking for everybody, and would zap unused code and variables automatically too. My feeling is a basic "so what?" if someone has to do an include statement in the code, and I even have reservations with the IDE inserting Arduino.h for people too. I think it prevents learning to do things the proper way, at the cost of not having to type one silly line in your code.
Thanks for sharing your view!
which would allow easier hacking for everybody, and would zap unused code and variables automatically too.
Hmm, not sure how correct this is? The compile already applies dead code removal etc. because of the gc-sections compiler option. If you're talking about the "only use what's needed" linker behaviour for libraries - this only happens for actual .a libraries, which the Arduino IDE only uses for the core, not for libraries.
and I even have reservations with the IDE inserting Arduino.h
Agreed - I'm brooding on a plan to (optionally - for backwards compatibility) disable all pre-processing behaviour at some point, but this is really a separate issue.
How about adding the include statement to a main ino if it isn't present upon loading? :-D Certainly that should be easy to do...
Also... Here is the nice difference with my makefile system v.s. the IDE with a project I am working on right at the moment:
IDE:
text data bss dec hex filename
8608 58 1040 9706 25ea /tmp/build4801155307851925034.tmp/testMultitask.cpp.elf
make:
text data bss dec hex filename
8384 58 1040 9482 250a build/mega2560/testMultitask.elf
This might not seem like a lot, but... every bit helps on an UNO. If I was to disable serial, I could save even more, but I need the output at present.
food for thought, sorry if it is off-topic.
How about adding the include statement to a main ino if it isn't present upon loading? :-D Certainly that should be easy to do...
There's also the automatic generation of prototypes, which is more of a problem IMHO. I've created arduino/Arduino#2406 to track this issue.
food for thought, sorry if it is off-topic.
Perhaps you should send over your test sketch, makefile and perhaps a log of commands from executing the makefile to the developers list? I'm wondering where the difference comes from - a Makefile is just calling the compiler just like the IDE. If there is something in the compiler flags or ordering that we can mimick in the IDE to save a few bytes, that's always good.
Sorry, it's secret sauce that everyone already knows about. Ok, not really ;-) https://github.com/xxxajk/Arduino_Makefile_master
@xxxajk, it seems the size difference might be due to --relax. If you have 1.5.x, could you try modifying your platform.txt to add --relax, e.g.: compiler.c.elf.flags=-w -Os -Wl,--gc-sections,--relax and see if the size becomes the same then? See also arduino/Arduino#2039.
On November 3, 2014 at 3:43 AM Matthijs Kooijman [email protected] wrote:
...
Do we think this is a problem? Or is serial printing from inside an ISR considered bad practice and to be used for debugging only and is dropping a byte here or there acceptable?
In my opinion it's bad practice. That sort of debugging should be done by buffering in memory and printing the buffer at non-ISR level. This could be difficult to do on one of the smaller machines if memory is tight (I'm mostly doing Due programming right now, so I guess I am spoiled).
Peter Olson
if you need simple debug from inside an ISR, write to a buffer, set a flag to print it in the next 'loop()' or similar. (I'm a fan of making a light blink to say "I was here"). or you can write a state variable on error to indicate where you were when the error happened, maybe clone some other debug info as binary values, then pick up on it in 'loop()' processing and report the problem.
Thinking on this a bit more, there's really two atomicity issues present:
- When a value is read-modified-written and an interrupt occurs between the read and write, the value can be wrong.
- When the buffer size is >= 256, the head/tail variables need two instructions to be read/written, so if an interrupt happens between those instructions, things will be inconsistent.
Neither of these issues are super-critical, since 1. can only occur when writing or reading inside an ISR, which isn't really a supported usecase and 2. can only occur when a user modifies the Arduino core to increase the buffer size. However, since the overhead is limited and since there is some code to support buffer sizes >= 256 (suggesting that it is properly supported), I'd vote for fixing both of these issues anyway.
To fix 1., any read-modify-write cycles must happen with interrupts disabled unconditionally. To fix 2., any reads and writes to head and tail pointers must happen with interrupts disabled, if buffer is > 256 bytes long.
There is also the lack of volatile, but that is a well-defined issue tracked by arduino/Arduino#2476.
For conditionally making code atomic, the ATOMIC_BLOCK macro would be useful, which makes code more readable and also allows disabling the atomicity at the top of the .cpp file, instead of having to check the buffer size in various places.
So, at the top of HardwareSerial.cpp you'd have:
#if (SERIAL_TX_BUFFER_SIZE>256)
#define TXBUF_ATOMIC_BLOCK ATOMIC_BLOCK
#else
#define TXBUF_ATOMIC_BLOCK(type)
#endif
And for accessing variables:
int HardwareSerial::availableForWrite(void)
{
TXBUF_ATOMIC_BLOCK(ATOMIC_RESTORESTATE) {
tx_buffer_index_t head = _tx_buffer_head;
tx_buffer_index_t tail = _tx_buffer_tail;
}
if (head >= tail) return SERIAL_TX_BUFFER_SIZE - 1 - head + tail;
return tail - head - 1;
}
(I didn't test the code, but you should get the idea. I'm not entirely sure if defining TXBUF_ATOMIC_BLOCK(type) as empty is possible like this, but it probably is)
So, any progress on this? Still see the warning in HardwareSerial.h in 2017.
@matthijskooijman Your observation is spot on, there are two different albeit related issues, just as you state. For buffers > 256 the warning in the header file is an understatement, havoc will occur.
I encountered this problem in my own projects long ago and made a fix that I never came around to submitting. Will revisit the issue and propose the fix, building on the pattern you showed.
@matthijskooijman Your observation is spot on, there are two different albeit related issues, just as you state. For buffers > 256 the warning in the header file is an understatement, havoc will occur.
I encountered this problem in my own projects long ago and made a fix that I never came around to submitting. Will revisit the issue and propose the fix, building on the pattern you showed.
@egilkv - did you ever come to the point to propose the fix? I am having some issues with large size buffers (> 256) and would be pleased if you can share your fix. Thnx
Should not an issue with 32 bit mcu , if head and tail is 32-bit aligned
Should not an issue with 32 bit mcu , if head and tail is 32-bit aligned
I'm not sure if this is really true (though there might be special atomic increment instructions), but it is also completely not relevant to this particular issue (since this issue is in the ArduinoCore-avr repository, which is an 8-bit MCU).
The module is also used in platformio/arduino lib of stm32
Nope, STM32 Arduino cores (there's at least two as far as I know) come from a different repository with unrelated hardwareserial code (maybe they have a filename by the same name, but the code is completely independent). And again, this repo and issue is about AVR, not STM32. Anyway, this is all detracting from the subject of this issue, so let's not waste more words about it.
//< If the buffer is too small, it will cause the program to crash. It is recommended to change it to a larger size, and change it to 128
#define SERIAL_RX_BUFFER_SIZE 128
#define SERIAL_TX_BUFFER_SIZE 128
If I don't want to modify SERIAL_RX_BUFFER_SIZE/SERIAL_TX_BUFFER_SIZE in the HardwareSerial.h header file, where can I modify it, I am using PlatformIO (based on STM32).
You can define it in main.cpp prior #include hardwareserial.h or in platform.ini
You can define it in main.cpp prior #include hardwareserial.h
That is a common misconception - the buffers are defined in (IIIRC) HardwareSerial.cpp, which does not see your define from main.cpp, so will just use the original. To make sure the defines end up there, you need to make sure to put them on the compiler commandline (which is not really supported by Arduino, I'm not sure about platformio, maybe that's done in platform.ini?). Another way could be to define it in any .h file that is (directly or indirectly) included by HardwareSerial.cpp or .h, but that should normally only include headers from within the core, so that's not much help.
Thank you, I have succeeded, after the hardwareserial.h file is modified, then the compilation is successful.