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
Merged
@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
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
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 2 times, most recently from 2f2ddb5 to 64739e1 Compare July 18, 2026 21:21
@grandixximo

grandixximo commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Thanks for the quick turnaround, checked the new push. The unload inversion, PyErr_Format swaps, alias pointers, halquery threads (all three), SET NET, and the ADDF bounds are all correctly fixed. On the legacy-param setter: agreed with your conclusion, diverting hal_set_p to type-width hal_data_u writes for old-style params until the API break closes the hole.

Two Out-of-Bounds of the same family as ADDF remain:

  1. DELF: table still declares nset=1 (halrmt.cc:2215) but setDelF reads ctx.toks[3] unconditionally. Same fix as ADDF: nset=2.
  2. SET SAVE: error path formats ctx.toks[3] (halrmt.cc:2092) with nset=1, so SET SAVE badkey without a filename reads out of bounds.

Minor: literal %d inside fmt::format strings (e.g. setAddf's error message) prints verbatim and drops rv, there are a few in src/hal/utils/halrmt.cc fix is mechanical, replace %d with {}.

Loadusr: agreed, the whole path wants a proper rewrite, here or in a follow up.

@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from 64739e1 to 695ac8f Compare July 19, 2026 07:14
@BsAtHome

BsAtHome commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

Should be all fixed now, except the param write in hal_set_p, which will take more than a mere moment to alter.

@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch 2 times, most recently from c6f9e2d to 7f5b5ae Compare July 19, 2026 10:54
@rene-dev

rene-dev commented Jul 19, 2026

Copy link
Copy Markdown
Member

I just tested this branch on a machine with smartserial slaves. the driver loads, and creates all the pins, but nothing works. IO are not getting to the hardware. I cannot see any error messages.

@BsAtHome

BsAtHome commented Jul 19, 2026

Copy link
Copy Markdown
Contributor Author

I just tested this branch on a machine with smartserial slaves. the driver loads, and creates all the pins, but nothing works. IO are not getting to the hardware. I cannot see any error messages.

Thanks for testing.
At least the pin creation went well and it didn't crash. That is not entirely bad.

But, I haven't run locally on the hardware for some time. Need to check up on that. Also, the changes in hostmot2 have not yet been properly reviewed, so there can be lurking any detail in any layer of the hm2 driver stack. I'll give a peep when this can be tested again.

@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from 7f5b5ae to 6d5e2e2 Compare July 19, 2026 13:21
@grandixximo grandixximo mentioned this pull request Jul 19, 2026
Draft
grandixximo
grandixximo reviewed Jul 20, 2026
View reviewed changes
Comment thread src/hal/drivers/mesa-hostmot2/abs_encoder.c Outdated
Comment thread src/hal/drivers/mesa-hostmot2/abs_encoder.c Outdated
Comment thread src/hal/drivers/mesa-hostmot2/abs_encoder.c Outdated
Comment thread src/hal/hal_lib.c Outdated
Comment thread src/hal/hal_lib.c Outdated
@grandixximo

grandixximo commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

@rene-dev

Does your test config string enable absenc (ssi/biss/fanuc formats)?
What does halcmd show pin hm2_*.sserial.port-0.port_state shows?
Could you grep dmesg for pin_new(...) called with already-initialized memory or Timeout waiting for CMD to clear

@grandixximo grandixximo mentioned this pull request Jul 20, 2026
Draft
grandixximo added a commit to grandixximo/linuxcnc that referenced this pull request Jul 20, 2026
@grandixximo
hal: Add halcompupdate to migrate .comp files to the new HAL API
5865295 
Out-of-tree .comp components using the legacy HAL types (float, bit,
s32, u32, s64, u64, signed, unsigned) and direct pin/param assignment
stop working when the HAL API break is performed (LinuxCNC#4099, LinuxCNC#4247).
halcompupdate rewrites them to the new API automatically:

* declaration types are converted: float->real, bit->bool, s32->si32,
  u32->ui32, s64->sint, u64->uint, signed->si32, unsigned->ui32
  ('port' is left alone, it has no new-style replacement yet)
* writes to out/io pins and to params become <name>_set(...) calls,
  including compound assignments, ++/--, array pins, chained
  assignments, *<name>_ptr dereferences and writes inside #define
  macros (macro parameters shadow same-named pins)
* legacy C types are modernized (double/real_t -> rtapi_real,
  hal_bit_t -> volatile rtapi_bool, ...)
* reads are unchanged and pins are never renamed, so existing HAL
  configurations keep working

Constructs that cannot be converted safely are left unchanged with a
warning for manual conversion: taking the address of a pin/param,
direct use of the legacy hal_pin_*_new/hal_param_*_new creation API,
postfix ++/-- whose value is used, and array indices with side
effects.

In-place rewriting is atomic (temp file + rename) and keeps a .bak
backup created with O_EXCL|O_NOFOLLOW.

halcompile now warns once per deprecated type, pointing to
halcompupdate(1), at the spot previously marked for this warning.
Docs: migration section in comp.adoc, new halcompupdate(1) manpage,
SEE ALSO in halcompile(1).  Regression test in
tests/halcompile/update-api.

Validated by converting all in-tree components from master: 119/124
compile (the other 5 need in-tree headers and fail identically for
the already-converted versions), and the output matches the hand
conversions in LinuxCNC#4247 functionally.
grandixximo
grandixximo reviewed Jul 20, 2026
View reviewed changes
Comment thread src/hal/components/reset.comp Outdated
Comment thread src/hal/components/histobinstream.comp Outdated
@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from 6d5e2e2 to ce3117d Compare July 20, 2026 12:12
grandixximo added a commit to grandixximo/linuxcnc that referenced this pull request Jul 20, 2026
@grandixximo
hal: Add halcompupdate to migrate .comp files to the new HAL API
937f5be 
Out-of-tree .comp components using the legacy HAL types (float, bit,
s32, u32, s64, u64, signed, unsigned) and direct pin/param assignment
stop working when the HAL API break is performed (LinuxCNC#4099, LinuxCNC#4247).
halcompupdate rewrites them to the new API automatically:

* declaration types are converted: float->real, bit->bool, s32->si32,
  u32->ui32, s64->sint, u64->uint, signed->si32, unsigned->ui32
  ('port' is left alone, it has no new-style replacement yet)
* writes to out/io pins and to params become <name>_set(...) calls,
  including compound assignments, ++/--, array pins, chained
  assignments, *<name>_ptr dereferences and writes inside #define
  macros (macro parameters shadow same-named pins)
* legacy C types are modernized (double/real_t -> rtapi_real,
  hal_bit_t -> volatile rtapi_bool, ...)
* reads are unchanged and pins are never renamed, so existing HAL
  configurations keep working

Constructs that cannot be converted safely are left unchanged with a
warning for manual conversion: taking the address of a pin/param,
direct use of the legacy hal_pin_*_new/hal_param_*_new creation API,
postfix ++/-- whose value is used, and array indices with side
effects.

In-place rewriting is atomic (temp file + rename) and keeps a .bak
backup created with O_EXCL|O_NOFOLLOW.

halcompile now warns once per deprecated type, pointing to
halcompupdate(1), at the spot previously marked for this warning.
Docs: migration section in comp.adoc, new halcompupdate(1) manpage,
SEE ALSO in halcompile(1).  Regression test in
tests/halcompile/update-api.

Validated by converting all in-tree components from master: 119/124
compile (the other 5 need in-tree headers and fail identically for
the already-converted versions), and the output matches the hand
conversions in LinuxCNC#4247 functionally.
@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch 3 times, most recently from 7de3e27 to 9783519 Compare July 20, 2026 14:56
@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from 9783519 to ce7a050 Compare July 20, 2026 15:39
@BsAtHome
BsAtHome force-pushed the wip_hal-types-and-isolation branch from ce7a050 to 2b8cd3f Compare July 20, 2026 16:20
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.

4 participants

@BsAtHome @grandixximo @rmu75 @rene-dev