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

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

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...

@rmu75

rmu75 commented Jul 18, 2026 via email

Copy link
Copy Markdown
Collaborator

@grandixximo

Copy link
Copy Markdown
Contributor

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

// Make sure we can query pins/params/signals
retval = hal_lib_init();
CHKS(retval < 0, "fetch_hal_param: hal_lib_init(): error=%d", retval);

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

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

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

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

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

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

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.

chan->params->float_param = 500;
chan->params->timer_num = 0;
hal_set_real(chan->params->param.r, 500);
hal_set_ui32(chan->params->timer_num, 0);

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.

param.r (frequency-khz) and timer_num are handles that are only created later in hm2_absenc_parse_md() (abs_encoder.c:~455-465).

If @rene-dev 's test setup has absenc channels configured, this is the prime suspect for IO not working

chan->params->float_param = 500;
chan->params->timer_num = 0;
hal_set_real(chan->params->param.r, 500);
hal_set_ui32(chan->params->timer_num, 0);

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.

same as above

chan->params->u32_param = 0xF;
chan->params->timer_num = 0;
hal_set_ui32(chan->params->param.u, 0xF);
hal_set_ui32(chan->params->timer_num, 0);

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.

same as above

Comment thread src/hal/hal_lib.c
@@ -1359,13 +1571,13 @@ int hal_link(const char *pin_name, const char *sig_name)
case HAL_U32:
*((hal_u32_t *) data_addr) = pin->dummysig.u;
break;
case HAL_S64:
case HAL_SINT:
*((hal_s64_t *) data_addr) = pin->dummysig.s;

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.

should this be .ls or that is after the transition period?

Comment thread src/hal/hal_lib.c
*((hal_s64_t *) data_addr) = pin->dummysig.s;
break;
case HAL_U64:
case HAL_UINT:
*((hal_u64_t *) data_addr) = pin->dummysig.u;

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.

same as above, should this be .lu ?

@grandixximo

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

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.

4 participants