Merge or split high-level and low-level API

Question

Merge or split high-level and low-level API

Opened this issue 4 years ago · 4 comments

~~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).

Answer 1 · 2020-08-17T09:44:33.000Z

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.

Answer 2 · 2020-08-17T10:08:41.000Z

@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.

Answer 3 · 2020-08-17T12:44:08.000Z

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.

Answer 4 · 2020-11-18T13:13:37.000Z

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.