Update Measuring_Resonances.md

[charminULTRA]

Current commands in the documentation, using the *, results in bad chart output when more than one .csv file exists in the tmp folder. This isn't obvious for people who may not know that the * is a wildcard character.

I was pulling out my hair trying to understand my results until I realized my multiple runs of data were all being used in the same graph! I think this simple pitfall should be fixed.

[freakydude]

I used this as a feature - so you can measure multiple times (e.g. with a full and empty spool on top) and the results will be merged.

If that was the intention, I would comment the script line with - "be aware, all matching csv will be merged" or something like that.

Kind regards

[JamesH1978]

Thank you for submitting a PR, pleas refer to point 3 in "What to expect in a review" in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md and provide a signed off by line.

Thanks
James

[github-actions]

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
   If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

[dewi-ny-je]

@charminULTRA can you apply the corrections I suggested? then we can hopefully have the changes approved

[charminULTRA]

@charminULTRA can you apply the corrections I suggested? then we can hopefully have the changes approved

I don't see any suggested corrections, where can I view that? @dewi-ny-je

[charminULTRA]

Thank you for submitting a PR, pleas refer to point 3 in "What to expect in a review" in https://github.com/Klipper3d/klipper/blob/master/docs/CONTRIBUTING.md and provide a signed off by line.

Thanks James

@JamesH1978 How do I add a sign off if I already submitted the commit?

Not entirely so sure why this is so rigorous considering the Klipper documentation still assumes use of OctoPrint lol

[charminULTRA]

Sign-off by Jonathan Williams [email protected]

[dewi-ny-je]

Do not change this line: grammatically brackets are NOT meant to include whole sentences. If it's more than few words, it's not for brackets.

Add a line before line 458, after the code, as suggested:

457bis Keep in mind that these commands (written with the "*") will merge all available CSV files
from the specified folder to generate a single calibration graph. It may be useful if you tested the
machine in various conditions and you want a single result which takes all of them into account.

Remember to split at column 80, as the file originally was, don't add long lines which make the code difficult to read. Lines are free of charge :)

[charminULTRA]

Do not change this line: grammatically brackets are NOT meant to include whole sentences. If it's more than few words, it's not for brackets.

Add a line before line 458, after the code, as suggested:

457bis Keep in mind that these commands (written with the "*") will merge all available CSV files
from the specified folder to generate a single calibration graph. It may be useful if you tested the
machine in various conditions and you want a single result which takes all of them into account.

Remember to split at column 80, as the file originally was, don't add long lines which make the code difficult to read. Lines are free of charge :)

I don't fully agree with this edit. There's no part of the documentation that already tells or assumes the user should or could use multiple CSV files, so the inherent assumption here is that the user is creating a single CSV for each axis. Given that, I don't think we should say things like "keep in mind" down below, but more of an upfront warning like "do not run this with multiple CSVs in your folder, unless you intend to blend the results". You want to alert the user before they perform an action, not after.

I assume the original intent of using a wildcard was to save the user time from writing out the file name, but again it's not clear from the documentation.

Will submit a modified commit.

[dewi-ny-je]

@KevinOConnor for as much as it can be worth, I now support and I checked the changes. I think they are ready to be merged.

[KevinOConnor]

Thanks. Looks fine to me. I'll wait a few days to see if @dmbutyugin has any comments.

-Kevin

[dmbutyugin]

Thanks! I agree that the documentation is not obvious in this regard. I think it may be valuable to provide some additional context, so I wrote some suggestion on how to expand the text without suggesting that multiple files are not supported, but highlighting the typical usecases and that most users would use it after a single test run per axis.

[dmbutyugin]

Sorry for jumping late into the conversation. But I'd rephrase and expand that sentence to have a note after the commands, something like this:

...
script on a Raspberry Pi. To do that, run the following commands:

~/klipper/scripts/calibrate_shaper.py /tmp/resonances_x_*.csv -o /tmp/shaper_calibrate_x.png
~/klipper/scripts/calibrate_shaper.py /tmp/resonances_y_*.csv -o /tmp/shaper_calibrate_y.png

Note that '*' in the above commands acts as a wildcard for matching files, thus these commands
should usually be executed after one measurement per axis, with a single CSV file created for each
axis measured. If multiple CSV files were generated, then the script will process them all and
average the results. This can be useful, for example, if a resonance test was done at multiple test
points. On the other hand, if a user was, say, adjusting the belt tension between the test runs,
it is not valid to use the above commands as-is, and the user should either delete all but the latest
measurements, or specify the exact file names of the CSV files for the appropriate test run.
Following the commands above, the script will generate the charts ....

[charminULTRA]

I think your suggestion is too verbose and you also moved the commentary AFTER the commands, rather than before. I think it's critical to have this information before the user is presented commands, because users tend to run commands when they are presented, rather than reading the information provided after.

[charminULTRA]

How about this combination of your comment and my existing PR (placed before commands)

...
script on a Raspberry Pi. This script is intended to be run with a single CSV file for each axis measured, although it can be used with multiple CSV files if you desire to average the results. Averaging results can be useful, for example, if resonance tests were done at multiple test points. Delete the extra CSV files if you do not desire to average them.

To run the processing script, execute the following commands:
...

[charminULTRA]

Bump

[dewi-ny-je]

I approve the final version proposed. I also think the suggestion by @dmbutyugin was too verbose.

[KevinOConnor]

Okay, thanks. If there are no further comments I'll look to commit in a few days.

-Kevin

[dmbutyugin]

OK, thanks, I'm also fine with the current version of the notice.

[KevinOConnor]

Thanks.

-Kevin