Rtapi cleanup

[hdiethelm]

Still WIP, i have to review and refine the changes if there are fine for the admin's.

Target: Move some classes out of the huge uspace_rtapi_app.cc

uspace_rtapi_main: Contains the main function and helpers
uspace_rtapi_app: Contains the RtapiApp class
uspace_posix: Contains the PosixApp class

Other fixes:

• Don't start master just for exit: 371793c
• Remove unused rtapi_task::ratio: 3097578
  • Set but only read to force a fixed ratio which probably was not the intention
• Slightly different lock to avoid needing reference to instance: c8af827

All real time implementations are now library's and handled the same way.

Sadly, rename tracking broke due to the amount of moved code, so it is a crap to review.
The following commands make it easier:

git difftool  master:./uspace_rtapi_app.cc rtapi_cleanup:./uspace_rtapi_app.cc
git difftool  master:./uspace_rtapi_app.cc rtapi_cleanup:./uspace_rtapi_main.cc
git difftool  master:./uspace_rtapi_app.cc rtapi_cleanup:./uspace_posix.cc

Do you think this is fine? So I will finalize it.

Advantage:

• You can now easily compare the different RT implementations
• Less code in one file

Disadvantage:

• A lot of code to review
• Merges from 2.9 could be hard

[BsAtHome]

Just a quick glance over this PR...

Where did the memory alloc+touch+locking go? Some of that code does not make sense only at first glance, but has an important function in conjunction with how pages are mapped in the process.

You have moved code and afaics without attribution (didn't look in all details). Regardless, you need to have the GPL header in place too.

[hdiethelm]

Just a quick glance over this PR...

Where did the memory alloc+touch+locking go? Some of that code does not make sense only at first glance, but has an important function in conjunction with how pages are mapped in the process.

That was here: https://github.com/LinuxCNC/linuxcnc/blob/master/src/rtapi/uspace_rtapi_app.cc#L814
And is now here: https://github.com/hdiethelm/linuxcnc-fork/blob/rtapi_cleanup/src/rtapi/uspace_rtapi_main.cc#L713

I tried to change nothing there.

The only change was this to avoid having an instance pointer: c8af827
It now locks the mutex before the thread is started instead in the thread, this should make no difference I think. I don't know if we could just remove it, it is only active in non-rt mode.

It was added here but I don't understand the description, so I left it (nearly) as it was: cbbd8e5

You have moved code and afaics without attribution (didn't look in all details). Regardless, you need to have the GPL header in place too.

Sure, I will do that.

[hdiethelm]

So, the lost GPL headers are restored and I added them where missing by tracking the author/date in git. I hope that is fine to.

[BsAtHome]

Where did the memory alloc+touch+locking go?

That was here: https://github.com/LinuxCNC/linuxcnc/blob/master/src/rtapi/uspace_rtapi_app.cc#L814 And is now here: https://github.com/hdiethelm/linuxcnc-fork/blob/rtapi_cleanup/src/rtapi/uspace_rtapi_main.cc#L713
I tried to change nothing there.

Right, didn't see that. Good.

If you please also get rid of the snprintf(..."%d", num) call(s) and incude <fmt/format.h> and replace them with fmt::format("{}", num). That will return you a std::string straight out that can be added. No need for static buffers of limited space.

I'm not sure I would have done using namespace std and would normally write it straight out. However, this comes from the original code, so that may be fine.

While you are at it, you moved quite a bit of code to a different file (without renaming so git doesn't track that as nicely; no prob). You might also want to run it through clang-format. Now that all lines are changes anyway... Better make that indent thingy consistent too (see also #2760 as a starting point). There may still be some things afterwards you'd want to reformat manually, but it does give a better starting point.

The only change was this to avoid having an instance pointer: c8af827 It now locks the mutex before the thread is started instead in the thread, this should make no difference I think. I don't know if we could just remove it, it is only active in non-rt mode.

This I need to evaluate in more detail. I'll get back to you on that one...

It was added here but I don't understand the description, so I left it (nearly) as it was: cbbd8e5

Well, you are also falling into the "it was added I don't know why and don't understand" trap. That seems to be a more regular feeling than I'd like it to be. ;-)

You have moved code and afaics without attribution (didn't look in all details). Regardless, you need to have the GPL header in place too.

Sure, I will do that.

Great!

[hdiethelm]

If you please also get rid of the snprintf(..."%d", num) call(s) and incude <fmt/format.h> and replace them with fmt::format("{}", num). That will return you a std::string straight out that can be added. No need for static buffers of limited space.

Done, I replaced also another snprintf.

I'm not sure I would have done using namespace std and would normally write it straight out. However, this comes from the original code, so that may be fine.

Sure, done.

I will look into clang-format. That random tab/space mix annoys me anyway... ;-)

Well, you are also falling into the "it was added I don't know why and don't understand" trap. That seems to be a more regular feeling than I'd like it to be. ;-)

That's why I tried to not change anything I don't understand. The locking in PosixApp seam to have the following target:
Only one thread can run at a time: pthread_mutex_lock() at end of wait(): Only one thread can exit wait(). So it behaves similar like SCHED_FIFO with a different priority for each thread.
If that was the target, moving the pthread_mutex_lock() shouldn't make a difference.
However, with SCHED_FIFO, a higher priority thread can interrupt a lower priority thread.

[hdiethelm]

So, clang-format of the moved files and the thread implementations where it is nice to be able to compare them.
I changed:

• AllowShortFunctionsOnASingleLine: Inline -> None due to I find this inline functions hard to read
  • I can change that, it is a matter of taste, don't really care as long as there is a common format
• Manually changed back a few breaks where it was hard to read
  • Might be increase ColumnLimit from 80 to 120 or so?

Not formatted but small changes in this MR:

• src/rtapi/rtapi_pci.cc
• src/rtapi/uspace_common.h
• src/rtapi/uspace_rtapi_parport.cc

I can do them also if desired.

[BsAtHome]

AllowShortFunctionsOnASingleLine: Inline -> None due to I find this inline functions hard to read
I can change that, it is a matter of taste, don't really care as long as there is a common format
Manually changed back a few breaks where it was hard to read

Readability is always a priority. When it bothers (you), then it is wrong and you fix it :-)

Might be increase ColumnLimit from 80 to 120 or so?

Please do.

[I'm using a 132 column terminal ever since I can remember from the old VT320 (132x24) and later 132x43 on the PC terminal emulation. And now using 20+ terminal windows of that size as a default and gvim also starts at that size.]

Not formatted but small changes in this MR:
* src/rtapi/rtapi_pci.cc
* src/rtapi/uspace_common.h
* src/rtapi/uspace_rtapi_parport.cc
I can do them also if desired.

Only is you already changed about the whole file. Otherwise we get a discontinuity in the blame diffs.

[hdiethelm]

So, changes from original clang-format file:

• AllowShortFunctionsOnASingleLine: Inline -> AllowShortFunctionsOnASingleLine: None
• #AllowShortLambdasOnASingleLine: Empty -> AllowShortLambdasOnASingleLine: Empty
• ColumnLimit: 80 -> ColumnLimit: 120

Looks nicer now and I had to do only a few manual changes which are now not changed back anymore by format.

[hdiethelm]

And a funny rebase due to changes on master in the files I moved. It's always the same hassle, when refactoring or formatting, you break git blame and generate merge conflicts but you have to do it from time to time if you don't want to be stuck with hard to manage code.

I checked it using:

FILES=$(git diff --name-only master rtapi_cleanup)

git diff 804fdc83c19d master $FILES
diff --git a/src/rtapi/uspace_rtai.cc b/src/rtapi/uspace_rtai.cc
index ff5f9d784e..dd6549e844 100644
--- a/src/rtapi/uspace_rtai.cc
+++ b/src/rtapi/uspace_rtai.cc
@@ -92,7 +92,8 @@ struct RtaiApp : RtapiApp {
         task->rt_task = rt_task_init_schmod(task->id, task->prio, 0, 0, SCHED_FIFO, cpus_allowed);
         rt_set_periodic_mode();
         start_rt_timer(nano2count(task->period));
-        if(task->uses_fp) rt_task_use_fpu(task->rt_task, 1);
+        /* uses_fp is deprecated and ignored; always save FPU state */
+        rt_task_use_fpu(task->rt_task, 1);
         rt_make_hard_real_time();
         rt_task_make_periodic_relative_ns(task->rt_task, task->period, task->period);
         (task->taskcode) (task->arg);
diff --git a/src/rtapi/uspace_rtapi_app.cc b/src/rtapi/uspace_rtapi_app.cc
index 30a159dc98..64fc5b32a8 100644
--- a/src/rtapi/uspace_rtapi_app.cc
+++ b/src/rtapi/uspace_rtapi_app.cc
@@ -974,7 +974,8 @@ int RtapiApp::task_new(void (*taskcode) (void*), void *arg,
   if(stacksize < (1024*1024)) stacksize = (1024*1024);
   task->id = n;
   task->owner = owner;
-  task->uses_fp = uses_fp;
+  /* uses_fp is deprecated and ignored; always save FPU state */
+  task->uses_fp = 1;
   task->arg = arg;
   task->stacksize = stacksize;
   task->taskcode = taskcode;

git diff rtapi_cleanup_before_rebase rtapi_cleanup $FILES
diff --git a/src/rtapi/uspace_rtai.cc b/src/rtapi/uspace_rtai.cc
index 982de62437..14218b382b 100644
--- a/src/rtapi/uspace_rtai.cc
+++ b/src/rtapi/uspace_rtai.cc
@@ -110,8 +110,8 @@ struct RtaiApp : RtapiApp {
         task->rt_task = rt_task_init_schmod(task->id, task->prio, 0, 0, SCHED_FIFO, cpus_allowed);
         rt_set_periodic_mode();
         start_rt_timer(nano2count(task->period));
-        if (task->uses_fp)
-            rt_task_use_fpu(task->rt_task, 1);
+        /* uses_fp is deprecated and ignored; always save FPU state */
+        rt_task_use_fpu(task->rt_task, 1);
         rt_make_hard_real_time();
         rt_task_make_periodic_relative_ns(task->rt_task, task->period, task->period);
         (task->taskcode)(task->arg);
diff --git a/src/rtapi/uspace_rtapi_app.cc b/src/rtapi/uspace_rtapi_app.cc
index 83720520e0..68be9a1b49 100644
--- a/src/rtapi/uspace_rtapi_app.cc
+++ b/src/rtapi/uspace_rtapi_app.cc
@@ -136,7 +136,8 @@ int RtapiApp::task_new(
         stacksize = (1024 * 1024);
     task->id = n;
     task->owner = owner;
-    task->uses_fp = uses_fp;
+    /* uses_fp is deprecated and ignored; always save FPU state */
+    task->uses_fp = 1;
     task->arg = arg;
     task->stacksize = stacksize;
     task->taskcode = taskcode;

[BsAtHome]

And a funny rebase due to changes on master in the files I moved. It's always the same hassle, when refactoring or formatting, you break git blame and generate merge conflicts but you have to do it from time to time if you don't want to be stuck with hard to manage code.

Always the same problem. Using diff -b helps to keep track of some of it.

I checked it using:
[snip]

The next thing you might find is the warning about unused vars/args #3910.

[hdiethelm]

The next thing you might find is the warning about unused vars/args #3910.

Would be nice to merge mine first... ;-) It's ready from my side.

[hdiethelm]

From my side, I reviewed the changes and the only might be functional changes are the once mentioned in the PR.

[hdiethelm]

Ok, rebased again, now i have the notes how to do it without to much effort...

[hdiethelm]

Wouldn't it make sense to activate -Werror so you can not build with warnings?

[andypugh]

For context the "UBC3" referenced in cbbd8e5 was "Unified Build Candidate 3" which was a refactoring of some of the massive amounts of work that mhaberler did, which did not get merged, and led to the schism that became Machinekit.
The aim there was to build on all platforms. which we could do for a while, until jepler broke it by wrongly assumimg that RTAI was unusable.

[hdiethelm]

For context the "UBC3" referenced in cbbd8e5 was "Unified Build Candidate 3" which was a refactoring of some of the massive amounts of work that mhaberler did, which did not get merged, and led to the schism that became Machinekit. The aim there was to build on all platforms. which we could do for a while, until jepler broke it by wrongly assumimg that RTAI was unusable.

Thanks for the info! What do you mean by "all platforms"? Machinekit seams to be dead now, no new commit since 4 years.

[BsAtHome]

Wouldn't it make sense to activate -Werror so you can not build with warnings?

I was looking into that. I created some infrastructure for that move quite some time ago (like cleaning up all the warnings). Not sure we should enable it on the distro builds. At least the gcc and clang rip-and-test targets should run with -Werror.

[hdiethelm]

Wouldn't it make sense to activate -Werror so you can not build with warnings?

I was looking into that. I created some infrastructure for that move quite some time ago (like cleaning up all the warnings). Not sure we should enable it on the distro builds. At least the gcc and clang rip-and-test targets should run with -Werror.

Looking at: https://github.com/LinuxCNC/linuxcnc/actions/runs/24134037596/job/70417430040?pr=3908 I did not find -Werror and I think #3910 would not have been necessary if that had worked.

I think just activate it for all and track back if there are too many issues, like a with configure --no-werror for specific builds.

Also the build pipelines are really slow due to a ton of packages are installed. Wouldn't it make more sense to create bookworm-linuxcnc-2.9 / bookworm-linuxcnc-master ... containers in the linuxcnc docker registry with all the dependency's preinstalled and update them only like once a week? I could do that easily in gitlab but sadly, I don't know github that well. I found:

https://docs.github.com/en/packages/working-with-a-github-packages-registry/working-with-the-container-registry

[BsAtHome]

As I said, I already made the infrastructure. The following is all that is needed to add it to rip-and-test and
rip-and-test-clang:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 15b68b8dee..7c75fe4326 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -35,7 +35,7 @@ jobs:
         sudo eatmydata apt --yes --quiet -o Acquire::Retries=5 upgrade
         cd src
         eatmydata ./autogen.sh
-        eatmydata ./configure --disable-check-runtime-deps
+        eatmydata ./configure --disable-check-runtime-deps --enable-werror
         eatmydata make -O -j$((1+$(nproc))) default pycheck V=1
         # Note that the package build covers html docs
         eatmydata ../scripts/rip-environment runtests -p
@@ -61,7 +61,7 @@ jobs:
         sudo eatmydata apt --yes --quiet -o Acquire::Retries=5 upgrade
         cd src
         eatmydata ./autogen.sh
-        CC=clang CXX=clang++ eatmydata ./configure --disable-check-runtime-deps
+        CC=clang CXX=clang++ eatmydata ./configure --disable-check-runtime-deps --enable-werror
         eatmydata make -O -j$((1+$(nproc))) default pycheck V=1
         # Note that the package build covers html docs
         eatmydata ../scripts/rip-environment runtests -p

[hdiethelm]

As I said, I already made the infrastructure. The following is all that is needed to add it to rip-and-test and rip-and-test-clang:

Ah just made but not yet activated. Thanks for the info!

[andypugh]

What do you mean by "all platforms"?

It was a long time ago, and I was pretty much just a normal user at the time, but I think we ran separate branches for preempt-rt and rtai.

[BsAtHome]

In the Submakefile it reads:

$(call TOOBJSDEPS, $(USPACE_POSIX_SRCS)): EXTRAFLAGS += -pthread -fPIC

Shouldn't that be (not adding to EXTRAFLAGS):

$(call TOOBJSDEPS, $(USPACE_POSIX_SRCS)): EXTRAFLAGS = -pthread -fPIC

[rmu75]

it seems this introduces a new shared library libuspace-posix.so.0 which seems odd? I really don't like the name. And #3915

Something like librtapi-uspace-posix or even liblinuxcnc-rtapi-uspace-posix would be more appropriate IMHO.

[NTULINUX]

I don't want to split hairs here but librtapi-uspace-posix seems better and not too long. The issue is that if you specify --libdir=/usr/lib64 on ./configure the install breaks, and if you don't use that, portage from Gentoo fails to install because the libdir path is wrong. You then need to specify FEATURES="-multilib-strict" to turn off the check every time you want to install it, which is just a quick hack to bypass the error.

[BsAtHome]

The issue is that if you specify --libdir=/usr/lib64 on ./configure the install breaks, and if you don't use that, portage from Gentoo fails to install because the libdir path is wrong.

Doesn't this happen for all .so libraries? There are also libnml.so, linuxcncini.so, linuxcnchal.so... and so on.

[NTULINUX]

The issue is that if you specify --libdir=/usr/lib64 on ./configure the install breaks, and if you don't use that, portage from Gentoo fails to install because the libdir path is wrong.

Doesn't this happen for all .so libraries? There are also libnml.so, linuxcncini.so, linuxcnchal.so... and so on.

Those work with --libdir=/usr/lib64, no lingering files in /usr/lib.

I think the issue is:

src/rtapi/uspace_rtapi_main.cc:        app = makeDllApp(EMC2_HOME "/lib/libuspace-posix.so.0", SCHED_OTHER);
src/rtapi/uspace_rtapi_main.cc:            app = makeDllApp(EMC2_HOME "/lib/libuspace-xenomai-evl.so.0", SCHED_FIFO);
src/rtapi/uspace_rtapi_main.cc:            app = makeDllApp(EMC2_HOME "/lib/libuspace-xenomai.so.0", SCHED_FIFO);
src/rtapi/uspace_rtapi_main.cc:            app = makeDllApp(EMC2_HOME "/lib/libuspace-rtai.so.0", SCHED_FIFO);
src/rtapi/uspace_rtapi_main.cc:            app = makeDllApp(EMC2_HOME "/lib/libuspace-posix.so.0", SCHED_FIFO);

Yup! If I change everything to app = makeDllApp(EMC2_HOME "/lib64 it works.

[hdiethelm]

Sorry, I forgot to mention: 62e5ea6
I changed posix to be a library like the other implementations. So that means probably also all the other variants already had the same issue.
Is someone of you fixing it and creating an MR or should I do it?

[hdiethelm]

So, I have some time to look into it. The reason behind 62e5ea6 is that posix and all the other real time implementations are equal in structure and usage which makes maintaining easier.
However, I forgot to test if linuxcnc still works after being installed. Sorry for this.
I silently assumed that if I just use the existing code from other real time implementations, it will work. But they probably all have the same issue.

[hdiethelm]

Something like librtapi-uspace-posix or even liblinuxcnc-rtapi-uspace-posix would be more appropriate IMHO.

I used the existing naming scheme. But yes, it's not that nice. Files in the lib folder:

liblinuxcnchal.so.0
liblinuxcncini.so.0
libnml.so.0
libposemath.so.0
libpyplugin.so.0
librs274.so.0
libtooldata.so.0
libuspace-posix.so.0
libuspace-rtai.so.0
libuspace-xenomai-evl.so.0
libuspace-xenomai.so.0

Any sugestions? I can change it, no big deal.

[rmu75]

I was a bit harsh and premature with my comment given that all those other libraries also pollute /usr/lib, apologies.

It would make sense to prefix everything with linuxcnc or lcnc or name it like liblinuxcnc-rtapi-posix or something like that. uspace doesn't give that much information.

libnml and libposemath could be a real problem because there is also the "real" (slightly incompatible) libnml from NIST, I guess nobody is using that alongside linuxcnc...

[hdiethelm]

In the Submakefile it reads:

$(call TOOBJSDEPS, $(USPACE_POSIX_SRCS)): EXTRAFLAGS += -pthread -fPIC

Shouldn't that be (not adding to EXTRAFLAGS):

$(call TOOBJSDEPS, $(USPACE_POSIX_SRCS)): EXTRAFLAGS = -pthread -fPIC

Hmm, I don't understand makefiles in depth. I just copied what was already there a few lines below and edited it to match my lib. As much as I understand this, it just adds this flags to the global EXTRAFLAGS in Makefile for one compile command only. I did not see any duplicated flags.