Fixes for issues #50, #53, #54

[mgkuhn]

See issues and individual commit comments for details.

[samuelpowell]

Thanks @mgkuhn, if you’re able to split the final three commits out (docs, type conversion, and loc calls) into a separate PR we can get those merged quickly as they’re not controversial IMHO. The other fixes will need someone to properly test so may take longer.

[mgkuhn]

I'd rather not split them: the final three are just cosmetics but separating them out is unproductive work. I'm really interested in fixing the current serious brokenness in the high-level interface that the first commit fixes. I did a lot of tests on Linux (including with a logic analyser on the loop-back wire), in an application with binary communication and tight timing requirements, and it quickly became clear that the current high-level interface had received little testing previously as several of its methods either just cause exceptions (issue #50 is just one example), or were implemented in a way that quietly loses bytes, that slows down communication, or that contradicts Julia's IO API conventions.

Here is some more detail about what was broken in the old write() methods that I removed or replaced:

• They called sp_nonblocking_write(sp.ref, data) and then ignored the return value that confirms how many bytes the operating-system was able to take on without blocking, i.e. they would quietly drop any bytes in a longer string sent that the OS wasn't able to pick up immediately. This was already pointed out as rendering the high-level API unusable on Windows by the author of PR Update write methods for windows compatibility #12, but actually affects all platforms (Linux just has bigger buffers so the bug manifests itself less commonly).
• They called sp_drain(sp.ref) after each write, which blocks and adds a substantial pause (on Linux in the region of a millisecond) on the wire after each call, severely limiting the data rate that can be achieved via the old API. I can only speculate that this was discovered by someone as a workaround for the problems caused by incorrect use of the non-blocking functions, as frequent draining will keep the buffers less full, and therefore reduce the chance of them overflowing. If so, it is a very ugly hack that will still fail for longer strings.
• The previous high-level API implementation failed to implement unsafe_write(sp::SerialPort, p::Ptr{UInt8}, nb::UInt), which is the interface method required by many of the existing IO methods in Base to get bytes to the device.
• Instead, it tried to implement a small fraction of the existing IO write methods, but in an incompatible way. For example write(sp::SerialPort, i::Integer) was redefined to write a decimal ASCII string representation of i to the serial port, whereas the documented purpose of write() in Base is to write a canonical binary representation. For writing string representations, there is print() instead, which now works as documented in Base.
• The previous implementation of sp_blocking_write(port::Port, buffer::Ptr{UInt8}, timeout_ms::Integer) was non-sensical, as it didn't receive the number of bytes to be transmitted, and always transmitted sizeof(Ptr{UInt8}) bytes (i.e., 8 bytes on a 64-bit machine). Looked like a basic programming error and strongly suggested lack of testing. Yes, my patch (introducing a length argument n) is not fully backwards compatible here, but since the previous implementation didn't work in any useful way, I didn't feel a strong urge to remain strictly compatible with it.
• At many places, sp_flush(), which potentially destroys queued data by emptying buffers, was called where really sp_drain() should have been called instead, if at all. There was obvious confusion between what Base.flush() and sp_flush() do (one waits until the OS has received all bytes buffered by the library, the other discards any bytes that have not yet reached the wire, quite a difference!), as evidenced by this comment in close():

    # Flush first, as is done in other close() methods in Base
    sp_flush(sp.ref, SP_BUF_BOTH)

In summary: the high-level write API implementation had some very severe problems, and this patch goes a long way towards eliminating those. I suspect I have already tested it much more thoroughly than the methods it replaces appear to have been. I hope the improvements are so significant and obvious that the best route is to test them in master.

[andrewadare]

@mgkuhn thanks for this merge request. Apologies; life circumstances have slowed my responsiveness to your recently filed issues. I will have a closer look and do some testing.

[andrewadare]

I pulled yourfixes branch and verified that test-high-level-api.jl and test-low-level-api.jl are working on my setup (macOS with an Arduino running serial_example.ino).

The examples/console.jl demo also runs as expected.

The "Reading with timeouts" test fails in runtests.jl, but that, along with #52, are topics covered by other issues.

[andrewadare]

I think that this PR should be merged, then subsequent work is needed for the reading methods.