Repository navigation
hal: Break the HAL API - Move to 64-bit exclusively - #4565
Conversation
|
Forgot to note: Any hm2_modbus communication mbccb files need to be recompiled from the mbccs source with mesambccc. The file version was increased because of the HAL type changes. |
grandixximo
left a comment
grandixximo
left a comment
There was a problem hiding this comment.
CI is green and I went through the diff by area; the things I would have expected to break silently (use_fp callers, pdir sets, the by-value port interface, removed symbols, save/show round trip, the halui pin set, motion locals, MesaMB02, python UINT above 2^63, the GUI compat warnings) all check out.
Three things in halmodule.cc are pre-existing and not from this PR, so I will send a separate PR unless you prefer to fold them in: from_python(PyObject*, bool*) line 150 does *b = 0.0 == v, so a float 1.0 into a bool pin writes False; halenum_rt_members[] at 2167 has no {} terminator and halenum_build("RTType", ...) reads past it; the PyErr_Format at 893 has three conversions and two arguments.
Yes, I was amazed myself how few problems I encountered. All the python code is rather transparent to the change. The Tcl code may still present problems because the default internal representation in 8.6 is still 32-bit. It changed to 64-bit in Tcl 9. Luckily, most values are rather small. There are still a lot of
Oops...
This is serious and may give spurious fails on memory layout changes on different compiles.
I thought that there was a printf attribute on the function. Obviously not. But, yes, these can be fixed in a separate PR (asap). There are no conflicts in that code |
b174ba7 to
4f58393
Compare
4f58393 to
a9982e2
Compare
grandixximo
left a comment
grandixximo
left a comment
There was a problem hiding this comment.
lib/hallib/hal_procs_lib.tcl:147
get_netlist switches on the type column of hal show signal and only knows float bit u32 s32; anything else hits return -code error "get_netlist: Unexpected <$line>". halcmd now prints bool real sint uint, so hookup_moveoff.tcl (every configs/sim/axis/moveoff/*.ini) dies at load. Add the four new names, or turn the switch around and let anything that is not ==>, <==, <=> set sname?
tcl/bin/halconfig.tcl:752
Same class as the halshow fix you already made ($type == "bit" || $type == "bool"): here and at 986 halconfig still tests for "bit" only, so bool params and pins fall into the text branch instead of the LED. Worth the same two-word patch, or is halconfig on its way out?
Second pass on a9982e2: all eleven points from the first round are in. I merged the head onto current master locally (that brings in #4568 and #4517, both touching files this PR touches): clean merge, warning-free build, runtests 318 run, 317 pass, 3 skipped, the one failure (ui-smoke/qtdragon-quit) is the load flake and passes rerun alone. The get_netlist item is the only thing I found that breaks a shipped config (verified: master OK, this head fails on both signals); the rest of the pass (hal.h/hal_lib.c/hal_priv.h, the conv template bounds, hm2_modbus and mesambccc, halui/motion/inihal NATIVE mapping, the component locals that went 64-bit, stepgen timers, python and qtvcp shims, halcmd/halrmt/halsh/halscope) reads consistent.
a9982e2 to
e32c8a0
Compare
Added the new types alongside.
Also fixed. This Tcl code will eventually be replaced and retired (I hope). Then it should no longer matter ;-) |
6acc773 to
2d310a1
Compare
2d310a1 to
ff27543
Compare
|
@andypugh The breaking change is in. You should update the VERSION file to pre2, tag the current HEAD appropriately and push both the file change and tags. |
|
@andypugh just a second reminder to set VERSION file to pre2, linuxcnc-ethercat CI is broken without a hook to differentiate before and after the API break, thanks. |
Warnings and notes still said a type "will be removed at the HAL API break". LinuxCNC#4565 is in, so say what is true now: some of these are gone, the rest are deprecated.
Warnings and notes still said a type "will be removed at the HAL API break". LinuxCNC#4565 is in, so say what is true now: some of these are gone, the rest are deprecated.
Warnings and notes still said a type "will be removed at the HAL API break". LinuxCNC#4565 is in, so say what is true now: some of these are gone, the rest are deprecated.
* 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.
This is the long awaited HAL API break. The main update is that all 32-bit HAL pins/params/signals are gone and everything is now 64-bit with backing storage access completely opaque with the previous getter/setter update.
This PR does not include the docs. The PR is already large enough without having to go filter the docs while reviewing. The docs will follow in the next PR and is also quite a large patch set.
If you wonder about the HAL type name change... It was done to make a clean break. It is a clear sign that "from this point on we have a new HAL interface".
What changed:
HAL_{S32,U32}have been removed. All places where they were used have been updated to use the new abstractHAL_{SINT,UINT}.HAL_{BIT,FLOAT,S64,U64}have been renamed toHAL_{BOOL,REAL,SINT,UINT}. Using any old types will result in a deprecation compiler warning.hal_data_u,hal_{bit,float,s32,u32,s64,u64}_t,real_tandireal_thave been removed. There is no longer direct access to the underlying data. The new HAL types arehal_{bool,real,sint,uint,port}_trequire you to use the getter/setter interface.hal_pin_dir_tandhal_param_dir_thave been unified intohal_pdir_t. The code has been updated and using the old types will result in a deprecation warning. They must be removed at a (slightly) later stage.HAL_PORTinterface has been changed to use the same interface as the other new HAL types to make the API as consistent as possible.hal_pin_XXX_new()andhal_param_XXX_new()have been removed. These were already replaced byhal_pin_new_YYYY()andhal_param_new_YYYY()hal_param_set*()and the value retrieval functionshal_get_{pin,param.signal}_value_by_name()have been removed. The query API has already replaced these in the previous updates to HAL.use_fpflag in functions and threads has been removed. It was already deprecated and fixed to true. The RTAPI has been updated as well.hal.hheader now has a version defineHAL_API_VERSIONand is set to1. It is now possible to track further changes in the API by inspecting this define to make user's code adapt more easily to future changes.tests/halmodule/comp-set-getandtests/mux). If it is required to test wrapping, then the tests may be altered at a later stage.abs_s32andabs_s64have been folded intoabs_sintconv_XXXX_YYYYhave all been removed and replaced by the new name versions{bool,real,sint,uint}.demux_genericandmux_generichave been updated to handle any-to-any conversion for the new types. The pins that included the old names were also renamed.scaled_s32_sumsgot renamed toscaled_sint_sums.si32andui32pins/params upgraded tosintanduint. However, there are still many components that need to be made 64-bit clean. This is a larger effort that will follow after integration of the PR. The[su]i32types do create the correct[su]inttyped pins and access through the getter/setter interface. The type marker is present as a reminder that they need updating. However, you cannot change thehal_[gs]et_[su]i32()getters/setters without properly inspecting the code where it is used. Every use needs to be inspected and traced to prevent intermediate variable truncation problems.s32oru32name is used in variables or pin/param/signal names. Not all can be changed easily while others may be replaced. Renaming must be a process that can be done after integrating these changes.halcmdandhalrmtnow issue a warning when the old HAL types are used. Printing/output data all uses the new types.halshowwill properly show the new typeshal.Type.XXXXandhal.Dir.YYY. All code has been updated to use the new types.HAL_XXXnames will result in a deprecation warning. The old 32-bit versions will map to the new types.pyvcpnow uses tagssint,uint,image_boolandimage_uint. The old tags{s32,u32,image_u32,image_bit}are still recognized for compatibility. All in-tree xml files have been updated. The old names are still recognized but will emit a deprecation warning (only once for each type).pyuipanelui now recognize{BOOL,REAL,SINT,UINT}as output type names. All in-tree examples have been updated. The old names are still recognized but will emit a deprecation warning (only once for each type).The changes have been successfully tested on a RPi5+Mesa-7I96S+Modbus.
If you can, you should pull this branch (https://github.com/BsAtHome/linuxcnc/tree/hal64_break-hal-api-nodocs) from my repo and test on your machine/hardware and report back. Not that problems are expected, but testing this is necessary.