Repository navigation
Conversation
schaubh
changed the title
- Replace tilde-vector products with cross products - Use fixed-size and bounded Eigen storage for up to six DOFs - Reuse the mass-matrix inverse and common kinematic terms - Use state and derivative views to avoid temporary allocations - Extend conservation tests to four, five, and six DOFs Computational speed of effector evaluation (by itself, not full simulation) increased 5-7x. Validation: 19 tests pass, with baseline agreement across 120 states.
schaubh
left a comment
schaubh
left a comment
There was a problem hiding this comment.
Reviewed commit 5836da1e2c2fc97b165e3913151f1161a81aa4b0 against the current effector lifecycle and coding guidance.
The computational implementation now follows the recent performance practices: cross products, fixed-size/bounded Eigen storage, borrowed state views, and reuse of the mass-matrix inverse and common terms. The lifecycle, ownership, configuration-validation, and output issues in the inline comments should be addressed before approval. Smaller follow-ups cover message isWritten() checks, Doxygen, physical units, reset/optional-input documentation, and regression coverage.
Validation performed:
- 19 relevant tests passed (17 effector configurations and two canonical-state-proxy regressions).
- 300 independent Newton–Euler/Jacobian comparisons passed with varied axes, joint orderings, states, and hub motion; maximum scaled acceleration discrepancy was approximately
2.1e-11. - A commanded translational force/reference case matched its analytical acceleration.
- Effector reset preserved the integrated state, and spacecraft reset restored the configured initial state.
- Focused probes reproduced the validation, topology, shared-DOF, stale-output, attitude-output, and singular-matrix findings. These probes were temporary review checks; the corresponding regression tests still need to be added to the repository.
This review contains eight findings and six smaller follow-up comments.
| void GeneralSingleBodyStateEffector::registerStates(DynParamManager& states) | ||
| { | ||
| this->betaState = states.registerState(this->numDOF, 1, this->nameOfBetaState); | ||
| this->betaDotState = states.registerState(this->numDOF, 1, this->nameOfBetaDotState); |
There was a problem hiding this comment.
[P1] Validate configuration before registering states.
The configuration checks are reachable only through the scheduled Reset(). An effector attached with addStateEffector() but omitted from AddModelToTask() bypasses them, and spacecraft initialization can evaluate its dynamics before the effector's scheduled reset. I reproduced attached-only initialization with a zero axis producing non-finite hub derivatives, whereas direct Reset() rejects that axis. Put the checks in a shared validator called here before registration and from Reset(), following the initialization contract in docs/source/Learn/makingModules/writingEffectors.rst.
There was a problem hiding this comment.
Addressed in 59d0187575, pushed to this PR branch.
Both registerStates() and Reset() now call the shared validateConfiguration() helper. Registration validates before creating or initializing the effector states, so attached-only effectors no longer bypass the checks. Effector reset preserves the live integrated states and cached kinematics.
Regression coverage includes attached-only initialization, both scheduled reset orders, and repeated effector/spacecraft resets. All 117 relevant tests passed locally before the commit.
| void GeneralSingleBodyStateEffector::addRotDOF(std::shared_ptr<DOF> newDOF) | ||
| { | ||
| if (!newDOF) { | ||
| bskLogger.bskError("A null DOF object was given. Provide a valid DOF object for each degree of freedom."); | ||
| } | ||
| if (this->isDOFAdded(newDOF)) { |
There was a problem hiding this comment.
[P1] Enforce the topology lock before modifying DOFs.
addRotDOF(), addTransDOF(), and clearDOFs() do not call requireMutableTopology(), even though Spacecraft freezes the effector after initialization. Both adding and clearing were accepted after InitializeSimulation(); the next spacecraft reset then failed with a state topology/shape mismatch. Continuing dynamics without resetting can also index beyond registered storage, while clearing deletes messages that readers may still reference. Guard the public operations before changing counters, DOF types, collections, or messages. Keep destructor cleanup separate from the guarded public clear operation so destruction remains safe after topology is frozen.
There was a problem hiding this comment.
Addressed in 46ed275815, pushed to this PR branch.
addRotDOF(), addTransDOF(), and clearDOFs() now call requireMutableTopology() before changing counters, DOF metadata, collections, or message ports. Destructor cleanup uses a separate private helper so destruction remains safe after topology is frozen.
Regression tests cover all three rejected operations after attached-only initialization and both scheduled reset orders, preservation of existing ports and continued integration, and destruction of frozen effectors. All 145 relevant tests passed locally before committing.
| void GeneralSingleBodyStateEffector::Reset(uint64_t currentSimNanos [[maybe_unused]]) | ||
| { | ||
| if (!eigenIsValidInertiaMatrix(this->IPntGc_G)) { | ||
| bskLogger.bskError("GeneralSingleBodyStateEffector: effector inertia tensor is invalid."); | ||
| } | ||
| if (!eigenIsRotationMatrix(this->dcm_G0B)) { | ||
| bskLogger.bskError("GeneralSingleBodyStateEffector: effector hub-relative attitude DCM is invalid."); | ||
| } | ||
| for (const auto& dof : this->jointDOFList) { |
There was a problem hiding this comment.
[P1] Validate mass and finite configuration values.
The reset validator checks only inertia, orientation, and axis norms. It currently accepts zero/negative/NaN mass, non-finite initial states and offsets, and invalid spring/damping coefficients. A one-translation effector with zero mass passes Reset() and produces NaN hub derivatives during initialization. Validate supported physical bounds and finiteness for mass, geometry, initial states/rates, and spring/damping coefficients, and reject an empty DOF configuration with a parameter-specific BasiliskError. Apply these checks through the shared initialization validator.
There was a problem hiding this comment.
Addressed in 59d0187575, pushed to this PR branch.
The shared validator now checks finite positive mass, the inertia tensor and mounting DCM, finite unit axes, finite position offsets and initial states/rates, nonnegative finite spring/damping coefficients, and a nonempty DOF configuration. Invalid configuration raises a parameter-specific BasiliskError before state registration as well as on effector reset.
The new tests exercise invalid bounds and NaN/infinite inputs through direct reset and attached-only initialization. All 117 relevant tests passed locally.
| void GeneralSingleBodyStateEffector::addDOF(std::shared_ptr<DOF> newDOF) | ||
| { | ||
| this->numDOF++; | ||
| newDOF->index = this->numDOF - 1; | ||
| this->jointDOFList.push_back(newDOF); |
There was a problem hiding this comment.
[P1] Prevent sharing a mutable DOF between effectors.
isDOFAdded() checks only the receiving effector, so a DOF already owned by another effector is accepted. The second attachment overwrites the shared type and index, and both owners later overwrite its runtime state. I reproduced a translational DOF in the first effector becoming rotational when added to a second effector, leaving the first with no corresponding rotational message port. Enforce exclusive ownership, or provide an explicit independent-copy mechanism, before modifying or storing the DOF.
There was a problem hiding this comment.
Addressed in 46ed275815, pushed to this PR branch.
Each DOF now tracks its exclusive owner. Both add methods reject an already-owned DOF before modifying its type or index, and DOF copying/assignment is disabled. Clearing or destroying an effector releases ownership. Release also clears cached motor force/torque and position/rate references while preserving configuration, so a reused DOF cannot inherit commands from its previous owner.
Regression tests cover duplicate and cross-effector attachment, rejection without changing the original owner, ownership release on clear/destruction, and six reuse cases with previous motor or reference commands across both DOF types. All 145 relevant tests passed locally.
| void GeneralSingleBodyStateEffector::writeOutputStateMessages(uint64_t currentSimNanos) | ||
| { | ||
| // Write the config log message if it is linked | ||
| if (this->generalSingleBodyConfigLogOutMsg.isLinked()) { | ||
| SCStatesMsgPayload configLogMsg = this->generalSingleBodyConfigLogOutMsg.zeroMsgPayload; | ||
| eigenVector3d2CArray(this->r_GcN_N, configLogMsg.r_BN_N); | ||
| eigenVector3d2CArray(this->v_GcN_N, configLogMsg.v_BN_N); | ||
| eigenMRPd2CArray(this->sigma_GN, configLogMsg.sigma_BN); | ||
| eigenVector3d2CArray(this->omega_GN_G, configLogMsg.omega_BN_B); |
There was a problem hiding this comment.
[P2] Refresh inertial states in the parent output callback.
This method publishes cached inertial values that are computed only by the effector's UpdateState(). Spacecraft calls this callback during reset and after integration, including for attached-only effectors. I reproduced an all-zero position after initialization and throughout an attached-only simulation; after a spacecraft reset it published the previous run's position. Refresh the inertial kinematics here before writing the configuration message so the parent callback produces current output independently of task scheduling.
There was a problem hiding this comment.
Addressed in dd58657ec2, pushed to this PR branch.
writeOutputStateMessages() now refreshes the inertial configuration immediately before publishing it. The refresh is no longer confined to the scheduled UpdateState(), so parent callbacks publish current values at initialization, after integration, and after spacecraft reset, including for attached-only effectors. The calculation runs only when the configuration message has a subscriber.
Added 24 regression cases checking position, velocity, attitude, and angular rate for translation-only and mixed joints across these lifecycle paths, both scheduling orders, and hub attitude-reference overrides. All 169 relevant tests passed locally, and the full documentation build passed with warnings treated as errors.
| /** Setter for `axis_G` property */ | ||
| void setDOFAxis(Eigen::Vector3d axis_G) { this->axis_G = axis_G; }; | ||
| /** Setter for `betaInit` property */ | ||
| void setBetaInit(double betaInit) { this->betaInit = betaInit; }; | ||
| /** Setter for `betaDotInit` property */ | ||
| void setBetaDotInit(double betaDotInit) { this->betaDotInit = betaDotInit; }; |
There was a problem hiding this comment.
Follow-up: complete the public Doxygen API comments.
The new public setters/getters and lifecycle methods mostly have one-line descriptions without parameter or return documentation. Add valid Doxygen @brief, @param, and @return sections as applicable, with declaration-matching parameter names, units, frames, and supported bounds. For example, document that axis_G is a unit vector in the joint frame and that betaInit/betaDotInit use rotational or translational units according to the DOF type. Use Doxygen syntax in C++ comments rather than reStructuredText parameter fields.
There was a problem hiding this comment.
Addressed in df5763a472, pushed to this PR branch.
Public setters, getters, and lifecycle callbacks now have Doxygen parameter/return documentation as applicable, including units, reference frames, supported configuration bounds, and reset/input behavior. Parameter names match the declarations. Checked the generated Doxygen XML and rendered HTML for all 35 documented public methods; the standalone header compilation and full Sphinx HTML build with -W passed.
| double betaInit{}; //!< [-] Initial DOF state | ||
| double betaDotInit{}; //!< [-] Initial DOF ratc | ||
| double k{}; //!< [N * m / rad] DOF spring constant | ||
| double c{}; //!< [N * m * s / rad] DOF damping constant |
There was a problem hiding this comment.
Follow-up: correct the mode-dependent units and spelling.
The DOF state and rate are not dimensionless: they are [rad]/[rad/s] for rotation and [m]/[m/s] for translation. Likewise, document both spring units ([N*m/rad] or [N/m]) and damping units ([N*m*s/rad] or [N*s/m]). Correct ratc to rate. The translation test helper also labels its spring constant as [N m/rad] at lines 225 and 229; those comments should be [N/m]. The velocity member v_GcN_N should be documented as [m/s] rather than [m].
There was a problem hiding this comment.
Addressed in df5763a472, pushed to this PR branch.
Corrected the rotational/translational state, rate, reference, spring, and damping units; the translational test helper now uses [N/m], v_GcN_N uses [m/s], and ratc is corrected to rate. Also corrected the mixed-unit kinematics/backsubstitution annotations and labeled the normalized conservation plots as dimensionless.
The unit annotations were checked against the equations and rendered API. All 205 relevant tests and the full Sphinx HTML build with -W passed locally.
| general_body.setMass(20.0) | ||
| general_body.setIPntGc_G([[50.0, 0.0, 0.0], | ||
| [0.0, 80.0, 0.0], | ||
| [0.0, 0.0, 60.0]]) |
There was a problem hiding this comment.
Follow-up: add physical units to the example values.
Add adjacent unit comments to the mass, inertia, offsets, initial displacement, and initial rate in the user-guide examples. For dense arrays, one unit comment on the argument or variable is sufficient. The unit test also needs [m^3/s^2] on earth.mu at line 96. These are newly introduced engineering values, so they should follow the repository's numeric-literal unit convention.
There was a problem hiding this comment.
Addressed in df5763a472, pushed to this PR branch.
Added adjacent unit comments to the user-guide mass, inertia, offsets, initial displacement/rate, DCM, and axis values, plus [m^3/s^2] on earth.mu. Added the missing NumPy import and clarified optional inputs and scheduling, including the spacecraft's default priority of -1.
All six user-guide code blocks were executed together in a simulation and initialized/integrated successfully. The full Sphinx HTML build passed with warnings treated as errors (-W).
| User Guide | ||
| ---------- | ||
| This section outlines how to set up the general effector in python using Basilisk. |
There was a problem hiding this comment.
Follow-up: document reset and optional-input behavior.
Add the module assumptions/limitations and lifecycle contract: supported DOF counts, axis/physical bounds, coordinate singularities, optional inputs and their zero defaults, and which operations require task scheduling. Explicitly document that effector Reset() preserves the live integrated states while spacecraft reset restores the configured initial values through registration; I verified both behaviors. Also state whether scheduled reset retains or clears cached motor commands and reference values. The current implementation retains them, so that policy should be deliberate and tested.
There was a problem hiding this comment.
Pushed 59d0187575 to address the configuration and integrated-state reset portions of this follow-up.
The module documentation now describes configuration bounds, coordinate singularities, validation for attached-only effectors, and the distinction between effector reset (preserves live integrated states) and spacecraft reset (restores configured initial values). Repeated resets and both scheduled reset orders are covered by regression tests.
The optional-input defaults, input scheduling, and cached command/reference reset policy remain for the planned input-handling group; this broader follow-up is only partially addressed by this commit.
There was a problem hiding this comment.
Pushed 46ed275815 with the topology and ownership lifecycle documentation: configure the DOF sequence before initialization; additions and clearing are rejected once topology is frozen; clearing before initialization deletes the message ports; and clearing/destruction releases DOFs for reuse while discarding cached commands and preserving configuration.
This commit also fixes the short heading underline that caused both Canary and docs CI failures. The full local Sphinx HTML build passed with warnings treated as errors (-W).
The remaining optional-input defaults, input scheduling, and cached command/reference behavior across scheduled reset are still assigned to the planned input-handling group, so this broader follow-up remains partially addressed.
There was a problem hiding this comment.
Pushed dd58657ec2 with the output lifecycle documentation. It now states that the spacecraft publishes fresh configuration and joint outputs after initialization, integration, and spacecraft reset even when the effector is not separately scheduled, and that inertial outputs use the accepted hub attitude/angular rate, including final attitude-reference overrides.
The full local Sphinx HTML build passed with warnings treated as errors (-W). Optional-input defaults, input scheduling, and cached command/reference behavior across scheduled reset remain assigned to the planned input-handling group; this broader follow-up remains partially addressed.
There was a problem hiding this comment.
Pushed 4a6594e704 to address the remaining optional-input and cached-command reset portions of this follow-up.
The module documentation now states that inputs are optional and cached commands/references start at zero; only linked, written inputs replace them; and disconnection or relinking to an unwritten producer preserves them. It explicitly documents that both effector and spacecraft resets retain cached motor commands and position/rate references. Effector reset preserves live joint states, while spacecraft reset restores configured initial states.
The documentation also explains that input consumption requires scheduling the effector, gives the producer/effector/spacecraft execution order for commands to affect the same spacecraft update, and states that inputs are read in UpdateState(), not during initialization or reset. Regression tests verify repeated resets in both orders with producers disconnected, preventing message rereads from masking accidental cache clearing. All 205 relevant tests and the full Sphinx HTML build with -W passed locally.
| This unit test sets up a spacecraft with a general rigid body attached to a rigid hub. | ||
| Earth's gravity is included in the simulation. The test configures one through six degrees of freedom for the | ||
| general body. Rotation and translation combinations cover one through three degrees of freedom, with mixed cases | ||
| exercising four, five, and six degrees of freedom. Conservation of energy and angular momentum verifies the dynamics. |
There was a problem hiding this comment.
Follow-up: add lifecycle, validation, and message regression coverage.
The present parameterized cases verify conservative dynamics, but they do not cover the failures identified above. Add tests for attached-only initialization, scheduled initialization in both reset orders, invalid/non-finite configuration, repeated effector and spacecraft resets, rejected topology changes, DOF ownership, and fresh inertial output. Cover motor force/torque, references, unwritten inputs, and damping with appropriate analytical/work/energy expectations. Also update the docstring at lines 77–79: it currently describes nonconservative tests that are not present, and the verification helper always checks rotational energy conservation.
There was a problem hiding this comment.
Pushed 59d0187575 with 98 new regression cases covering configuration validation, attached-only initialization, both scheduled reset orders, repeated resets, singular configurations and motion, large angles, and mixed inertia scales. The mixed-scale tests also check motor force and torque responses against analytical results. The combined relevant suite passed all 117 cases locally.
This addresses the validation/reset portion of this follow-up. Topology and ownership tests, fresh inertial outputs, references/unwritten inputs, damping coverage, and the existing conservation-test docstring correction remain for the subsequent commit groups.
There was a problem hiding this comment.
Pushed 46ed275815 with 28 additional regression cases covering topology locking, exclusive DOF ownership, rejected additions, clearing/destruction, and reuse of previously commanded DOFs. The combined relevant suite now passes all 145 cases locally, including the existing conservation and canonical state-proxy tests.
This addresses the topology and ownership portion of this follow-up. Fresh inertial output, unwritten-input handling, broader reference/damping coverage, and the existing conservation-test docstring correction remain for the subsequent commit groups.
There was a problem hiding this comment.
Pushed dd58657ec2 with 24 additional regression cases for fresh inertial outputs. They cover translation-only and rotation-then-translation configurations at initialization, after integration, after spacecraft reset, and with a hub attitude reference, for attached-only operation and both scheduled execution orders. Each case checks message timestamp plus position, velocity, attitude, and angular rate. The combined relevant suite passes all 169 cases locally.
This addresses the fresh-output portion of this follow-up. Unwritten-input handling, broader reference/damping coverage, and the existing conservation-test docstring correction remain for the subsequent commit groups.
There was a problem hiding this comment.
Pushed 4a6594e704 with 36 additional regression cases for input handling and cached-command retention.
The tests compare joint position and rate against an analytical damped spring response for motor force/torque, position references, and rate references. They cover optional-input defaults, unwritten inputs, first writes, relinking to unwritten producers, explicit zeros, attached-only operation, and repeated effector/spacecraft resets in both orders after disconnecting the producer. The combined relevant suite passes all 205 cases locally.
An additional 18 temporary review cases passed for combined motor/reference commands, resets with unwritten replacement producers, both execution orders, and six interleaved rotational/translational DOFs. These review probes are separate from the committed regression tests.
This addresses the remaining input/reference/damping coverage. The existing conservation-test docstring correction remains for the planned documentation cleanup in group 5, so this broader follow-up is not yet fully addressed.
There was a problem hiding this comment.
Pushed df5763a472 to address the remaining conservation-test documentation portion of this follow-up.
The docstring now states that these cases use conservative gravity and springs with no motor commands or damping, describes the DOF flags, and identifies the initial/final energy and angular-momentum checks. It points to test_generalSingleBodyValidation.py for the separate input-command and damping regressions. The normalized conservation plots now have dimensionless labels.
Together with groups 1–4, this completes the requested coverage and documentation changes. The combined relevant suite passed all 205 cases locally; the full Sphinx HTML build with -W, generated API checks, and executable user-guide checks also passed.
Run shared configuration validation before state registration and from Reset so attached-only effectors reject invalid inputs before dynamics. Preserve live integrated states and cached kinematics on effector reset. Check initial and runtime joint mass matrices with coordinate-scaled LU factorization, rejecting singular matrices and non-finite inverses. Use PRV2C consistently in initial validation and runtime attitude calculations. Document configuration bounds, coordinate singularities, and reset semantics. Add regressions for invalid inputs, initialization order, repeated resets, singular motion, large angles, and mixed inertia scales with analytical motor force and torque responses. Validation: 117 relevant pytest cases passed; pre-commit checks passed.
Guard DOF additions and clearing before any mutation once spacecraft initialization freezes the effector topology. Keep destructor cleanup separate so frozen effectors can still release their resources safely. Track exclusive DOF ownership and disable copying or assignment. Release ownership when clearing or destroying an effector, clearing cached motor and reference commands while preserving configuration for later DOF reuse. Add 28 regression cases for topology locking, rejected sharing and limit changes, ownership release, and reuse of previously commanded DOFs. Document these lifecycle rules and fix the short RST heading underline that caused the Canary and documentation builds to fail. Validation: 145 relevant pytest cases passed; pre-commit checks passed; full Sphinx HTML build passed with warnings treated as errors (-W).
Refresh inertial configuration values in writeOutputStateMessages() so spacecraft initialization, integration, and reset publish current outputs even when the effector is not separately scheduled. Compute these values only when the configuration message has a subscriber. Link the configured hub attitude and angular-rate states and read their current const views when publishing. This avoids using an intermediate integrator-stage attitude and includes final hub attitude-reference overrides while preserving the existing joint kinematics calculations. Add 24 regression cases covering all four configuration-message fields for translation-only and mixed joints across initialization, integration, reset, attitude-reference overrides, and both scheduled execution orders as well as attached-only operation. Document the output lifecycle. Validation: 169 relevant pytest cases passed; standalone header compilation passed with -Werror; pre-commit checks and the full Sphinx HTML build with warnings treated as errors (-W) passed.
Read motor and reference inputs only when linked and written so reconnecting to an unwritten producer cannot replace a valid cached command with zeros. Continue accepting explicitly written zero commands for all four input families. Document the existing policy of retaining cached motor commands and position/rate references across effector and spacecraft resets, together with optional-input defaults and the required task scheduling order. Add 36 analytical regression cases for force/torque, position and rate references, damping, unwritten inputs, relinking, explicit zeros, attached-only operation, and repeated resets in both orders with producers disconnected. Validation: 205 relevant tests passed; 18 additional review cases passed; the native target and full documentation built successfully, with Sphinx warnings treated as errors.
Document public setters, getters, and lifecycle callbacks with Doxygen parameters, returns, physical units, reference frames, configuration bounds, and reset/input behavior. Correct rotational and translational state, spring, damping, velocity, kinematics, and backsubstitution units. Label normalized conservation plots as dimensionless and describe the conservation tests' actual coverage. Fix the module I/O directive, add units and the NumPy import to the user guide, and clarify optional inputs and effector scheduling relative to the spacecraft's default priority of -1. Validation: 205 relevant tests passed; native target and standalone header compilation passed; full Sphinx HTML build passed with warnings treated as errors. Verified all 35 documented public methods, the generated I/O diagram, and execution of all six user-guide code blocks.
Demonstrate three rigid blocks attached to a freely reacting spacecraft hub: a translating slider, a two-axis gimbal, and a mixed rotation-translation mechanism. Tune spring and damping gains for visible underdamped motion that approaches the fixed joint references over the 30-second simulation. Configure colored Vizard box models and optional binary export, and plot joint responses, block paths, and hub angular rates. Document the setup, units, default hub visualization, and export instructions, and link the example from the scenario index and existing effector release note. Add the example to the shared scenario test runner for execution and documentation figure generation. Validation: 204 scenario and effector tests passed; documentation built with warnings treated as errors. Additional review checks verified joint kinematics, damping, timestep convergence, and Vizard binary states.
This branch has not been deployed
2 participants
Description
Verification
A unit test is added for the new effector.
Documentation
RST documentation is added for the new effector.
Future work
Future work involves expanding the general single body derivation to N general bodies, and enabling coupled screw motion for both the general single-body and N-body effectors.