Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

recenter toolhead before each measurement #196

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Zeanon
Copy link
Contributor

@Zeanon Zeanon commented Jan 19, 2025

fixes #194

Summary by Sourcery

Bug Fixes:

  • Fix an issue where the toolhead was not centered before each measurement, resulting in inaccurate measurements.

Copy link
Contributor

sourcery-ai bot commented Jan 19, 2025

Reviewer's Guide by Sourcery

This pull request introduces a change to recenter the toolhead before each measurement in both the axes shaper calibration and compare belts responses commands. This ensures that measurements are taken from a consistent starting point, improving the accuracy and reliability of the calibration process.

Sequence diagram for toolhead recentering before measurement

sequenceDiagram
    participant C as Calibration System
    participant T as Toolhead
    participant M as Measurements Manager

loop For each axis configuration
    C->>T: manual_move(point, feedrate_travel)
    C->>T: dwell(0.5)
    C->>T: wait_moves()
    C->>M: Initialize MeasurementsManager
    Note over C,M: Continue with measurements
end
Loading

Flow diagram for enhanced measurement process

flowchart TD
    A[Start Measurement] --> B[Filter Axis Configuration]
    B --> C[Enter Axis Loop]
    C --> D[Recenter Toolhead]
    D --> E[Wait 0.5s]
    E --> F[Wait for Moves to Complete]
    F --> G[Initialize Measurements]
    G --> H[Perform Measurements]
    H --> I{More Axes?}
    I -->|Yes| C
    I -->|No| J[End]
Loading

File-Level Changes

Change Details Files
Recenter toolhead before each measurement.
  • Added a manual move to the starting point before each measurement.
  • Added a dwell after the move to allow the toolhead to settle.
  • Added a wait for moves to ensure the toolhead is in position before starting the measurement.
shaketune/commands/axes_shaper_calibration.py
shaketune/commands/compare_belts_responses.py

Assessment against linked issues

Issue Objective Addressed Explanation
#194 Ensure toolhead returns to a centered position before starting each belt measurement
#194 Prevent toolhead position shift between A and B belt measurements
#194 Maintain consistent starting point for belt measurements to ensure accurate tension comparison

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Zeanon - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider extracting the recentering code into a shared helper function to avoid duplication, but this can be done as a separate improvement
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +98 to +100
toolhead.manual_move(point, feedrate_travel)
toolhead.dwell(0.5)
toolhead.wait_moves()
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Consider extracting these toolhead operations into a shared utility function

This exact sequence appears in multiple files. To improve maintainability and reduce duplication, consider moving it to a shared utility function like ensure_toolhead_position().

Suggested implementation:

        a for a in AXIS_CONFIG if a['axis'] == axis_input or (axis_input == 'all' and a['axis'] in ('x', 'y'))
    ]

def ensure_toolhead_position(toolhead, point, feedrate):
    """Moves toolhead to specified position and ensures movement is complete.

    Args:
        toolhead: The toolhead object to control
        point: The target position to move to
        feedrate: The feedrate for the movement
    """
    toolhead.manual_move(point, feedrate)
    toolhead.dwell(0.5)
    toolhead.wait_moves()

    for config in filtered_config:
        ensure_toolhead_position(toolhead, point, feedrate_travel)
        measurements_manager = MeasurementsManager(st_process.get_st_config().chunk_size)

Since this sequence appears in multiple files, you may want to:

  1. Move the ensure_toolhead_position function to a shared utilities module (e.g., shaketune/utils/toolhead.py)
  2. Import and use it from all the files where this sequence is needed
  3. Update all other occurrences of this sequence to use the new utility function

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.

X/Y Axis aren't centered after A belt resonance measurement.
1 participant