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

shaper_calibrate: store accel_per_hz #224

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

rogerlz
Copy link
Contributor

@rogerlz rogerlz commented Apr 30, 2024

#221

image

Checklist

  • pr title makes sense
  • squashed to 1 commit
  • added a test case if possible
  • if new feature, added to the readme
  • ci is happy and green

@Frix-x
Copy link
Contributor

Frix-x commented May 10, 2024

Hello,

Good initiative, I kind of tweaked something in Shake&Tune to print it on the header as well, but I much prefer to get a proper access like you've done.

As a side note, this will probably break current Shake&Tune with DangerKlipper until a fix is done on my side to account for the changes in the CSV, etc...

@rogerlz
Copy link
Contributor Author

rogerlz commented May 16, 2024

Hello,

Good initiative, I kind of tweaked something in Shake&Tune to print it on the header as well, but I much prefer to get a proper access like you've done.

As a side note, this will probably break current Shake&Tune with DangerKlipper until a fix is done on my side to account for the changes in the CSV, etc...

would it help if I make it a parameter default to False?

[resonance_tester]
store_accel_per_hz: False

@Frix-x
Copy link
Contributor

Frix-x commented May 16, 2024

As discussed with bwnance on Discord, I don't think adding another parameter is needed: I don't want to add even more complexity to this.

I'll try to account for it and auto-detect the CSV file format directly :)

@rogerlz
Copy link
Contributor Author

rogerlz commented May 16, 2024

As discussed with bwnance on Discord, I don't think adding another parameter is needed: I don't want to add even more complexity to this.

I'll try to account for it and auto-detect the CSV file format directly :)

great! we will hold off merging this until you are ready.

@Frix-x
Copy link
Contributor

Frix-x commented May 17, 2024

I think since it doesn't block the use of the default script, you can merge it now.
I don't want to block it just for Shake&Tune compatibility, since it's not problematic and works on its own with the original Klipper tools that are still available. But I'll try to fix the compatibility as soon as possible.

@rogerlz rogerlz merged commit 782e062 into master Jun 10, 2024
2 checks passed
@rogerlz rogerlz deleted the accels_per_hz_shaper branch June 10, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants