Skip to content

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

Merged
grandixximo merged 1 commit into
LinuxCNC:masterfrom
BsAtHome:halgs_components-simple-2
Jul 28, 2026
Merged

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

Conversation

@BsAtHome

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

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.

grandixximo added a commit to grandixximo/linuxcnc that referenced this pull request Jul 28, 2026
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

Copy link
Copy Markdown
Contributor Author

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:

  1. 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.
  2. 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
BsAtHome force-pushed the halgs_components-simple-2 branch from a61d487 to f1b579f Compare July 28, 2026 08:06
@BsAtHome

Copy link
Copy Markdown
Contributor Author

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

Will modify halcompile/multiswitch.comp and resubmit separately.

@grandixximo

Copy link
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
Contributor Author

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.

grandixximo added a commit to grandixximo/linuxcnc that referenced this pull request Jul 29, 2026
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.
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.

2 participants