make serial.available, peek, read atomic

[robert-rozee]

when SERIAL_RX_BUFFER_SIZE is defined as greater than 256, the current HardwareSerial.cpp code almost works correctly. but not quite. the function serial.available may return an incorrect number of characters waiting (though when treated purely as a boolean it seems not to fault).serial.peek has a similar issue detecting if a character is waiting, while serial.read may also leave _rx_buffer_tail corrupt to an interrupt occurring mid-update (this corruption may cause the ISR to be mistaken about how much free space is available in the Rx buffer).

the proposed changes to these three functions add cli/sti pairings around the critical pieces of code. i have tested and verified the change made to serial.available as correcting the problem, while the change to serial.peek follows the exact same pattern. the changes to serial.read have been confirmed as not breaking the function, but i have not been in a position to test the failure of the original (non-atomic) version to make comparisons.

multiple testing was conducted at 115,200 baud and 500,000 baud using data streams of 1,000,000 characters sent over a 2 minute interval. any error causing loss of character would have resulted in a catastrophic (ie, very obvious) failure.

cheers,
rob :-)

[matthijskooijman]

Thanks for picking this up, this was a known flaw in the code (and I believe no PR for it existed yet). I do have some remarks:

• Good that you use a temporary variable to store the loaded values, this ensures the interrupts are disabled as short as possible. It might be slightly better to even store both values separately and do the subtraction after re-enabling interrupts too.
• The temporary variable you use know always uses the "int" type, but I believe that should be tx_buffer_index_t or rx_buffer_index_t for consistency.
• I believe the read access of _rx_buffer_tail in read() and peek() should also be made atomic, since that value can now still change if the RX ISR triggers between the two parts of the read.
• The write() function does not have any atomicity added at all?
• Disabling interrupts now happens automatically, even when the indices are just a single byte (which is the normal case), causing unneeded overhead. This should be made conditional based on the buffer size). availableForWrite() already does this.
• The code now touches SREG directly, but I'd rather see it used ATOMIC_BLOCK (see http://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html).
• In addition to atomicity issues caused by indices being 2-bytes for values normally only written by the ISRs, some issues can also occur when accessing variables that are normally only written by the loop, if e.g. write() or read() is called from inside an ISR. This requires some additional atomicity.

For the last three points, see this comment for more info and some proposed code. The last point might be a bit tricky to understand, feel free to ask more questions if my point isn't entirely clear :-) (also, I think fixing that one might be more tricky than just adding an ATOMIC_BLOCK due to the structure of write())

[matthijskooijman]

One more thing: You should probably remove this comment from the code in your commit as well.

[robert-rozee]

1. HmT may be negative, hence why i left it as an integer. if using separate variables (local_head and local_tail for example) the type could then be made rx_buffer_index_t without any problems. would it be preferable to do this?
2. i deliberately didn't make the compile conditional on buffer size in order to keep the changes - for the moment - more readable. is it worth making this check to save just a few bytes of code? is there a policy on this?
3. reading _rx_buffer_tail does not need to be made atomic, as (i believe) it is never written to by the ISR. writing _rx_buffer_tail does need to be atomic, as the ISR may read it to determine if the buffer is full. i am happy to make ALL accesses to the head and tail variables atomic if desired, though it will consume a few extra bytes of code.
4. as yet i've made no attempt to fix the corresponding issues with serial writing, only receiving. i would prefer to leave this for a later occasion.
5. if some is reckless enough to call write() or read() from inside an ISR, there is little that can be done to protect them. no matter how much care is made to make the code more bulletproof, someone will manage to find a way to break it.

please do advise your suggestions and i shall amend the code accordingly.

cheers,
rob :-)

[robert-rozee]

have updated the three changed functions to match the same style as used in serial.availableForWrite so now is conditional on buffer being > 256 bytes. also brought tail within the atomicity.

again, serial.available and serial.read confirmed as working. serial.peek assumed to be ok as is almost identical in structure to serial.available.

cheers,
rob :-)

[matthijskooijman]

HmT may be negative, hence why i left it as an integer. if using separate variables (local_head and local_tail for example) the type could then be made rx_buffer_index_t without any problems. would it be preferable to do this?

Yeah, separate variables would be preferable to keep interrupts disabled as short as possible. Also, then we won't have to worry about the typing :-)

i deliberately didn't make the compile conditional on buffer size in order to keep the changes - for the moment - more readable. is it worth making this check to save just a few bytes of code? is there a policy on this?

The gain is not really in saving a few bytes of code, it's about not disabling interrupts when not needed. Disabling interrrupts can increase the time it takes for interrupts to run, so it should not happen if not strictly needed.

reading _rx_buffer_tail does not need to be made atomic, as (i believe) it is never written to by the ISR. writing _rx_buffer_tail does need to be atomic, as the ISR may read it to determine if the buffer is full.

Ah, good point. However, if we want to also make reading and writing from an ISR safe, this assumption no longer holds, and all accesses need to be made atomic.

if some is reckless enough to call write() or read() from inside an ISR, there is little that can be done to protect them. no matter how much care is made to make the code more bulletproof, someone will manage to find a way to break it.

I don't actually think this is true, and it would be elegant if this would just work. Especially reading from serial in an ISR should be doable. writing is a bit more tricky, since write can potentially block, of course.

have updated the three changed functions to match the same style as used in serial.availableForWrite so now is conditional on buffer being > 256 bytes. also brought tail within the atomicity.

In the interest of readability, could you see if you can use the TXBUF_ATOMIC_BLOCK macro proposed in this comment (and similarly define an RXBUF_ATOMIC_BLOCK)? I'd like to see availableForWrite() modified to use this as well.

Overall, I think that:

• All access of index variables should be wrapped in TXBUF_ATOMIC_BLOCK or RXBUF_ATOMIC_BLOCK, so it happens atomically when the respective index variable is > 1 byte.
• All read-modify-write cycles of index variables should be wrapped in ATOMIC_BLOCK, so that they happen atomically even when the index is just 1 byte.
• write() will need more extensive changes (because it now does a read before the loop and the write after the loop, so disabling interrupts all that time is too long). For now, just leave write() unchanged, I guess.

If you locally edit your commits, you can use a force push to overwrite the commit in this pull-request, without having to create a new PR.

Thanks!

[robert-rozee]

i've asked about ATOMIC_BLOCK on the developers forum, and received quite a bit of feedback. it seems like a rather complicated macro that not everyone understands the workings of, and at this point i'd not feel comfortable using it myself - i would far rather leave it for someone else to add in a later revision to the code.

similarly, as you say, write() is a much more complicated issue and safest left well alone at this stage. my personal interest is only in the Rx buffer anyway.

so, in terms of functionality (ignoring style), is there anything else that needs changing?

cheers,
rob :-)

[matthijskooijman]

've asked about ATOMIC_BLOCK on the developers forum, and received quite a bit of feedback. it seems like a rather complicated macro that not everyone understands the workings of, and at this point i'd not feel comfortable using it myself - i would far rather leave it for someone else to add in a later revision to the code.

Good thing to aks! I would say most responses are positive about ATOMIC_BLOCK are positive though, for being more readable. As for understanding it - the block disabled interrupts when entered and enables them when the block is exited through any means (except maybe longjmp) which seems simple enough? No need to actually understand how it's implemented to use it, right?

so, in terms of functionality (ignoring style), is there anything else that needs changing?

Well, it seems your changes sufficiently fix atomicity issues caused by having two-byte index variables.

However, you're doing more than that, partially (but not completely) fixing the "read() called in ISR" atomicity issue as well. I guess this should either happen completely, or not at all.

Some new things I realized:

• When you just want to fix the two-byte issue, only the load of the _rx_buffer_head needs to be atomic, since the tail is assumed not to change inside the ISR.
• When you want to fix the read-in-ISR issue, all read-modify-write cycles should be fixed. However, I just realized that essentially the entire read() function is one big read-modify-write cycle, so it should be pretty much entirely made atomic.
• When you want to fix the read-in-ISR issue, you should assume that the head and the tail can change at any time. This means that they should be loaded atomically together, so the atomicity in e.g. peek() should be made unconditional (since even when an index is one byte, together they are still two bytes and need to be protected).

Not sure whether supporting read/write in an ISR is worth the trouble. It would be nice to have things just work, though with the current code it will at least usually work (and, AFAICS cause random reads and writes in the worst case, never locking up). @cmaglie, any idea about this?

[robert-rozee]

my intention in modifying Serial.read was purely with regard to the issues with two-byte index variables; while i've not observed it, the current Serial.read could potentially misbehave with the Rx buffer size >256 bytes when called from loop() - ie, not from within an ISR. any other 'improvements' are unintended side effects.

the whole idea of calling from within an ISR adds a layer of complexity that i'd rather leave to someone else to address. if it were my choice, i'd just be bailing (with a -1 error) from the routines if they were called while the interrupt enable flag is cleared. this would help discourage users from trying to do something that i consider bad programming practice. but this is just my opinion.

cheers,
rob :-)

[NicoHood]

You should probably also add an atomic check around the first statement inside the if (16 bit only):
https://github.com/arduino/Arduino/pull/3865/files#diff-c69f730a10ad2a6aa038511536b0b353L220

This fix would suit better in here, instead of my PR.

[robert-rozee]

what line number do you mean?

[NicoHood]

The linked one. 220.
if (_tx_buffer_head == _tx_buffer_tail

[robert-rozee]

aha, highlighted in brown, yes? that line is in Serial.write

my proposed changes are strictly limited to the serial received routines only. i'm not trying to produce a perfect solution that fixes all problems in the serial library, but just to fix some specific issues that i have encountered and produce code that is an incremental improvement on what is already there.

cheers,
rob :-)

[NicoHood]

We should fix this somewhere anyways. I've added this to my PR now.

[facchinm]

Superseded by #6855

Edit: moved here 95bb83e

[matthijskooijman]

I don't actually think that #6855 includes these changes, see #6855 (comment) and #6855 (comment)

But note that these fixes should probably use the new TX_BUFFER_ATOMIC_BLOCK macro, as introduced by c64363d. @johnholman already offered to create a PR for these changes (but probably after #6855 is merged).

@johnholman, if you're going to create that PR, this one might be a good one to check for where the changes need to be applied.

@facchinm, I think this PR can still remain closed, though it might be good to have an issue open for this (not sure if there is one already).

[facchinm]

Hi @matthijskooijman ,
you are totally right; in fact I wanted to reopen it but the original repo/branch is gone so we can't do it. I recreated it here 95bb83e, preparing the PR on core-avr later tomorrow.

[robert-rozee]

after 2 years of inaction, is it entirely surprising that the original author of the edits had thrown his hands in the air and walked away? he probably decided it was less frustrating to just fix bugs in his own (local) copy of the arduino code rather than sharing with the community. not wishing to criticise, but it does seem that the human brain is wired to reject a partial solution to a problem, preferring to live with no solution rather than adopt a solution that is 'merely' an incremental improvement.

just dug around in my local archives, attached is a .txt file containing what were most probably the edits.

cheers,
rob :-)

atomic hardware serial.txt

[johnholman]

Hi @matthijskooijman @facchinm

Yes #6855 does not address the large buffer atomicity issues, although it does tidy up the one place in available_for_write() where there was already a guard by using the TX_BUFFER_ATOMIC_BLOCK macro.

I'm just looking at @robert-rozee's code and think the macro would help avoid a lot of extra code clutter that is otherwise needed. Should I go ahead and create a new PR along those lines or @facchinm are you planning to do it? Also should it now be against ArduinoCore-avr rather than the Arduino repo?

[matthijskooijman]

@robert-rozee, I actually think the changes can still be seen in this PR (under the "Commits" tab), it's just that the PR cannot be re-opened. In any case, even though this has been stale for so long, it's good you created the PR back then, since now we'll hopefully see some movement on this issue :-)

[johnholman]

I've created a new PR #5 against ArduinoCore-avr repository to address atomicity problems with
large receive buffers - hopefully that was the right place to make it.

Dealing with large write buffers might be more tricky and for a later commit.
@matthijskooijman @facchinm feedback on work so far would be welcome.

The new pull request is similiar to this one but with these differences:

1. a new RX_BUFFER_ATOMIC macro helps keep the code readable

2. guards are only placed on pointer reads when necessary, which may
   improve performance for large read buffers slightly

3. an additional guard is placed on the final read buffer clear in
   HardwareSerial::end()