Add missing timeout implementation from #61 and fixed #72 (missing flow-control setting in tests)

[mgkuhn]

No description provided.

[samuelpowell]

(P.S. Nice to see the file renaming, they have always grated slightly!)

[mgkuhn]

@samuelpowell commented on commit 189cf36:

Module names should be capitalised. Can I suggest Lib rather than Wrap as it more closely follows typical conventions for library wrappers amongst packages, and makes it a bit more clear what it is.

Sure. I have now renamed in the replacement commit 61d1a59 the new module LibSerialPort.wrap into LibSerialPort.Lib, as you suggested. (The file containing that module still remains wrap.jl.)

By the way, is there anywhere a “Style Guide for Julia C wrapper module authors” that collects such conventions?

[samuelpowell]

This looks really nice, thanks for all your work.

I don't know of any style guide on this topic, and things have changed quite a lot with the advent of JLL packages and autogeneration through Clang.jl. A common pattern for this type of package used to be, e.g., to have the package and Julia API module called SerialPort with submodule LibSerialPort, such that the distinction between the Julia side and the raw C bindings was very clear, and there was notionally some acceptance that the submodule with the name of the C library was a direct (and potentially unsafe) mapping. That was never the case in this repo, not least because of the existence of SerialPorts.jl.

Thus I think this boils down to style. In my view maintaining the Lib in the submodule reinforces the idea that this is clearly low level library access, whereas Wrap or Wrapper leaves some ambiguity with regard to how high- or low-level (or indeed, how safe) the functions in the submodule are.

Would you let us know when you are happy with this PR, I'll try and do some local testing, hopefully others too?

[IanButterworth]

Would you let us know when you are happy with this PR

@mgkuhn I had assumed this wasn't ready given this and no reply from you. Sorry for the mixup

I'll give this a test today. Hopefully we can wrap it up and release promptly

[mgkuhn]

Sorry, hadn't spotted the last question. Yes, I'm happy for this PR to be merged. Note that this PR is meant to be backwards compatible to previous versions. However, if you accept this PR, I will then follow it up with another non-backwards-compatible tidy-up PR that at least renames the three pass-through methods

# pass through some methods from the low-level interface
sp_output_waiting(sp::SerialPort) = sp_output_waiting(sp.ref)
sp_flush(sp::SerialPort, args...) = sp_flush(sp.ref, args...)
sp_drain(sp::SerialPort)          = sp_drain(sp.ref)

because if we are now cleanly splitting the low-level and high-level API, they should certainly not be sharing functions, as they do at the moment still thanks to the above three lines (which I added in c903d98 when I still thought we might unify rather than cleanly separate the low-level and high-level API). The sp_* prefix should be reserved entirely for the low-level API. (The difficulty here is that the word "flush" means something quite different in libserialport and Base, see #68.)

[IanButterworth]

This PR has proven to be stable for me in real-world use

However, on both master and this PR the tests fail with:

OS error code 5: Input/output error
Low level API: Error During Test at /home/ian/.julia/dev/LibSerialPort/test/runtests.jl:37
  Test threw exception
  Expression: test_low_level_api(port, baudrate) == nothing
  libserialport returned SP_ERR_FAIL - Host OS reported a failure.

OS error code 5: Input/output error
High level API: Error During Test at /home/ian/.julia/dev/LibSerialPort/test/runtests.jl:43
  Test threw exception
  Expression: test_high_level_api(port, baudrate) == nothing
  libserialport returned SP_ERR_FAIL - Host OS reported a failure.

Have others found tests to work locally? I have a few serial devices embedded in my system, so cannot try disconnecting them

[IanButterworth]

Would anybody object to merging and releasing 0.5? If not I will do so at 6pm ET today

[mgkuhn]

Happy for merge. Also happy to look at your test failures (next weekend?) if you open a new issue about it, including a description of what serial devices you have in your test system.

Note that the tests do require connection of a loop-back adapter (i.e. connecting RX and TX with each other), and the tests do not currently make it very clear if there is no suitable test setup present. Initialization of the tests and documentation of the test setup can certainly be improved (all entirely separate issues from this PR).

[mgkuhn]

Other question: how does one join JuliaIO? How does this group/organization work? Is there a mailing list, slack/IRC/etc. channel, Discourse group, web page, video conference, etc. where the JuliaIO crowd hangs out?

I'd be quite interested in helping to drive a number of Julia IO packages forward (audio and serial-port IO are current priorities for me, because both are used in student projects that I supervise, but I'm also interested in other file formats and forms of hardware IO). But the relatively slow merge cycles can be a bit discouraging.

[IanButterworth]

I think responsibility with this repo was just falling through the cracks, largely my fault. Sorry about that.

I'll look into how to add people to JuliaIO. There isn't really a hangout spot I know of, beyond the general julia slack. Perhaps we should set up a dedicated slack channel!

[mgkuhn]

I would have preferred a “Rebase and merge”, which would have simply fast-forwarded my commits, as this PR ended up doing quite a number of different things that would have been worth being preserved separately in the git history on master. I'm keen to put more work into this package, but first need to understand better how contributions should best be made in JuliaIO. If merges happen very rarely (months apart), there is an incentive to put a lot of different things on a single "fixes" branch into a single PR, otherwise I won't myself have a single branch that has all my fixes to use. But then you shouldn't squash them all together into a single commit on master. If I instead open lots of different PRs, one for each issue, that works only if these PRs get reviewed and merged very quickly, within a day or two, as otherwise I will have to juggle with too many branches at my end, and lose track on in which order they remain applicable (especially with bigger changes that involve e.g. renaming or moving content between files).

[IanButterworth]

Please feel free to request specific types of merges in the future. Conceptually I prefer smaller PRs that address specific issues that can be squashed, so my default is to assume that that is what people want/expect.

And moving forward we can work to be more nimble with changes, keeping PRs smaller, and merge and release quicker.