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