v5.0.0 #181
Merged
v5.0.0 #181
Conversation
This file contains 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
Colors were previously inverted on the left graph but it was forgotten to also do it on the versus plot...
accel vs vibrs performance plot added
to avoid having multiple CSV files open at once and accelerometer measurements on-going at the same time
feat(kinematics): add support for danger-klipper limited_ kinematics
feat(ci): expand matrix accross python and klipper versions
with all the logic to load them back and reconstruct the .stdata file
with all the logic to load them back and reconstruct the .stdata file
Adding the chunk size as a shaketune parameter
Support alternate klipper venv paths
Trying to fix Klipper TTC errors
* implemented a Factory design pattern for graph creators * factorized Klipper commands * separation of computations vs plotters * simplify logical expressions using De Morgan identities * replace mutable default logo position argument with None * inline computation results in the return statement directly * using union operator for the graph creators parameters * use next() to return the formatted vector in axes map function * using walrus operator to simplify logic in GraphCreatorFactory
* small refactoring of k_shapers imports and variables to be easier to read * added draft for tradeoff shaper plots (commented for now as it's a WIP for later use)
* fixed compatibility with the new version of Klipper for pulse-only test * added sweeping compatibility (#179) * added documentation about sweeping mode
Reviewer's Guide by SourceryThis PR refactors the Sequence diagram for graph creation processsequenceDiagram
participant User
participant ShakeTune
participant GraphCreatorFactory
participant GraphCreator
participant MeasurementsManager
User->>ShakeTune: Execute command
ShakeTune->>GraphCreatorFactory: create_graph_creator(type, config)
GraphCreatorFactory-->>ShakeTune: graph_creator
ShakeTune->>GraphCreator: configure(params)
ShakeTune->>MeasurementsManager: collect measurements
MeasurementsManager-->>ShakeTune: measurements
ShakeTune->>GraphCreator: create_graph(measurements_manager)
GraphCreator->>GraphCreator: compute()
GraphCreator->>GraphCreator: plot()
GraphCreator-->>ShakeTune: figure
ShakeTune-->>User: Save graph
Class diagram showing the new graph creator architectureclassDiagram
class GraphCreator {
<<abstract>>
+configure()
+create_graph(measurements_manager)
}
class GraphCreatorFactory {
+create_graph_creator(type, config)
}
class VibrationsGraphCreator {
-_kinematics: str
-_accel: float
-_motors: List[Motor]
+configure(kinematics, accel, motor_config_parser)
+create_graph(measurements_manager)
}
class BeltsGraphCreator {
-_kinematics: str
-_accel_per_hz: float
+configure(kinematics, accel_per_hz)
+create_graph(measurements_manager)
}
class StaticGraphCreator {
-_freq: float
-_duration: float
-_accel_per_hz: float
+configure(freq, duration, accel_per_hz)
+create_graph(measurements_manager)
}
GraphCreator <|-- VibrationsGraphCreator
GraphCreator <|-- BeltsGraphCreator
GraphCreator <|-- StaticGraphCreator
GraphCreatorFactory ..> GraphCreator : creates
Class diagram showing the new measurement management systemclassDiagram
class MeasurementsManager {
-measurements: List[Measurement]
+get_measurements()
+add_measurement(measurement)
}
class Measurement {
+name: str
+samples: List
}
class ShakeTuneConfig {
+result_folder: Path
+keep_n_results: int
+keep_raw_data: bool
+chunk_size: int
+max_freq: float
+dpi: int
}
MeasurementsManager --> "*" Measurement : manages
MeasurementsManager --> "1" ShakeTuneConfig : uses
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}
There was a problem hiding this comment.
Hey @Frix-x - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 3 issues found
- 🟡 Complexity: 3 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
HeavilyModdedEnder3
approved these changes
Dec 23, 2024
|
Ok, now I think I'm ready to merge v5.0.0 with the couple of additions from tonight (new MAX_SCALE parameter and more infos in the header about the current test mode). Can you all confirm that it's still good for you? |
This was
linked to
issues
Dec 29, 2024
|
Will you also include this in the release? #140 |
|
Looks good. Like the additional info in the header. Very helpful. I have the Dev branch running on three different machines with no issues on two of them. On the third machine I've switched back to traditional "no sweep" behavior because the sweep speed of 60mm/s just happens to be where my motors resonate the most. I know mine is an edge case, but would be nice to be able to adjust the sweep speed. It has a sharp resonate peak between 57 and 73 mm/s. |
That's a Klippain config issue, but I will switch to it after v5.0.0 of S&T is released and this will likely be part of it
This can already be done by tweaking the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Shake&Tune v5.0.0 is a big release with many changes to the internal code, adding a dedicated
.stdataformat, a new sweep test (compatibility with the latest Klipper release). It also includes various improvements to mitigate "timer too close" errors on lower-end hardware, and a refactoring of the graph generation code for future extensibility, etc.Changes
Measurements are now stored in a new custom
.stdataformat, giving more control over what is saved to disk and how it is saved. This is easily extensible to store more things in the future. This new approach to data handling also reduces the chance of "timer too close" errors, reducing problems on less powerful hosts (e.g. CB1). But it's still not perfect, so if you still encounter problems, try stopping Crowsnest, Neopixels, etc. before running a test...Fixes the bug introduced with the latest Klipper version ('ResonanceTester' object has no attribute 'test' (latest Klipper break Shake&Tune) #172). Now Shake&Tune is compatible with the new sweep test, allowing pulse-only (original test) with or without additional motion sweeps to get better graphs, especially on less rigid machines. Please refer to the relevant part of the documentation to choose which test to use and when. In the same vein, the graph headers have also been updated to show what type of test was used and the associated parameters.
Introduced a proper command line interface (CLI) for generating graphs from stored data, with support for the new compressed
.stdatafile format, but also compatible with traditional.csvfiles. This is super useful for me to debug new features offline, but you can also use it to manually (re)generate some graphs from the CSV you saved.The plotting code has been completely rewritten and refactored. The plotting functions are now separated from the data calculations, making it easier to add new tests or switch to a different plotting backend in the future (and yes, I do have plans for that... Stay tuned!).
I've also added some draft code for a future Acceleration vs. Vibration vs. Smoothing trade-off plot. But the current plotting backend doesn't allow all the things I want to make it easy to read, so the code is there but disabled in this release until I find a proper way to present the data.
Added compatibility with the "limited" kinematics of Danger-Klipper / Kalico.
An optional
MAX_SCALE=parameter has been added to the macros to force the energy scale on the belts and input shaper graphs to certain values. This makes it easier to compare several consecutive runs by viewing the graphs side by side with the same scale.Miscellaneous fixes
Improve the startup logic to fail fast during Klipper initialization if a configuration problem is detected. This did not work properly in the past and led to problems where e.g. the
[resonance_tester]section was missing but remained undetected and led to a crash when running a test.Updated CI smoke tests to run against better Klipper mockups using a fake board and kinematics instead of the previously used
kinematics: noneand also by trying against multiple Python and Klipper versions.Fixed incorrect marker colors in the cross-belt plots (purple/orange points were inverted on this plot).
Increased the default timeout to 10 minutes, as the vibrations graph can take a long time to run, and some people were experiencing frequent failures due to this.
Since the
.stdatafile format is added, I now have full control over how and when the files are written to disk. So now Shake&Tune will also wait for the file writing to be finished before starting a new accelerometer measurement. This helps with timer too close errors, but also fixes a rare bug where the vibrations graph would not completely record all the data for one of the motors.Added support for alternative Klippy venv in case you don't have a standard installation