Nml cleanup - #4627
Nml cleanup#4627rene-dev wants to merge 6 commits into
Conversation
grandixximo
left a comment
grandixximo
left a comment
There was a problem hiding this comment.
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.
| 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); " |
There was a problem hiding this comment.
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 {})?
| set_rcs_print_flag((long)*dbg); | ||
| } | ||
| // output infinite RCS errors by default | ||
| max_rcs_errors_to_print = -1; |
There was a problem hiding this comment.
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.
| // 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); |
There was a problem hiding this comment.
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?
|
|
||
| #include "config.h" | ||
| #include "libnml/rcs/rcs.hh" // NML classes, nmlErrorFormat() | ||
| #include "rcs_status.hh" // NML classes, nmlErrorFormat() |
There was a problem hiding this comment.
Does this comment still fit? // NML classes, nmlErrorFormat() described the old rcs.hh include; rcs_status.hh only provides the RCS_STATUS enum, no?
| #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. |
There was a problem hiding this comment.
Same question here: // RCS_CMD_CHANNEL, etc. described rcs.hh. What does a reader learn from it on rcs_status.hh?
| enum ANGULAR_UNIT_CONVERSION : int; | ||
|
|
||
| std::optional<RCS_PRINT_DESTINATION_TYPE> mapRcsDestination(const linuxcnc::IniFile &ini, | ||
| const std::string &var, const std::string &sec); |
There was a problem hiding this comment.
With this gone, is the forward declaration of RCS_PRINT_DESTINATION_TYPE at the top of the file still needed?
|
I doubt anyone uses RCS_DEBUG_DEST or RCS_PRINT_DESTINATION_TYPE. google only has 2 results on this string. |
2 participants
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.