-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
Conversation
Realign develop with main
Yet another attempt to solve TTC errors...
Reviewer's Guide by SourceryThis 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 measurementsequenceDiagram
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
Sequence diagram for saving stdatasequenceDiagram
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
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this 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
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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