posemath: split the header, move EmcPose, stop exporting gomath and sincos - #4411
posemath: split the header, move EmcPose, stop exporting gomath and sincos#4411grandixximo wants to merge 3 commits into
Conversation
|
Wouldn't it be more logical to fold sincos and gomath into the posemath header? It seems that we are including these from the local pool and then link to the .so library. But wouldn't it make more sense to isolate the library by public interface? A few observations:
|
posemath.h declares two libraries in one file: a set of C++ classes with their operators, and the C API that the real time side uses. The tree already has a shape for that, in inifile.h and inifile.hh and in hal.h and hal.hh, where the plain suffix is the C interface and the double one is the C++ interface, so follow it. posemath.h keeps the C API and gains posemath_types.h, which holds the data types and the constants. posemath.hh takes the classes, the operators and the copy templates, and refuses to be compiled as C. posemath.h ends by including posemath.hh under __cplusplus, so C++ code that includes posemath.h still gets the classes and nothing out of tree changes. The two headers include each other and the guards make either order safe. What this buys is for C++ code: a file that wants no more than PmCartesian, which is most of what reaches posemath through emcpos.h, can include posemath_types.h and stop parsing four hundred lines of class declarations. C compilers were never affected either way, since the classes have always sat behind an ifdef. Declaration text is moved verbatim, indentation included. The one edit is PmAxis, which was an unnamed enum in C and a typedef of the C++ enum PM_AXIS in C++; the types come first now, so the enum takes its tag and one definition serves both languages. Preprocessing posemath.h before and after, in C and in C++, gives the same declarations in both languages, differing only in the order they appear and in that PmAxis line. libposemath.so.0 exports the same 390 symbols.
EmcPose is a machine pose made of a PmCartesian and six more doubles, and emcpose.c is arithmetic on it. Neither is an NML message, so neither belongs in nml_intf. The split across two headers was historical as well: emcpos.h held the type and emcpose.h the operations, and code that wanted the type included the one that could not stand alone. Fold both into src/libposemath/emcpose.h and move emcpose.c with it, into libposemath rather than liblinuxcnc, which is the only in-tree caller anyway by way of tpmod. emcpos.h stays where it is and includes the new header, so the hundred and twenty odd files that include it are untouched and out of tree code keeps compiling. The declarations gain an extern "C" block. They had none, so a C++ caller would have compiled and then failed to link; there is no such caller today. libposemath.so.0 gains the fifteen emcPose entry points and liblinuxcnc.a loses them. Nothing in userspace calls them.
The build copies gomath.h, gotypes.h and sincos.h into include/, which makes them part of what LinuxCNC offers to code built against it. Nothing outside libposemath needs them to be: gomath and gotypes are included by two files, both part of genserkins, and sincos.h declares one function, a shim over sin() and cos() that posemath itself calls. Those users include them by path instead, which is what src/-relative quoted includes are for, and the three headers come off the exported list. With the split in place the installed pose math surface is posemath.h and the two headers under it, plus emcpose.h, in one directory. Porting genserkins and scorbot off gomath, so that the N-DOF matrix code can move into posemath and gomath can go, is easier once the symbols are private. scorbot is already off it: scorbot-kins.c includes gotypes.h and uses nothing from it, so that include goes here. gomath.h included rtapi_math.h with quotes, which was right while gomath.h was itself exported and had to carry its siblings to include/. It is a user now, so it takes angle brackets, and it reaches the standard headers through rtapi_math.h rather than around it. gotypes.h wanted float.h for FLT_MAX and DBL_MAX and gomath.h wanted stddef.h for NULL; rtapi_math.h includes float.h and rtapi.h includes stddef.h, in both the kernel and the userspace branch.
7396a9e to
5532d37
Compare
|
The scorbot include and the two rtapi_math ones are in, folded into the third commit. On sincos, before I touch it. On genserkins.h, On folding them into posemath.h: with scorbot's include gone, gomath and gotypes have two includers left, |
|
Two corrections to my own numbers, before you spend any time on them.
And I conceded too much on genserkins.c. It does reach into gotypes.h: lines 48 and 49 |
|
The When RT links with libposemath objects, then it must have been compiled with the RT compiler options. You cannot link libposemath.so to RT. There are generated objects in ( We may want to make a parallel RT version of libposemathRT.a instead if that is necessary. I'm trying to wrap my head around the intertwined way things are built and how to untangle it... |
|
On the RT side I do not think there is anything to untangle: the tree already builds posemath twice, and has for a long time. Every source named in a module's Nothing realtime links the The A On On untangling, generallyYou said you were trying to wrap your head around how the tree is built and how to untangle it. So did I, and guessing was not working, so I measured it. Below is a directory level dependency report for The short version is that it is a lot less bad than it looks. There is exactly one cycle, it has 17 directories in it, and 18 of its 68 edges are a single The script resolves every Summary
The emc core of the cyclegraph LR
n0["emc/ini"]
n1["emc/kinematics"]
n2["emc/motion"]
n3["emc/nml_intf"]
n4["emc/pythonplugin"]
n5["emc/rs274ngc"]
n6["emc/tooldata"]
n7["emc/tp"]
n0 -->|1| n2
n0 -->|17| n3
n1 -->|9| n2
n1 -->|4| n3
n2 -->|1| n0
n2 -->|3| n1
n2 -->|3| n3
n2 -->|7| n7
n3 -->|3| n2
n3 -->|4| n5
n3 -->|1| n6
n4 -->|1| n0
n5 -->|2| n0
n5 -->|1| n2
n5 -->|18| n3
n5 -->|5| n4
n5 -->|2| n6
n6 -->|4| n3
n7 -->|11| n2
n7 -->|4| n3
Cycle edges that are one includeEach of these is a single line. Cutting it removes a directory-level dependency outright.
The heavier knotsCycle edges carrying two or more includes
Exported headers with no in-tree user outside their own directoryA header reached only from its own directory is a candidate for coming off
Full directory edge list
What it says about this PRTwo of the edges this PR touches are in that cycle, and the report puts numbers on them:
The single include edges look like the cheapest place to start, whatever we do about the rest. |
|
Very nice work. That script is a nice to have :-) A few things: Secondly, much more difficult, is the fact that includes (can) span userspace and RT space. This is good visible in what motion and TP include and what others include from them. Several includes do not differentiate between what is needed to build (local inter-file defines, types and prototypes) and what is needed to interface to other code. That line has been a vague one, which is not exposed as well by the graph, besides it noting the inclusion. It is here that the "untangling" becomes more involved. That said, your analysis does show where to look. Thirdly, the uspace_rtapi_main.cc use of hal.h is completely redundant. There are no references to its content whatsoever. This brings us to the point where includes are there just "because" and are not necessary at all. The include's content is not always a good well-designed separator for exposed interfaces, touching my second point. Therefore, some includes may actually be removed and would already trim down the tree a bit. Others will need some redesign. |
2 participants
C, D and E of the plan agreed in #4387. Each commit stands alone.
Split the header by language. posemath.h declared the C++ classes and the C API in one file. The tree already has a shape for that, inifile.h/.hh and hal.h/.hh, so follow it: posemath.h keeps the C API and gains posemath_types.h for the types and constants, posemath.hh takes the classes, operators and copy templates and refuses to compile as C. posemath.h ends by including posemath.hh under __cplusplus, so C++ code including posemath.h still gets the classes and nothing out of tree changes. The gain is for C++ files wanting no more than PmCartesian, which can include posemath_types.h instead of four hundred lines of class declarations.
Declaration text moves verbatim. The one edit is PmAxis, an unnamed enum in C and a typedef of the C++ enum PM_AXIS in C++; the types come first now, so the enum takes its tag and one definition serves both.
Move EmcPose. emcpos.h held the type and emcpose.h the operations, and code wanting the type included the one that could not stand alone. Neither is an NML message. Both fold into src/libposemath/emcpose.h and emcpose.c moves with them. emcpos.h stays and includes the new header, so the 126 files that include it are untouched. The declarations gain an extern "C" block they never had.
Stop exporting gomath.h, gotypes.h and sincos.h. Five in-tree users between them, all kinematics, which include them by path instead. What remains installed is posemath.h plus its two headers, plus emcpose.h.
Testing. Build clean, no errors or warnings. runtests over blendmath, realtime-math, interp, ccomp and matrixkins: 91 run, 91 successful, 0 failed. All ten kinematics modules that link a posemath object load under halrun. Preprocessing posemath.h before and after, in C and in C++, gives the same declarations apart from that PmAxis line. libposemath.so.0 gains the fifteen emcPose entry points and liblinuxcnc.a loses them, and nothing in userspace calls them. Installed headers go from 38 to 37.
tpcomp.comp is filtered out of COMPS so nothing in tree compiles it, and it references emcpose.c by path; checked separately by setting TOPDIR and running halcompile on a copy.