Skip to content

Motor resonance filter - #131

Open
Frix-x wants to merge 5 commits into
developfrom
mot-res
Open

Motor resonance filter#131
Frix-x wants to merge 5 commits into
developfrom
mot-res

Conversation

@Frix-x

@Frix-x Frix-x commented Jun 23, 2024

Copy link
Copy Markdown
Owner

WIP

Summary by Sourcery

This pull request introduces a motor resonance filter feature to Shake&Tune, allowing users to apply and remove motor resonance filters for X and Y axes. It also includes refactoring of the ShakeTune class for better configuration and command handling, enhancements to motor frequency profile computation, and updates to the documentation to reflect the new configuration options.

  • New Features:
    • Introduced a motor resonance filter feature to Shake&Tune, allowing users to apply and remove motor resonance filters for X and Y axes.
  • Enhancements:
    • Refactored the ShakeTune class to improve configuration initialization and command registration.
    • Enhanced the motor frequency profile computation by separating the calculation of motor characteristics and updating the plotting functions to include these characteristics.
  • Documentation:
    • Updated README.md to include new configuration options for motor resonance filters, including motor_freq, motor_freq_x, motor_freq_y, motor_damping_ratio, motor_damping_ratio_x, and motor_damping_ratio_y.

@Frix-x
Frix-x changed the base branch from main to develop June 23, 2024 21:32
@Frix-x Frix-x added the enhancement New feature or request label Jun 23, 2024
@Frix-x
Frix-x marked this pull request as ready for review June 24, 2024 17:11
@sourcery-ai

sourcery-ai Bot commented Jun 24, 2024

Copy link
Copy Markdown
Contributor

Reviewer's Guide by Sourcery

This pull request introduces a motor resonance filter feature to Shake&Tune, allowing users to apply and remove motor resonance filters. It also includes refactoring for better configuration initialization and command registration, and updates the documentation to reflect the new configuration options.

File-Level Changes

Files Changes
shaketune/shaketune.py
shaketune/motor_res_filter.py
Introduced motor resonance filter feature, including configuration, command registration, and filter application/removal logic.
shaketune/graph_creators/vibrations_graph_creator.py Refactored motor profile computation and plotting to integrate motor resonance characteristics.
README.md Updated documentation to reflect new motor resonance filter configuration options.

Tips
  • Trigger a new Sourcery review by commenting @sourcery-ai review on the pull request.
  • You can change your review settings at any time by accessing your dashboard:
    • Enable or disable the Sourcery-generated pull request summary or reviewer's guide;
    • Change the review language;
  • You can always contact us if you have any questions or feedback.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

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 and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 9 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟡 Documentation: 2 issues found

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 to tell me if it was helpful.

Comment thread shaketune/shaketune.py
Comment thread shaketune/shaketune.py Outdated
Comment thread shaketune/shaketune.py
Comment thread shaketune/shaketune.py
Comment thread shaketune/shaketune.py
Comment thread shaketune/motor_res_filter.py
Comment thread shaketune/motor_res_filter.py
Comment thread shaketune/motor_res_filter.py Outdated
Comment thread README.md Outdated
Comment thread README.md Outdated
@Frix-x

Frix-x commented Jun 29, 2024

Copy link
Copy Markdown
Owner Author

@sourcery-ai review

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

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 and they look great!

Here's what I looked at during the review
  • 🟡 General issues: 8 issues found
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟡 Documentation: 3 issues found

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 to tell me if it was helpful.

Comment thread shaketune/shaketune.py
Comment thread shaketune/shaketune.py
Comment thread shaketune/shaketune.py
Comment thread shaketune/graph_creators/vibrations_graph_creator.py Outdated
Comment thread shaketune/graph_creators/vibrations_graph_creator.py Outdated
Comment thread shaketune/motor_res_filter.py
Comment thread README.md
Comment thread README.md Outdated
Comment thread README.md
Comment thread shaketune/motor_res_filter.py Outdated
# In DangerKlipper, the pulse train is large enough to allow the
# convolution of any shapers in order to craft the new combined shapers
# so we can use the MZV shaper (that looks to be the best for this purpose)
df = math.sqrt(1.0 - self.damping_x**2)

@Zeanon Zeanon Aug 25, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You only use damping_x here for both x and y (while technically they should be the same it doesnt make sense to have them seperate and then just ignore the value for y)
Same thing for the freuqncy

@Frix-x Frix-x Feb 17, 2025

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

You are right, it's a mistake from my side, but it's due to the fact that currently I only compute an averaged set of values that are the same for both motors.

So I also need to decompose the frequency calculation to provide it for each motor instead of providing it like it's done for now

@Frix-x Frix-x added the experimental This is very experimental and not ready to be merged for all label Dec 30, 2024
@Frix-x
Frix-x force-pushed the develop branch 2 times, most recently from 4b783f8 to d99b787 Compare February 24, 2025 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request experimental This is very experimental and not ready to be merged for all

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants