Code Sim Improvements [SYNTH-255] & [SYNTH-260] - #1476
Conversation
afc61f3 to
85f2e6f
Compare
[SYNTH-255][SYNTH-255] & [SYNTH-260]
added a generic type for maintainability, and now differentiate between purposes for nora types (e.g., now have gyro nora 6 and accel nora 6) so that you can't connect gyro data to an accel input
85f2e6f to
ba2b8b3
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves the code simulation sensor workflow by adding an in-assembly “Sensors” configuration UI (to place gyro/accelerometer stimuli on robot nodes) and tightening wiring compatibility by introducing purpose-specific Nora types (e.g., GYRO, ACCEL) so same-arity signals are no longer universally interchangeable.
Changes:
- Adds a new Assembly Config “Sensors” panel to create/edit/delete sensors and place them via a transform gizmo.
- Replaces the old chassis stimulus with dedicated
GyroStimulusandAccelStimulus, and updates WPILib receiver typing to purpose-specific Nora types. - Updates wiring UI type colors to be dynamically derived from Nora type strings, and removes unused RoboRIO device config modals.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| fission/src/ui/panels/configuring/assembly-config/interfaces/sensors/SensorConfigInterface.tsx | New sensor editing UI (name/type/device/parent node + gizmo placement). |
| fission/src/ui/panels/configuring/assembly-config/interfaces/sensors/ManageSensorsInterface.tsx | New sensor list management UI (add/edit/delete + persistence). |
| fission/src/ui/panels/configuring/assembly-config/interfaces/sensors/ConfigureSensorsInterface.tsx | New subpanel wrapper for managing vs. editing a single sensor. |
| fission/src/ui/panels/configuring/assembly-config/ConfigurePanel.tsx | Registers the new Sensors config mode/panel. |
| fission/src/ui/panels/configuring/assembly-config/ConfigTypes.ts | Adds ConfigMode.SENSORS and its selection option text. |
| fission/src/ui/modals/configuring/RoboRIOModal.tsx | Deletes obsolete/placeholder RoboRIO modal. |
| fission/src/ui/modals/configuring/rio-config/RCCreateDeviceModal.tsx | Deletes obsolete device-creation modal flow. |
| fission/src/ui/modals/configuring/rio-config/RCConfigPWMGroupModal.tsx | Deletes obsolete PWM group config modal. |
| fission/src/ui/modals/configuring/rio-config/RCConfigEncoderModal.tsx | Deletes obsolete encoder config modal. |
| fission/src/ui/modals/configuring/rio-config/RCConfigCANGroupModal.tsx | Deletes obsolete CAN group config modal. |
| fission/src/test/PreferencesSystem.test.ts | Updates robot preferences test fixtures to include sensors: []. |
| fission/src/systems/simulation/wpilib_brain/WPILibState.ts | Updates receiver typing for gyro/accel to new Nora purpose types. |
| fission/src/systems/simulation/wpilib_brain/WPILibBrain.ts | Removes hardcoded sim IO defaults and updates enable/disable behavior. |
| fission/src/systems/simulation/wpilib_brain/sim/SimGyro.ts | Switches receiver value typing to NoraValue<6> and removes old SimInput implementation. |
| fission/src/systems/simulation/wpilib_brain/sim/SimCANEncoder.ts | Switches receiver value typing to NoraValue<2>. |
| fission/src/systems/simulation/wpilib_brain/sim/SimAccel.ts | Switches receiver value typing to NoraValue<6> and removes old SimInput implementation. |
| fission/src/systems/simulation/stimulus/WheelStimulus.ts | Switches supplier value typing to NoraValue<2>. |
| fission/src/systems/simulation/stimulus/Stimulus.ts | Replaces chassis accel stimulus enum with dedicated accel/gyro stimulus types. |
| fission/src/systems/simulation/stimulus/SliderStimulus.ts | Switches supplier value typing to NoraValue<2>. |
| fission/src/systems/simulation/stimulus/HingeStimulus.ts | Switches supplier value typing to NoraValue<2>. |
| fission/src/systems/simulation/stimulus/GyroStimulus.ts | Adds new gyro stimulus implementation (mount frame + integration + supplier value). |
| fission/src/systems/simulation/stimulus/ChassisStimulus.ts | Deletes the old chassis stimulus implementation. |
| fission/src/systems/simulation/stimulus/AccelStimulus.ts | Adds new accelerometer stimulus implementation (mount point kinematics + specific force). |
| fission/src/systems/simulation/SimulationSystem.ts | Builds and refreshes per-sensor gyro/accel stimuli based on robot preferences. |
| fission/src/systems/simulation/SimConfigShared.ts | Adds gyro device handles, adjusts accel handles, and replaces static Nora color map with dynamic generation. |
| fission/src/systems/simulation/Nora.ts | Adds purpose-specific NoraTypes and introduces NoraValue<N> generic tuple typing. |
| fission/src/systems/preferences/PreferenceTypes.ts | Adds SensorPreferences and wires it into RobotPreferences defaults. |
Suppressed comments (2)
fission/src/systems/simulation/SimConfigShared.ts:342
- Accel device handles are created from all
getAccelDevices()entries but are always markedenabled: true. If the sim map contains entries that haven’t been initialized (<init>false), they’ll still show up as connectable targets, unlike CAN/PWM devices which are filtered/gated by<init>.
This issue also appears on line 355 of the same file.
getAccelDevices().forEach(([id]) => {
const handle: HandleInfo = {
id: "",
nodeId: NODE_ID_ROBOT_IO,
noraType: receiverTypeMap[SimType.ACCELEROMETER]!,
fission/src/systems/simulation/SimConfigShared.ts:359
- Gyro device handles are created from all
getGyroDevices()entries but are always markedenabled: true. This can surface uninitialized (<init>false) gyro devices as connectable targets in the wiring UI.
getGyroDevices().forEach(([id]) => {
const handle: HandleInfo = {
id: "",
nodeId: NODE_ID_ROBOT_IO,
noraType: receiverTypeMap[SimType.GYRO]!,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 56 out of 57 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
fission/src/systems/simulation/SimulationSystem.ts:130
guidis derived fromsensor.name, so duplicate sensor names will collide and later sensors will overwrite earlier ones in_stimuli(keyed by the GUID). Consider incorporating a stable unique component (index/device/UUID persisted in preferences) so each sensor gets a distinct stimulus ID.
const guid = `SENSOR_${sensor.name}_GUID`
const info = { GUID: guid, name: sensor.name }
fission/src/systems/simulation/wiring/NodeKinds.tsx:77
- Leftover
console.login the junction node supplier will spam logs during compilation / graph evaluation. This should be removed (or guarded behind a debug flag) to avoid noisy output in production.
makeSupplier: (handle, ctx) => {
const [input] = nodeTargets(ctx.config, handle.nodeId)
console.log(input)
return input && compileSuppliersFor(ctx, input.id)
fission/src/util/Colors.ts:11
- The JSDoc says the palette entries are
hsl(h, s, v), but the implementation uses HSL lightness (and includes%). Updating the doc avoids confusion for future callers.
fission/src/systems/simulation/wiring/Factories.ts:282 - Leftover
console.log(JSON.stringify(componentType))will spam logs whenever a deconstructor node is created. Please remove it (or gate it behind a debug flag).
targetNoraType.forEach((componentType, i) => {
console.log(JSON.stringify(componentType))
addHandle(config, {
I added the sensor device to the generated guid so it includes both name and halsim device. I think it's fine since it would make sense for a later sensor with the same name and target device to overwrite a previous one.
RoushilS
left a comment
There was a problem hiding this comment.
Sim and diff both look good to me. I especially liked how clean and extensive the type system groundwork was (nora and stimulus). The wiring system being broken up makes it much more accessible now.
azaleacolburn
left a comment
There was a problem hiding this comment.
Mostly LGTM, but I have a few nitpicks that you can address, dismiss, or push off to a separate PR. Nice work on this one.
| /// the underlying primitive data type, a unit, and a derivative order. | ||
| /// E.g., an angular velocity would be a number that is the first derivative of an angle | ||
| /// | ||
| /// A base unit of NONE is included for unitless values such as a motor's percent output. |
There was a problem hiding this comment.
| private _prevVel = new THREE.Vector3() | ||
| private _prevOmega = new THREE.Vector3() | ||
|
|
||
| private _accel = new THREE.Vector3() | ||
| private _vel = new THREE.Vector3() |
There was a problem hiding this comment.
Not for this PR, but I feel like we should have a separate Vector class that isn't jolt or three, since they're both rather misleading.
| this._prevVel = velCom.clone() | ||
| this._prevOmega = omega.clone() |
There was a problem hiding this comment.
Do we need to clone the vectors here?
| const accelCom = velCom.clone().sub(this._prevVel).divideScalar(deltaT) | ||
| const alpha = omega.clone().sub(this._prevOmega).divideScalar(deltaT) | ||
|
|
||
| const accelPoint = accelCom.add(alpha.cross(r)).add(omega.clone().cross(omega.clone().cross(r))) |
There was a problem hiding this comment.
If we never use omega again, do we need to clone?
| protected supplyValue(): NoraValueOf<typeof ACCEL_TYPE> { | ||
| return [ | ||
| { value: this._accel.x, baseType: ACCEL_TYPE[0] }, | ||
| { value: this._accel.y, baseType: ACCEL_TYPE[1] }, | ||
| { value: this._accel.z, baseType: ACCEL_TYPE[2] }, | ||
| { value: this._vel.x, baseType: ACCEL_TYPE[3] }, | ||
| { value: this._vel.y, baseType: ACCEL_TYPE[4] }, | ||
| { value: this._vel.z, baseType: ACCEL_TYPE[5] }, | ||
| ] | ||
| } |
There was a problem hiding this comment.
This is really sexy
| import Stimulus, { type StimulusID } from "./Stimulus" | ||
|
|
||
| export const GYRO_TYPE = noraType([ | ||
| numAxis(BaseUnit.ANGLE, DerivativeOrder.ZERO, BaseAxis.X, "Angle X"), // TODO: yaw/pitch/roll? |
There was a problem hiding this comment.
Are we coming back to this distinction in another PR?
| private integrateAngle(axis: "x" | "y" | "z", rate: number, deltaT: number): number { | ||
| this._accumulated[axis] += rate * deltaT | ||
|
|
||
| const external = SimGeneric.getUnsafe<number>(SimType.GYRO, this._device, GyroStimulus.ANGLE_FIELD[axis]) | ||
| if (external !== undefined && external !== this._lastWritten[axis]) { | ||
| this._offset[axis] = this._accumulated[axis] - external | ||
| } | ||
|
|
||
| const angle = this._accumulated[axis] - this._offset[axis] | ||
| this._lastWritten[axis] = angle | ||
| return angle | ||
| } |
There was a problem hiding this comment.
This function is very unclear to me, and should probably be documented (can be a future PR). It feels weird can't just set this, but whatever.
There was a problem hiding this comment.
This looks basically identical to FunctionNode.tsx
There was a problem hiding this comment.
In the future, this class and SimOutputNode.tsx should be merged into a single function with a const param.
| try { | ||
| const flows = Object.values(config.handles) | ||
| .filter(h => !h.isSource && h.enabled && edgesOf(config, h.id).length > 0) | ||
| .filter(h => NODE_KINDS[config.nodes[h.nodeId]!.kind].makeReceiver !== undefined) | ||
| .map(h => { | ||
| const receiver = NODE_KINDS[config.nodes[h.nodeId]!.kind].makeReceiver!(h, ctx) | ||
| const supplier = compileSuppliersFor(ctx, h.id) | ||
| if (!receiver) throw new Error(`Could not create a receiver for '${h.displayName}'`) | ||
| if (!supplier) throw new Error(`Could not create a supplier for '${h.displayName}'`) | ||
| return { supplier, receiver } | ||
| }) | ||
| return { flows } | ||
| } catch (error) { | ||
| console.error("Compilation failed", error) | ||
| return { error: error instanceof Error ? error.message : String(error) } | ||
| } |
There was a problem hiding this comment.
I don't love the error handling here.
addressed feedback, approved in person
Code Sim Improvements
Task
SYNTH-255, SYNTH-260
Symptom
Many sim devices weren't showing up in the wiring panel and needed to be hardcoded in Fission. It was also possible to connect any node to another node as long as their arity was the same, meaning you could connect a Fission accelerometer to a Java gyro and it would feed the data correctly even though that shouldn't be allowed.
Solution
Migrated the Chassis stimulus to individual Gyro and Accel ones that show up in the wiring panel. Also changed the Nora type system to include the purpose of the node (e.g., gyro, accel, etc.). The node colors are also dynamically calculated based on the hash of the serialized type so we don't need to remember to add new ones.
Verification
Created gyros and accelerometers in the code and simulator and verified that the data shows up in the Java simulator only when wired properly.
Before merging, ensure the following criteria are met: