1302 revise stepper minimal example UI#1336
Conversation
|
Needs to be tested on hardware before ready to merge. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Updates the “test_machine_stepper” minimal machine to support a more structured UI by introducing explicit operation modes and typed frequency/acceleration selections, and wiring these through the backend machine API and the Electron control page.
Changes:
- Add
Mode,Frequency, andAccelerationFactortypes to the stepper minimal machine API and expose new mutations (SetMode,SetFreq,SetAccFactor). - Extend EtherCAT stepper wrappers/modules with start/stop behavior and a new
Readyinit state. - Revamp the Electron stepper control page to use
SelectionGroup-based controls for mode/frequency/acceleration.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| machines/src/minimal_machines/test_machine_stepper/new.rs | Initialize the new mode field when constructing the machine. |
| machines/src/minimal_machines/test_machine_stepper/mod.rs | Add mode to machine state, emit new structured state, and implement mode-driven behavior. |
| machines/src/minimal_machines/test_machine_stepper/api.rs | Define new enums/state structs and add new API mutations for mode/frequency/acceleration. |
| ethercat-hal/src/io/stepper_velocity_wago_750_672.rs | Add start/stop helpers and adjust frequency setter naming. |
| ethercat-hal/src/io/stepper_velocity_wago_750_671.rs | Add start/stop helpers. |
| ethercat-hal/src/devices/wago_modules/wago_750_672.rs | Introduce InitState::Ready and associated state-machine handling. |
| ethercat-hal/src/devices/wago_modules/wago_750_671.rs | Introduce InitState::Ready and adjust control-byte generation in init states. |
| electron/src/machines/minimal_machines/testmachinestepper/useTestMachineStepper.ts | Update hook to send new mutations and manage optimistic state for new structured fields. |
| electron/src/machines/minimal_machines/testmachinestepper/testMachineStepperNamespace.ts | Add zod schemas/types for mode/frequency/acceleration and update StateEvent shape. |
| electron/src/machines/minimal_machines/testmachinestepper/TestMachineStepperControlPage.tsx | Replace old button UI with selection-group UI for mode/frequency/acceleration. |
| self.set_enabled(true); | ||
| self.stop_motor(); | ||
| } | ||
| Mode::Turn => self.start_motor(), |
There was a problem hiding this comment.
set_mode sets Mode::Turn by calling start_motor() only. If the machine is currently in Standby (disabled/off), start_motor() in the stepper wrappers is a no-op unless the device is already initialized, so switching Standby → Turn will not start the motor. Consider enabling/initializing first (e.g., call set_enabled(true) before requesting start, or make start_motor able to request a start even when not initialized).
| Mode::Turn => self.start_motor(), | |
| Mode::Turn => { | |
| self.set_enabled(true); | |
| self.start_motor(); | |
| } |
| pub fn get_state(self) -> InitState { | ||
| let dev = block_on(self.device.read()); | ||
|
|
||
| dev.state.clone() | ||
| } |
There was a problem hiding this comment.
get_state takes self by value, which consumes the stepper wrapper and makes it unusable after calling this method. This is very likely unintended for a simple read accessor; change it to take &self (and return the cloned state) to avoid accidental moves.
| export const accelerationSchema = z.enum(["Default", "Low", "Mid", "High"]); | ||
| export type AccelerationFactor = z.infer<typeof accelerationSchema>; | ||
|
|
There was a problem hiding this comment.
The exported type name AccelerationFaktor mixes languages and differs from the Rust/backend AccelerationFactor, which makes the API surface harder to follow (and forces aliases in consumers). Consider renaming this to AccelerationFactor for consistency across frontend/backend.
| className: "h-full", | ||
| }, |
There was a problem hiding this comment.
UI label spelling: the card title uses "Acceleration Faktor". If this UI is intended to be English (other labels are), this should be "Acceleration Factor" for consistency.
| InitState::Enable => { | ||
| // set the specific bits of Control Byte C1 | ||
| let mut c1 = ControlByteC1::new().with_flag(C1Flag::Enable); | ||
| if self.desired_stop2_n { | ||
| c1 = c1.with_flag(C1Flag::Stop2N); | ||
| } | ||
| let c1 = ControlByteC1::new().with_flag(C1Flag::Enable); | ||
| let c1 = c1.bits(); | ||
| self.rxpdo.control_byte1 = c1; |
There was a problem hiding this comment.
InitState::Enable now sets only the Enable bit in control byte C1, but later states (Ready/Running/StartPulse*) always set Stop2N. Previously desired_stop2_n defaulted to true, so Stop2N was always asserted during init. Dropping Stop2N in early init states may change safety/stop behavior and could prevent expected acknowledgements depending on module config; consider restoring Stop2N in Enable (and adjusting the ack condition as your comment suggests).
| }; | ||
| use crate::{AsyncThreadMessage, Machine, MachineMessage}; | ||
| use control_core::socketio::namespace::NamespaceCacheingLogic; | ||
| use core::convert::Into; |
There was a problem hiding this comment.
use core::convert::Into; is unnecessary (Rust prelude already includes Into) and may trigger an unused-import warning. Removing it keeps the module clean and avoids CI lint failures.
| use core::convert::Into; |
| acceleration_state: AccelerationState { | ||
| factor: stepper_velocity_wago750672.acc_range_sel.into(), | ||
| }, |
There was a problem hiding this comment.
mode: self.mode.clone().into() is an identity conversion (Mode → Mode) and the .into() adds noise (and is the only reason Into was imported). Prefer assigning the cloned Mode directly.
| import { | ||
| useTestMachineStepperNamespace, | ||
| StateEvent, | ||
| modeSchema, | ||
| Mode, | ||
| AccelerationFactor as AccelerationFactor, | ||
| Frequency, | ||
| } from "./testMachineStepperNamespace"; |
There was a problem hiding this comment.
modeSchema is imported but never used in this file. If the Electron project enforces @typescript-eslint/no-unused-vars (it appears to elsewhere), this can fail lint/CI; remove the unused import.
| /** | ||
| * Accleration Factor Schema | ||
| */ | ||
| export const accelerationStateSchema = z.object({ | ||
| factor: accelerationSchema, | ||
| }); |
There was a problem hiding this comment.
Spelling: "Accleration" is misspelled in the comment. Correcting it helps keep the public-facing schema/docs readable.
| if self.desired_stop2_n { | ||
| c1 = c1.with_flag(C1Flag::Stop2N); | ||
| } | ||
| let c1 = ControlByteC1::new().with_flag(C1Flag::Enable); |
There was a problem hiding this comment.
InitState::SetMode similarly builds C1 without Stop2N, while subsequent states always include it. If Stop2N is meant to be asserted continuously (as in the rest of this state machine), include it here too to avoid an init cycle that momentarily drops the stop bit.
| let c1 = ControlByteC1::new().with_flag(C1Flag::Enable); | |
| let c1 = ControlByteC1::new() | |
| .with_flag(C1Flag::Enable) | |
| .with_flag(C1Flag::Stop2N); |
fix #1302