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

Run ShakeTune as an in-process Klipper module #100

Merged
merged 3 commits into from
May 8, 2024

Conversation

ozelentok
Copy link
Contributor

@ozelentok ozelentok commented May 4, 2024

  • Removes the need for the G-Code Shell Command extension.
  • A separate python venv is not required anymore since ShakeTune now uses Klipper's python environment.
  • Allows future code to access Klipper internal functions to fetch data instead of parsing the commands output.

Possible Issues:

  • git versioning functions require gitpython, which needs to be installed manually since we don't have a seperate venv anymore (Without gitpython installed the tuning will work but will throw a couple of warnings about it).
  • Before this PR the code would run in a different process entirely and pipe the entire output to the klipper console.
    • With this PR the code will be run in a low priority thread. I used threading instead of a process since it now needs access to gcode.respond_info to output the information to the console.
    • print_with_c_locale prints using gcode.respond_info instead of using stdout, I'm not too happy with that patch but I didn't want to make any more heavy changes to files referencing it and I wanted to keep working if invoked from CLI.
  • argparse is used to parse the parameters from the macros, a bit hacky but I wanted to reduce the amount of changes in this PR.

@ozelentok ozelentok force-pushed the develop branch 2 times, most recently from 3be91fd to f7609c9 Compare May 5, 2024 11:06
@Frix-x
Copy link
Owner

Frix-x commented May 5, 2024

Hello,

Thank you very much for this PR. As we already discussed together, it was part of my plans to start working on this so I'm pretty happy with it :)

Some quick pre-review comments just for the context:

  • print_with_c_locale was created due to a problem in how the internal klipper console works with the gcode shell command plugin: it doesn't support special characters. So in the past, if the OS locale was configured in some exotic language and the bash script printed to stdout some strange system messages, then this would cause Klipper to crash (and require a firmware restart) and the graph wouldn't be generated. So I added this to force all system messages to the C locale, which is safe in this regard, to fix this problem. But this may not be needed anymore if you stop using the gcode_shell_command plugin.
    Just for reference, the error was due to the decode() in this line of the plugin:
    https://github.com/dw-0/kiauh/blob/f2691f33d30bfac26591361cdb2295741a401bc1/resources/gcode_shell_command.py#L35
    -> So this hacky patch can probably be removed now in favor of proper internal Klipper logging functions when run from Klipper (or by using a basic print("blabla blah") when a graph creator is run manually)

  • Regarding the suppression of the venv, this is understandable if you are running it as a thread, since it's the one from the klipper that will be used. But this makes me wonder how to streamline the user experience. Indeed, GitPython is one of the packages, but there's also matplotlib/numpy which are needed as a base also by original Klipper scripts and they may or may not be already installed and there's also for S&T the need for scipy which for sure is not installed in default Klipper venv. Do you think it would be possible to add to the install script the installation of the requirements.txt in the Klipper venv directly? and update the moonraker.conf update manager section to point to the Klipper venv to keep the python packages up to date with each S&T update?

  • argparse to parse macro parameters is indeed a pretty hacky way to do it: I would say that if we run S&T as a Klipper module, this will need to be removed in the future. But ok to leave it for now for this first PR. But for context, the is_worflow.py (renamed here as __init__.py) doesn't need to be run from the command line. Say this is Klipper's "automated" entry point. However, what I want to keep is the ability to manually run each graph generator on the CSV files I choose, with the parameters I want (that's why optparse is still available in each of them with if __name__ == '__main__': main()). But I never run is_workflow.py manually at the moment, only the subscripts.

@Frix-x Frix-x added the enhancement New feature or request label May 5, 2024
@ozelentok ozelentok force-pushed the develop branch 2 times, most recently from 2eb0363 to d587017 Compare May 7, 2024 12:33
@ozelentok
Copy link
Contributor Author

Hey,

A few updates

  • print_with_c_locale has been replaced with ConsoleOutput.print that outputs to the Klipper console (and to stdout when running from CLI)
    • It was possible to use the logging module Klipper uses but it doesn't output to the Klipper console.
    • Might be useful for hiding lengthy confusing technical details from end-users in the future
  • Dependencies are now installed to klipper venv via the install script and updated though Moonraker's update manager

Copy link
Owner

@Frix-x Frix-x left a comment

Choose a reason for hiding this comment

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

Thank you very much! I think it's a good first step to get something better integrated! I will do some testing on my end to validate that the installation is ok and there are no regressions and I will merge to the develop branch!

Here are some open notes to myself about things that still need to be done for a later commit (or if you want to continue working on this on another PR):

  • Remove the SHAKETUNE_POSTPROCESS and declare specialized commands for each graph type that doesn't use argparse.
  • Remove the need for [include K-ShakeTune/*.cfg], as I think we can also put it in the plugin folder and load it automatically in the ShakeTune class (as it's done for example in Tmc Autotune for the engine database: https://github.com/andrewmcgr/klipper_tmc_autotune/blob/6463d8ae896193d718563efa39dbc46db881f7fe/autotune_tmc.py#L62-L71). This would simplify the configuration for users and avoid having to symlink anything in their ~/printer_data/config folder. The only section needed would be [shaketune].
  • For even later, the gcode macros could even be implemented directly in Python, but that's not for today as I think it's a bit of work, but at least the previous point about automatic macro loading would allow for a smooth transition when this will be available.
  • Modify the motor log parser to not use the actual log, but to access the TMC fields directly from Klipper objects (this is something I find pretty dirty right now, but I had no other solution before your PR).
  • Add some variables in the [shaketune] section to configure some defaults or allow to change like the result folder, the default used accelerometer name, etc...
  • Add some clean mechanism in the install script to remove the old venv if found

@Frix-x Frix-x merged commit 3a0c0c4 into Frix-x:develop May 8, 2024
@Frix-x
Copy link
Owner

Frix-x commented May 10, 2024

EDIT: working on it right now. I have something working, just need to finish converting the jinja macros to Python :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants