RFC: Fan* refactoring

Developers
1

As a continuation of RFC: Heaters/temperature_* refactoring.

There are now several fan objects, which sometimes lack something, for example:

There is now a recent support of display_templates with makes things even more complicated.

Basically, all fans are just: fan + controller, without exception.
The only fan that I do not like to touch is the default [fan] because of the legacy and risk of misconfiguration and breakage of everything.

So, basically, one should be able to define a typical machine as:

[fan] # Old definition for all the legacy
...

[display_template heatbreak]
param_target: 50
text:
    {% set heater_temp = printer["extruder"].temperature %}
    {(heater_temp - param_target)}

[fan heatbreak]
# To validate it upon initialization and avoid use of delayed_gcode hacks
control: display_template heatbreak

[fan aux]
control: manual # SET_FAN...
...

Basically, instead of defining several custom controllers, replace them with template examples.
That is it.
If something is hard to do with templates, for example controller_fan, one can simply define the list of appropriate objects in the API, we already have heaters and steppers, so it should not be so hard.

It is possible, though, to simply load the controller within the fan object, but to my understanding, the only reason for this is the PID controller, which seems to me not so good or important in practice.

Probably it is also worth considering to do something with the display_templates naming in this context, basically they are output_pin templates, think I.

Thanks,
-Timofey

P.S.
Probably we can leave the fan_generic, instead of using [fan ...] to make it compatible with the existing API/UI control.

2

Yeah - I agree that controller_fan, fan, fan_generic, heater_fan, and temperature_fan all have a lot of similarity. One might add servo, output_pin, static_digital_output, and static_pwm_clock to that list as well.

For what it is worth, though, a high-level development approach exploring some kind of python “extensions” might be better than an approach using jinja2 templates. That is, making it easier to deploy custom Python code might be a better long-term approach.

Anyway, something to think about.
-Kevin

3

Just as a Jinja2 alternative RestrictedPython: The idea behind RestrictedPython - RestrictedPython 8.2 documentation
I guess it is also possible to implement a sort of “safe” access to internal objects with that, which is complicated with direct access to the code.

(I didn’t find previous mentions on the Discourse, Discord, so maybe I was just lucky to find it).

Regards,
-Timofey

Just my local PoC check, that it is possible to even startup something this way:

diff --git a/klippy/klippy.py b/klippy/klippy.py
index d8a268a9f..2059546ba 100644
--- a/klippy/klippy.py
+++ b/klippy/klippy.py
@@ -7,6 +7,7 @@
 import sys, os, gc, optparse, logging, time, collections, importlib
 import util, reactor, queuelogger, msgproto
 import gcode, configfile, pins, mcu, toolhead, webhooks
+import RestrictedPython
 
 message_ready = "Printer is ready"
 
@@ -100,11 +101,28 @@ class Printer:
             if default is not configfile.sentinel:
                 return default
             raise self.config_error("Unable to load module '%s'" % (section,))
-        mod = importlib.import_module('extras.' + module_name)
+        # mod = importlib.import_module('extras.' + module_name)
+        source = open(py_name).read()
+        byte_code = RestrictedPython.compile_restricted(
+            source,
+            filename=py_name,
+            mode='exec',
+            policy=None # Null-Policy -> unrestricted
+        )
+        logging.info(py_name)
+        module_globals = {
+            "__name__": 'extras.' + module_name,
+            "__builtins__": globals()["__builtins__"],
+            "__file__": py_name,
+            "_getattr_": RestrictedPython.Eval.default_guarded_getattr,
+            "_getitem_": RestrictedPython.Eval.default_guarded_getitem,
+            "_getiter_": RestrictedPython.Eval.default_guarded_getiter,
+        }
+        exec(byte_code, module_globals, None)
         init_func = 'load_config'
         if len(module_parts) > 1:
             init_func = 'load_config_prefix'
-        init_func = getattr(mod, init_func, None)
+        init_func = module_globals.get(init_func, None)
         if init_func is None:
             if default is not configfile.sentinel:
                 return default
1 Like
4

Interesting, thanks.

For what it is worth, though, I suspect it would be difficult to scale “extensions” with a simple python library.

A big issue with current “extras” is that they can interfere with the hard real-time deadlines of the main Klipper python process. This makes 3rd party “extras” very challenging to support, because an error in an external “extra” can easily destabilize the main process in a way that is very difficult for users/developers to identify which module is at fault. We can mitigate this with in-tree “extras” by doing code reviews, but that isn’t an option for 3rd party “extras”.

So, for a scalable extension system, I’d expect we’d need the following:

  1. Run the 3rd party code in a separate unix process. This makes it much less likely that 3rd party code that invokes blocking OS calls will cause the main code to miss its deadlines. It should also make it less likely that 3rd party memory allocations cause Python’s garbage collection to cause missed deadlines.
  2. Explicitly list the exported functions of the main code (and in-tree “extras”) and make it difficult for 3rd party code to call non-exported functions or access non-exported internal member variables. That is, make it dramatically harder for 3rd party code to directly access fields like toolhead.max_velocity instead of using exported functions like toolhead.get_max_velocity(). This makes it much more likely that we can improve the upstream code without fear of it breaking unknown 3rd party code.
  3. Make sure exported functions that require the g-code mutex (or similar locks) explicitly check that the external code holds those locks. This should reduce the chance of hard to trace crashes because of unsupported requests from 3rd party code.

The above is challenging and I’d be surprised if an “off the shelf” library would be able to implement the above. (If it does exist, though, that would be great.)

Cheers,
-Kevin

5

With all regards and as far as my understanding goes:
As soon as we talk about another process, there is IPC, and no issue with “API” or object states. A child Python process cannot access the internal state of the parent, and vice versa.
And it sounds like your previous (New proposal for Klipper "extension" support) proposal.

So, as far as my understanding goes, the question is not how to guard the object state, but how to provide IPC, and then which calls to export/how to make them useful, unless one plans to pass pickle objects back and forth.

Where many things seem to be impossible to do (and it is okay), my opinion is roughly the same as last time on the topic.

*With exclusion to: Multiple interpreters in a Python process — Python 3.14.6 documentation, which we cannot use.

A big issue with current “extras” is that they can interfere with the hard real-time deadlines of the main Klipper Python process.

We can track the execution time (and by so blocking) of any code within the reactor with: PoC reactor: timers tracker by nefelim4ag · Pull Request #7170 · Klipper3d/klipper · GitHub
This should allow us to answer where the process was busy.
But of course, it is impossible to track any possible intrusions in the code, unsafe operations on the state & etc.

Explicitly list the exported functions of the main code (and in-tree “extras”) and make it difficult for 3rd party code to call non-exported functions or access non-exported internal member variables.

In my rough understanding, this is basically what RestrictedPython should allow to do: one can construct thin proxy objects, which, for example, can make internal calls, and external code should not be able to even “compile”: Policies & builtins - RestrictedPython 8.2 documentation.
Same about imports, syscalls & etc.

Where building a direct replacement of Jinja2 sounds straightforward (one function/class/object):

  • define a function that gets a dict/state as an arg and returns a list of G-Code commands
  • define a function which yields a G-Code command (should allow to work with control flow).
  • define a function which returns new PWM/LED/Display data, whatever.

The scope is “limited”.

Building the extension API within the process is complicated, but a simple enforced restriction to access private attributes sounds already like something in that direction.

Regards,
Timofey