Bug in UARTImpl's receive triggering

[alexey karaksin]

Hi Curran,

On 07.09.2014 5:13, Curran D. Muhlberger wrote:
> Hello DIO developers,
>
> There appears to be a bug (two bugs, in fact) in `UARTImpl`'s use of
> `receiveTriggerLevel`.  This causes the behavior of `read` to deviate from
> the API specifications.
>
> To jump right to the code, I think the bugs were introduced in
> changeset be924b3ef380,, and both occur on line 265 in
> "src/share/classes/com/oracle/dio/uart/impl/UARTImpl.java".  My proposed
> minimal fix is:
>
>      -                if(!buffer.hasRemaining() || ( receiveTriggerLevel !=0
> && buffer.position() > receiveTriggerLevel) || (-1 == bytesProcessed)){
>      +                if(!buffer.hasRemaining() || ( receiveTriggerLevel !=0
> && (buffer.position() - readBuffersPositions[readBufferIdx]) >=
> receiveTriggerLevel) || (-1 == bytesProcessed)){
>
> Now for the explanation.  The two bugs are an off-by-one error and an
> invavlid assumption about the buffer's initial position.  Since client code
> is not required to clear the buffer it passes to `read`, its initial
> position may be non-zero.  `UARTImpl` already stores the initial position
> in `readBuffersPositions[]`, but the difference in positions (which equals
> the number of bytes read into the buffer) is not currently being computed
> in order to compare with the trigger level.  Instead, the absolute
> position, which is arbitrary, is used.  Additionally, the comparison should
> be ">=", not just ">" (if 1 byte has been read, then the position
> difference will be 1, and if `receiveTriggerLevel` is 1, then the body of
> the `if` statement should be executed).
>
> Some examples to illustrate the bugs:
>   1. The client calls `setReceiveTriggerLevel(1)`, then calls `read` on an
> empty buffer.  Subsequently, a producer transmits 1 byte to the UART's
> receive line.
>     * Expected behavior: The call to `read` returns, putting 1 byte in the
> buffer
>     * Actual behavior: The call to `read` continues to block.  If the
> producer transmits a second byte, then the call will return, putting 2
> bytes in the buffer
>   2. The client calls `setReceiveTriggerLevel(4)`, then calls `read` on a
> buffer with `position() == 4`.  Subsequently, a producer transmits 1 byte
> to the UART's receive line.
>     * Expected behavior: The `read` call blocks until three more bytes are
> sent (or possibly a timeout occurs)
>     * Actual behavior: The `read` always returns after reading just 1 byte
>
> The above patch yields the expected behavior in both cases.
the fix looks good. the saved buffer`s position has been forgotten.
> On a related note, the API does not mention any interaction between
> `startReading` and receive trigger levels and seems to imply that a round
> listener will only be notified when the buffer is completely full.  Yet the
> current `UARTImpl` will call the listener with partially-full buffers in
> accordance with the trigger level.  I actually think this is desirable
> behavior (enabling asynchronous reads of finite data streams of unknown
> length), but I can't rely on it under the current version of the API.
async read operation is connected to trigger level because of words 
"input buffer" in the trigger level description "specified number of 
bytes have been received in the input buffer", the only mandate input 
buffer is during the read operations.

Think that it make sense in case of if the underlying implementation 
supports javacall buffering, send |UARTEvent.INPUT_DATA_AVAILABLE| 
<cid:part1.05060303.02030804 at oracle.com>
only if the native buffer has more or equal bytes than defined by 
receiveTriggerLevel.

> Thanks in advance for taking a look at these UART `read` issues.
>
>   - Curran