Skip to content

Remove rtapi_get_clocks()#4082

Draft
hdiethelm wants to merge 2 commits into
LinuxCNC:masterfrom
hdiethelm:rtapi_get_clocks_removal_v2
Draft

Remove rtapi_get_clocks()#4082
hdiethelm wants to merge 2 commits into
LinuxCNC:masterfrom
hdiethelm:rtapi_get_clocks_removal_v2

Conversation

@hdiethelm
Copy link
Copy Markdown
Contributor

@hdiethelm hdiethelm commented May 30, 2026

As discussed in #4029, I think it would make sense to remove rtapi_get_clocks().

An optimized function using rdtsc() and calculating the ration to ns would be possible, but I see no point in over complicating things due to my tests showed next to no impact.

On my setup, I don't see any change before and after this PR. Tested with:
Debian Trixie / i5-6500 / Latest RT kernel

Both latency_test and linuxcnc with my CNC show the exact same CPU load (+-0.1%) and timing behavior.

This PR also fixes a few bugs where the CPU frequency was assumed to be 1GHz, so some timings where applied wrongly.

Additionally, all thread.time values are now in ns and not an arbitrary clock count.

Open points:

  • Test with RTAI: My PC is unstable with RTAI, so I can not test this.
  • Docs: I updated the man page. But I do not really know how to manage all translation files. Can someone help me or show me the tools needed to do that? I can do English / German.
  • Test on different systems
  • Test hal_gpio / hal_parport: I have no parport on my PC. Can someone do this?

This function returns an arbitrary time base depending on CPU type / actual
and frequency is thus not really usable. According to the doc, each core
can have a different tsc value at the same time, thus it can randomly jump
when core locking is not active.

Testing shows, that rtapi_get_time() uses 20ns on
6.12.86+deb13-rt-amd64, so no need to use rdtsc.

5.4.302-rtai-amd64 uses 112ns which is a bit slower but should still be
no issue.

rtapi_get_clocks() needs approx 10ns, so it is faster but the impact
should be negligible.
@hdiethelm hdiethelm force-pushed the rtapi_get_clocks_removal_v2 branch 2 times, most recently from 6c32795 to 4200048 Compare May 30, 2026 12:43
Comment thread src/emc/motion/motion.c
Comment on lines -596 to -598
#ifdef HAVE_CPU_KHZ
CALL_CHECK(hal_pin_float_newf(HAL_OUT, &(emcmot_hal_data->last_period_ns), mot_comp_id, "motion.servo.last-period-ns"));
#endif
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this could be a problem. Or, is nobody using it internally or externally?

Or, if the define HAVE_CPU_KHZ is not generally available, then it it would be a non-consistent pin and can/should be dealt with in some way.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HAVE_CPU_KHZ was only defined with RTAI: https://github.com/LinuxCNC/linuxcnc/blob/master/src/emc/motion/mot_priv.h#L352

I removed it all together. I grep'ed the repo for last_period_ns, no hits except the two I removed. If someone uses it with RTAI in his .hal file, he can change to use the pin last_period which was in clocks and is now in ns.

The change from clocks to ns for the .time / .last_period might confuse some users but I somewhat expect that most users expected these signals to be in ns. But they where in tsc clocks before this PR.

Is there any reference in the doc about this .time pin's? I did not find anything the last time I searched it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (small) catch is that last-period is integer and last-period-ns is floating point.

The pins are (wrongly) documented as parameters in docs/src/config/core-components.adoc in line 173-176.

There may be some history here to this pin/param confusion that may need some investigation. Commit 2b22052 in 2017 changed both from params to pins.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah of course, i grepped for last_period instead of last-period. There are only occurrences in the doc's, nowhere in code or hal files. I fixed the doc for this pin.

However, I am completely new to adoc / po translations. Is there any way to update the .po files in an automated way?

Thanks for the link. I did not even know hal-histogram exists. This commit was mainly about converting motion.servo.last-period from a parameter to a pin. motion.servo.last-period-ns was added but it works only with RTAI. Probably it was added as float, so the division by cpu_khz is straight forward. The other time pins are in s32.

I tested hal-histogram motion.servo.last-period which works. However, hal-histogram is quite slow in updating and doesn't scale nicely. Just to test, I changed the pin to float, same issue.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the right config, it works fine:
grafik

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I am completely new to adoc / po translations. Is there any way to update the .po files in an automated way?

Translations are done in weblate and they are synced once in a while. Just write text in English in the source adoc and they will be marked in weblate that they need attention for translation.

I tested hal-histogram motion.servo.last-period which works. However, hal-histogram is quite slow in updating and doesn't scale nicely. Just to test, I changed the pin to float, same issue.

The slowness of the histogram is how data is taken out of RT into non-RT. It sets an index to read the +/- binnumber[index] and that takes at least 1 thread cycle per bin. So, for +/- 200 bins it takes 0.2 seconds (on a 1kHz thread), just to get the data out (and it is race-tainted data). Actually, the delay is set default at 10 ms and then takes 2 seconds to get the data out.

There is work in progress to fix these histogram programs (none of them work on systems with Tcl9).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so it should be fine then. Just some help needed from others for testing.
@grandixximo fixed already the latency-histogram as I remember. It works perfectly now.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only fixed the UI, @BsAtHome actually did the rework on the Hal component, credit goes to him for the response improvement 😁

@hdiethelm hdiethelm force-pushed the rtapi_get_clocks_removal_v2 branch 3 times, most recently from b9a07ac to a9f0e6e Compare May 30, 2026 21:42
@hdiethelm
Copy link
Copy Markdown
Contributor Author

hdiethelm commented May 30, 2026

^^ Found other clocks -> ns instance.

While grep'ing for .time, I found this:
https://github.com/LinuxCNC/linuxcnc/blob/master/docs/src/drivers/hal_pi_gpio.txt#L77

I guess it should be documented globally that each hal function gets a .time pin and a .tmax, .tmax-increased parameter for timing analysis, not only motion. It is documented in in several docs in different ways.

The code is this one:

if (hal_pin_s32_newf(HAL_OUT, &(new->runtime), comp_id,"%s.time",name)) {

rtapi_snprintf(buf, sizeof(buf), "%s.tmax-increased", name);

rtapi_snprintf(buf, sizeof(buf), "%s.tmax-increased", name);

@hdiethelm hdiethelm force-pushed the rtapi_get_clocks_removal_v2 branch 2 times, most recently from a39056e to 6210d80 Compare May 30, 2026 22:09
@hdiethelm
Copy link
Copy Markdown
Contributor Author

Disregard, I found also this part and corrected it. It is here:
https://linuxcnc.org/docs/html/hal/basic-hal.html 4. HAL Parameter
.tmax-increased was missing, I added it.

@hdiethelm hdiethelm force-pushed the rtapi_get_clocks_removal_v2 branch from 6210d80 to 217cfb8 Compare May 30, 2026 22:18
@hdiethelm
Copy link
Copy Markdown
Contributor Author

I tested RTAI with axis_mm example / latency-test.
With this PR:
latency-test:

  • Jitter approx. +100ns
  • .time values: Increase from 70 to 100ns (Calculated using cpu_khz, the old values are in clocks, the new in ns)

axis-mm:

  • Hard to say anything, lot of jitter with my PC, uspace runs way better. Approx. + 15% on thread.time.

@NTULINUX
Do you have time to test this? Is there a way to see CPU load in RTAI?

In uspace, I see no noticeably impact. On RTAI, it is a bit slower with this PR as expected. The question is if this is acceptable or mitigations are needed.

@NTULINUX
Copy link
Copy Markdown
Contributor

I'm going to try and test this on RTAI later today, sorry for the delay! Hosting a network here so whenever I reboot or switch distros, the network for everyone here goes down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants