Make SAVE_CONFIG work cleanly #534
Comments
Frix-x
added
enhancement
|
I think all that is a lot of work and new code vs someone taking 3 values that rarely change and updating things manually Would you also have to mimic the format, or just put the proper stuff at the bottom? Are you also going to manage finding and commenting out the old values (or just updating them) Having dupes in overrides would be something to avoid. |
No I was just thinking about a very simple awk command like this that would alter an existing override or add one: awk '{x+=sub(/search/,"replace")}END{if(!x)print "replace"}1' overrides.cfgIt's a first draft, it would need some changes to work correctly, but the idea is here.
For sure. The goal is to look at the overrides and if the entry is already here, just change the value by the new one. If the entry is not here, then add it at the end as a proper Klippain override. I don't want to deal with anything from the original mechanism like commenting, etc... Not the goal and indeed I don't like it either... |
|
While I do think this will work, I do think it's the hard way to fix a simple problem unless there is another reason for it. If the reason is to have those entries listed in overrides.cfg vs the end of printer.cfg, then that makes sense. I still would ask why that would be a problem to have it listed in the auto section of printer.cfg at all. For example. When auto bed mesh does its thing, it gives the option in mainsail to save config and will write that at the bottom of printer.cfg. If we did this override of save_config, then it would remove it from ever doing this edit to printer.cfg. I do think this could create an incongruity between Klipper and Klippain that would lead some people trying to use both documentation in to why the base Klipper function isn't working like it's suppose to (why it's not writing anything to printer.cfg) Currently save_config doesn't work with klippain for 2 functions, PIDs and z_offset but it will work for bed_mesh. It can easily be made to work for PIDs by moving those lines from overrides.cfg to printer.cfg. Z_offset is not as easy since it's in a read only file. If you wanted to keep klippain functioning like it currently does, then moving z_offset would solve it all for the user if they want to make save_config work for everything as it currently does. I could also see doing it this way to keep klippers documentation correct in its function would be to do a hybrid. Keep the PIDs and z_offset in overrides.cfg, and use the script to comment out those lines in overrides.cfg then call klippers own save_config and let it write those values in printer.cfg. If they understood more of the thought process or aversion to having it in printer.cfg was then I could dive in more. |
I agree with this. But then I could definitely check which value to save, and if it's a bed, then use
Actually, this issue is more complicated, since removing it from the read-only file will break Klipper startup, and I would absolutely need to set it directly in printer.cfg. But the problem is that while this is ok for physical endstop pin users, it's not ok for virtual Z endstop probe users because it's a different entry. And these two entries are not compatible with the Klippain probe files. So it would lead to potential fail if the user doesn't select the right combination and I want to avoid that as much as I can since it's already a lot of support (I got dozen of PM everyday for stuff like this) and I want to focus on things that just work to lower this. At the moment it's just an idea and I need to validate that it's actually feasible and how much work it would take. For example, I know that |
The problem is that it's already poorly documented or a bit confusing for even relatively technical users to do initial setup of klippain... and if you're writing this stuff into printer.cfg then you're basically breaking consistency... saying "you need to setup your printer in |
I don't think this would break Klipper startup any more than having the PIDs commented out in the overrides section. I would say having the z_offset in the overrides.cfg wouldn't be any more disruptive than the PIDs and wouldn't need to be put in printer.cfg any differently than the PIDs are. To get Klippain to save_config for the PIDs requires moving those sections to printer.cfg. This wouldn't be any different for z_offset.
I don't think this is correct, but there is something that I didn't realize. The z_offset: isn't related to the endstop_pin, virtual or physical (if I recall correctly), the z_offset it either under [bltouch] or [probe]. That is the only thing that needs to be changed. Now correct me if I'm wrong with klippain operation, as I've not used klippain with bltouch, but from what I understand, but the z_offset under [probe] will not work, it would need to be z_offset under [bltouch] correct? This is where I think what you're talking about different entries is that a bltouch requires there to be a z_virtual_endstop for the z endstop_pin. I'm actually using voron tap which has endstop_pin: probe:z_virtual_endstop, and the only thing that I need to change is to remove the z_offset from the [probe] section and I can get save_config to work. I think the problem is [probe] and [bltouch] is where the issues lie, and would require z_offset to be either under [probe] or [bltouch]. This is where I would argue it's more potential fails and support for you on the PMs if you were to do it this way (and would be easier to code it too.)
I believe that both options above should be done regardless of if you do a script to get save_config working. Include a note in the overrides.cfg file that mentions that the z_offset: line needs to be uncommented, and even add a commented out [bltouch] with z_offset line with a note stating what needs to be done. Yes, this would cause klipper to not start up if the user doesn't uncomment the z_offset lines, but klipper doesn't start up unless you uncomment the PIDs sections if you have a heat bed and extruder (because the PIDs are not included in any of the other read only files. I would assume this is because everyone's PIDs will be slightly different even on the same hardware, just like z_offset is different for every printer on the same hardware). It would functionally be the same as it is now. This is where you could leave it alone if you wanted to, because the functionality of klippain would be 100% the same as it is now, and if the user wanted to make save_config to work, then they can move the PIDs and z_offset sections over to printer.cfg and make save_config work. Currently save_config already works in this regard with the PIDs. If I move them to printer.cfg, do a PID config, it works that way now. Now if your end goal is to not put any entries in printer.cfg and to keep printer.cfg sterile, then I would say that using a script to edit the entries in overrides.cfg would be the way to go. |
I can completely agree here. This is where I think you would need to make a decision on if you want to diverge Klippain from the klipper's documentation and functionality a little bit. I can agree with keeping all of the overrides in overides.cfg as the documentation says, but if you have someone that knows a little bit about klipper and knows when they click save_config in the UI, that usually puts the SAVE_CONFIG section at the bottom of the printer.cfg file with #*# before it. I could see where some people might save_config after a z_offset or PID calibration and that doesn't go into printer.cfg, that could be a little confusing, but I think it wouldn't be as confusing as it is now. It's more of a matter is if the juice is worth the squeeze in writing a whole new script vs using what's already there. |
the script shouldn't be very complicated, and if it makes expected behaviors work in a way that's also smooth from a UX perspective, I would say yes. |
|
Why does it have to be in printer.cfg? If frix is writing it it can update overrides and everyone wins |
|
Also most people have zero clue how save config works or where it's saved to. |
|
Ok, I do think I see what @Frix-x might be talking about in some regards to the virtual_endstop. If you select no_probe.cfg, then there is no virtual_endstop, and the section to change would be position_endstop: and not z_offset. My question, from back when I had my ender 3 with a physical endstop for the z height, But how many people would use an offset to change the position_endstop vs moving the physical endstop to be at the correct height?
The only reason would be time/effort vs reinventing a current function in klipper. The only reason to have it in printer.cfg would be if Klippain would want to leverage and use the function inside klipper. If the z_offset was removed from all of the read only files and used only in overrides.cfg, with a note in overrides.cfg that explains that, there is very little time and effort in doing this. Then it would be up to the user if they wanted to move the relevant sections needed for klipper's save_config function to work properly over to printer.cfg. There is very little time/effort to make this change, and wouldn't change any functional impact of klippain as it currently sits. To write a script to make the edits under overrides.cfg will take more time/effort. If it is very imperative that you want to get save_config operating AND keep all changes in overrides.cfg instead of printer.cfg, then there is nothing wrong with that. |
What about? |
Yes, that's exactly what I'm talking about. If you're not using a virtual probe, it's the stepper z_endstop position value that needs to be changed to adjust the Z offset. When using a virtual probe, it depends on the type of probe (BLTouch, probe or smart_effector). And this can also be extended in case of using the auto_z_calibration plugin, it's again another place called "switch_offset" but this only works if the position_endstop of the stepper Z is already set correctly. So this topic is very complex and that's why I want to write this simple script to add the overrides automatically instead of letting the user do it and fail if he clicks the SAVE_CONFIG button as it is currently the case... And it would be a lot easier and more transparent for the user, since all this would have to be defined in printer.cfg and would cause incompatibilities if someone didn't choose the right combination. So better to just make it automatic and invisible.
This is currently how stock Voron printers are working when not using TAP: the physical Z endstop pin located in the rear of the printer is fixed and you calibrate the Z offset using Z_ENDSTOP_CALIBRATE Klipper stock function (instead of PROBE_CALIBRATE). |
This is exactly what I wanted to see if it existed. Glad you confirmed it! Now I know I can replicate it ! 😄 |
Ok, so that makes sense and I've not used a voron in that aspect. When you do a save_config after a z_offset_apply_endstop, what does klipper put in the printer.cfg for that? |
|
it would update the appropriate position_endstop or z_offset |
I know it does that, but I would need to know what section. Is it [z_stepper]? [probe]? etc More detail is needed to know if things would function like I think they would. From what I can see in klippy/extra/probe.py, it does set the config line as z_offset, but I can't tell what config section it saves it under. I have a feeling that a base install of klippain with the physical endstop pin wouldn't have any issues with save_config as there is no other z_offset setting through out any of the read only files and wouldn't cause an issue with save_config with that setup. |
|
Perhaps the user variables that klippain sets on files being included would point the way |
for virtual probe it's what type of probe has a real interest in doing a SAVE_CONFIG regularly? |
So when you do a save_config with [stepper_z] It changes the value of position_endstop?
I'm needing to babystep my tap many times. It's usually very close, but sometimes it might be .1 or .2 +/- |
Yes |
I could still argue that the simple way to resolve it would be to move all of the changeable values (position_endstop, z_offset, etc) into overrides.cfg and align them with the way PIDs are handled with documentation in overrides.cfg. I think this could be done in a y.X or a y.y.X release to allow more time for a macro/script to be developed. The reason that I say that the z_offset and position_endstop belong in the same category as the PIDs, when you include files like /config/hardware/bed_heaters/keenovo.cfg, klipper will not start unless the PIDs section in overrides.cfg is uncommented as the PIDs are not included in keenovo.cfg or any of the other heated items. I again assume that the reason the PIDs are not included in the other files is because PIDs will change for everyone. Items like z_offset/position_endstop will also change for everyone and should be included in overrides.cfg, which the probe section is already included there, but there is read only configurations of the z_offset where there are no read only configurations of the PID entries. In the release notes, it could be stated that to get save_config to work, those entries can be moved over to printer.cfg if the user wants to move them. I personally would be happy with the changes ending there and not needing to write a script to do anything more. For the technical users that understand, they can move those entries over to printer.cfg, and for the average user that is already accustomed to how klippain works, they would still understand that all changes to the baseline config is to be put into overrides.cfg. |
|
@Frix-x @smwoodward |
|
@Surion79 thats not true though. If you have a heated bed or extruder section in Klipper, the PIDs section MUST exist or it will not start. |
I even went through Klippers documentation to verify this, and also looked through the configuration of an enclosure that I have that doesn't have any stepper motors in it. Under [stepper] position_endstop is required for klipper to start. under [extruder] or [heater_bed] control: is required in the configuration, and depending on pid or watermark will depend on if the pid_*: lines or max_delta: is required. My argument is that there is an incongruency with the way Klippain handles the PIDs and z_offset/position_endstop with the z axis. From a fresh install of klippain as it currently exists, if you go through and setup a kinematics printer with steppers and no heating elements, then klipper will start because the required z_offset/position_endstop is included in the read only files. This is without making any edits in overrides.cfg Yes, you can override those entries in overrides.cfg. Once you add any heating elements, IF YOU DO NOT EDIT overrides.cfg to uncomment the PID sections, klipper WILL NOT START. This is because the PID section does NOT exist in any of the read only configuration files. One way to fix the incongruency issue above would be to add the PID config lines to the extruder and heater_bed read only files. This way Klipper would startup without editing overrides.cfg. HOWEVER, I would assume that the reason the PIDs are in the overrides section and not the read only config files is because PIDs are very specific to every machine, and thus MUST be overridden for everyone. z_offset is exactly the same way. While the base config files might get it close, it still would need to be overridden. @Surion79 I'm sorry but your argument on this is demonstratively false. The reason being is because if there is no heating element in the configuration (extruder, heated_bed, etc) then you are correct, PIDs do NOT need to be in the configuration, HOWEVER the same is true for z_offset/position_endstop does NOT need to be in the configuration if you do not have a [probe] or [stepper] section for klipper to start. Once you add either of those configuration sections, then those config lines are REQUIRED. So, if an end user setups an instance of klippain, doesn't make any changes to overrides.cfg, and doesn't setup any settings that require steppers and heaters in printer.cfg, yes, Klipper will start. If they configure it with steppers and no heaters in printer.cfg, yes, Klipper will start because the required config lines are included in the read only config files. If they configure it with steppers and heaters in printer.cfg, and do NOT uncomment PIDs in overrides.cfg, klipper will NOT start because the control:. config line and the corresponding config lines for the control: section (i.e PIDs) do not exist in any of the read only config files. As I mentioned above, your argument leans towards adding the PID lines into the read only config file. While this would make sense in a congruency fashion, and if that's the direction that Klippain would want to go, that would make sense, even if it could be argued not the proper way to go. But as long as PIDs are required to get klipper to start (again, depending on the other configuration lines), AND z_offset/position_endstop is required to get klipper to start, I think that ALL configuration lines that will be required to be changed for every printer, should be put into overrides.cfg and kept out of the read only files. Since PIDs are specific to each individual machine, they should be in overrides.cfg. Since z_offset/position_endstop is specific to each individual machine, they should be in overrides.cfg. |
Yes, you are right. And this is my mistake, as I was pretty sure that the PIDs entries in overrides.cfg were uncommented by default.
Your point is perfectly valid, yes, and I will do that if I can get SAVE_CONFIG to work the way I described in the first post. The main difference for now is that it's pretty impossible to get "reasonable" defaults for PIDs, which is why I wanted them directly in the overrides (and they should indeed be uncommented by default), since this is something that EVERYONE will need to set. For Z offsets, as discussed above, it's a different story because there are several different ways to do it that have incompatibilities if not set in the right combination. The thing is that for safety purposes and to avoid crashes as much as possible, having a good default is mandatory, like having the real probe heights as [probe] z_offset values for all kind of probes (even if there is still some work to do in Klippain to correctly separate the different kind of probes like "klicky", "unklicky", "zeroprobe", etc... instead of just "dockable" for example) or having a good zero position for the [stepper_z] endstop_position. So I think everyone now understands the pros and cons of everything for this topic. I'm going to close the conversation for now until I develop a proof of concept for this mechanism and then reopen it to validate that it works correctly for everyone. |
Describe the feature or hardware support you'd like
Ok, I had an idea. Since we are already using gcode shell commands in Klippain, why not use this to make
SAVE_CONFIGwork.Here is my idea:
SAVE_CONFIGmacro that renames the existing one. This macro will retrieve the last pending value calibrated from the console. I'll have to do some digging, but I'm pretty sure we can get it using Jinja in the printer object. Then this macro would send it as an argument to a special shell command.SAVE_CONFIG' macro would continue WITHOUT calling the original renamedBASE_SAVE_CONFIG', but instead would end with a `RESTART'.This way we would have
SAVE_CONFIGwork by automatically adding (or modifying) the correct values to overrides, and without the weird commented autogenerated section that duplicates values and is not really clear from a user perspective.Additional context or information
This comes from the infamous question I get every week about SAVE_CONFIG that doesn't work in Klippain. Also there were already a couple of PR with some tricks that would fragilize Klippain to make it work like the latest one: #468
@smwoodward would this work for you?
@EricZimmerman what do you think? I know you're an advocate of "don't use SAVE_CONFIG because it's evil" on Discord, so you might be interested in this idea.
The text was updated successfully, but these errors were encountered: