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

add a variable to be able to override some future mmu variables #501

Closed
wants to merge 2 commits into from

Conversation

Benoitone
Copy link
Collaborator

in the future HH some variables are already define in HH. This variable can be use to choose between klippain default variables (for exemple for park position....) or the one define in the future upgrade of HH

@Benoitone
Copy link
Collaborator Author

@Frix-x please check this before merge develop in main...

@Frix-x
Copy link
Owner

Frix-x commented Feb 25, 2024

Mhmm I'm not sure this is a good idea: I would use the Klippain vars all the time to keep some consistency and avoid problems. Is this possible? I'm not sure to understand all the impact of this and what are the HH variables in question...

@Frix-x Frix-x added the need to understand Bug look to be identified but more time need to be involved to understand and solve label Feb 25, 2024
@Benoitone
Copy link
Collaborator Author

The future version of HH has is own macros and introduce some new "variables" already use in klippain like for Park position, travel speed, home mmu, check gates... with HH default values. So if user want to use HH automatics macros it use HH variables.
For exemple: For park position: see HH variables
So I have 2 choises: let like this is actualy, and depending on the choices made in the slicer configuration by the user, 2 differents park position will be used (klippain PARK and/or _MMU_PARK), which involves the user having to define the same things in 2 different files.
Or force MMU to use klippain variable like I would like to do.
By introducing this new klippain variable i can let the choise to the user...

The HH macro are very cool and it's a good think if I replace the one in klippain to use the one from HH.

I don't know if I made myself understood correctly?

@Frix-x
Copy link
Owner

Frix-x commented Feb 26, 2024

Ok, so I think the best would be to remove the Klippain variables and use only HH vars and macros to leverage everything on HH: this will avoid duplicates and will also make it easier to maintain since there is less code in Klippain :)

@Surion79
Copy link
Collaborator

Ok, so I think the best would be to remove the Klippain variables and use only HH vars and macros to leverage everything on HH: this will avoid duplicates and will also make it easier to maintain since there is less code in Klippain :)

so Klippain requires HH installed to run a park macro for non-HH users? the wording is confusing for me "use only HH vars and macros"

@Frix-x
Copy link
Owner

Frix-x commented Feb 26, 2024

No, I was just saying that it would be easier to use HH variables for HH macros and Klippain variables for Klippain macros, and also the easier since there would be nothing to do. But now that I'm looking a bit deeper, I see that HH does a lot of stuff like defining another PARK, etc... So I would say that these functionalities should stay in Klippain as they are used by a lot of people and would avoid having different behavior when we use an MMU or not. What do you think about this?

@Surion79
Copy link
Collaborator

I haven't looked deeper into it yet. I would rather go with your suggestion and if there are potentials for optimizing I would go for it then. (Without deeper look: like include only one park defined if mmu is used)

@Benoitone
Copy link
Collaborator Author

I must make some test but I think with the new version of HH we can remove all the user_variable in variable.cfg
And let HH manage everything related to the mmu...

My question is rather about the new variables that are used by HH and that sometimes duplicate that of Klippain.

My opinion would be that they should be overrided. But in this case either we just add a variable in variables.cfg (like variable_allow_klippain_override_mmu_variables: True ) to give the user the choice... Or we do it automatically and the variables concerned in the HH configuration file will be ignored without the user being able to do anything about it.

@Surion79
Copy link
Collaborator

could you please list the vars in question and their same/different functionality?

there are at least three things to consider:

  • Klippain is used by more non-mmu users than mmu users: we need to keep them both in our minds when changing stuff
  • Dependancy of external repo: merging variables with an external repo makes it dependant, so if something HH is changed which is relevant, Klippain need to jump and correct it, (e.g. change a value type of a variable) and release it right away.
  • Release schedule between a configuration "tool" and a functional "extra" is different. If something in Klippain is not working, you can basically add and/or override stuff. that is not possible for solutions which are integrated in "klipper extras".

@Frix-x
Copy link
Owner

Frix-x commented Feb 27, 2024

Or we can just put a routine in the startup delayed gcode to just "copy" the Klippain vars into the HH vars to avoid having to override the macros but still have the same values copied. This will be much easier to maintain and will allow both to work independently while allowing them to be decoupled.

I mean having in the startup things like this for every duplicated variables:

{% set klippain_park = printer["gcode_macro _USER_VARIABLES"].park_position_xy %}
SET_GCODE_VARIABLE MACRO=_MMU_SEQUENCE_VARS VARIABLE=park_xy VALUE={klippain_park}

@Benoitone
Copy link
Collaborator Author

Benoitone commented Feb 27, 2024

Or we can just put a routine in the startup delayed gcode to just "copy" the Klippain vars into the HH vars to avoid having to override the macros but still have the same values copied. This will be much easier to maintain and will allow both to work independently while allowing them to be decoupled.

I mean having in the startup things like this for every duplicated variables:


{% set klippain_park = printer["gcode_macro _USER_VARIABLES"].park_position_xy %}

SET_GCODE_VARIABLE MACRO=_MMU_SEQUENCE_VARS VARIABLE=park_xy VALUE={klippain_park}

That exactly what I have made... but just with an if with a variable to let user choose which one he wants to use with mmu macro.
Only with MMU macro from HH...

i use this:

[delayed_gcode _MMU_OVERRIDES]
initial_duration: 1
gcode:
    {% set klippain_mmu_enabled = printer["gcode_macro _USER_VARIABLES"].klippain_mmu_enabled %}
    {% set allow_klippain_override_mmu_variables = printer["gcode_macro _USER_VARIABLES"].allow_klippain_override_mmu_variables %}
        {% if klippain_mmu_enabled %}
            {% if printer["gcode_macro _USER_VARIABLES"].allow_klippain_override_mmu_variables %}
            ### _MMU_SEQUENCE_VARS overrides
                {% set Px, Py = printer["gcode_macro _USER_VARIABLES"].park_position_xy|map('float') %}
                SET_GCODE_VARIABLE MACRO=_MMU_SEQUENCE_VARS VARIABLE=park_xy VALUE={Px},{Py}
                SET_GCODE_VARIABLE MACRO=_MMU_SEQUENCE_VARS VARIABLE=park_z_hop VALUE={printer["gcode_macro _USER_VARIABLES"].park_lift_z}
                SET_GCODE_VARIABLE MACRO=_MMU_SEQUENCE_VARS VARIABLE=travel_speed VALUE={printer["gcode_macro _USER_VARIABLES"].travel_speed}
                SET_GCODE_VARIABLE MACRO=_MMU_SEQUENCE_VARS VARIABLE=lift_speed VALUE={printer["gcode_macro _USER_VARIABLES"].z_drop_speed}
            {% endif %}
    {% endif %}

@Frix-x
Copy link
Owner

Frix-x commented Feb 27, 2024

Do we really need a variables for this? I would prefer to do it automatically for everyone to avoid having different behavior between people. It will probably also help debugging.

But at least we are ok on the technique, that's a good point :)

@Surion79
Copy link
Collaborator

i think this is not even all of it.
Also it is possible to deactivate the MMU, so the state can change after startup.

And regarding integration, i'd rather have it not as delayed gcode but within startup macro with proper print output that MMU is enabled.
We have now more than 1 delayed gcode with initial_duration of 1. I don't like to start going into possible race conditions since the last parsed code is the winner.

@Benoitone Benoitone closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need to understand Bug look to be identified but more time need to be involved to understand and solve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants