Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read and write methods working with a timeout #94

Closed
wants to merge 1 commit into from

Conversation

stepanoslejsek
Copy link

I recently discovered that the already implemented read and write methods do not trigger the set timeout. I went through the source code and implemented additional read and write methods with the number of bytes that need to be read or written.

Using these methods, the timeout was triggered and the exception was handled as it should be.

Read/write specific number of bytes -> now working with timeout
Copy link
Collaborator

@mgkuhn mgkuhn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't just redefine

function write(sp::SerialPort, nb::UInt, b::UInt8)

because the standard Base.write(io::IO, x1, xs...) Julia meaning of that method is to output the binary representation of a UInt value followed by the binary representation of a UInt8 value, and LibSerialPorts just implements that normal IO interface. Your change would cause a very nasty and undocumented API surprise to anyone trying to call e.g.

write(sp, 0x0000000000000000, 0x01)

Likewise, what does your new read method give you that e.g. Base.readbytes! doesn't?

Could you please explain first what problem you are trying to solve, with a concrete test example, because I suspect your PR is the result of a misunderstanding of how LibSerialPort extends the existing IO methods of Base, rather than tries to reimplement that interface from scratch.

@stepanoslejsek
Copy link
Author

I am trying to solve the following problem: I have a communication between 2 devices via serial port. The 'master' sends a frame of 8 bytes and waits for a response of the 'slave' device that is 20 bytes long. However sometimes the response (all the 20 bytes) are sent but the read method (using blocking read) only read 19 and get stuck. Therefore I tried to set the read timeout which is not working.

@mgkuhn
Copy link
Collaborator

mgkuhn commented May 11, 2023

Doesn't read!(::IO, ::Array{UInt8}) already do exactly what you need?

Example:

using LibSerialPort
sp = LibSerialPort.open("/dev/ttyUSB0", 9600)
set_flow_control(sp)
sp_flush(sp, SP_BUF_BOTH)
timeout = 2
set_read_timeout(sp, timeout)
request = UInt8[1,2,3,4,5,6,7,8]
reply = Vector{UInt8}(undef, 20)
write(sp, request)
try
  read!(sp, reply)
catch e
    if isa(e, LibSerialPort.Timeout)
        println("Timed out after $timeout seconds")
    else
        rethrow()
    end
end
clear_read_timeout(sp)
print(reply)

However, I just realized that the docstring of read! doesn't actually describe how to use it.

If the above example solves your problem, I think improving the docstring in Base is the solution, rather than this PR.

@mgkuhn
Copy link
Collaborator

mgkuhn commented May 11, 2023

Your read function also causes a heap overflow: you allocate memory for one byte, and then write nb bytes into that memory. That looks totally unsafe.

@mgkuhn
Copy link
Collaborator

mgkuhn commented May 11, 2023

PR for improved docstring of read! in Base: JuliaLang/julia#49769

@stepanoslejsek
Copy link
Author

I am gonna try this, thank you, I will let know if it works or not.

@stepanoslejsek
Copy link
Author

clear_read_timeout(sp)

This exactly worked, thank you for brief explanation.

@mgkuhn
Copy link
Collaborator

mgkuhn commented May 19, 2023

Thanks for reporting back. I'll close this PR here, as the main improvement needed appears to be the docstring of read! in Base.

@mgkuhn mgkuhn closed this May 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants