hal: Add new types and API framework for the getter/setter change.

[BsAtHome]

This is the first in a series of updates to move HAL pin and param access over to getter/setter methods and eliminate direct user access to the underlying data. This PR sets the stage and puts the type, data and access infrastructure in place.

The new data interface is neither slower nor faster than direct access. All access is expanded through inline function expansion. The new interface prevents access to the wrong data because access is strongly typed and clearly separates the application and implementation layers.

The new interface will replace the current HAL types when we perform the API break. The new types are (underlying type in parentheses):

• hal_real_t - (rtapi_real) floating point
• hal_sint_t - (rtapi_sint) 64-bit signed integer
• hal_uint_t - (rtapi_uint) 64-bit unsigned integer
• hal_bool_t - (rtapi_bool) boolean
• hal_port_t - (rtapi_sint) [not yet exposed because a change breaks the current API]

The new HAL types cannot be dereferenced and the compiler will emit an error if you try:

hal_sint_t mypin;
...
*mypin = 10; // Compiler barfs
rtapi_sint bla = *mypin; // Compiler barfs

You must use the proper access functions:

• rtapi_<type> hal_get_<type>(hal_<type>_t pin)
• rtapi_<type> hal_set_<type>(hal_<type>_t pin, rtapi_<type> value)
• special for compatibility (setter will always write the larger type):
  • rtapi_s32 hal_get_si32(hal_sint_t pin)
  • rtapi_s32 hal_get_si32_clamped(hal_sint_t pin)
  • rtapi_u32 hal_get_ui32(hal_uint_t pin)
  • rtapi_u32 hal_get_ui32_clamped(hal_uint_t pin)
  • rtapi_s32 hal_set_si32(hal_sint_t pin, rtapi_s32 value)
  • rtapi_u32 hal_set_ui32(hal_uint_t pin, rtapi_u32 value)

This change also makes it clear that setting a pin is not possible with simple assignment. The setter and getter are not valid lvalues. Both can be used as rvalues. The 32-bit compatibility works as expected and is transparent for the user API. All current code will be updated to use the si32 and ui32 variants where appropriate. Then, when all code is upgraded, we can start fixing the code to be 64-bit clean. The *_clamped() versions read the larger type and convert to the smaller type with min/max bounds.

The distinction 32/64 bit will be gone completely in the underlying data once all code uses the getter/setter access. That is the point where the user relevant code can be changed and break the API to eliminate userspace visible 32-bit values (i.e. HAL_S32, HAL_U32, HAL_S64, HAL_U64 type indicators will vanish and replaced by HAL_SINT and HAL_UINT).

A new set of pin/param creation functions have been made that use the underlying old API for compatibility until we do the full API break. New pins and params are created with:

• int hal_pin_new_<type>(int comp_id, hal_pdir_t direction, hal_<type>_t *pinref, rtapi_<type> dflt, const char *namefmt, ...)
• int hal_param_new_<type>(int comp_id, hal_pdir_t direction, hal_<type>_t *paramref, rtapi_<type> dflt, const char *namefmt, ...)

There are two additional pin/param new functions that use the si32 and ui32 markers. These are there for compatibility with the current system such that we can use both APIs concurrently until all code has been updated/fixed/ported. They can be eliminated once all code is 64-bit clean with respect to pin and param use and access.

The underlying HAL pin/param structures have been adapted to match each other's layout. Access to params are treated the same as pins from the user's (component) perspective and the underlying structure types will be merged in a future PR. This also allows to merge the lists and use a RB-tree for faster access from the python interface (halmodule).

The halcompile program, to create components, has been updated to support the new types. It is an addition to the status quo so we can use both systems while we migrate. Using the new types is as easy as:

pin real in input;
pin real out output;
;;
output_set(input);

Note that setting a pin must be done using the <pinname>_set() format. Reading a pin is transparant in halcompile compiled programs (just like today). An update for all components is already in the pipeline to be submitted after this PR is merged. Halcompile can drop the old style types once everything is moved over and we do the API break.

[grandixximo]

After this all out-of-tree components must be recompiled, add a note in the commit message? Not that anyone reads those...

[andypugh]

The new HAL types cannot be dereferenced and the compiler will emit an error if you try:

Does this cause any problems for Smart Serial or any other driver that determines pin types at load-time?

eg: https://github.com/LinuxCNC/linuxcnc/blob/master/src/hal/drivers/mesa-hostmot2/sserial.c#L350

Where p is dereferenced, and than dereferenced again in line 359.

(Because smart-serial has variable-size arrays of HAL pins, because the hardware itself defines the pin names)

[BsAtHome]

The new HAL types cannot be dereferenced and the compiler will emit an error if you try:

Does this cause any problems for Smart Serial or any other driver that determines pin types at load-time?
(Because smart-serial has variable-size arrays of HAL pins, because the hardware itself defines the pin names)

Should not be any problem. The construct uses a union of old hal types and is IMO wrongly implemented (also has invalid types long long s64_written and long long s64_param). It should have used the hal_data_u type in the first place.

Run-time determined types already require you to multiplex/demultiplex the reference/dereference at run-time. This still needs to be done with the new system. There is no actual change there, just written in a different way. BTW, the hm2_modbus component also uses run-time determined types and uses the types of multiplexing I am referring to.

The new system has a hal_refs_u, a union of the new hal types, that is created to support the run-time type determination.

For sserial (and most of hostmot2), it will just move over to the new access patterns. It'll be some (more) work, but no blockers have been detected.

The change for parameters will be a bit more complex, but that should not pose any real problem. I have not seen any constructs in the code that would prevent us from migrating everything to setter/getter and unifying the underlying pin/param API.

The biggest problems detected:

• the pyhal module using CDLL access. Already retired.
• hal.hh bypasses any usual access and is another (re)implementation of bad access patterns. Luckily, it is only used in emc/task/taskclass.{cc,hh} and is already retired in my tree.
• the hal/components/xhc-hb04.cc has a simulate mode that replaces the hal memory with local memory. That cannot work anymore. The simulate allocation style has to go. You can always simulate. You just need to have the hal shared memory segment available and that is no problem at all. So, there is no need for the complexity.

There may be other details lurking, but so far none have been of any blocking character.

[BsAtHome]

Just force pushed a cppcheck update. It turns out that cppcheck "older" wrongly trips on an added #error directive because it fails to evaluate a preprocessor directive correctly. This would trip CI as Debian/Ubuntu are on the older version. My local version is 2.19.1 and has no such problem.

BTW, Debian trixie running on 2.17.1 has one more false positive, but that is not seen in CI. Needs to be sorted out in another PR.

[grandixximo]

With the stricter CI have to be more picky, unrelated off topic about the docs, should we also force the docs to build clean with the CI? Like second build of docs should be nothing to do, then pass the CI?

[BsAtHome]

With the stricter CI have to be more picky, unrelated off topic about the docs, should we also force the docs to build clean with the CI? Like second build of docs should be nothing to do, then pass the CI?

That would be a whole new can of worms... Let us discuss that somewhere else.

[rene-dev]

• hal.hh bypasses any usual access and is another (re)implementation of bad access patterns. Luckily, it is only used in emc/task/taskclass.{cc,hh} and is already retired in my tree.

Dont do this. hal.hh is my attempt to create a cpp interface, which is then used in the pybind11 branch to create the python bindings automatically. this already works, but needs cleaning.

[rene-dev]

https://github.com/rene-dev/linuxcnc/blob/pybind11/src/hal/halmodule.cc

[grandixximo]

Rather than retire hal.hh vs keep it, could we land on re-basing it on the new getter/setter API? hal_pin<T> holds the new handle and calls hal_get_*/hal_set_* internally, so Rene keeps the C++ interface for pybind11 and Bertho gets the no-bypass invariant.

[BsAtHome]

• hal.hh bypasses any usual access and is another (re)implementation of bad access patterns. Luckily, it is only used in emc/task/taskclass.{cc,hh} and is already retired in my tree.

Dont do this. hal.hh is my attempt to create a cpp interface, which is then used in the pybind11 branch to create the python bindings automatically. this already works, but needs cleaning.

The hal.hh is incompatible with HAL getter/setter because it accesses HAL private constructs it should not be accessing and is using assumptions on the underlying structure. It is also incomplete and uses the wrong types. Every HAL pin/param access needs to go through the proper API and there should not be exceptions. I'm not sure you can fix the pin/param access in a proper way using the current constructs because the types have changed to non-dereferencable opaque pointers (and you are not allowed to re-implement access to the underlying memory to prevent the same problem as we have today from happening again).

If you want to make a C++ wrapper, then it must be transparently wrapping and using the C API. It must not bypass it. You also want to use the exact same HAL types as basis for pins/params and not a newly defined C++ template construct. It may be better to model it somewhat after halmodule, although there are some (solvable) problems there as well. However, I do not see the advantage in having it abstracted too much so you have to multiplex/demultiplex the types at every pin/param access as is required in halmodule. Then it is easier to use the C API. Besides, it may not always be a good thing to hide function call by call type selection in the code.

[BsAtHome]

Rather than retire hal.hh vs keep it, could we land on re-basing it on the new getter/setter API? hal_pin<T> holds the new handle and calls hal_get_*/hal_set_* internally, so Rene keeps the C++ interface for pybind11 and Bertho gets the no-bypass invariant.

You do not want a template type for HAL pins and params. It would be rather pointless because the template has to expand the HAL type directly as its passed type (remember it is an opaque type). The template cannot use a generic accessor in HAL types because there is no generic accessor. All acessors are HAL type bound. You'd need to demutiplex on basis of a saved type indicator to call the proper access function. So, instead you are using more storage for no gain and worse performance?

If you want to wrap pins and params in a C++ class, then you will need one explicitly for each HAL type where both lvalue and rvalue overloads are present and use the proper hal_set_xxxx() and hal_get_xxxx(). This will still be not enough because we have the 32/64 bit legacy. You'd have to make exceptions in the hal_sint_t and hal_uint_t wrapper classes because there are multiple accessors defined to ensure compatibility with the current code base.

[grandixximo]

Fair on the 32/64-bit point, I missed that. A single operator T() can't express the sint/uint accessor variants, so those wrappers need explicit methods (get() / get32() / get32_clamped() and the matching setters) rather than one operator. Agreed.

On the template though, I think we're picturing different things. I'm not suggesting a value-typed or type-erased template, which would need a stored type tag and runtime demultiplex like you describe. I mean a template keyed on the opaque handle type, resolved at compile time:

template<> struct hal_traits<hal_real_t> {
    static rtapi_real get(hal_real_t h)               { return hal_get_real(h); }
    static rtapi_real set(hal_real_t h, rtapi_real v) { return hal_set_real(h, v); }
};

hal_pin<hal_real_t> then stores only the 8-byte handle, operator rtapi_real() calls hal_get_real(h), all inline. No type indicator, no branch, no extra storage, same machine code as the bare C call. That is exactly your "one explicit class per HAL type with lvalue/rvalue overloads using the proper hal_set/get_xxxx()", just generated once instead of copy-pasted. The template is the DRY way to produce those classes, not an alternative to them.

The demultiplex cost is real but only for genuinely runtime-typed access (the Python/pybind11 path), which has to happen in any design since the C API has no generic accessor, halmodule included. So I'd split it: zero-cost per-type wrappers for component code, and a variant/registry layer on top for the runtime-typed Python bindings. Both go through the C API, neither bypasses it.

That keeps Rene's C++ interface alive without reintroducing the access problems you're removing. Does that reframing change the "retire it" call?

[BsAtHome]

Two problems: 1) the hal_xxxx_t lives in HAL memory and 2) the C++ version wants to put pins/params into a map. The first is solved by doing a hal_malloc on the target, but the second is not solvable using templates. You need the type-information of the class/pin retained in the map for it to be dereferenced using the proper accessor (and besides, it incurs an extra indirection).

There is a genuine need for unionized hal types in, to name a few, halmodule, hm2/sserial and hm2_modbus. But these always use stored type information to multiplex/demultiplex. If you want a C++ wrapper, then it must store type information. There is no free ride to hal memory anymore, by design. You either know the pin/param type by using the hal types or you use a unionized version and store the type to multiplex/demultiplex.

[grandixximo]

Agreed, you're right. The map is the deciding factor: a string-keyed std::map of heterogeneous pins forces type erasure, so it has to store the type and demultiplex on access. Templates only buy you zero-cost typing for compile-time-known named pins, which is not what hal.hh does. So my template framing doesn't apply here.

That still points at a rehab rather than a removal: rebuild hal.hh as a union + stored-type-tag wrapper over the proper hal_get_*/hal_set_* (using hal_refs_u), modeled on halmodule, no memory bypass. Runtime-typed by necessity, but going through the C API. That keeps Rene's C++ interface for the pybind11 bindings while satisfying the no-exceptions access rule. Does that work as the direction for hal.hh?