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

Allow to install Klippain in a given directory #455

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

arcadien
Copy link
Contributor

@arcadien arcadien commented Feb 6, 2024

Allow to install Klippain for a given Kiauh installed printer.

@arcadien arcadien marked this pull request as draft February 6, 2024 08:45
@arcadien arcadien force-pushed the feat/multi_printer branch 2 times, most recently from 457a34e to 6422329 Compare February 6, 2024 20:37
@arcadien arcadien marked this pull request as ready for review February 6, 2024 20:38
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.

Thanks for this PR!

This is something that was indeed needed and that I planned to work on at some point so thank you very much!

My only concern is about the cloned klippain repository: if you update one printer, all the Klippain printers of the host will be updated as well. But I think it's a good starting point already and not necessarly a problem if it's stated in the documentation.

As a side note, the Shake&Tune package will also need to be updated to be able to follow this change. Do you want to do a PR here as well?


printf "\n======================================\n"
echo "- Klippain install and update script -"
printf "======================================\n\n"

# Run steps
preflight_checks
BACKUP_DIR="${BACKUP_PATH}/$(date +'%Y_%m_%d-%H%M%S')"
Copy link
Owner

Choose a reason for hiding this comment

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

Is it possible to add the printer name in the backup name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello, I tried to pack all changes at the same place. BACKUP_DIR is updated (printer name is append) at line 59.
I may propagate to Shake&Tune once I do it for my CR-10.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I discover an issue with the patch. printer_data is also hardcoded in shell_commands.cfg. I had to manually update.
The file is a symlink from Klippain installation directory, and hence is shared across all printer configuration.

Copy link
Owner

Choose a reason for hiding this comment

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

BACKUP_DIR is updated (printer name is append) at line 59

Oops, my bad, I did not see that. But can you change the "_" to a "/" to keep all the backups in the same folder and avoid very long folder names?

I discover an issue with the patch. printer_data is also hardcoded in shell_commands.cfg. I had to manually update.
The file is a symlink from Klippain installation directory, and hence is shared across all printer configuration.

Mhmm, you are indeed right. But I don't think there's any solution for this other than stating in the documentation that an override is needed... But since "multi-klipper" is not an officially supported way to install Klipper, this case should be very marginal and I think it's ok until we find a better way (maybe with the new Klipper plugin system that will probably come later this year).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok done

@Frix-x Frix-x added the tracking This issue is tracked and work will be done label Feb 7, 2024
@tsk-2222
Copy link
Contributor

tsk-2222 commented Feb 7, 2024

I did a kiauh multi install of 10 printers early last year. At the time, the installed directories were named printer_1_data to printer_10_data. It also created klipper-1.service to klipper-10.service.

When I try to run arcadien's install script with this command: './install.sh 8', the script fails because the '8_data' directory does not exist.

When I try to run the install script with this command: './install.sh printer_8', the script fails because it is looking for klipper-printer_8.service which doesn't exist.

Are you guys seeing this or do I have a 'funny' installation?

@arcadien
Copy link
Contributor Author

arcadien commented Feb 8, 2024

@tsk-2222 i'll double check that later. AFAIR in my install, Kiauh created /home/pi/xx_data and klipper-xx.service for each xx named printer. It looks you have an extra printer- prefix to data folder which i have not.
I'll keep PR posted.

@tsk-2222
Copy link
Contributor

tsk-2222 commented Feb 8, 2024

@arcadien I can't test the kiauh scripts now but it looks like my situation happens when you tell kiauh how many printer instances to setup and DO NOT assign custom names. In this case, # by KIAUH convention, all instance names of only numbers need to be prefixed with 'printer_'

You can see this in the kiauh utilities.sh at line 700.
if [[ ${name} =~ ^[0-9]+$ ]]; then cfg_dirs+=("${HOME}/printer_${name}_data/config") else cfg_dirs+=("${HOME}/${name}_data/config")

@arcadien
Copy link
Contributor Author

Ok so in that case, there will be printer_xx_data folder, but klipper-xx.service

@arcadien arcadien force-pushed the feat/multi_printer branch 2 times, most recently from bdcb8ee to 1c09eb8 Compare February 10, 2024 11:10
@arcadien
Copy link
Contributor Author

@tsk-2222 it should be fixed by last commit

@tsk-2222
Copy link
Contributor

tsk-2222 commented Feb 17, 2024

@arcadien
I'm not a bash pro by any means, but...
If I'm not mistaken, there's still a problem. If the printer is called 'printer_8' then the directory check will succeed, but then the next check at line 56 will always fail.

I think that maybe the script should consider the full name of the printer to be 'printer_8' for example and not just '8'. Then it could be handled in a similar way as if it was manually named it something like voron_8. Then during the check for the klipper service, just strip out any name beginning with 'printer_' and search for the klipper.8 service that kiauh created.

I hope that makes sense.

@arcadien arcadien force-pushed the feat/multi_printer branch from 282c361 to fd95850 Compare March 3, 2024 21:37
@Frix-x
Copy link
Owner

Frix-x commented Mar 4, 2024

What is the state of this PR. Is it ok now? @tsk-2222 can you confirm it's ok for you now?
Unfortunately, I can only test it on a "single printer" host on my side.

@tsk-2222
Copy link
Contributor

tsk-2222 commented Mar 4, 2024

It's still broken on my end. Something like the following snippet seems to work for the Klippain install. It just strips off any leading "printer_" characters from SERVICE_NAME and allow the script to be called as such: "./install.sh printer_8"

if [ "$FORCE_PRINTER_NAME" != "" ]; then
      if [ -d "${HOME}/${FORCE_PRINTER_NAME}_data" ]; then
        echo "[PRE-CHECK] Installing Klippain for printer '${FORCE_PRINTER_NAME}'"
        PRINTER_NAME=$FORCE_PRINTER_NAME
#        if [[ ${PRINTER_NAME} =~ ^[0-9]+$ ]]; then
#          TARGET_DIR=${HOME}/printer_${PRINTER_NAME}_data
#        else
          TARGET_DIR=${HOME}/${PRINTER_NAME}_data
#        fi
        USER_CONFIG_PATH="${TARGET_DIR}/config"
        SERVICE_NAME=klipper-${PRINTER_NAME#printer_}   #strip off printer_ prefix to match kiauh service naming

The call to the shaketune install script will of course still fail until that gets updated also.

@tsk-2222
Copy link
Contributor

tsk-2222 commented Mar 12, 2024

I made some changes to install.sh and I believe it should work but I would like some more eyeballs.

tsk-2222@dfd8e61

@arcadien arcadien force-pushed the feat/multi_printer branch from d952cac to 50903fd Compare April 26, 2024 11:18
@arcadien arcadien force-pushed the feat/multi_printer branch from 50903fd to 66de9e4 Compare May 25, 2024 07:44
@arcadien arcadien force-pushed the feat/multi_printer branch from 66de9e4 to f36308a Compare May 25, 2024 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tracking This issue is tracked and work will be done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants