Skip to content

Conversation

@TheBest6337
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings January 2, 2026 17:56
@TheBest6337 TheBest6337 linked an issue Jan 2, 2026 that may be closed by this pull request
@TheBest6337 TheBest6337 force-pushed the 1027-add-bottlecapsorter-using-ip20-module-and-one-wago-stepper branch from e7e8f73 to 6be7f9a Compare January 2, 2026 17:56
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a new machine type called "Minimal Bottle Sorter" to the control software. The machine uses a Wago IP20-EC-DI8-DO8 module for digital I/O and a Beckhoff EL7041-0052 stepper motor terminal. The implementation follows the existing pattern used for test machines in the codebase, providing both backend machine control logic in Rust and a frontend React-based UI for operator control.

Key changes:

  • Backend Rust implementation with EtherCAT device configuration, stepper motor control, and digital output pulse functionality
  • Frontend TypeScript/React UI with stepper motor controls and digital output pulse buttons
  • Machine registration and routing integration

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
machines/src/registry.rs Registers the new MinimalBottleSorter machine type in the machine registry
machines/src/lib.rs Adds module declaration and machine identification constant (0x0036)
machines/src/minimal_bottle_sorter/mod.rs Core machine struct and implementation with stepper control and digital output pulse logic
machines/src/minimal_bottle_sorter/new.rs Device initialization and configuration for EK1100 coupler, EL7041-0052 stepper, and IP20 DI8/DO8 module
machines/src/minimal_bottle_sorter/api.rs Socket.io event definitions and API mutation handlers for frontend communication
machines/src/minimal_bottle_sorter/act.rs Main control loop implementation with periodic state emission and pulse timing
electron/src/routes/routes.tsx Adds routing for the bottle sorter pages
electron/src/machines/properties.ts Defines machine properties including device roles and hardware identifications
electron/src/machines/minimalbottlesorter/useMinimalBottleSorter.ts React hook providing state management and mutation functions
electron/src/machines/minimalbottlesorter/minimalBottleSorterNamespace.ts Socket.io namespace configuration and event schema definitions
electron/src/machines/minimalbottlesorter/MinimalBottleSorterPage.tsx Main page component with navigation topbar
electron/src/machines/minimalbottlesorter/MinimalBottleSorterControlPage.tsx Control interface with stepper motor controls and output pulse buttons

} else {
-speed_steps_s
};
let _ = self.stepper.set_speed(signed_speed);
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error result from set_speed is silently ignored. If setting the speed fails, the system will be in an inconsistent state where the internal speed variable doesn't match the actual hardware speed. Consider logging the error or propagating it to the caller.

Suggested change
let _ = self.stepper.set_speed(signed_speed);
if let Err(err) = self.stepper.set_speed(signed_speed) {
eprintln!(
"Failed to set stepper speed to {signed_speed}: {err}"
);
}

Copilot uses AI. Check for mistakes.

/// Set stepper speed in mm/s
pub fn set_stepper_speed(&mut self, speed_mm_s: f64) {
self.stepper_speed = speed_mm_s.abs().max(0.0).min(100.0); // Limit to 0-100 mm/s
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The speed_mm_s.abs() call makes the subsequent .max(0.0) redundant since the absolute value is already non-negative. Consider removing either abs() or max(0.0).

Suggested change
self.stepper_speed = speed_mm_s.abs().max(0.0).min(100.0); // Limit to 0-100 mm/s
self.stepper_speed = speed_mm_s.abs().min(100.0); // Limit to 0-100 mm/s

Copilot uses AI. Check for mistakes.
MachineMessage::UnsubscribeNamespace => self.namespace.namespace = None,
MachineMessage::HttpApiJsonRequest(value) => {
use crate::MachineApi;
let _res = self.api_mutate(value);
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The result of api_mutate is silently ignored. If the mutation fails (e.g., due to invalid parameters), no feedback is provided and the error is not logged. Consider logging the error or handling it appropriately to aid in debugging and troubleshooting.

Suggested change
let _res = self.api_mutate(value);
if let Err(err) = self.api_mutate(value) {
eprintln!(
"MinimalBottleSorter: api_mutate failed for HttpApiJsonRequest: {:?}",
err
);
}

Copilot uses AI. Check for mistakes.

export const liveValuesEventDataSchema = z.object({
stepper_actual_speed: z.number(),
stepper_position: z.number(),
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The backend uses i128 for stepper_position, but the frontend schema uses z.number() which represents a JavaScript number (max safe integer is 2^53-1). This type mismatch may cause precision loss or overflow issues if the stepper position exceeds JavaScript's safe integer range. Consider using z.bigint() or documenting the expected range of the stepper position.

Suggested change
stepper_position: z.number(),
stepper_position: z.bigint(),

Copilot uses AI. Check for mistakes.
Comment on lines +12 to +27
static mut LAST_PULSE_UPDATE: Option<Instant> = None;
unsafe {
let should_update = match LAST_PULSE_UPDATE {
Some(last) => now.duration_since(last) > Duration::from_millis(10),
None => true,
};

if should_update {
let delta_ms = match LAST_PULSE_UPDATE {
Some(last) => now.duration_since(last).as_millis() as u32,
None => 10,
};
self.update_pulses(delta_ms);
LAST_PULSE_UPDATE = Some(now);
}
}
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using mutable static variables without synchronization is unsafe and can lead to data races in multi-threaded contexts. This implementation violates Rust's safety guarantees. Consider using a thread-local variable or storing LAST_PULSE_UPDATE as an instance field in the MinimalBottleSorter struct instead.

Copilot uses AI. Check for mistakes.

use crate::{
MachineNewHardware, MachineNewParams, MachineNewTrait, get_ethercat_device,
validate_no_role_dublicates, validate_same_machine_identification_unique,
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spelling "dublicates" is incorrect. It should be "duplicates".

Copilot uses AI. Check for mistakes.
.map(|device_identification| device_identification.clone())
.collect::<Vec<_>>();
validate_same_machine_identification_unique(&device_identification)?;
validate_no_role_dublicates(&device_identification)?;
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spelling "dublicates" is incorrect. It should be "duplicates".

Copilot uses AI. Check for mistakes.
if self.stepper_enabled {
// Convert mm/s to steps/s (assuming 200 steps per revolution and some mechanical conversion)
// This is a placeholder - adjust based on your actual mechanical setup
let steps_per_mm = 100.0; // Adjust this value based on your setup
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hardcoded value of 100.0 steps per mm should be documented or made configurable. This magic number represents a critical mechanical configuration that will need adjustment for actual hardware. Consider adding a comment explaining this is a placeholder value or making it a constant with a descriptive name.

Copilot uses AI. Check for mistakes.
import {
useMinimalBottleSorterNamespace,
StateEvent,
LiveValuesEvent,
Copy link

Copilot AI Jan 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import LiveValuesEvent.

Suggested change
LiveValuesEvent,

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add bottlecapsorter using IP20 module and one wago stepper

2 participants