petercorke / petercorke/robotics-toolbox-python
fknm/frne C++ extension health: refactor phases, nanobind thread leak, Eigen vendoring, IK typing
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 3.5k
- Forks
- 624
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 53
Description
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 isdef 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. RaisesTypeErrorif 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. ifswiftever 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 throughfkine()/jacob0()/jacobe(), numericaljacob0/jacobe/hessian0/hessianeagainst a reference robot (Puma560), dynamics (rne()via frne/ne against known torque values), and a Pyodide-simulation path (fknm import mocked asImportError). -
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 atry/except BaseException: passthat swallows real bugs. Fix: rename the C extension to_fknm_csoroboticstoolbox/fknm.pycan be pure Python, tryingfrom roboticstoolbox._fknm_c import ...and falling back to pure-Python implementations (already exist, scattered inETS.py, just need consolidating) onImportError. Symbolic detection (_is_symbolic(q)) moves inside each facade function; callers stop needingdtype == 'O'guards. -
Phase 2 --
BaseETSstructural fix + unified fknm/frne lifecycle.BaseETS(UserList)stores its list inUserList.self.databut shadows it with a@propertyredirecting toself._data; thedatasetter doesn't call_copy_to_cpp(), so mutation viaself.data.append(x)bypasses all dirty-tracking hooks. Fix: dropUserList, inheritcollections.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/_frnehandles,_fknm_stale/_frne_staleflags,@_dirties_fknm/@_dirties_frnedecorators). -
Phase 3 -- nanobind port. Port
fknm.cppandfrne.c's CPython glue (notne.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 thebuild_pyodideCI job still passes. -
Phase 3.5 -- per-ET result buffer (post-nanobind perf).
rx/ry/rz/tx/ty/tzeach 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/_ETSObjwhen created from a background thread. Confirmed via direct testing: 2000fkine()/jacob0()calls in a loop on the main thread is clean; the same loop on a backgroundthreading.Threadfor 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 toswift-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 onlyEigen/Core+src/Core/+src/plugins/) after verifying no cross-references into the removed modules. RTB'sfknm.cppalmost 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
Mapobjects, 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__.pyloadsroboticstoolbox.toolsbeforeroboticstoolbox.robot, butp_servo.py(intools/) needsAngle_Axisfromrobot/fknm.py-- a true circular dependency between the two packages. Current workaround: the import is deferred insideangle_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_axisis conceptually a robot pose-error/servoing function, not a generic tool -- move it intorobot/(orrobot/control.py) so the dependency direction becomesrobot → robot. Requires updating call sites and thetools/__init__.py/top-level__init__.pyre-exports. -
IK.pysolvers are typed againstETSbut only need a small FK/Jacobian surface.IKSolver._solve/step/_random_q/_check_jl/_null_Σtakeets: "rtb.ETS"but only ever usen,qlim,jindices,joints(),eval(q),jacob0(q),jacobm(q)-- a 7-item surface.RobotProto.pyalready has this pattern for two other mixins (KinematicsProtocol,RobotProto) viatyping.Protocol. Proposed: add anIKProtocolalongside those, declaring the 7-item surface; change everyets: "rtb.ETS"inIK.pytoets: "IKProtocol"(no runtime change needed,ETSalready satisfies it structurally). Deferred until the/etspackage split (Phase 1 above) is underway -- worth doing together since the import surface is the same shape this protocol would formalize.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
This is an umbrella issue covering fknm.py, ETS.py, IK.py, RobotProto.py, tools/p_servo.py, the C++ extension sources, CMakeLists.txt, and the Pyodide CI job. Start by reading the Phase 0 coverage requirements and the old tech-debt.md history, then choose a separately scoped phase or subtask. Done should be defined by the selected phase's tests and build or CI checks passing.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, python
- Domain
- backend, build-system, testing-qa
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Needs clarification
- Newbie friendliness
- 20/100