Skip to content

Draft - HAL types update and HAL isolation#4247

Draft
BsAtHome wants to merge 1 commit into
LinuxCNC:masterfrom
BsAtHome:wip_hal-types-and-isolation
Draft

Draft - HAL types update and HAL isolation#4247
BsAtHome wants to merge 1 commit into
LinuxCNC:masterfrom
BsAtHome:wip_hal-types-and-isolation

Conversation

@BsAtHome

@BsAtHome BsAtHome commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

This is the full tree conversion for the HAL types where #4099 would be the first step.
Changes:

  • finalize (small minor) details regarding the new HAL types
  • retire code that performs direct access to the underlying HAL memory for all HAL types
  • covert all HAL components to the new API
  • convert all kinematics to the new API
  • convert all HAL drivers to the new API
  • convert halmodule and other code that used the "old" pin/param access method(s)
  • rewrite halrmt such that it is actually functional
  • introduce a HAL query API for user-space, including hal_get_p, hal_set_p, hal_get_s and hal_set_s and more. This reduces code duplication and re-implementations that may or may not be fully identical
  • make the HAL mutex recursive (for query callback reentry) and only accessible by the HAL library
  • convert all code that included hal_priv.h and used direct HAL memory access to use the new query API
  • revamp of the hal_*(3) man pages and full documentation of all the new API (old stuff will be removed eventually)

State of progress of these changes:

  • the HAL library's internals are now fully private to the library
  • the hal_data_u is never used in the code anymore (except for HAL library internal) and is ready to be retired
  • the HAL API still retains full compatible with the "old" ways until the API break is performed
  • all CI passes, but there may be some missed issues. Every (incremental) change obviously needs critical review before submitted
  • waiting to move forward...

Missing in this tree:

  • HAL_PORT/hal_port_t changes because they would be a breaking change

@grandixximo

grandixximo commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Never cease to amaze me...
Are we having a meeting with #4099 as central discussion? We skipped a few weeks, not sure why @rmu75
Seems about time... or is everyone on summer vacation? Probably better send to the mailing list...

@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from 7bb25b0 to c4f0b6f Compare July 17, 2026 16:05
@BsAtHome BsAtHome mentioned this pull request Jul 17, 2026
Open
@rmu75

rmu75 commented Jul 18, 2026 via email

Copy link
Copy Markdown
Collaborator

@grandixximo

grandixximo commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

Should send to the mailing list, my bad for starting it here 😞

BsAtHome
BsAtHome commented Jul 18, 2026
View reviewed changes
Comment thread src/emc/rs274ngc/interp_namedparams.cc
// Make sure we can query pins/params/signals
retval = hal_lib_init();
CHKS(retval < 0, "fetch_hal_param: hal_lib_init(): error=%d", retval);

@BsAtHome BsAtHome Jul 18, 2026

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.

FIXME: init/exit is reference counted

grandixximo
grandixximo reviewed Jul 18, 2026
View reviewed changes
Comment thread src/hal/utils/halcmd_commands.cc Outdated
Comment thread src/hal/hal_lib_query.c
case HAL_S32: hal_set_si32(u.s, v->s); break;
case HAL_U32: hal_set_ui32(u.u, v->u); break;
case HAL_SINT: hal_set_sint(u.s, v->s); break;
case HAL_UINT: hal_set_uint(u.u, v->u); break;

@grandixximo grandixximo Jul 18, 2026

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.

If I understand correctly, these all store through the 8-byte union member in hal.h
AFAIK old-API params keep caller-sized storage hal_bit_t=1B, hal_s32_t=4B. So halcmd setp / hal_set_p on any out-of-tree legacy BIT/S32/U32 param overruns it corrupting adjacent shmem. Won't that already break the old API?

@BsAtHome BsAtHome Jul 18, 2026

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.

The HAL allocation has been changed never ever to allocate a chunk less than 8 bytes (and at least 8-byte aligned). So any write is always safe.

Older code may indeed write to the smaller part. That is why the getters also only address the smaller part. The setters will always write to the larger type (even bool). We are assured of always full writes only when the old code is disallowed (removal of hal_data_u and the old hal types). In the transition, we only have compatibility to assure.

And, old style code cannot link to HAL unless recompiled. The HAL memory version has been changed.

@grandixximo grandixximo Jul 18, 2026 •
edited
Loading

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.

The 8-byte minimum covers standalone allocations, but how does it protect this case?

struct comp_data {
    hal_s32_t speed;   // hal_param_s32_new(..., &speed) -> 4-byte storage
    hal_s32_t accel;   // adjacent field, same hal_malloc'd struct
};

A recompiled legacy component still exports fields inside one larger struct allocation. When hal_set_si32 writes the sign-extended 64-bit value at &speed, where do the upper 4 bytes (0xFFFFFFFF for -1) land? Does the compiler fixes this? Or will it not compile?

@BsAtHome BsAtHome Jul 18, 2026

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.

The case where new code will write an old style param. That is the problem. Need to think a bit more on the scenario.

@BsAtHome BsAtHome Jul 18, 2026

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.

As long as the support programs (mostly halcmd and halmodule) are "old style", then there is no problem because they will dereference a hal_data_u construct and write the smaller type.

The new library with hal_set_p() abstracted needs to divert the new setter construct and use hal_data_u dereferencing for parameters until the API break. Then you won't have any problems.

Comment thread src/hal/halmodule.cc Outdated
Comment thread src/hal/halquery.c Outdated
Comment thread src/hal/halquery.c
Comment thread src/hal/utils/halrmt.cc
Comment thread src/hal/utils/halrmt.cc
Comment thread src/hal/utils/halrmt.cc
Comment thread src/hal/utils/halrmt.cc
Comment thread src/hal/utils/halrmt.cc
@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from c4f0b6f to bce8942 Compare July 18, 2026 14:32
@BsAtHome

BsAtHome commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

@grandixximo the whole loadusr code needs to be reviewed separately. The constructs in both halcmd and halrmt feel off seem to have a bad smell. I don't think all cases of what can happen are caught correctly in either program. Luckily, it will be some time before this code will make its way. So time to put the thinking cap on and see if it can be made into something smelling fresh and likable ;-)

@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from bce8942 to 2f2ddb5 Compare July 18, 2026 15:52
@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from 2f2ddb5 to 64739e1 Compare July 18, 2026 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grandixximo grandixximo grandixximo left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BsAtHome @grandixximo @rmu75