Skip to content

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

Open
BsAtHome wants to merge 1 commit into
LinuxCNC:masterfrom
BsAtHome:fix_hal-new-api-and-types
Open

hal: Add new types and API framework for the getter/setter change.#4099
BsAtHome wants to merge 1 commit into
LinuxCNC:masterfrom
BsAtHome:fix_hal-new-api-and-types

Conversation

@BsAtHome
Copy link
Copy Markdown
Contributor

@BsAtHome BsAtHome commented Jun 1, 2026

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.

Copy link
Copy Markdown
Contributor

@grandixximo grandixximo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread src/hal/hal.h
volatile rtapi_sint _s; \
volatile rtapi_uint _u; \
volatile rtapi_real _r; \
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small thought on the __hal_mapped_type union: since it gets type-punned against hal_data_u (a different union type), strict aliasing technically applies. The RT side is covered because RTFLAGS adds -fno-strict-aliasing, but the userspace ULAPI build runs at -O2 without it, so there the access leans on the volatile members to stay well-behaved. Would it be worth tagging the union with __attribute__((__may_alias__)) to make it portably safe in both builds regardless of flags? Happy to defer if you think the volatile guarantee already covers it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, technically (the right kind of right), and understand the problems. However, the implementation has some guarantees that should mitigate the problem. Also, the compiler is not complaining ;-)

We know that all hal_malloc allocated hal_data_u units are always guaranteed at a 8-byte boudary (always enforced because the alloc implementation now enforces minimum size of 8). This also includes the units in the pin/param/signal structures, which also are properly aligned afaik.

We also know that the the new hal_<type>_t are actually pointers into hal memory to the internal hal_data_u, so they should always be properly aligned because they are supplied by the HAL API. This is now also true for params because they are treated the same as pins.

The only (unaligned) aliasing problem would then arise from someone punning a pointer that is not supplied by the new API. That would be an error because access to that part should not be done anymore. Punning the new hal_<type>_t types is possible, but risky because they are opaque pointers (to structures that do not exist). If someone does type-pun to/from the new hal types, then they are already in very muddy waters and I sincerely hope they pay for it with a crash. It is exactly this coding behaviour we want to avoid.

BTW, the public hal_data_u type will have to be retired in the end.

Comment thread src/hal/hal_lib.c
{
va_list ap;
va_start(ap, fmt);
int ret = hal_param_new_newapi(HAL_BIT, dir, (void**)ref, compid, fmt, ap);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used (void**)ref here and ref in the others, unify the style?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

used (void**)ref here and ref in the others, unify the style?

Only in the boolean version the compiler sees a size difference and complains. All others are implicit.

All of this will vanish again. These call the old API now, but will eventually be the only ones standing where casting like this is no longer necessary.

Comment thread src/hal/utils/halcompile.g Outdated
@BsAtHome BsAtHome force-pushed the fix_hal-new-api-and-types branch from 7726780 to 5542239 Compare June 2, 2026 10:36
@andypugh
Copy link
Copy Markdown
Collaborator

andypugh commented Jun 2, 2026

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
Copy link
Copy Markdown
Contributor Author

BsAtHome commented Jun 2, 2026

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.

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

Successfully merging this pull request may close these issues.

3 participants