machinekit-hal
machinekit-hal copied to clipboard
machinekit
commit e7f5d5f02bee10c46b44d58869d50987279e1a37 "ignore fp" is incomplete
On May 20, Michael Haberler changed hal_lib.c so that it would allow a HAL function that uses floating point to be added to a thread that does not support floating point.
The commit message indicates that since machinekit no longer uses kthreads, all threads actually support floating point regardless of whether HAL thinks so or not.
The commit does NOT change the fact that both threads and functs can be defined in HAL as either FP or no-FP. Nor does it change the fact that "halcmd show thread" displays whether or not a thread supports floating point, and "halcmd show funct" displays whether or not a function requires floating point. The only thing the commit does is allow the previously illegal act of adding an FP function to a non-FP thread.
This is very confusing and looks like a bug to anyone who reads the documentation and expects things to work they way they are described.
There are better ways to implement this change:
The minimalist approach would be to change hal_lib to eliminate the ability to create a non-fp thread. Simply modify hal_create_thread() such that if "uses_fp" is zero, it would print a "non-fp threads are deprecated" warning, then make "uses_fp" non-zero and continues on. All threads would be FP capable (and would be displayed as such in "halcmd show thread"), and any function could be added to any thread.
The "newthread" command should either have its "fp" and "nofp" options completely removed, or the "nofp" option should generate either a warning or error. The documentation for this command (currently completely missing, see issue 1021) should be updated accordingly.
The "threads" component should either have its "fp" argument completely removed, or "fp1=0" should generate either a warning or error. The documentation for this component should be updated accordingly. NOTE: the man page for "threads" ( http://www.machinekit.io/docs/man/man9/threads/ ) is inconsistent. The "fp1=
If floating-point vs non-floating-point is truly irrelevant, then a proper fix would be to completely eliminate the distinction. That means removing "uses_fp" from both hal_create_thread() AND hal_export_funct(), removing the "uses_fp" field from all thread and function data structures, and no longer displaying anything about floating point in the "halcmd show thread" and "halcmd show funct" commands.
I am willing to make either change - simple (all threads must be FP capable) or complex (remove all signs of FP vs non-FP from threads and functions). However the community will have to decide which approach is correct, and whether attempts to create a non-FP thread should emit an warning and continue on (for backwards compatibility) or generate an error.
Hi John,
newthread predated the end of kernel threads which is why it still has the 'fp' and 'nofp' options.
As you say, the fp property for RTAPI and hence HAL threads is meaningless and ignored for userland threads. halcmd could be changed to reflect this by issuing a deprecation comment that this flag is unnecessary; using it is not an error. The fp/nofp fields from halcmd output can go.
Since newthread is a superset of the 'loadrt threads' method, the best course of action is to document the newthread method in the halcmd manpage, and remove the threads component and its manpage altogether as there is no reason left to keep it; only 8 configs use it and the change is trivial.
There should be only one way to do things, and as far as I'm concerned that is newthread.
Since there are no functional consequences in using or not using fp or nonfp, IMO an API change is not warranted as would be failing newthread ... fp/nofp . Fixing up the documentation makes sense.
I'll post a PR later.
agreed 100% that there should be only one way to create a thread, and newthread is it.
Not comfortable that it is still possible to create a non-fp thread, and possible to add an fp function to a non-fp thread. Fails the KISS principle - if there is no difference between fp and non-fp, why does HAL allow creation of both kinds of threads and functions, and why are there struct members dedicated to keeping track of that? Future readers of the code are going to say WTF?
Agreed that passing "fp" or "nofp" to newthread should only print a warning, not fail.
But I think the API change is warranted. Neither functs nor threads should have a "uses_fp" attribute.
My thought is this - the code should be internally consistent. Either 1) or 2)
-
threads and functs have a "uses_fp" attribute. "Show" commands display the attribute. "addf" commands respect it (no fp functs in non-fp threads). But newthread ignores the option and always creates an fp thread (at least on platforms where all threads are by nature FP capable).
-
there is no "uses_fp" attribute anywhere - neither threads nor functs. All vestiges of it are removed to avoid confusing future maintainers. All threads are assumed to be fp capable, and functions have no need to declare that they use fp.
The main argument I see in favor of approach #1 is backward compatibility of "script-mode" output - I noticed that the commit was complicated by the desire to keep that compatibility.
A second argument in favor of approach #1 is that there may someday be a platform where it is indeed beneficial to have non-fp threads. But retaining the uses_fp attribute is only helpful if it is displayed by show and respected by addf. Having the "uses_fp" infrastructure in place but broken seems like a bad thing.
Approach #2 is definitely a bigger change, I'd be willing to contribute to that.