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

Merge or split high-level and low-level API #68

Open
mgkuhn opened this issue Jul 9, 2020 · 4 comments
Open

Merge or split high-level and low-level API #68

mgkuhn opened this issue Jul 9, 2020 · 4 comments

Comments

@mgkuhn
Copy link
Collaborator

mgkuhn commented Jul 9, 2020

This suggestion would be a big breaking change, but possibly worth it.

I believe LibSerialPort would become far more user friendly if we merged the high-level and low-level APIs. The resulting single API is closer to the current high-level API, in that almost all methods in it have a first argument of type LibSerialPort.SerialPort (or the IO supertype), but it also provides all the advanced facilities currently only found in the low-level API.

  • Users of the high-level interface will often want to have access to many of the methods of the low-level interface (e.g., draining buffers, accessing modem control lines, etc.). But duplicating large parts of the low-level interface in the high-level interface could leads to duplication of code, documentation and tests.

  • It is much more user friendly if there is only one documented function for each purpose, and that function should copy an adapted version of most of the text found in the documentation of the corresponding libserialportfunction (such that LibSerialPort users do not have to read the libserialport C documentation).

  • New users should not have to be confronted with first having to make a choice between using one of two APIs, unless there are extremely good technical reasons for having to make such a choice (which I doubt we have here).

@mgkuhn
Copy link
Collaborator Author

mgkuhn commented Aug 17, 2020

Actually, on second thoughts, I think a much cleaner solution would be to move the high-level and low-level APIs into separate modules, e.g. called LibSerialPort and LibSerialPort.wrap (still distributed within the same package).

Advantages:

  • Separate modules would allow users to import the name spaces of the high-level and low-level APIs separately. At present, using LibSerialPort exports a huge number of sp_* and SP_ objects that the vast majority of users never ever should be using, due to the memory and thread safety risks posed by and very non-Julian/C-ideosynchratic nature of the low-level API. These symbols clutter various help features in IDEs and therefore can confuse/scare-off new users.
  • Separate modules make it clearer that the low-level and high-level APIs occupy separate name spaces, and therefore can follow entirely independent naming conventions. That makes it easier to clean up some existing terminology confusions, such as flush(::IO) really corresponding to sp_drain(::Port) and not sp_flush(::Port, ::SPBuffer)
  • Separate modules would make it easy to typeset the manual pages of the high-level and low-level APIs separately, using the @autodocs feature of Documenter.jl, which selects objects per module.
  • Putting the low-level API into a separate module would even (optionally) allow us to drop all the sp_* and SP_* prefixes from it's symbols. Instead, users could pick their own SP. prefix using Julia's name-space mechanics, without polluting their module's own namespace:
    import LibSerialPort.wrap
    SP = LibSerialPort.wrap
    SP.flush(port, SP.BUF_BOTH)     # formerly sp_flush(port, SP_BUF_BOTH)
    
    That would also make it easier to drop the very C-ideosyncratic SP_ prefixes in enum constants such as SP_BUF_BOTH when passing them through via the high-level interface.

@mgkuhn mgkuhn changed the title Merge high-level and low-level API Merge or split high-level and low-level API Aug 17, 2020
@samuelpowell
Copy link
Collaborator

@mgkuhn I agree with your updated suggestion (apart from the final point), and this is how most Julia C wrappers are constructed. On the final point, in my view it is best to have the C wrapper follow the library naming as closely as possible, and to perform the translation to Julia names in the Julia (higher level) API because:

  • it allows easy and automatic (re-) generation of the binding using Clang.jl, which is very effective and helps with correctness;
  • expert users can use the underlying library according to its documentation without having to guess at names, etc.

mgkuhn added a commit to mgkuhn/LibSerialPort.jl that referenced this issue Aug 17, 2020
The low-level API wrappers now sit in a submodule LibSerialPort.wrap.

The high-level API currently still re-exports a few symbols from that
low-level API module, to preserve backwards compatibility.

This is due to

* my previously started attempt to merge the low and high-level
  APIs for the three functions sp_flush, sp_drain, and sp_output_waiting

* the fact that several high-level APIs currently still refer to
  enum types and constants defined by the low-level API

We may want to phase out either or both in future, for a cleaner
separation, and a more Julian high-level API.
@mgkuhn
Copy link
Collaborator Author

mgkuhn commented Aug 17, 2020

I have now implemented this split, i.e. moved the low-level wrappers into a separate module LibSerialPort.wrap, in commit 189cf36, which I have appended to the rebased PR #74. (I suspect the reported Travis CI build faults on Linux Arm64 #75 are not my fault.)

If this gets merged, I'll then also start to tidy up the situation around sp_flush, sp_drain, etc. for a cleaner split.

mgkuhn added a commit to mgkuhn/LibSerialPort.jl that referenced this issue Aug 17, 2020
The low-level API wrappers now sit in a submodule LibSerialPort.Lib.

The high-level API currently still re-exports a few symbols from that
low-level API module, to preserve backwards compatibility.

This is due to

* my previously started attempt to merge the low and high-level
  APIs for the three functions sp_flush, sp_drain, and sp_output_waiting

* the fact that several high-level APIs currently still refer to
  enum types and constants defined by the low-level API

We may want to phase out either or both in future, for a cleaner
separation, and a more Julian high-level API.
IanButterworth pushed a commit that referenced this issue Nov 11, 2020
…ow-control setting in tests) (#74)

* fix test_low_level_api() by actually disabling flow control

This test routine claimed in a comment to disable flow control. It
actually requires the serial port to be transparent for all 256 byte
values, which in turn requires deactivation of software flow control,
because otherwise the byte-transparency tests fail for bytes 0x11
(Ctrl-Q, XON) and 0x13 (Ctrl-S, XOFF) on systems such as Ubuntu Linux
where software flow control is active by default.

* completed read/write timeout implementation in high-level API

Added new functions to set and clear cumulative timeouts for blocking
read and write functions, a new `Timeout` exception that will be
thrown, and time-tracking code in calls to the blocking read/write
functions. This way, users can now set overall timeouts on other IO
functions, such as `readuntil`, that make multiple calls to our
blocking functions.

* change close(::SerialPort) to return nothing

That what other close(::IO) methods in Base do. It looks quite odd in the
REPL to get the entire closed SerialPort object thrown back at you.

* build HTML documentation using Documenter.jl

also polished some docstrings and fixed a parameter name inconsistency

* split low-level API into separate module (#68)

The low-level API wrappers now sit in a submodule LibSerialPort.Lib.

The high-level API currently still re-exports a few symbols from that
low-level API module, to preserve backwards compatibility.

This is due to

* my previously started attempt to merge the low and high-level
  APIs for the three functions sp_flush, sp_drain, and sp_output_waiting

* the fact that several high-level APIs currently still refer to
  enum types and constants defined by the low-level API

We may want to phase out either or both in future, for a cleaner
separation, and a more Julian high-level API.

* documentation typo fixed

* allow arm64 to fail

Arm64 builds have long filed, therefore let's allow them to fail in Travis until that problem is fixed, such that PR authors for other issues do not get build errors that they are not responsible for.
@mgkuhn
Copy link
Collaborator Author

mgkuhn commented Nov 18, 2020

The main part of this issue is now fixed in release v0.5.0. However there is still a left-over of my previous attempt of merging the two APIs to sort out, namely in the high-level API the lines

import .Lib: sp_flush, sp_drain, sp_output_waiting
export
    # Low-level functions currently extended by the high-level API
    sp_flush,
    sp_drain,
    sp_output_waiting

# fixes https://github.com/JuliaIO/LibSerialPort.jl/issues/53
@deprecate flush(sp::SerialPort, buffer::SPBuffer=SP_BUF_BOTH) sp_flush(sp, buffer)

# 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)

These currently simply pass through to the high-level API three functions from the low-level API, by adding corresponding ::SerialPort methods. I believe these should really be separate high-level API functions, rather than additional methods to low-level functions. The main difficulty with naming these functions (other than simply stripping of the sp_ prefix that is characteristic of the low-level API) is explained already in the note at the end of this low-level API docstring:

"""
    sp_flush(port::Port, buffers::SPBuffer)
    sp_flush(port::SerialPort, buffers::SPBuffer)

Discard data in the selected serial-port buffer(s).

Supported values for `buffers`: `SP_BUF_INPUT`, `SP_BUF_OUTPUT`, `SP_BUF_BOTH`

Returns `SP_OK` upon success or raises an `ErrorException` otherwise.

!!! note

    Not to be confused with `Base.flush`, which writes out buffered
    data rather than discarding it: the underlying `libserialport` C
    library unfortunately uses the verb “flush” differently from its
    normal meaning for `Base.IO` (`sp_drain` provides the latter
    in this library).
"""

What should the high-level API equivalents of sp_flush and sp_drain be called, considering that Base.flush does the equivalent of sp_drain (i.e., wait until the last byte has been transmitted from the buffer) and not the equivalent of sp_flush (i.e., discard any bytes in the buffer that have not yet been transmitted)? The low-level API functions follow the naming convention of the POSIX functions tcdrain and tcflush and not the C/POSIX function fflush.

Anyone here who is better than me with English verbs of getting rid of things (discard, dispose, jettison, eject, scrap, ditch, dump, flush, drain, empty, reset, zeroize, ...)?

How about using truncate and flush as in

flush(sp::SerialPort) = sp_drain(sp.ref)
truncate(sp::SerialPort) = sp_flush(sp.ref, SP_BUF_OUTPUT)

As these two existing Julia I/O functions probably have the closest semantic equivalence to sp_drain and sp_flush, respectively.

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

No branches or pull requests

2 participants