Improvements to interface, testing, and examples

[andrewadare]

This PR is intended to follow up on work done by @mgkuhn in #56. The scope has grown from a focus on read methods to a package-wide overhaul.

• The read methods defined in high-level-api.jl were mostly removed, allowing io.jl in the Julia Base module to do its job. See avoid duplicating read functions from base/io.jl #58.
• A read_timeout_ms field was added to the SerialPort type.
• As done for write in Fixes for issues #50, #53, #54 #56, the read method now calls into sp_blocking_read instead of the nonblocking version.
• The old read behavior is still available as the newly-renamed nonblocking_read function. This is likely a breaking change.
• The "tests" were more like example scripts. Now they actually use @test.
• The read methods introduced in Add readline and readuntil with timeout #45 were removed. They did not work and the equivalent functionality is now provided in other ways.
• A minimal working example script was added to examples/, and is now used as the README example (see readline() example in README.md does not work #52).

I still haven't fully worked out the timeout logic for the blocking read methods. @mgkuhn provided valuable suggestions in #58, which I have partially implemented. All tests and examples now work on my computer (macOS Catalina) with an Arduino Uno running serial_example.ino.

[samuelpowell]

I know @ianshmean had some particular use cases that were causing him trouble, perhaps it's worthwhile seeing if he has time to comment?

[mgkuhn]

Ah, I hadn't realized that a conversion from the enum type SPResult returned by handle_error to Int is needed. Can we not do this in the first line of function handle_error, by changing that to ret >= SP_OK && return Int(ret)? (Or have multiple versions of handle_error, one of which returns an Int and one of which returns nothing?) It would be nice to have most wrapper functions consist of just a single line, and move all the return-value processing functionality required into (a version of) handle_error(). Then commit fb64277 would become unnecessary: whatever the handle_error() call in the last line of the wrapper function returns also becomes the return value of the wrapper function.

[mgkuhn]

Overall the PR looks like a big step in the right direction, thanks!

Note that I just use a USB serial adapter with its RX and TX wires directly connected with each other, so just a hardware loopback wire and no Arduino board involved. I suspect that is why the tests still fail for me:

Read timeout (ms, forever if 0): 1000

Read all data currently waiting in the serial input buffer...
Wrote test message 1
test message 1
Blocking read/write: Test Failed at /home/mgk25/.julia/dev/LibSerialPort/test/test-high-level-api.jl:21
  Expression: startswith(line, "Received test message")
   Evaluated: startswith("test message 1", "Received test message")
Stacktrace:
 [1] test_blocking_serial_loopback(::SerialPort) at /home/mgk25/.julia/dev/LibSerialPort/test/test-high-level-api.jl:21
 [2] macro expansion at /home/mgk25/.julia/dev/LibSerialPort/test/test-high-level-api.jl:71 [inlined]
 [3] macro expansion at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.4/Test/src/Test.jl:1113 [inlined]
 [4] test_high_level_api(::String, ::Vararg{String,N} where N) at /home/mgk25/.julia/dev/LibSerialPort/test/test-high-level-api.jl:71

It would be nice if the tests could also be made to work with such a very basic “loopback” wire. At least, runtests.jl should print an explanation of what hardware configuration is required for the tests to pass.

Idea: In the absence of a port parameter, runtests.jlcould also try to send a test byte to all serial ports found, and then use the first one that echoed that byte back for the remaining tests, i.e. auto-detect the loopback needed for testing.

[andrewadare]

It would be nice if the tests could also be made to work with such a very basic “loopback” wire.

Good idea, this is addressed in 25b2405 and c855eb9.

[mgkuhn]

Ok, thanks, the tests now work also for a simple loop-back wire, except that I still need to uncomment the switches to 6-bit frames as mentioned in #55.

[andrewadare]

I think I have addressed all review comments received so far. I still haven't implemented anything sophisticated with read timeouts, but I'm not convinced that we need to do anything more than set SerialPort.read_timeout_ms. For blocking read calls, the default behavior is to wait forever instead of missing bytes.

All the examples and tests work for me with three different hardware configurations (which are listed atop runtests.jl).

This is unfortunately a fairly big change set. It would not be simple to split up. In the end, some of the biggest problems were in the tests more than the library code under test.

I have not done any Windows testing, so it would be great if anyone could easily do that.

[wsphillips]

Hello, I was just about to open a PR for an extra readuntil method when I saw the changes here.

* The read methods introduced in #45 were removed. They did not work and the equivalent functionality is now provided in other ways.

What's the proposed alternative for getting the same functionality after the changes made in this PR? Will the user need to roll their own alternative to the Base extension methods of readuntil() contained in LibSerialPort.jl? Will the proposed changes allow fall-back methods in Base to fill in for it's absence here?

[andrewadare]

Since LibSerialPort.SerialPort is an IO subtype, the readuntil method is provided by Julia's base module (see here). You should just be able to call e.g. readuntil(sp, '\n') to get a line of characters (note that it will block forever by default after this PR). These higher-level methods like readline and readuntil just delegate to byte-level read methods, which are provided by this package, and that's all we've found to be necessary.

[touchft]

#66 (comment)

[touchft]

I miss the function readuntil() in master branch. Can we have it in branch: ama/fix-read-methods ?

[mgkuhn]

The solution to users not understanding that standard methods for the IO type also work for the LibSerialPort.SerialPort subtype is that LibSerialPort should have a Documenter-produced manual, and then docs/src/index.md should list all the base methods that can usefully be used with LibSerialPort.SerialPort, which would cause readers of the documentation to also discover the availability of readuntil.

[mgkuhn]

In README.md:

• please change name = "/dev/ttyUSB1" to portname = "/dev/ttyUSB1" (as used by the following open)
• perhaps review the reference to “support for args in the Pkg REPL”; while there appears to be no REPL support yet, one can now at least pass test arguments via Pkg.test(), as in

  Pkg.test("LibSerialPort", test_args=["/dev/ttyUSB1"])
  

If tests/runtests.jl is called without any arguments, it should

• print the description of the loopback hardware required to perform the test, which is currently just a comment at the start of the file (simply make that a triple-quoted string literal instead of a comment, so you can print it)
• print instructions on how to actually run a loop-back test
• print a list of serial ports found (e.g. in form of a sequence of Pkg.test() commands)
• terminate with success

The loopback tests should only be run by tests/runtests.jl if a serial port name is explicitly provided as an argument:

• this will remove the need to make running on a CI server a special case
• this removes safety concerns about sending test bytes to a serial port without explicit user consent (test bytes sent to serial ports unintentionally can cause real damage, e.g. recall that the maiden flight of the Ariane 5 rocket blew up after an unhandled exception caused a test dump being sent via a serial port to the motors that rotate the nozels!)
• in particular, please remove the potentially dangerous port = "/dev/ttyS0" default from tests/runtests.jl (which for all we know might be connected to highly explosive equipment, such as a big rocket motor ;-)
• optional: one could implement a "--scan" argument option that sends one single probe byte (perhaps NUL?) to each port found, and then looks (with say 250 ms timeout) which ports echoed that byte back, then runs the full test on each port that did and therefore appears to have a loopback in place, for the benefit of people who have a test machine with say five serial port adapters (and nothing explosive connected to them) that they all want to test routinely

[mgkuhn]

(Apologies for the late response, I'm back now!)

When I run the tests on Ubuntu 18.04, I get many (shortened output)

Bytes missing:  UInt8[0x11, 0x13]
┌ Warning: Received data does not match transmitted data:
└ @ Main ~/.julia/dev/LibSerialPort/test/test-low-level-api.jl:59
┌ Warning: Received data does not match transmitted data:
└ @ Main ~/.julia/dev/LibSerialPort/test/test-high-level-api.jl:72
Test Summary:              | Pass  Fail  Total
LibSerialPort              |   68    22     90
  Low level API            |   44    20     64
    Blocking read/write    |   20    10     30
    Nonblocking read/write |    1            1
    Blocking read/write    |   20    10     30
    Nonblocking read/write |    1            1
  High level API           |   24     2     26
    Blocking read/write    |   10           10
    Nonblocking read       |    1            1
    Bytestring roundtrip   |          1      1
    Blocking read/write    |   10           10
    Nonblocking read       |    1            1
    Bytestring roundtrip   |          1      1
ERROR: LoadError: Some tests did not pass: 68 passed, 22 failed, 0 errored, 0 broken.

When doing binary tests that include the bytes 0x11 (Ctrl-Q, XON) and 0x13 (Ctrl-S, XOFF), XON/XOFF flow control obviously must be switched off first.

The documentation string for test_low_level_api() claims “Hardware and software flow control measures are disabled by
default”. That would seem sensible, but seems currently to be a lie: I can't find any call to set_flow_control() to actually disable flow control. I suspect we instead get the operating-system default, which on my machine is

$ stty -F /dev/ttyUSB1 -a
[...] ixon -ixoff [...]

Could you please explicitly switch off flow control, at least in the tests? It might even be a good idea to make flow-control a default-off parameter of open().

[touchft]

Can I make a PR for this branch ama/fix-read-methods ? Based on branch ama/fix-read-methods and master branch, I make a new readuntil() and want to add it to branch ama/fix-read-methods.

[andrewadare]

What is wrong with the existing readuntil method? It works fine in my tests.

[mgkuhn]

@touchft What is wrong with the existing implementation of Base.readuntil, which is already working for SerialPort because SerialPort is a subtype of IO, i.e. inherits all methods from IO. Why do you think the SerialPort type needs a specialization of Base.readuntil and what would it do that Base.readuntil(s::IO, delim::AbstractChar; keep::Bool=false) doesn't already do?

If this is about timeouts: we are going to implement timeouts in read() and unsafe_read() via an exception and a new timeout parameter inside the SerialPort struct, and that new parameter and exception with then just be passed on through Base.readuntil(s::IO,...) (and lots of other standard IO methods) as well.

[touchft]

What is wrong with the existing readuntil method? It works fine in my tests.

Nothing wrong. Three things I consider for the change are:
1.the return from readuntil (String) is not consistent with the return of read (UInt8[...]), I like the later due to binary transport of serial port.
2.lack the control keep while readuntil in io.jl have this.
3.I want to one more keyword to control the maximum number of bytes read.

Maybe I am too greedy ^-^

Code is here:

"""
`readuntil(sp::SerialPort, delim::Vector{UInt8}, timeout_ms::Real; max_read_size::Int = typemax(Int16), keep::Bool = false)`

Read until the specified delimiting byte (e.g. `'\\n'`) is encountered, or until timeout_ms has elapsed, or maximum size of bytes readed, whichever comes first. The delimiter can be a UInt8, AbstractChar, AbstractString, or Vector{UInt8}. Keyword argument keep controls whether the delimiter is included in the result. The text is assumed to be encoded in UTF-8.
"""
function Base.readuntil(sp::SerialPort, delim::Vector{UInt8}, timeout_ms::Real; max_read_size::Int = typemax(Int16), keep::Bool = false)
    read_timeout_ms = sp.read_timeout_ms
    start_time = time_ns()
    read_start_time = start_time
    num_read_bytes = 0
    out = IOBuffer(maxsize=max_read_size)
    find_delim = false
    if length(delim) == 0
        nodelim = true
    else
        nodelim = false
        lastchars = UInt8[0 for i=1:length(delim)]
    end

    while !eof(sp)
        # set read_timeout
        current_time = time_ns()
        if (current_time - read_start_time)/1e6 > read_timeout_ms
            # @info "No data available. Time out!"
            break
        end
        # set total timeout
        if (current_time - start_time)/1e6 > timeout_ms
            # @info "Total time (read) is out."
            break
        end

        # The space in IOBuffer remains for write
        maxbuffer = max_read_size - num_read_bytes
        n = bytesavailable(sp)

        if maxbuffer <= 0
            # @info "Read buffer is full."
            break
        end

        if n > 0
            n = min(maxbuffer, n)
            if nodelim
                c = read(sp,n) # max number of bytes for once read(sp) is: 4095
                write(out, c)
                num_read_bytes += n
            else
                for i in 1:n
                    c = read(sp,1)
                    write(out,c)
                    lastchars = circshift(lastchars,-1)
                    lastchars[end] = c[1] # c isa Vector{UInt8}
                    num_read_bytes += 1
                    if lastchars == delim
                        find_delim=true
                        # println("find delim !")
                        break
                    end
                end
            end
            read_start_time = time_ns()
            find_delim && break
        end
    end
    # @show num_read_bytes
    if find_delim && !keep
        return take!(out)[1:end-size(delim,1)]
    else
        return take!(out)
    end
end

Base.readuntil(sp::SerialPort, delim::Union{AbstractString,AbstractArray{Int,1}}, timeout_ms; max_read_size::Int = typemax(Int16), keep::Bool = false) = Base.readuntil(sp, Vector{UInt8}(delim), timeout_ms; max_read_size = max_read_size, keep=keep)

Base.readuntil(sp::SerialPort, delim::Union{UInt8,AbstractChar}, timeout_ms; max_read_size::Int = typemax(Int16), keep::Bool = false) = Base.readuntil(sp, Vector{UInt8}([delim]), timeout_ms; max_read_size = max_read_size, keep=keep)

Base.readuntil(sp::SerialPort, timeout_ms::Real; max_read_size = typemax(Int16), keep::Bool = false) = Base.readuntil(sp, Vector{UInt8}(), timeout_ms; max_read_size = max_read_size, keep=keep)

[mgkuhn]

1.the return from readuntil (String) is not consistent with the return of read (UInt8[...]), I like the later due to binary transport of serial port.

The existing readuntil(stream::IO, delim; keep::Bool) method from base/io.jl accepts both UInt8 or Vector{UInt8} values for delim, and in both cases will return Vector{UInt8}:

julia> readuntil(stdin, 0x40)
abc@def
3-element Array{UInt8,1}:
 0x61
 0x62
 0x63
julia> readuntil(stdin, UInt8[0x40])
abc@def
3-element Array{UInt8,1}:
 0x61
 0x62
 0x63

So no need to “reinvent the wheel” inside this package's high-level API, which is essentially just the serial-port driver for the IO system of Base.

3.I want to one more keyword to control the maximum number of bytes read.

That sounds like a perfectly reasonable pull request for base/io.jl, as there is nothing specific to this feature request for SerialPort. The same feature request seems perfectly useful for use with lots of other IO drivers than just this one.

The only other feature of your routine, the timeout, we will be doing in a way that works fine with all the many different readuntil methods in Base.

[touchft]

1.the return from readuntil (String) is not consistent with the return of read (UInt8[...]), I like the later due to binary transport of serial port.

The existing readuntil(stream::IO, delim; keep::Bool) method from base/io.jl accepts both UInt8 or Vector{UInt8} values for delim, and in both cases will return Vector{UInt8}:

julia> readuntil(stdin, 0x40)
abc@def
3-element Array{UInt8,1}:
 0x61
 0x62
 0x63
julia> readuntil(stdin, UInt8[0x40])
abc@def
3-element Array{UInt8,1}:
 0x61
 0x62
 0x63

So no need to “reinvent the wheel” inside this package's high-level API, which is essentially just the serial-port driver for the IO system of Base.

3.I want to one more keyword to control the maximum number of bytes read.

That sounds like a perfectly reasonable pull request for base/io.jl, as there is nothing specific to this feature request for SerialPort. The same feature request seems perfectly useful for use with lots of other IO drivers than just this one.

The only other feature of your routine, the timeout, we will be doing in a way that works fine with all the many different readuntil methods in Base.

Great! I think I miss checking the master branch of LibSerialPort.jl (I checked the Julia repo and branch ama/fix-read-method ). I found that one useful blocking read method is missing in the master and ama/fix-read-method branch, i.e, "readavailable(::SerialPort)".

[In branch: ama/fix-read-method]:
The nonblock read method "nonblocking_read(sp::SerialPort)" seems result no difference with methd "read(sp::SerialPort)".

[in branch master]:
I think the method read or write for a serial port should output the data type Array{UInt8,1} or UInt8 as default for consistency, which is also widely used in the branch: ama/fix-read-method.