Nml cleanup by rene-dev · Pull Request #4627 · LinuxCNC/linuxcnc

rene-dev · 2026-10-04T02:04:07Z

this is the first step of removing NML: getting rid of dependencies, where it really isn't needed.
This should not affect any user facing things, so it can go straight in.

grandixximo

Overall this looks good to me. The dependency reductions build cleanly across the CI matrix, the new libnml defaults (STDOUT, PRINT_RCS_ERRORS) match the removed setup, and I checked the etime() wall-to-monotonic switch against every call site: only linuxcncrsh TIME and the Tcl emc_time report a wall timestamp to a client, and both correctly use wall_etime(); everything else measures local intervals. The emcOperatorErrorNoEcho() replacement for the RCS_PRINT_TO_NULL dance in emcMotionUpdate() is a nice cleanup.

One spot outside this diff: linuxcnc_check_ini.py still validates [EMC]RCS_DEBUG_DEST (line 178 and lines 212-214). Since nothing reads that variable anymore, should the check be dropped so the validator does not keep blessing a setting that does nothing?

Inline are mostly questions. The only real bug I found is the %g in initraj.cc.

grandixximo · 2026-10-04T02:45:45Z

    if (planner_type == 1 && jerk < 1.0) {
-        rcs_print_error("[TRAJ]PLANNER_TYPE = 1 (S-curve) requires "
+        fmt::print(stderr, "[TRAJ]PLANNER_TYPE = 1 (S-curve) requires "
                        "[TRAJ]MAX_LINEAR_JERK >= 1.0 (got %g); "


This format string kept its printf %g, but the call is now fmt::print, where % is not a specifier. Won't this print the literal text (got %g) and silently drop the jerk argument? Should it be (got {})?

grandixximo · 2026-10-04T02:45:45Z

-        set_rcs_print_flag((long)*dbg);
-    }
-    // output infinite RCS errors by default
-    max_rcs_errors_to_print = -1;


Dropping this leaves libnml's built-in default of 30 in every client process (same in the emcsvr, milltask and halui copies of this block). So after 30 RCS errors, further errors stop being reported, where before the default was unbounded. Is that cap deliberate? If yes, could the commit message say so? It is a user visible change, while the PR description says there are none.

grandixximo · 2026-10-04T02:45:45Z

-    // enable all debug messages by default if RCS or NML debugging is enabled
-    if ((emc_debug & EMC_DEBUG_RCS) || (emc_debug & EMC_DEBUG_NML)) {
-        // output all RCS debug messages
-        set_rcs_print_flag(PRINT_EVERYTHING);


With this hook (and -rcsdebug in emcGetArgs) gone, is there any way left to switch on libnml's rcs_print_debug() output? EMC_DEBUG_RCS and EMC_DEBUG_NML are still documented in debugflags.h but no longer reach PRINT_EVERYTHING anywhere. If NML debugging is intentionally retired, should those debugflags.h entries go with it?

grandixximo · 2026-10-04T02:45:45Z


 #include "config.h"
-#include "libnml/rcs/rcs.hh"		// NML classes, nmlErrorFormat()
+#include "rcs_status.hh"		// NML classes, nmlErrorFormat()


Does this comment still fit? // NML classes, nmlErrorFormat() described the old rcs.hh include; rcs_status.hh only provides the RCS_STATUS enum, no?

grandixximo · 2026-10-04T02:45:45Z

-#include "libnml/rcs/rcs.hh"		// RCS_CMD_CHANNEL, etc.
-#include "libnml/rcs/rcs_print.hh"
-#include "libnml/os_intf/timer.hh"             // esleep, etc.
+#include "rcs_status.hh"		// RCS_CMD_CHANNEL, etc.


Same question here: // RCS_CMD_CHANNEL, etc. described rcs.hh. What does a reader learn from it on rcs_status.hh?

grandixximo · 2026-10-04T02:45:45Z

 enum ANGULAR_UNIT_CONVERSION : int;

-std::optional<RCS_PRINT_DESTINATION_TYPE> mapRcsDestination(const linuxcnc::IniFile &ini,
-	const std::string &var, const std::string &sec);


With this gone, is the forward declaration of RCS_PRINT_DESTINATION_TYPE at the top of the file still needed?

rene-dev · 2026-10-04T17:17:28Z

I doubt anyone uses RCS_DEBUG_DEST or RCS_PRINT_DESTINATION_TYPE. google only has 2 results on this string.

rmu75 · 2026-10-05T08:05:43Z

I somewhat don't like replacing rcs_print_... directly with fmt::print because it looses "debug-level" information. At some point we might introduce a standard logging facility like spdlog and then it would be nice to have that information still available.

grandixximo · 2026-10-05T08:32:01Z

I doubt anyone uses RCS_DEBUG_DEST or RCS_PRINT_DESTINATION_TYPE. google only has 2 results on this string.

The Google count does not settle it either way; ini files live on private machines, so usage barely shows up in a web index. But if usage really is that close to zero, isn't that all the more reason to finish the removal: drop the dead validation in linuxcnc_check_ini.py (lines 178 and 212-214) and the forward declaration in mapini.hh? Independently of that, the one thing that does need fixing is the %g in initraj.cc.

I somewhat don't like replacing rcs_print_... directly with fmt::print because it looses "debug-level" information. At some point we might introduce a standard logging facility like spdlog and then it would be nice to have that information still available.

I agree the severity is worth keeping, and there is a cheap way to keep it with fmt: route the prints through small named wrappers instead of calling fmt::print directly, for example a header-only shim in the style of this PR's strutil.hh/timeutil.hh:

template <typename... T>
void log_error(fmt::format_string<T...> f, T&&... args) {
    fmt::vprint(stderr, f, fmt::make_format_args(args...));
}

template <typename... T>
void log_info(fmt::format_string<T...> f, T&&... args) {
    fmt::vprint(f, fmt::make_format_args(args...));
}

Call sites then read log_error(...) / log_info(...), so severity stays explicit at every call site, and if spdlog ever arrives only these two functions change. A debug-gated variant (log_debug(EMC_DEBUG_..., ...)) would also answer my inline question about EMC_DEBUG_RCS/EMC_DEBUG_NML becoming unreachable, since the gate would live in one place again.

BsAtHome · 2026-10-05T11:01:30Z

I somewhat don't like replacing rcs_print_...

I agree. We should instead introduce a proper channelized logging facility where we can use runtime config to enable/disable classes and individual messages.

rmu75 · 2026-10-05T12:58:56Z

I agree. We should instead introduce a proper channelized logging facility where we can use runtime config to enable/disable classes and individual messages.

#4638

…rplist

Nothing in LinuxCNC reaches any of these; linking every libnml user with --gc-sections discards all of it. - cms/cmssvrp.cc, os_intf/inetnull.cc, os_intf/inetfile.hh: never built. - os_intf/timer.cc: RCS_TIMER, replaced by linuxcnc::CyclicTimer. timer.hh keeps the etime()/esleep() declarations libnml still uses. - cms/cms_pm.cc: CMS::update() for the POSEMATH types. The only one any message uses is PM_CARTESIAN (EMC_TRAJ_CIRCULAR_MOVE), which moves to cms.cc. - nml/nmldiag.cc, nml/nmldiag.hh: NML_DIAGNOSTICS_INFO::print() and nml_print_diag_list(), reachable only through the equally unused NML::get_diagnostics_info(), which goes too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Nothing in the tree sends any of these: - EMC_TOOL_HALT had no handler either. - EMC_TOOL_UNLOAD, EMC_TRAJ_ABORT, EMC_TRAJ_PAUSE and EMC_TRAJ_RESUME were handled by task, but no GUI, halui, linuxcncrsh or the Python and Tcl bindings ever sends them. GUIs pause and resume through EMC_TASK_PLAN_PAUSE/RESUME and abort through EMC_TASK_ABORT. emcTrajPause(), emcTrajResume() and emcTrajAbort() stay, task calls them itself. emcToolUnload() was only reachable through EMC_TOOL_UNLOAD and goes with it. An external program speaking NML directly that sends one of these will now get "ignoring issue of unknown command". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

BsAtHome · 2026-10-06T12:33:53Z

This should have been discussed more thoroughly before being merged.

The logging question has not been answered adequately IMO.

rene-dev · 2026-10-06T12:58:56Z

logging is now in its own file, and can be dealt with in a single place, and debug flags have been restored.
only thing lost is the redirection, which I doubt was ever used for anything.
https://github.com/LinuxCNC/linuxcnc/blob/master/src/emc/logutil.hh

BsAtHome · 2026-10-06T13:14:39Z

That isn't the issue. You merged it without consensus about the direction it needs to go. There were open questions here about logging. There are multiple things in logging that need to be fixed. You changed it single handed into this version and just merged it. That is the real problem, technical issues aside.

rene-dev · 2026-10-06T13:25:43Z

which question is still open? I addressed the things that were mentioned. I asked on emc-users who used redirection, and why. google results in nothing. forum results in nothing.
there is also a discussion on which lib to use, and how. #4638
for sure we dont want to use rcs for logging.

snowgoer540 · 2026-10-07T00:16:53Z

That isn't the issue. You merged it without consensus about the direction it needs to go. There were open questions here about logging. There are multiple things in logging that need to be fixed. You changed it single handed into this version and just merged it. That is the real problem, technical issues aside.

This has been my experience with @rene-dev throughout my involvement in the project. Rene tends to make big changes, with seemingly little discussion, usually dropped out of nowhere, and then move on and let everyone else fix the associated issues.

Also, there seems to be a bunch of AI written code getting merged into the project, but this is the first time I've seen "and Claude" in our commit history. Is the project OK with this? This project is not perfect, but it's been slop free until recently. Are we sure that all of these commits are getting reviewed properly, by actual human beings, or are we just accepting the AI generated stuff as "good enough"? The rate of change here has been staggering lately. It certainly has me nervous.

One of these commits has caused this warning to show in my terminal:

* * * * * * * * * * - - - - - WARNING - - - - - * * * * * * * * * *
*    CMS_DISPLAY_ASCII_UPDATER may not function properly due      *
*    to range limitations of some of the update() functions.      *
* * * * * * * * * * - - - - - WARNING - - - - - * * * * * * * * * *

I can bisect if we really need to know the specific commit, but I did check out the last commit before @rene-dev merged this, and the warning is not there.

rmu75 · 2026-10-07T06:05:20Z

Also, there seems to be a bunch of AI written code getting merged into the project, but this is the first time I've seen "and Claude" in our commit history. Is the project OK with this? This project is not perfect, but it's been slop free until recently. Are we sure that all of these commits are getting reviewed properly, by actual human beings, or are we just accepting the AI generated stuff as "good enough"? The rate of change here has been staggering lately. It certainly has me nervous.

There is a whole bunch of AI generated / assisted stuff going on in linuxcnc recently. Consensus seems to be that everything has to be human reviewed and as far as I have looked the AI stuff indeed looks correct. It's like a very powerful reactoring tool.

The warning you see was introduced while bugfixing around NML/CMS and has nothing to do with AI. It just says that 64bit values can't be properly "serialized" by the CMS DISPLAY_ASCII transport, and that is not fixable without breaking the output format. I wonder how you triggered that warning...

rene-dev added 3 commits October 4, 2026 03:48

replace libnml timing functions with std::chrono

f5c8ef9

remove unused libnml linkage from, motionlogger

9dc6ac3

remove dependency on libnml string handling

b42a8cb

grandixximo reviewed Oct 4, 2026

View reviewed changes

rene-dev added 5 commits October 6, 2026 12:20

replace rcs_print with libfmt, remove nml dependency from interplist

1ea510d

fix cppcheck

b57499c

fixup! replace rcs_print with libfmt, remove nml dependency from inte…

0869c90

…rplist

fixup! replace rcs_print with libfmt, remove nml dependency from inte…

7b22f37

…rplist

fixup! fix cppcheck

8401265

rene-dev force-pushed the nml-cleanup2 branch from 3785a6b to 8401265 Compare October 6, 2026 10:21

rene-dev and others added 2 commits October 6, 2026 13:40

rene-dev merged commit 7f67ef2 into LinuxCNC:master Oct 6, 2026
17 checks passed

Conversation

rene-dev commented Oct 4, 2026

Uh oh!

grandixximo left a comment

Choose a reason for hiding this comment

Uh oh!

grandixximo Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

grandixximo Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

grandixximo Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

grandixximo Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

grandixximo Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

grandixximo Oct 4, 2026

Choose a reason for hiding this comment

Uh oh!

rene-dev commented Oct 4, 2026

Uh oh!

rmu75 commented Oct 5, 2026

Uh oh!

grandixximo commented Oct 5, 2026

Uh oh!

BsAtHome commented Oct 5, 2026

Uh oh!

rmu75 commented Oct 5, 2026

Uh oh!

Uh oh!

BsAtHome commented Oct 6, 2026

Uh oh!

rene-dev commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BsAtHome commented Oct 6, 2026

Uh oh!

rene-dev commented Oct 6, 2026

Uh oh!

snowgoer540 commented Oct 7, 2026

Uh oh!

rmu75 commented Oct 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rene-dev commented Oct 6, 2026 •

edited

Loading