hal: Update more easy-to-convert components to getter/setter by BsAtHome · Pull Request #4272 · LinuxCNC/linuxcnc

BsAtHome · 2026-07-27T13:42:01Z

This PR updates the next set of components to getter/setter. These components should also be rather easy as most changes are to the type names and using pinname_set(...) for the write action. The changes are not big, but there are just many components.

A few places a local variable was introduced to prevent multiple reads if the change was trivial. Pin/param names max/min/clamp were suffixed with an underscore to prevent kernel identifier clashes. Any inappropriate use of the old hal types has been cleaned up.

grandixximo · 2026-07-28T01:50:55Z

I load-tested all 40 components on a build of this PR (cherry-picked onto current master). 38 load and run fine, pin and param names are unchanged, and setp on the renamed min/max/clamp params works as before.

One problem: loadrt multiswitch (also with cfg=4) segfaults rtapi_app. Backtrace from the core:

#0 hal_set_ui32 (ref=0x0, val=3) at hal.h:531
#1 extra_setup (...) at hal/components/multiswitch.comp:73
#2 export (...) at objects/hal/components/multiswitch.c:63

top_position_set() runs in EXTRA_SETUP with a NULL ref. The generated export() calls extra_setup at line 63 but only creates the params at line 81+. This might be because the old scheme stored params by value in the instance struct, so an EXTRA_SETUP assignment worked before registration, while the new ref only exists after hal_param_new_ui32 runs. If that reading is right, other components that write a param in EXTRA_SETUP (bldc, carousel, filter_kalman, message, pushmsg, spindle come to mind) could hit the same on their turn, so it might be worth a look at the generation order in halcompile rather than only at multiswitch.

Everything else in the batch tested OK.

grandixximo · 2026-07-28T02:29:42Z

Follow-up with precise data instead of my earlier guess list. I scanned every in-tree .comp for writes to pins/params in EXTRA_SETUP. The full list of affected components is exactly two:

multiswitch.comp (top_position, the crash above)
serport.comp (ioaddr, serport.comp:80; a read-only param the component writes at the end of EXTRA_SETUP)

serport is not in this batch, but it will crash the same way when its conversion comes. The other comps I named (bldc, carousel, filter_kalman, message, pushmsg, spindle) only assign locals or modparam arrays in EXTRA_SETUP, so they are not affected.

I also added a warning to halcompupdate (#4256) that flags this pattern during conversion, distinguishing params (regression, direct assignment worked before) from pins (already invalid). Running it over the tree now catches these cases before any load test.

Writes to pins/params inside EXTRA_SETUP convert cleanly to setters, but the setter uses a reference that halcompile initializes only after extra_setup() has run, so the converted component crashes when loaded (seen with multiswitch: top_position_set() on a NULL ref in extra_setup, LinuxCNC#4272). Track the current body section while rewriting and warn when a converted write lands in EXTRA_SETUP, distinguishing params (direct assignment worked before, this is a regression) from pins (direct writes were already invalid). EXTRA_CLEANUP and FUNCTION writes stay silent; the conversion itself is unchanged.

BsAtHome · 2026-07-28T07:36:25Z

Yes, the problem is that the EXTRA_SETUP() assigns a value to a parameter. However, the parameters (and pins) are not created until after the EXTRA_SETUP() has finished.

There are two ways to fix this:

Introduce an init state in the main loop. It is simple but needs to test every cycle. The real disadvantage (problem) is that the value of the parameter is not visible until after at least one thread cycle.
Modify halcompile to add a POST_EXPORT() function that is called just before the generated export() function exits. You can do whatever you like in the function and the pins/params/functs for the instance exist.

I'd opt for option 2.

BsAtHome · 2026-07-28T08:07:57Z

I took the multiswitch.comp out of this batch so it can be integrated.

Will modify halcompile/multiswitch.comp and resubmit separately.

grandixximo · 2026-07-28T08:13:47Z

Agreed, option 2.
My halcompupdate warning (#4256) already flags the pattern, I will revise the warnings, to point to POST_EXPORT if that's where we land, the tool was not a complete waste after all...

BsAtHome · 2026-07-28T10:01:18Z

If that reading is right, other components that write a param in EXTRA_SETUP (bldc, carousel, filter_kalman, message, pushmsg, spindle come to mind) could hit the same on their turn, so it might be worth a look at the generation order in halcompile rather than only at multiswitch.

Just went through all components with "option extra_setup" and only serport.comp was writing to a parameter (fixed). The others use it to init data, other setup or create their own pins/params prior to access and that is fine.

Writes to pins/params inside EXTRA_SETUP convert cleanly to setters, but the setter uses a reference that halcompile initializes only after extra_setup() has run, so the converted component crashes when loaded (seen with multiswitch: top_position_set() on a NULL ref in extra_setup, LinuxCNC#4272). Track the current body section while rewriting and warn when a converted write lands in EXTRA_SETUP, distinguishing params (direct assignment worked before, this is a regression) from pins (direct writes were already invalid). EXTRA_CLEANUP and FUNCTION writes stay silent; the conversion itself is unchanged.

* hal: Add halcompupdate to migrate .comp files to the new HAL API 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 (#4099, #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 #4247 functionally. * halcompupdate: Warn on pin/param writes in EXTRA_SETUP Writes to pins/params inside EXTRA_SETUP convert cleanly to setters, but the setter uses a reference that halcompile initializes only after extra_setup() has run, so the converted component crashes when loaded (seen with multiswitch: top_position_set() on a NULL ref in extra_setup, #4272). Track the current body section while rewriting and warn when a converted write lands in EXTRA_SETUP, distinguishing params (direct assignment worked before, this is a regression) from pins (direct writes were already invalid). EXTRA_CLEANUP and FUNCTION writes stay silent; the conversion itself is unchanged. * halcompupdate: Point EXTRA_SETUP write warnings at POST_EXPORT POST_EXPORT() (halcompile option post_export yes) is now the supported place for pin/param initialization that must happen after creation, so advise it instead of the first FUNCTION pass. Assert the advice in the test. * halcompupdate: Preserve volatile for legacy hal_*_t types, print summary Body code converts legacy hal_*_t types to the volatile-preserving form ('hal_bit_t x' -> 'volatile rtapi_bool x'), semantics-identical whether or not the qualifier is load-bearing; each such conversion is reported with its line number so the spots can be reviewed. 'variable' declarations cannot express volatile, so legacy hal_*_t types there are reported and left unchanged for manual conversion. Body warnings now report file line numbers instead of body-relative ones. Each file ends with a summary of mechanical changes applied and constructs left for manual review. * halcompupdate: note names that spell a legacy type A pin, param, component or function name containing a legacy type as a segment (out_s32, input_bit, conv_s32_float) is stale once the type is removed at the API break. Note it and suggest the new-style spelling (out_si32, input_bool, conv_si32_real), leaving the rename to the author: names are part of the HAL configuration interface - the component name is the loadrt/loadusr argument and module name, functions are exported as comp.N.name - so they are never changed, and the note does not affect the --check exit status or the manual-review count. The word in a name may not mean the type (plasmac's float_switch is a hardware float switch, demux's sel-bit counts bits of a value); the note is then simply dismissed. Only full segments match, so 'floating' is not noted for 'float', and duplicated segments are mentioned once. 'signed' and 'unsigned' are not checked in names: no sensible word replaces them there. The note points at the line of the declaration itself, and the per-file summary reports the note count even when nothing else needed conversion. Run over all in-tree components: 54 names are noted (the 34 conv_*, abs_*, scaled_s32_sums and tristate_* component names, and 20 pins in demux, histobins, histobinstream, multiswitch, plasmac, reset, mesa_7i65), each a legacy type word used as a name segment. * halcompupdate: point 32-bit pins at sint and uint HAL is 64-bit only now. The tool still converts s32 and u32 to si32 and ui32, which keep the component's 32-bit behaviour for now, but that is a stopgap: the component has to be made 64-bit clean, and that takes an analysis of its code, since a value that passes through 32-bit intermediate storage or a 32-bit calculation is silently truncated. Each such declaration is noted with its line and counted in the summary, the documentation says so as a requirement, and a name that spells a type is suggested with sint and uint, as the in-tree renames spell it. The documentation now describes HAL after the API break: plain assignment to a pin no longer compiles and the legacy declaration types are accepted with a warning. * halcompupdate: leave C types to the author Only pins and params are safe to convert blind. A hal_*_t was a volatile value, nearly always wrong as a variable, and propagating the volatile into a conversion only hides that, so the tool no longer writes one. Every use of a hal_*_t or real_t is reported at its line and stays, with the plain type it most likely wants; the component does not compile until the author has looked at it. double and rtapi_real are the same type, and rewriting a variable float to rtapi_real changes its precision, so the rest of the C type conversion goes too, with --no-c-types. comp.adoc gets a section on what the old types were and what replaces them, as Bertho suggested. * halcompupdate: the API break has happened Warnings and notes still said a type "will be removed at the HAL API break". #4565 is in, so say what is true now: some of these are gone, the rest are deprecated.

hal: Update more easy-to-convert components to getter/setter

f1b579f

BsAtHome force-pushed the halgs_components-simple-2 branch from a61d487 to f1b579f Compare July 28, 2026 08:06

grandixximo merged commit d6aa0c8 into LinuxCNC:master Jul 28, 2026
16 checks passed

Navigation Menu

Repository navigation

hal: Update more easy-to-convert components to getter/setter - #4272

hal: Update more easy-to-convert components to getter/setter#4272
grandixximo merged 1 commit into
LinuxCNC:masterfrom
BsAtHome:halgs_components-simple-2

BsAtHome commented Jul 27, 2026

Uh oh!

grandixximo commented Jul 28, 2026

Uh oh!

grandixximo commented Jul 28, 2026

Uh oh!

BsAtHome commented Jul 28, 2026

Uh oh!

BsAtHome commented Jul 28, 2026

Uh oh!

grandixximo commented Jul 28, 2026

Uh oh!

Uh oh!

BsAtHome commented Jul 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

BsAtHome commented Jul 27, 2026

Uh oh!

grandixximo commented Jul 28, 2026

Uh oh!

grandixximo commented Jul 28, 2026

Uh oh!

BsAtHome commented Jul 28, 2026

Uh oh!

BsAtHome commented Jul 28, 2026

Uh oh!

grandixximo commented Jul 28, 2026

Uh oh!

Uh oh!

BsAtHome commented Jul 28, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants