bed_mesh: Restore originally-saved profiles on BED_MESH_CLEAR

[voidtrance]

BedMeshCalibrate.probe_finalize() updates the config file with the newly generated mesh data. That method is called during the completion of meshing. This means that every BED_MESH_CALIBRATE command updates the config file with new mesh data.

This can lead to mesh profiles being unintentionally overwritten. This can happen because BED_MESH_CLEAR does not restore the old mesh data in the config file. So, if a user runs the following commands:

BED_MESH_CALIBRATE BED_MESH_CLEAR ... SAVE_CONFIG

the mesh will be saved despite the user having run BED_MESH_CLEAR.

Fortunately, the newly generated bedmesh is stored in a separate section of the config that holds pending changes. Therefore, all that is needed in order to avoid storing a bed mesh is to remove the section for the current profile, which removes it from the set of pending changes.

[Arksine]

I'm not sure this is the correct behavior. It could unintentionally remove a saved profile when the user runs BED_MESH_CLEAR.

For example:

BED_MESH_PROFILE LOAD=bed60
..do stuff
BED_MESH_CLEAR

BED_MESH_CALIBRATE PROFILE=bed80
SAVE_CONFIG

The above would remove bed60, however its unlikely that the user really intended to remove it.

[voidtrance]

I'm not sure this is the correct behavior. It could unintentionally remove a saved profile when the user runs BED_MESH_CLEAR.

For example:

BED_MESH_PROFILE LOAD=bed60
..do stuff
BED_MESH_CLEAR

BED_MESH_CALIBRATE PROFILE=bed80
SAVE_CONFIG

The above would remove bed60, however its unlikely that the user really intended to remove it.

I'd argue that they did. Isn't that the point of the BED_MESH_CLEAR? If I am misunderstanding how BED_MESH_CLEAR is supposed to work, I am sorry. But in that case, there is still the problem of the unintentional saving of a profile. How would you suggest that is fixed?

[voidtrance]

I'm not sure this is the correct behavior. It could unintentionally remove a saved profile when the user runs BED_MESH_CLEAR.

For example:

BED_MESH_PROFILE LOAD=bed60
..do stuff
BED_MESH_CLEAR

BED_MESH_CALIBRATE PROFILE=bed80
SAVE_CONFIG

The above would remove bed60, however its unlikely that the user really intended to remove it.

I just noticed that your example uses BED_MESH_PROFILE LOAD=bed60. As far as I can tell, BED_MESH_PROFILE LOAD does not update the config file as there are no calls to ProfileManager.save_profile() in that call stack. Therefore, there would be no pending changes beyond the profile created by the BED_MESH_CALIBRATE PROFILE=bed80. Therefore, the bed60 profile should not be removed as the config is not updated to remove it.

[Arksine]

I'd argue that they did. Isn't that the point of the BED_MESH_CLEAR? If I am misunderstanding how BED_MESH_CLEAR is supposed to work, I am sorry. But in that case, there is still the problem of the unintentional saving of a profile. How would you suggest that is fixed?

BED_MESH_PROFILE has an explicit REMOVE parameter that should be used when a user wants to remove a profile. The BED_MESH_CLEAR method is intended to clear the internal (volatile) mesh, it is not intended to remove persistent data.

I just noticed that your example uses BED_MESH_PROFILE LOAD=bed60. As far as I can tell, BED_MESH_PROFILE LOAD does not update the config file as there are no calls to ProfileManager.save_profile() in that call stack. Therefore, there would be no pending changes beyond the profile created by the BED_MESH_CALIBRATE PROFILE=bed80. Therefore, the bed60 profile should not be removed as the config is not updated to remove it.

The example presumes a previously saved profile named bed60 exists in the autosave section. It is intended to show that a user could load a profile, clear the volatile mesh with BED_MESH_CLEAR, then do something that requests SAVE_CONFIG. It could be BED_MESH_CALIBRATE, PROBE_CALIBRATE, etc. The bed60 profile would be removed with no notification to the user.

With regard to existing behavior, the user is notified that the autosave section has been updated after BED_MESH_CALIBRATE. It behaves like all other calibration routines in Klipper that save persistent state, a restart will reverse it, SAVE_CONFIG will commit it.

[Arksine]

One additional comment...I believe that your proposed changes wouldn't just revert the pending changes to the profile, it would remove that section entirely if it exists in the autosave section of printer.cfg.

[voidtrance]

BED_MESH_PROFILE has an explicit REMOVE parameter that should be used when a user wants to remove a profile. The BED_MESH_CLEAR method is intended to clear the internal (volatile) mesh, it is not intended to remove persistent data.

I don't want to remove a saved profile. I want to avoid saving the current mesh, potentially overriding a profile.

With regard to existing behavior, the user is notified that the autosave section has been updated after BED_MESH_CALIBRATE. It behaves like all other calibration routines in Klipper that save persistent state, a restart will reverse it, SAVE_CONFIG will commit it.

The problem is that there is no way to "throw away" a mesh that is undesired, yet save something else that is needed. If I ran:

BED_MESH_CALIBRATE
BED_MESH_CLEAR

PROBE_CALIBRATE

and I only wanted to save the probe settings, there is no way for me to do that without restarting and re-running PROBE_CALIBRATE.

One additional comment...I believe that your proposed changes wouldn't just revert the pending changes to the profile, it would remove that section entirely if it exists in the autosave section of printer.cfg.

Yes, you are correct on this. Personally, I think this is a bug with how the autosave data is handled. Having said that, it appears that this change, as is, is not what I wanted to implement. So, I would appreciate some advice on how to approach this.

It seems to me that in addition to this change, the PrinterConfig.set(), PrinterConfig.remove_section(), and PrinterConfig.cmd_SAVE_CONFIG() will have to be changed as such:

1. PrinterConfig.set() does not update self.autosave.fileconfig.
2. PrinterConfig.remove_section() does not remove anything from self.autosave.fileconfig
3. PrinterConfig.cmd_SAVE_CONFIG updates self.autosave from self.status_save_pending.

Of course, I'll have to check all the places where set and remove_section are used...

[Arksine]

I don't want to remove a saved profile. I want to avoid saving the current mesh, potentially overriding a profile.

Understood. The default behavior of BED_MESH_CALIBRATE is to save the latest result to the generic default profile. When desired, the profile can be saved to a more descriptive name using BED_MESH_PROFILE. This avoids unintentionally overwriting a profile.

At request, a PROFILE parameter was added to BED_MESH_CALIBRATE allowing the user to forego the second step.

Yes, you are correct on this. Personally, I think this is a bug with how the autosave data is handled. Having said that, it appears that this change, as is, is not what I wanted to implement. So, I would appreciate some advice on how to approach this.

It is behaving as intended, so it is not a bug. I do not think the default behavior of BED_MESH_CLEAR should change, however I would not be opposed to adding a parameter that allows the user to revert back to the original profile. This would require some tracking in the profile manager to store the original state before a profile is saved, and a method to revert the changes.

If you desire a more generic method to revert changes to the autosave data, I think that is something Kevin would need to provide guidance on.

[voidtrance]

It is behaving as intended, so it is not a bug. I do not think the default behavior of BED_MESH_CLEAR should change, however I would not be opposed to adding a parameter that allows the user to revert back to the original profile. This would require some tracking in the profile manager to store the original state before a profile is saved, and a method to revert the changes.

I am not sure if that is enough to prevent the issue described, unless reverting to the old settings also updates the autosave state. Is that what you were suggesting/thinking?

[github-actions[bot]]

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.