Conversation
… position Add X_POS and Y_POS parameters to EXCITATE_AXIS_AT_FREQ, following the same pattern as the existing Z_HEIGHT parameter. Each independently overrides its respective coordinate from [resonance_tester] probe_points. Use case: belt tension measurement requires parking the toolhead at a specific Y position (calibrated belt span), not the default resonance test point.
feat(excitate): add X_POS and Y_POS parameters to override test point position
Contributor
Reviewer's GuideAdds positional overrides to the excitate_axis_at_freq command, introduces a configurable minimum speed for create_vibrations_profile, and updates the dummy macros and docs to expose these new parameters and validations. Sequence diagram for excitate_axis_at_freq position resolution with X_POS/Y_POS overridessequenceDiagram
actor User
participant Macro_EXCITATE_AXIS_AT_FREQ as Macro_EXCITATE_AXIS_AT_FREQ
participant Command_excitate_axis_at_freq as Command_excitate_axis_at_freq
participant Klipper_gcmd as Klipper_gcmd
participant Klipper_toolhead as Klipper_toolhead
participant Klipper_config as Klipper_config
User->>Macro_EXCITATE_AXIS_AT_FREQ: Invoke with parameters (ACCEL_PER_HZ, AXIS, TRAVEL_SPEED, X_POS, Y_POS, Z_HEIGHT, ACCEL_CHIP)
Macro_EXCITATE_AXIS_AT_FREQ->>Command_excitate_axis_at_freq: Call excitate_axis_at_freq(gcmd, klipper_config, st_process)
Command_excitate_axis_at_freq->>Klipper_gcmd: get_float ACCEL_PER_HZ
Command_excitate_axis_at_freq->>Klipper_gcmd: get AXIS
Command_excitate_axis_at_freq->>Klipper_gcmd: get_float TRAVEL_SPEED
Command_excitate_axis_at_freq->>Klipper_gcmd: get_float X_POS (default None)
Command_excitate_axis_at_freq->>Klipper_gcmd: get_float Y_POS (default None)
Command_excitate_axis_at_freq->>Klipper_gcmd: get_float Z_HEIGHT
Command_excitate_axis_at_freq->>Klipper_gcmd: get ACCEL_CHIP
alt resonance_tester has probe_points
Command_excitate_axis_at_freq->>Klipper_config: Read resonance_tester test_points
Klipper_config-->>Command_excitate_axis_at_freq: test_points
Command_excitate_axis_at_freq->>Command_excitate_axis_at_freq: x, y, z = test_points[0]
alt Z_HEIGHT provided
Command_excitate_axis_at_freq->>Command_excitate_axis_at_freq: Override z with Z_HEIGHT
end
else test_points is [-1, -1, -1]
Command_excitate_axis_at_freq->>Klipper_toolhead: toolhead.kin.get_status(systime)
Klipper_toolhead-->>Command_excitate_axis_at_freq: axis_minimum, axis_maximum
Command_excitate_axis_at_freq->>Command_excitate_axis_at_freq: x = (axis_minimum.x + axis_maximum.x) / 2
Command_excitate_axis_at_freq->>Command_excitate_axis_at_freq: y = (axis_minimum.y + axis_maximum.y) / 2
Command_excitate_axis_at_freq->>Command_excitate_axis_at_freq: z = Z_HEIGHT
end
alt X_POS provided
Command_excitate_axis_at_freq->>Command_excitate_axis_at_freq: Override x with X_POS
end
alt Y_POS provided
Command_excitate_axis_at_freq->>Command_excitate_axis_at_freq: Override y with Y_POS
end
Command_excitate_axis_at_freq->>Command_excitate_axis_at_freq: point = (x, y, z)
Command_excitate_axis_at_freq->>Klipper_toolhead: manual_move(point, TRAVEL_SPEED)
Command_excitate_axis_at_freq->>Klipper_toolhead: dwell(0.5)
Klipper_toolhead-->>User: Toolhead positioned at final test point
Flow diagram for create_vibrations_profile speed range with configurable MIN_SPEEDflowchart TD
A["Start CREATE_VIBRATIONS_PROFILE"] --> B["Read SIZE from gcmd (default 100.0, min 50.0)"]
B --> C["Read Z_HEIGHT from gcmd (default 20.0)"]
C --> D["Read MIN_SPEED from gcmd (default 2.0, min 1.0)"]
D --> E["Read MAX_SPEED from gcmd (default 200.0, min 10.0)"]
E --> F["Read SPEED_INCREMENT from gcmd (default 2.0, min 1.0)"]
F --> G["Read ACCEL and other parameters"]
G --> H{"Is MIN_SPEED >= MAX_SPEED?"}
H -->|Yes| I["Raise error MIN_SPEED must be lower than MAX_SPEED"]
H -->|No| J{"Is size / (MAX_SPEED / 60) < 0.25?"}
J -->|Yes| K["Raise error size too small for given speed"]
J -->|No| L["Compute nb_speed_samples = int((MAX_SPEED - MIN_SPEED) / SPEED_INCREMENT + 1)"]
L --> M["Initialize main_angles loop"]
M --> N["For each curr_angle in main_angles"]
N --> O["Compute radian_angle and segment geometry"]
O --> P["Initialize speed sample loop"]
P --> Q["For curr_speed_sample in range(nb_speed_samples)"]
Q --> R["Compute curr_speed = MIN_SPEED + curr_speed_sample * SPEED_INCREMENT"]
R --> S["Print current speed and generate movement segments"]
S --> T["Record vibrations at curr_speed"]
T --> U{"More speed samples?"}
U -->|Yes| Q
U -->|No| V{"More angles?"}
V -->|Yes| N
V -->|No| W["Finish measurement, save profile files"]
W --> X["End"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `docs/macros/excitate_axis_at_freq.md:16` </location>
<code_context>
|ACCEL_PER_HZ|None (default to `[resonance_tester]` value)|accel per Hz value used for the test|
|AXIS|x|axis you want to excitate. Can be set to either "x", "y", "a", "b"|
|TRAVEL_SPEED|120|speed in mm/s used for all the travel movements (to go to the start position prior to the test)|
+|X_POS|None|X position wanted for the test. This value can be used if needed to override the X value of the probe_point set in your `[resonance_tester]` config section|
</code_context>
<issue_to_address>
**issue (typo):** Consider changing "excitate" to "excite" in this description.
In this parameter description, "axis you want to excitate" should be "axis you want to excite" for correct English; this only changes the prose, not any macro or configuration names.
```suggestion
|AXIS|x|axis you want to excite. Can be set to either "x", "y", "a", "b"|
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -15,6 +15,8 @@ Here are the parameters available: | |||
| |ACCEL_PER_HZ|None (default to `[resonance_tester]` value)|accel per Hz value used for the test| | |||
| |AXIS|x|axis you want to excitate. Can be set to either "x", "y", "a", "b"| | |||
Contributor
There was a problem hiding this comment.
issue (typo): Consider changing "excitate" to "excite" in this description.
In this parameter description, "axis you want to excitate" should be "axis you want to excite" for correct English; this only changes the prose, not any macro or configuration names.
Suggested change
| |AXIS|x|axis you want to excitate. Can be set to either "x", "y", "a", "b"| | |
| |AXIS|x|axis you want to excite. Can be set to either "x", "y", "a", "b"| |
Values like ACCEL_CHIP="adxl345 T0" were losing their quotes when the
dummy macro reconstructed the command string, causing shlex parsing
failures in the internal command. Fix by wrapping all values in double
quotes in the for-loop output.
Also simplifies all 5 macros to use the same pattern: {% set dummy %}
for web UI hints + params.items() iteration. This unifying them all
with consistent empty-string filtering.
Fixes #234
Supersedes the previous approach in PR #239
improved dummy macros handling of spaces in parameters strings values
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add configurability for vibration tests and document the new parameters.
New Features:
Bug Fixes:
Documentation:
Chores: