HardwareSerial improvements and fixes #1369
Conversation
Since the buffers aren't bigger than 64 bytes, these values can be smaller. This saves a few bytes of ram, but also saves around 50 bytes of program space, since the values can now be loaded using a single instruction. To prevent problems when people manually increase the buffer size, a compile-time check is added. Closes: arduino#1078
This removes the need for doing an extra pointer dereference on every access to the buffers, shrinking the code by around 100 bytes. The members for these buffers must be public for now, since the interrupt handlers also need to access them. These can later be made private again. Furthermore, the struct ring_buffer was removed. This allows the all head and tail pointers to be put into the HardwareSerial struct before the actual buffers, so the pointers all end up in the first 32 bytes of the struct that can be accessed using a single instruction (ldd). References: arduino#947
Previously, the constants to use for the bit positions of the various UARTs were passed to the HardwareSerial constructor. However, this meant that whenever these values were used, the had to be indirectly loaded, resulting in extra code overhead. Additionally, since there is no instruction to shift a value by a variable amount, the 1 << x expressions (inside _BV and sbi() / cbi()) would be compiled as a loop instead of being evaluated at compiletime. Now, the HardwareSerial class always uses the constants for the bit positions of UART 0 (and some code is present to make sure these constants exist, even for targets that only have a single unnumbered UART or start at UART1). This was already done for the TXC0 constant, for some reason. For the actual register addresses, this approach does not work, since these are of course different between the different UARTs on a single chip. Of course, always using the UART 0 constants is only correct when the constants are actually identical for the different UARTs. It has been verified that this is currently the case for all targets supported by avr-gcc 4.7.2, and the code contains compile-time checks to verify this for the current target, in case a new target is added for which this does not hold. This verification was done using: for i in TXC RXEN TXEN RXCIE UDRIE U2X UPE; do echo $i; grep --no-filename -r "#define $i[0-9]\? " /usr/lib/avr/include/avr/io* | sed "s/#define $i[0-9]\?\s*\(\S\)\+\s*\(\/\*.*\*\/\)\?$/\1/" | sort | uniq ; done This command shows that the above constants are identical for all uarts on all platforms, except for TXC, which is sometimes 6 and sometimes 0. Further investigation shows that it is always 6, except in io90scr100.h, but that file defines TXC0 with value 6 for the UART and uses TXC with value 0 for some USB-related register. This commit reduces program size on the uno by around 120 bytes.
The actual interrupt vectors are of course defined as before, but they let new methods in the HardwareSerial class do the actual work. This greatly reduces code duplication and prepares for one of my next commits which requires the tx interrupt handler to be called from another context as well. The actual content of the interrupts handlers was pretty much identical, so that remains unchanged (except that store_char was now only needed once, so it was inlined). Now all access to the buffers are inside the HardwareSerial class, the buffer variables can be made private. One would expect a program size reduction from this change (at least with multiple UARTs), but due to the fact that the interrupt handlers now only have indirect access to a few registers (which previously were just hardcoded in the handlers) and because there is some extra function call overhead, the code size on the uno actually increases by around 70 bytes. On the mega, which has four UARTs, the code size decreases by around 70 bytes.
This is slightly more clear than the previous explicit comparison.
The flush() method blocks until all characters in the serial buffer have been written to the uart _and_ transmitted. This is checked by waiting until the "TXC" (TX Complete) bit is set by the UART, signalling completion. This bit is cleared by write() when adding a new byte to the buffer and set by the hardware after tranmission ends, so it is always guaranteed to be zero from the moment the first byte in a sequence is queued until the moment the last byte is transmitted, and it is one from the moment the last byte in the buffer is transmitted until the first byte in the next sequence is queued. However, the TXC bit is also zero from initialization to the moment the first byte ever is queued (and then continues to be zero until the first sequence of bytes completes transmission). Unfortunately we cannot manually set the TXC bit during initialization, we can only clear it. To make sure that flush() would not (indefinitely) block when it is called _before_ anything was written to the serial device, the "transmitting" variable was introduced. This variable suggests that it is only true when something is transmitting, which isn't currently the case (it remains true after transmission is complete until flush() is called, for example). Furthermore, there is no need to keep the status of transmission, the only thing needed is to remember if anything has ever been written, so the corner case described above can be detected. This commit improves the code by: - Renaming the "transmitting" variable to _written (making it more clear and following the leading underscore naming convention). - Not resetting the value of _written at the end of flush(), there is no point to this. - Only checking the "_written" value once in flush(), since it can never be toggled off anyway. - Initializing the value of _written in both versions of _begin (though it probably gets initialized to 0 by default anyway, better to be explicit).
…hile It turns out there is an additional corner case. The analysis in the previous commit wrt to flush() assumes that the data register is always kept filled by the interrupt handler, so the TXC bit won't get set until all the queued bytes have been transmitted. But, when interrupts are disabled for a longer period (for example when an interrupt handler for another device is running for longer than 1-2 byte times), it could happen that the UART stops transmitting while there are still more bytes queued (but these are in the buffer, not in the UDR register, so the UART can't know about them). In this case, the TXC bit would get set, but the transmission is not complete yet. We can easily detect this case by looking at the head and tail pointers, but it seems easier to instead look at the UDRIE bit (the TX interrupt is enabled if and only if there are bytes in the queue). To fix this corner case, this commit: - Checks the UDRIE bit and only if it is unset, looks at the TXC bit. - Moves the clearing of TXC from write() to the tx interrupt handler. This (still) causes the TXC bit to be cleared whenever a byte is queued when the buffer is empty (in this case the tx interrupt will trigger directly after write() is called). It also causes the TXC bit to be cleared whenever transmission is resumed after it halted because interrupts have been disabled for too long.
When interrupts are disabled, writing to HardwareSerial could cause a lockup. When the tx buffer is full, a busy-wait loop is used to wait for the interrupt handler to free up a byte in the buffer. However, when interrupts are disabled, this will of course never happen and the Arduino will lock up. This often caused lockups when doing (big) debug printing from an interrupt handler. Additionally, calling flush() with interrupts disabled while transmission was in progress would also cause a lockup. When interrupts are disabled, the code now actively checks the UDRE (UART Data Register Empty) and calls the interrupt handler to free up room if the bit is set. This can lead to delays in interrupt handlers when the serial buffer is full, but a delay is of course always preferred to a lockup. Closes: arduino#672 References: arduino#1147
This prevents a potential race condition where for example write() would detect that there is room in the tx buffer, an interrupt would trigger where the interrupt handler writes to Serial filling up that room, and write() would then reset the head pointer, effectively throwing away the bytes written by the interrupt handler. References: arduino#1147
Before, the interrupt was disabled when it was triggered and it turned out there was no data to send. However, the interrupt can be disabled already when the last byte is written to the UART, since write() will always re-enable the interrupt when it adds new data to the buffer. Closes: arduino#1008
This allows users to create subclasses. Closes: arduino#947
There are two begin methods, one which accepts just a baud rate and uses the default bit settings and one which accepts both a baudrate and a bit config. Previously, both of these contained a complete implementation, but now the former just calls the latter, explicitely passing the default 8N1 configuration. Technically, this causes a small change: Before the UCSRC register was untouched when calling begin(baud), now it is explicitely initialized with 8N1. However, since this is the default configuration for at least the Uno and the Mega (didn't check any others), probably for all avrs, this shouldn't effectively change anything. Given that the Arduino documentation also documents this as the default when none is passed, explicitly setting it is probably a good idea in any case.
This simplifies the baud rate calculation, removing the need for a goto and shortening the code a bit. Other than that, this code should not use any different settings than before. Code was suggested by Rob Tillaart on github. Closes: arduino#1262
This introduces two new methods, Serial.setBlocking() and Serial.getBlocking(), which can be used to enable or disable blocking. References: arduino#672 --- TODO: This commit is probably not ready to merge, I'm not sure what the proper API for this feature would be (the current enum approach requires qualifying the constants with HardwareSerial:: for example). I'm also not sure where and how to add the new function to a keywords.txt file.
|
Impressive. Thanks for your hard work, and also for the time taken to document everything in detail. I must admit that I'm tempted to merge it all at once, but some commit in this pull request requires careful review. I'll get back to this issue in the coming days, just want to give you an answer. I'll probably start merging the simplest commits first into the ide-1.5.x branch. C |
|
@cmaglie, Thanks for you reply! If you add comments to the commits where you think they need to be changed, I'll be happy to update the pullreq with any needed changes. |
|
@cmaglie Did you ever get around to having a closer look at my commits? |
|
WARNING about the popular atomic fixes cli(). It's BAD programming! |
|
See my comment on #1334: here we also cannot simply disable specific interrupts (at least not in all cases, perhaps some can). In particular, any interrupt handler could decide to write to the Serial port. So if a write is already in progress, things get messed up. Again, I could imagine making the atomicity a compile-time option which can be disabled for time-critical applications (that accept they can't use the Serial port from interrupt handlers). |
|
I think spi and serial write has to be atomic, they are fast and timer
|
|
I have not tested any of these patches, but do these also solve #1425 too? |
|
Nope, though I had half a plan of taking your commit and integrating it into my pull request as well (for easier review / merge by the devs). Didn't get around to this yet, though.. |
|
@cmaglie I saw you picked three of my commits from this pullrequest into your own repository. What's you plan going forward? Review and pick up the rest if they seem ok? I had a plan to revisit this pullreq again, to address some comments. In particular, I still wanted to:
Furthermore, as said before I don't think the last commit in this request, regarding blocking behaviour, is ready to merge. I don't think I've received really useful feedback about it, so it might be better to just drop it for now. |
|
for now, I'd like to merge only the three commits I've already taken, so:
If you want to rework your patch, probably you would consider these three commits as merged, and rebase on them? C |
|
Please include the disable of unused ports via define too.
|
|
Tried using it, and I get: |
|
I only find it in one place.... ./hardware/tools/avr/lib/avr/include/compat/ina90.h:#define _NOP() do { asm volatile ("nop"); } while (0) |
|
Adding |
|
Not sure why yet, but I still get stack corruption when there is data to be sent. Could be my code, I'm not sure yet. Only seems to happen if I cli() at the wrong time. If I flush() immediately, nothing bad happens. |
|
@xxxajk which version of HardwareSerial are you using? line 406 of my HardwareSerial.cpp shows: I see no NOPs at all |
|
https://github.com/matthijskooijman/Arduino/compare/hardware-serial#L0R406 On Fri, Aug 2, 2013 at 4:31 AM, Cristian Maglie notifications@github.comwrote:
Visit my github for awesome Arduino code @ https://github.com/xxxajk |
|
I remember someone reporting this before, but I couldn't find that report anymore. The problem here is I've been testing with a newer version of avr-gcc, since I'm on Debian and not using the avr-gcc version shipped with the Arduino. I guess that re-defining the NOP macro, guarded with #ifndef NOP is the best way to fix this here? Or are there other parts of the Arduino libraries that also use nop somehow? |
|
Bah, wrong button... |
|
Is the willful ability to disable unused serial instances going to be done? Seriously, saving 510 bytes because you don't use 3 of the four ports is a large amount. |
|
I'm closing this pullrequest. I'll open a new one, against ide-1.5.x instead of master in a minute, with a new and improved set of patches which I think incorporates all of the comments above. |
This patch series applies a number of cleanups and fixes to the HardwareSerial code. The individual commit messages should be self-explanatory (and I've tried to reference existing tickets where possible), but I'll summarize what's in this series:
The last patch in this series, which adds an API to toggle blocking behaviour of HardwareSerial::write is not ready to merge yet, since I'm not entirely sure what the best form for this API is (methods vs an attribute, defines vs an enum, etc.) and I couldn't find any similar examples just now.