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

Refactor accelerometer data storage to fix timer issues #206

Merged
merged 9 commits into from
Feb 24, 2025
Merged

Conversation

Frix-x
Copy link
Owner

@Frix-x Frix-x commented Feb 24, 2025

Summary

This new version finally fixes the "timer too close" errors that many people were getting when using low-power host computers like the CB1. To allow this, I introduced a better .stdata file format based on JSON and Zstandard compression to store accelerometer readings, with improvements to the way data is written to disk asynchronously, and some tricks to limit RAM, CPU, and I/O usage. It was hard, but I think we finally did it, guys!

I want to thank all the people who helped over the last few months by diagnosing and reproducing this with dozens of tests on their own printers while sharing their logs to try my changes, and who finally helped solve it.

New Features and fixes

  • Implements a new '.stdata' file format based on JSON and Zstandard compression for storing accelerometer readings, replacing the previous pickle-based format. This is a complete rewrite of how the data is collected from Klipper and stored to disk in an asynchronous manner, without interfering with the main Klipper process or doing heavy processing like it did before. In fact, it's now using a dedicated sub-process with a queue to manage write operations outside of the main Klipper process, with tricks to store chunks of accelerometer readings over time and synchronization mechanisms to wait when something is currently being written to disk.
  • Improved CLI support to work with the latest .stdata file format
  • Improved accuracy of the static frequency command by allowing the use of floats instead of integers for the frequency parameter
  • Fixed the peak alignment issue in the input shaper graphs that could cause out-of-bounds errors in some rare cases

Copy link
Contributor

sourcery-ai bot commented Feb 24, 2025

Reviewer's Guide by Sourcery

This pull request introduces significant changes to the data writing and processing pipeline in ShakeTune. It implements a dedicated writer process for asynchronous data writing, improves file handling, and updates the graph creation process to use the new APIs. These changes aim to improve performance, prevent errors, and provide a more robust and reliable experience.

Sequence diagram for adding a measurement

sequenceDiagram
  participant MM as MeasurementsManager
  participant WP as WriterProcess
  participant Q as Queue

  MM->MM: add_measurement(name, samples)
  alt writer process is None
    MM->WP: Create Process(target=_writer_loop)
    WP->WP: start()
  end
  MM->Q: put(measurement)
  alt len(measurements) > chunk_size
    MM->MM: _flush_chunk()
    loop for each measurement in chunk
      MM->Q: put(measurement)
    end
    MM->MM: clear_measurements(keep_last=True)
  end
Loading

Sequence diagram for saving stdata

sequenceDiagram
  participant MM as MeasurementsManager
  participant WP as WriterProcess
  participant Q as Queue
  participant K as Klipper Reactor

  MM->MM: save_stdata()
  alt writer process is None
    MM->MM: _flush_chunk()
    loop for each measurement in chunk
      MM->Q: put(measurement)
    end
    MM->MM: clear_measurements()
  end
  MM->Q: put(STOP_SENTINEL)
  MM->K: monotonic()
  loop until writer process is not alive
    K->K: pause(0.05)
  end
  MM->MM: rename temp file to final file
Loading

File-Level Changes

Change Details Files
Refactored the MeasurementsManager class to use a dedicated writer process for asynchronous writing of measurement data to disk, improving performance and preventing Timer too close errors in Klipper.
  • Replaced the chunk-based file saving mechanism with a dedicated writer process using a queue for asynchronous writing.
  • Implemented Zstandard compression directly within the writer process using streaming to reduce memory footprint.
  • Added a STOP_SENTINEL to signal the writer process to terminate gracefully.
  • Introduced a timeout mechanism to ensure the writer process completes within a reasonable time.
  • Modified the add_measurement method to enqueue measurements for writing and flush chunks to disk.
  • Updated the save_stdata method to signal the writer process to finish and wait for its completion.
  • Replaced pickle with json for serialization.
  • Added a temporary file to avoid data loss in case of errors.
  • Added a check to avoid starting a new measurement while the previous one is still being written on disk.
  • Added a check to ensure that the Klipper reactor is available before waiting for the writer to finish.
shaketune/helpers/accelerometer.py
Modified the ShakeTuneProcess class to accept a list of filenames as input, load measurements from either .stdata or .csv files, and pass the MeasurementsManager instance to the graph creation process.
  • Updated the run method to accept a list of filenames instead of a MeasurementsManager instance.
  • Added logic to load measurements from .stdata or .csv files based on the file extension.
  • Modified the _shaketune_process method to receive a list of filenames, load the measurements and pass the MeasurementsManager instance to the graph creator.
  • Added file existence checks and error handling for invalid or missing input files.
shaketune/shaketune_process.py
Updated the graph creation process to define an output target before saving the figure and to remove the raw data file after graph creation.
  • Added a define_output_target method to the GraphCreator class to set the output file path.
  • Modified the _save_figure method to use the defined output target when saving the figure.
  • Removed the measurements_manager parameter from the _save_figure method.
  • Added logic to delete the raw data file after graph creation if keep_raw_data is disabled.
shaketune/graph_creators/graph_creator.py
Adjusted the accelerometer commands to use the new MeasurementsManager and GraphCreator APIs, including defining output targets and saving data.
  • Modified the accelerometer commands to create a MeasurementsManager instance with the correct filename.
  • Updated the accelerometer commands to define the output target for the graph creator.
  • Modified the accelerometer commands to save the measurements data using the save_stdata method.
  • Removed the wait_for_data_transfers calls.
  • Added date to the output filename.
shaketune/commands/axes_shaper_calibration.py
shaketune/commands/excitate_axis_at_freq.py
shaketune/commands/compare_belts_responses.py
shaketune/commands/create_vibrations_profile.py
shaketune/commands/axes_map_calibration.py
Updated graph creators to remove the measurements_manager parameter from the _save_figure method.
  • Removed the measurements_manager parameter from the _save_figure method.
shaketune/graph_creators/axes_map_graph_creator.py
shaketune/graph_creators/shaper_graph_creator.py
shaketune/graph_creators/vibrations_graph_creator.py
shaketune/graph_creators/belts_graph_creator.py
shaketune/graph_creators/static_graph_creator.py
Updated the CLI to use the new define_output_target method.
  • Replaced override_output_target with define_output_target.
shaketune/cli.py
Fixed a typo in the resonance test.
  • Fixed a typo in the resonance test.
shaketune/helpers/resonance_test.py

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. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the 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 exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

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 @Frix-x - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider adding a method to MeasurementsManager to stop the writer process and wait for it to finish.
  • The MeasurementsManager class has grown quite large; consider refactoring it into smaller, more manageable classes.
Here's what I looked at during the review
  • 🟡 General issues: 1 issue found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟡 Complexity: 1 issue found
  • 🟢 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.

@Frix-x Frix-x changed the title New release New release @sourcery-ai Feb 24, 2025
@sourcery-ai sourcery-ai bot changed the title New release @sourcery-ai Refactor accelerometer data storage to fix timer issues Feb 24, 2025
@Frix-x Frix-x merged commit 265e8f6 into main Feb 24, 2025
11 checks passed
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.

1 participant