Repository navigation
hal: C++ API (hal.hh) and pybind11 bindings on the new HAL API - #4251
grandixximo wants to merge 3 commits into
Conversation
|
This seems to be working, but I am not sure if it is the right shape we want, these are the direction I took on the issue raised in the meeting, subject to revision.
@BsAtHome A bare hal_sint_t is width-blind, and wrapping foreign raw handles across widths is unsafe in the interim. But the C++ layer never selects from the handle alone. Component-created pins carry the type at construction via traits (int32_t vs int64_t are distinct C++ types, compile-time selection), and the runtime path uses q->pp.type from your query API to pick the variant alternative, the stored-type-tag multiplex you described in #4099. The only unsafe path is explicitly reinterpreting a raw handle, which can be guarded with a query lookup. So the layer works transitionally, it just gets simpler after the break. |
|
oh, that was quick. I think this should be separate from the other big PR, as this can go in master fairly quickly. the only reason I stopped was that buster had no pybind11 package. this should also replace the current pyhal/halmodule instead of adding a third module. and, the same cpp api should be used in xhc, task and other places which are implemented in cpp. |
|
Buster is basically not supported anymore with master, I will fix the review points tomorrow, thank you both |
a8c2a71 to
87ff92b
Compare
|
There is another issue. The new hal_lib uses a reference counted init/exit and has separated out Calling The python class must initialize using hal_lib_init() and terminate by calling hal_lib_exit(). If the user has not called hal_exit() on any created components, you will now get a proper error message (because the user forgot to terminate correctly). |
|
can you rename the module to halmodule, so that the UIs and the tests that are already in place use the new bindings, and remove the old ones? |
Replacing halmodule is the goal, agreed, but renaming now would break things for two separate reasons. First, a hard technical one: on master the new bindings cannot cover the existing Python surface. Second, the compatibility tail: So the proposal stands as: land this additive now (hal.hh + halpp alongside, nothing replaced), and do the rename + replacement as a follow-up once #4247 is in. Your old branch's consumer sweep (hal_glib, qtvcp core, raster, tests) is the starting checklist for that PR. Is this acceptable? Open for discussion... |
|
I see that #4231 changes halmodule.cc, we should also add those functions via c++/pybind11 |
No, all of this should be in halquery. |
What is halquery? |
That is the new interface to all of HAL's internals nicely wrapped up from the hal query interface without someone needing to dig into HAL's internals. Hal_lib's internals are fully isolated in #4247 and nobody should be writing code that circumvents that. Inclusion of hal_priv.h is strictly forbidden in any code that is not part of hal_lib. |
|
ah, found it. I would much prefer if it would be a c++ api, which can be used to automatically generate the python bindings. |
|
It is a pure C version, so it does not have the same problems as C++ interfacing C-based Python ;-) |
|
This is exactly the split #4251 (this PR) implements: the query API stays pure C in the library (Bertho's layer, no bypass possible), and the C++ API sits on top of it (hal.hh), which is where Python bindings get generated (halpybind) , no hand-written CPython code. halquery.c proves the query API is sufficient; the same functions can be exposed as pybind wrappers over |
|
Fine by me to get it C++ wrapped. It does not yet have priority one. There is one thing that should be fixed in both halmodule and halquery: use IntEnum types for both type and direction. It may be necessary to have a |
|
A shared pure-Python |
That would be the simple way. If it works, then it should be nice. The real benefit is in halquery to have the iteration result show the right name instead of a number. There it is big value. Probably also need to add the same for other constants.
Global namespace naming and fixing hierarchy is a problem we need to fix in a later version. This would be a version 3.0 thing, I guess ;-)
Ha, and so much for "C++ is easy to maintain" ;-) |
202386a to
6925b79
Compare
|
Going back to draft, waiting for master new API to materialize ;-) |
33012db to
63c8a05
Compare
|
Branch reworked after feedback Bertho sent me in a private mail (63c8a05 => 7beaaf3). The haltype.py commit is gone, replaced by:
pybind11 note: real enum.IntEnum via py::native_enum needs pybind11 >= 3.0 and trixie ships 2.13, so construction goes through the enum module's functional API. The classes are genuine IntEnums either way, so moving to py::native_enum later (TODO in the header) is invisible to user code. Query results stay plain dicts as discussed; the future hal.query bindings tag type/dir with these classes, no per-function wrappers. tests/haltype.0 is now a sanity suite for the classes, tests/halpp checks halpp shares them. All HAL suites pass. |
7beaaf3 to
d24bbd7
Compare
d24bbd7 to
e9fd2bf
Compare
e9fd2bf to
9de1f11
Compare
9de1f11 to
8980d3e
Compare
|
@BsAtHome one API question from adding port pins: hal_pin_new(3) says hal_get_p(3) cannot be called on a HAL_PORT pin, but the library returns success with value 0 for it, and hal.query reports the pin's buffer size. hal.hh's get_value() currently relies on hal_get_p() succeeding for port pins (a callback fills in the buffer size). Which behavior is intended? If get_p on a port pin should fail, I'll switch get_value() to another route and the man page stays as is; otherwise the man page needs a line. |
|
Tricky question... The port pins are only end-point nodes. They do not have any real meaning besides read/write. The size of the buffer should ideally be retrieved using hal_get_s because the port pins can never stand alone and need a binding signal. The hal_set_s on that signal sets the buffer size, which makes it logical to retrieve the size using hal_get_s. The fact that the hal_get_p on the pins also return the size is due to legacy IIRC. There is no "harm" in that you can call hal_get_p on a port pin. However, the question in the room is whether hal_get_p on an output pin really should be retrieving one byte of the queue and hal_set_p on the input pin writes one byte into the queue. The queue's size is managed by the signal. |
|
Thanks, that settles it. I'll keep the pin side out of it: in hal.hh a port pin gets no scalar value (get_value() on a port pin and the runtime-typed get() raise), the buffer size comes from the signal via hal_get_s(), and port::size() stays as the explicit call on the handle. That leaves hal_get_p()/hal_set_p() on port pins free for whatever you decide, including byte I/O. Small correction for the record: the library's hal_get_p() on a port pin returns 0, not the size (get_common() is called with getport=0 for pins); only hal.query computes the size for pins itself. |
8980d3e to
e5e5920
Compare
2599339 to
a3df33f
Compare
a3df33f to
37c161a
Compare
|
@BsAtHome Rebased onto current master, CI is green. The helpers in |
| std::map<std::string, pin_t> items_; | ||
| std::map<std::string, pin_t> params_; |
There was a problem hiding this comment.
Why are pins called items_ and parameters params_? Wouldn't make more sense to call these pin_items_ and param_items_?
And now I'm at it,... You could also just remove params_ and put everything into the items_ bucket. The namespaces cannot overlap. You don't seem to have an API to retrieve either pins or params and the find() will find either.
What you do need to add to the pin_t construct (above) is an ispin() API. Otherwise it is no longer possible to distinguish. Unless, you want the caller to get the direction and test for the distinction there, but I don't see you storing the direction.
There was a problem hiding this comment.
One map now. Each item knows whether it is a pin and its direction, and the component has ispin().
There was a problem hiding this comment.
One map now. Each item knows whether it is a pin and its direction, and the component has
ispin().
You have a is_pin() method in the anypin construct. But the component's version is called ispin().
There was a problem hiding this comment.
Renamed to is_pin() on both.
| return 0 == hal_comp_by_name(name.c_str(), &q); | ||
| } |
There was a problem hiding this comment.
The function name implies that you want to know whether it exists, which is not a boolean question. You can have an error here. A non-existing component returns -ENOENT and one that exists returns zero.
It is unclear how this should be handled. You can throw or ignore the problem. Ignoring it may cause follow-up problems. Throwing demands more complicated code. Can you convert this lose-lose into a win-win?
(similar problem other places)
There was a problem hiding this comment.
My try: not found answers false, any other failure throws std::system_error. Does that work for you?
There was a problem hiding this comment.
Yes, as long as it is documented and handled. Exceptions reveling up the tree until an abort causes a lot of grief.
There was a problem hiding this comment.
Agreed. The exceptions are documented once at the top of hal.hh, and in _hal every one ends up as a Python exception.
|
@grandixximo can you change it, so it replaces halmodule.cc? otherwise it cant be tested properly. |
Reintroduce a C++ interface for HAL, replacing the old hal.hh that was removed ahead of the API break. It is built strictly on the public C API and the query API: no hal_priv.h, no direct shared memory access, no re-implemented library internals. - hal.hh: type-safe, header-only C++ layer in linuxcnc::hal, kept in include/ with the other exported headers. Typed pin/param handles via traits<T> over rtapi_bool, rtapi_sint, rtapi_uint and rtapi_real (compile-time accessor selection, no 32-bit handles), a runtime-typed pin_t variant and anypin for name-based access, a component class with add_pin for the struct-member idiom, and ULAPI by-name query/set functions on hal_get_p/hal_set_p/hal_get_s/hal_set_s/hal_comp_by_name. Handles re-read the shmem slot on every access so hal_link() slot rewrites stay visible (C pointer-variable semantics). Handles are move-only; HAL objects are unique. Query callback paths are exception-free; range errors are reported after the library releases the HAL mutex. - Streams: linuxcnc::hal::stream, a move-only wrapper around hal_stream_t, created with a depth and a typestring or attached to an existing key. Element types drive the conversion of samples in both directions with the same range checks as the by-name setters. Library failures reported as a negative errno are thrown as std::system_error. - halpybind.cc: pybind11 module (halpp.so) exposing component, Pin, stream and the by-name functions. Built alongside _hal/hal.py, replacing nothing. Type and direction tags are the _hal.Type/_hal.Dir IntEnum classes: arguments accept the members or plain ints, results come back as members. Importing _hal also initializes the HAL library. String set values go through setps_common_cb for halcmd-consistent parsing, std::system_error becomes OSError, and SIGTERM raises KeyboardInterrupt as it does in _hal. - Ports: linuxcnc::hal::port, a move-only handle for HAL_PORT pins with all-or-nothing read/peek/peek_commit/write plus readable/writable/size/clear, created with component::newport() or runtime-typed newpin(). The buffer belongs to the linking signal and is sized with set_signal(); an unlinked port has no buffer and its reads and writes fail quietly. A port pin has no scalar value: get_value() and the runtime-typed get/set raise for it, leaving hal_get_p()/hal_set_p() semantics on port pins to the library. The size is read from the signal (get_value() on a port signal) or with port::size(). halpp exposes the same calls on Pin, with bytes in and out (str is written as UTF-8). - tests/halpp: Python and native C++ smoke suites, plus a stream create/attach pair across two processes the way sampler and streamer are used. The C++ suite compiles against the tree, so the test is skipped for installed packages. Based on the pybind11 branch by rene-dev, rebuilt on the new HAL API.
Two Python interfaces to HAL are one too many, and hal.hh needs real users to be tested. _hal is now a pybind11 binding of hal.hh with the API it always had, so hal.py and every `import hal` in the tree run on it unchanged. halpybind.cc and the halpp module are gone; tests/halpp becomes tests/hal-cpp and drives the same checks through `import hal`. The Python rules for turning a value into a HAL value stay as they were (a float into a bool, "yes" into a bool, int() into the integer types). The conversion now happens before any HAL call, so no Python code runs while the library holds its mutex. Fixed on the way: `del` of a pin or param crashed, so did repr() of an exited component; an item now keeps its component, and with it the HAL memory it points into, alive; setting a port item raises instead of leaving an exception pending; a failed stream constructor no longer detaches a stream that was never attached; the "Too many/Too few elements" messages of stream.write() were swapped; get_info_* read a port's reference as an integer. hal.shm, deprecated and unused, is dropped with its documentation. new_sig(), connect() and disconnect() still return True on failure, callers depend on it. hal.hh, from Bertho's review: unready(); pins and params in one map, each item knowing whether it is a pin and its direction; one exact conversion for item, by-name and stream sets, refusing NaN, fractions and out-of-range values instead of truncating; component lookups by id; a lookup that finds nothing answers false while any other failure throws std::system_error, as do library failures of the component calls.
37c161a to
fba9f00
Compare
|
@rene-dev @BsAtHome Following your reviews, Could you take a look and tell me whether the shape is right? |
| #!/usr/bin/env python3 | ||
| # Attaches to the stream created by stream_writer.py and reads back the | ||
| # samples it wrote. | ||
| import time |
There was a problem hiding this comment.
We already have a stream reader/writer test, don't we? What is this test adding that is so special that it cannot be done with the other test. Or, why not "fix" the other test?
There was a problem hiding this comment.
Fair, it was a copy. Its few extra checks moved into tests/halmodule/stream and the copy is gone.
| constexpr double two63 = 9223372036854775808.0; | ||
| constexpr double two64 = 18446744073709551616.0; |
There was a problem hiding this comment.
If you want to be exact, wouldn't you then normally write 0x1p63 and 0x1p64?
There was a problem hiding this comment.
I had them; cppcheck misreads them and calls the test always true. Back to 0x1p63/0x1p64 with an inline suppression.
| if constexpr(std::is_same_v<T, rtapi_bool>) | ||
| return false; |
There was a problem hiding this comment.
Why false? Normal floating point to bool conversion can apply couldn't it?
Your comment on top says it wont convert float->bool, but I don't understand why.
There was a problem hiding this comment.
No good reason. A bool takes any value now, non-zero is true, as in C.
| std::map<std::string, pin_t> items_; | ||
| std::map<std::string, pin_t> params_; |
There was a problem hiding this comment.
One map now. Each item knows whether it is a pin and its direction, and the component has
ispin().
You have a is_pin() method in the anypin construct. But the component's version is called ispin().
From Bertho's second look: a bool takes any value, non-zero is true, as in C; the 2^63 and 2^64 limits are written as hex floats again, with an inline suppression for cppcheck, which misreads them as an always-true test; component::ispin() is is_pin(), like anypin's; the exceptions hal.hh throws are documented once at its top, and get_value() reports a missing item with std::system_error like the other lookups. The stream writer/reader pair in tests/hal-cpp duplicated tests/halmodule/stream: its few extra checks move there and the copy goes.
include/hal.hhis a header-only C++ layer on the public HAL API, with nohal_priv.hand no direct shared memory access: typed pin and param handles overrtapi_bool,rtapi_sint,rtapi_uintandrtapi_real, a runtime-typedanypin, components, port pins, streams, and the by-name query and set functions. Values convert exactly or are refused, so NaN, fractions and out-of-range values are never truncated or wrapped. A lookup that finds nothing answers false; any other failure throwsstd::system_error._halis a pybind11 binding of it insrc/hal/halmodule.cc, withhal.queryattached as before, andhal.pyis unchanged. Names, exceptions, messages, return values and the lenient Python conversion rules are kept as they were; the conversion now happens before any HAL call, so no Python code runs under the HAL mutex. Fixed on the way: crashes ondel h.pinand onrepr()of an exited component, items outliving their component, setting a port item, a failed stream constructor, and the swapped stream length messages. halpp is gone, and so ishal.shm(deprecated, unused).tests/hal-cppcovers the Python API throughimport haland a native C++ suite compiled against the tree; it skips underSYSTEM_BUILD. The full suite passes locally (368/368), the GUI smoke tests included.