Skip to content

fknm/frne C++ extension health: refactor phases, nanobind thread leak, Eigen vendoring, IK typing #572

Description

@petercorke

Migrated from tech-debt.md (deleted, see repo history via git log -- tech-debt.md). Groups everything about the health/maintenance of the compiled fknm/frne C++ extensions and the packages/typing around them.

  • fknm.r2q() is broken and unused. roboticstoolbox.ets.fknm.r2q()'s Python facade is def r2q(R): return _c_r2q(R) (one argument), but the real nanobind binding requires two -- r2q(r_in, q_out), writing into a caller-supplied output array. Raises TypeError if called. Grepped every call site in this repo: nothing calls it. Not fixed since it has zero callers -- fix only if/when something starts calling it (e.g. if swift ever wants it for faster quaternion conversion; that was investigated and deliberately not pursued for unrelated correctness-risk reasons).

  • ETS/fknm/frne refactor, Phase 0 -- test coverage first. Prerequisite for all phases below. eval()/fkine() with and without fknm (mock the C import to force the Python fallback), symbolic (SymPy) inputs through fkine()/jacob0()/jacobe(), numerical jacob0/jacobe/hessian0/hessiane against a reference robot (Puma560), dynamics (rne() via frne/ne against known torque values), and a Pyodide-simulation path (fknm import mocked as ImportError).

  • Phase 1 -- facade module. from roboticstoolbox.fknm import ETS_fkine, ... is currently a hard import of the .so; if unavailable the module fails to load, and the fallback is a try/except BaseException: pass that swallows real bugs. Fix: rename the C extension to _fknm_c so roboticstoolbox/fknm.py can be pure Python, trying from roboticstoolbox._fknm_c import ... and falling back to pure-Python implementations (already exist, scattered in ETS.py, just need consolidating) on ImportError. Symbolic detection (_is_symbolic(q)) moves inside each facade function; callers stop needing dtype == 'O' guards.

  • Phase 2 -- BaseETS structural fix + unified fknm/frne lifecycle. BaseETS(UserList) stores its list in UserList.self.data but shadows it with a @property redirecting to self._data; the data setter doesn't call _copy_to_cpp(), so mutation via self.data.append(x) bypasses all dirty-tracking hooks. Fix: drop UserList, inherit collections.abc.MutableSequence, implement the five required abstract methods decorated with @_dirties_fknm. Pairs with unifying the fknm/frne lifecycle pattern (lazy rebuild: dirty flag set on mutation, _copy_to_cpp() called only when a C function is about to run) across both extensions -- see the parallel-structure table in the old tech-debt.md entry for the exact naming convention (_fknm/_frne handles, _fknm_stale/_frne_stale flags, @_dirties_fknm/@_dirties_frne decorators).

  • Phase 3 -- nanobind port. Port fknm.cpp and frne.c's CPython glue (not ne.c's pure-C maths) to nanobind -- same performance, less raw-CPython-API boilerplate, safer refcounting, better Emscripten/Pyodide support. Both already build via the existing CMakeLists.txt/scikit-build-core pipeline. Verify the build_pyodide CI job still passes.

  • Phase 3.5 -- per-ET result buffer (post-nanobind perf). rx/ry/rz/tx/ty/tz each write all 16 elements of the output matrix every call even though only 1-4 actually change between evaluations. Once each joint ET owns its own result buffer (Phase 2), each op function can overwrite only the elements that depend on eta -- eliminates ~12-15 unnecessary zero-stores per ET per FK/Jacobian/Hessian call.

  • _fknm_c (nanobind) leaks _ETObj/_ETSObj when created from a background thread. Confirmed via direct testing: 2000 fkine()/jacob0() calls in a loop on the main thread is clean; the same loop on a background threading.Thread for as little as 0.5s leaks every time (nanobind's own refleak detector fires at interpreter shutdown). Not simply "many calls" -- reproduces regardless of call volume once creation happens off the main thread. Directly relevant to swift-sim, whose render/step loop runs on background threads. Not root-caused: leading hypothesis is a reference cycle between the Python ETS wrapper and the C++ object that only cyclic GC breaks, and a non-main-thread-created object's cycle isn't collected before thread teardown the way a main-thread one is. Needs checking against nanobind's refleak guide (https://nanobind.readthedocs.io/en/latest/refleaks.html). Also unconfirmed: whether this is purely a cosmetic shutdown-time diagnostic or a real growing-memory leak in a long-lived Swift session.

  • Trim vendored Eigen (3.4.0, Aug 2021) before any version bump. src/roboticstoolbox/ets/cpp-extensions/Eigen/ vendors Eigen 3.4.0 in full (337 files); latest is 5.0.0. spatialgeometry's Coal migration hit the same situation and trimmed 337→175 files (kept only Eigen/Core + src/Core/ + src/plugins/) after verifying no cross-references into the removed modules. RTB's fknm.cpp almost certainly uses more of Eigen (FK/Jacobian/Hessian/IK all live here) -- audit #includes and transitive deps before trimming, don't assume the same file list applies.

  • Eigen version bump 3.4.0 → 5.0.0. Real risk: Eigen 5 tightens const-correctness on Map objects, modernizes its CMake, and makes some previously-tolerated internal-header inclusions a hard error. Needs its own dedicated pass with the full fknm test suite (Phase 0 above) as the regression net. Do the trim (above) first -- makes this diff much smaller to review.

  • tools/p_servo.py -- cross-package import papered over with a lazy import. roboticstoolbox/__init__.py loads roboticstoolbox.tools before roboticstoolbox.robot, but p_servo.py (in tools/) needs Angle_Axis from robot/fknm.py -- a true circular dependency between the two packages. Current workaround: the import is deferred inside angle_axis()'s function body rather than module scope, which works but makes the dependency invisible at a glance (a future edit that hoists it back to module scope silently re-breaks the import chain -- has recurred once already). Proper fix: p_servo/angle_axis is conceptually a robot pose-error/servoing function, not a generic tool -- move it into robot/ (or robot/control.py) so the dependency direction becomes robot → robot. Requires updating call sites and the tools/__init__.py/top-level __init__.py re-exports.

  • IK.py solvers are typed against ETS but only need a small FK/Jacobian surface. IKSolver._solve/step/_random_q/_check_jl/_null_Σ take ets: "rtb.ETS" but only ever use n, qlim, jindices, joints(), eval(q), jacob0(q), jacobm(q) -- a 7-item surface. RobotProto.py already has this pattern for two other mixins (KinematicsProtocol, RobotProto) via typing.Protocol. Proposed: add an IKProtocol alongside those, declaring the 7-item surface; change every ets: "rtb.ETS" in IK.py to ets: "IKProtocol" (no runtime change needed, ETS already satisfies it structurally). Deferred until the /ets package split (Phase 1 above) is underway -- worth doing together since the import surface is the same shape this protocol would formalize.

Metadata

Metadata

Assignees

No one assigned

    Labels

    tech-debtKnown technical debt / deferred cleanup, not a live bug

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions