bed_mesh: Support reporting actual mesh profiles

[Misterke]

This change was reduced to reporting the mesh profiles actually defined in bed_mesh.py via the status. That way GUIs can correctly reflect the profiles that are available via BED_MESH_PROFILE commands.

[Misterke]

Do I need to do anything special to get a reviewer to look at this pull-request?

[Arksine]

I think we could just return a reference to self.profiles rather than do a deep copy. The get_status() methods are called frequently, so we want them to be as fast as possible. A user would have to intentionally call update() or __setitem__() in a template to modify the dict, so it isn't a big risk.

[Misterke]

Hi @Arksine , thanks for the review.

I understood from the documentation that the items being returned as status should not be allowed to change under the hood. The profiles can be updated by the other commands to load/save a profile and hence returning a reference to the existing profiles would be subject to such change. That was one reason for doing it this way.

If I recall correctly, there was also the problem of some of the items (mesh_params) being ordered dictionaries rather than plain ones and that was throwing of things when then trying to store the information from the status into a saved variable or passing it to some other gcode. I'm not sufficiently knowledgeable on Klipper and Python yet to fully grasp how the passing of such a status dictionary to a gcode parameter works, but it looked to me like a kind of implicit string conversion, which for an OrderedDict ended up being a string that would not get processed correctly by ast.literal_eval. So, the conversion here was also to get rid of the OrderedDict. I don't know why the original code was using an OrderedDict for mesh_params. An alternative could be to remove that and if I would then ignore the rule that returned status should not get changed after returning, then not doing a copy might work ...

Is there any way for modules to provide data back to a macro other than via the get_status()? If so, then that might be a more appropriate way to allow access to the actual list of profiles. As said, I might just not be aware of the existence of such a mechanism, hence my current implementation via the get_status().

Please advice how to proceed.

[Arksine]

While it is true that none of the items referenced in get_status() should be modified, I don't believe we take any action to prevent it from happening. As mentioned above, the only way they could be modified by a template is if the user directly calls a dict method on the result. Anyone knowledgeable enough to do that in Jinja2 will likely know the source of any pain this induces.

An OrderDict was used to keep the output consistent when saving a mesh to the config file. To be honest, I may have introduced a regression here in a later commit, as it appears that the mesh params are converted to a regular dict in the finalize() method. This isn't a deal breaker, we could probably remove the ordered dict and just sort the keys prior to setting the new config.

You are correct that the ordered dict would break ast, and the best way of handling it is to just remove it and sort the mesh params in save_profile(). That said, there are also the alternatives methods of saving variables that I outlined below.

[Arksine]

I agree that reporting the profiles in get_status() is useful and added a related comment.

I don't see anything wrong with the implementation of the SET parameter of BED_MESH_PROFILE, however I'm not sure if serializing profiles and passing them around via gcode parameters is ideal. If users forget to quote the result, or if the dict somehow contains an unescaped quote, the macro will fail. We'll need to wait for Kevin's feedback on this. I know he's incredibly busy, so it may take some time for him to review.

[Misterke]

It also seems to be the way to set saved variables. If you forget quotes around the value of SAVE_VARIABLE and that value has special characters or spaces, then there too the same problems would occur. I fully agree that this does not look super clean to me. I would prefer having a mechanism to be able to invoke save_variable and bed_mesh.set_profile from Jinja code with the variables set there without having to squash them to a string. If such a mechanism exists, I would gladly learn about it.

[Arksine]

I can think of two alternatives, both however would require changes to save_variables.py:

1. Extend save_variables.py to include get_variable() and set_variable() methods that can be invoked internally. Rather that add a SET parameter in BED_MESH_PROFILE, we could add an optional VARIABLE parameter. If present, bed_mesh would look up the save_variables object and attempt to save, load, or remove the profile from the variable rather than from the configfile.

2. An action_save_variable() method could be added to the jinja2 environment. When invoked it could be passed a python object directly.

[KevinOConnor]

FYI, the objects returned from get_status() need to be treated as immutable; otherwise a change to the object wont be detected by the webhooks.py code and wont be propagated to API users listening for changes. It is not necessary to ensure the returned objects are actually immutable, as no caller of get_status() will modify the returned objects. The macro code always makes a deep copy of the get_status() results to ensure macros can not alter the objects.

Separately, FWIW, it's not clear to me the high-level benefit of this change. What real-world tasks will real-world users be able to accomplish after making this change, and can we reasonable assume at least 100 users will use it?

-Kevin

[Misterke]

Hi @KevinOConnor,

One clear example that this type of operation is missing can be found in Mainsail's Heightmap tab: you can only see there the profiles stored in the printer.cfg and that list is static. You can ex. delete a profile via the trashcan button next to it, but that doesn't change the list of configs shown as those are not the set of profiles available in bed_mesh, but rather just what was in the printer.cfg at the last startup. So, providing access to the actual set of profiles in bed_mesh and allowing manipulation of those would definitely also make this tab in Mainsail behave more logically.

I personally also hate the fact that some configuration like bed meshes and z-offset (tuned by baby-stepping while already printing) by default needs to be saved to the printer.cfg and hence trigger a restart. When you have heated up the bed and created a mesh and then want to save that mesh for future use, you do not want that to hamper your current print by forcing a restart. So, you don't do a SAVE_CONFIG at that time ... and in my case an external process turns off the printer once the print is done and things are cooled down, so then you lose the config change. Sure, there are ways to have the end-print macro check for modified config and then save that, but I prefer not having to rely on a future action for saving such settings.

In general I feel there should actually be 2 types of settings storage:

1. the config stuff like pins, hard limits, voltage, ... which is matching perfectly with the way Klipper handles the printer.cfg
2. values which can be changed during operation and where hence there is no reason why the storage of the default value of that setting would need a restart as it doesn't change anything which wasn't already changed. For this type of setting I would expect that there should be a mechanism to get/set the value from macros so that people can create their own mechanisms for managing those values.

Currently I have macro code where I store my bed type in a saved variable and have a G29 implementation which checks whether for the active bed type and bed temperature a mesh already exists and if so uses that stored mesh else creates a new one and stores that (without the need for a SAVE_CONFIG as it is stored in a saved variable). I plan to do something similar for the z-offset to store that per bed-type and then when baby-stepping also update the z-offset for the active bed-type without needing a SAVE_CONFIG.

I'm not saying this behavior is for everyone, but giving get/set access to such parameters does allow people the freedom to do this type of thing and hence I feel that part would be a good addition to Klipper.

[Misterke]

FYI, the objects returned from get_status() need to be treated as immutable; otherwise a change to the object wont be detected by the webhooks.py code and wont be propagated to API users listening for changes. It is not necessary to ensure the returned objects are actually immutable, as no caller of get_status() will modify the returned objects. The macro code always makes a deep copy of the get_status() results to ensure macros can not alter the objects.

So, does this mean that I should ensure a deep copy is returned (as was now the case) or not? Do I understand correctly that if I would just return the self.profiles and it gets updated internally, then the next time webhooks.py would poll the status and compare it against what it already had, the latter would have a reference to the same profiles dict and hence it would not detect the change. If so, then the current implementation of returning a copy of the profiles instead is correct, right?

[KevinOConnor]

I'm not saying this behavior is for everyone, but giving get/set access to such parameters does allow people the freedom to do this type of thing and hence I feel that part would be a good addition to Klipper.

I'll defer to Arksine on this PR. In general, though, I like to understand "who the users are and what they're doing" with code changes. I don't have a feel for that even after your description. I get the feeling you're saying "someone may find this useful", but that doesn't provide me further insight. There's some comments on this at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review

So, does this mean that I should ensure a deep copy is returned (as was now the case) or not?

As per @Arksine's comments above, get_status() should not deep copy the contents - due to a concern about cpu performance. If self.profiles isn't treated as immutable, then I'd guess there would be other code changes to make it be treated as immutable before it could be exported from get_status().

-Kevin

[Arksine]

I do think its valuable to report the profiles in get_status() for the reasons stated, it will allow front ends to display profiles in memory that haven't been written to the config. As Kevin mentioned, its going to require some changes to save_profile() and remove_profile(). Something like the following should work:

diff --git a/klippy/extras/bed_mesh.py b/klippy/extras/bed_mesh.py
index 24d190b4..674203e2 100644
--- a/klippy/extras/bed_mesh.py
+++ b/klippy/extras/bed_mesh.py
@@ -1170,10 +1170,12 @@ class ProfileManager:
         for key, value in mesh_params.items():
             configfile.set(cfg_name, key, value)
         # save copy in local storage
-        self.profiles[prof_name] = profile = {}
+        profiles = dict(self.profiles)
+        profiles[prof_name] = profile = {}
         profile['points'] = probed_matrix
         profile['mesh_params'] = collections.OrderedDict(mesh_params)
         self.current_profile = prof_name
+        self.profiles = profiles
         self.gcode.respond_info(
             "Bed Mesh state has been saved to profile [%s]\n"
             "for the current session.  The SAVE_CONFIG command will\n"
@@ -1195,9 +1197,11 @@ class ProfileManager:
         self.bedmesh.set_mesh(z_mesh)
     def remove_profile(self, prof_name):
         if prof_name in self.profiles:
+            profiles = dict(self.profiles)
             configfile = self.printer.lookup_object('configfile')
             configfile.remove_section('bed_mesh ' + prof_name)
-            del self.profiles[prof_name]
+            del profiles[prof_name]
+            self.profiles = profiles
             self.gcode.respond_info(
                 "Profile [%s] removed from storage for this session.\n"
                 "The SAVE_CONFIG command will update the printer\n"

With regard to the SET implementation, the presence of the OrderedDict prevents deserialization via ast. My suggestion would be to implement reporting the profiles in this PR, then open a discussion on the Discourse to gauge interest in saving mesh profiles to save_variables. If there is enough interest we could look into ways of supporting it, either through removing the OrderedDict or though directly accessing the save_variables object in bed_mesh.

[Misterke]

I do think its valuable to report the profiles in get_status() for the reasons stated, it will allow front ends to display profiles in memory that haven't been written to the config. As Kevin mentioned, its going to require some changes to save_profile() and remove_profile().

I've reduced this branch to just reporting the profiles via the status. And moved my other changes to another branch (which I will be using myself, since I really want to be able to store meshes without needed to SAVE_CONFIG). Still, I agree that having reporting of the actual profiles in the status could help MainSail and other UIs to at least correctly reflect the state of bed_mesh.py and solve some of the issues they have open on this.

[Arksine]

Thanks, the changes look good to me.

[zellneralex]

@KevinOConnor that would be useful for the UIs to know what meshes are still loaded internal since klipper init. Currently fluidd and Mainsail cache the meshes that the user remove since last init until SAVE_CONIG.

See also: mainsail-crew/mainsail#548

[KevinOConnor]

Thanks. The change looks fine to me, but we really want new exported status variables to be documented in docs/Status_Reference.md ( note 4 at https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review ).

-Kevin

[Misterke]

Thanks. The change looks fine to me, but we really want new exported status variables to be documented in docs/Status_Reference.md ( note 4 at https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review ).

-Kevin

Sorry ... I had that, but the rewinding had me remove it by accident ... Will add it again ...

[KevinOConnor]

Thanks.

-Kevin