TMC2240: Allow control and use of temperature report

[nefelim4ag]

My personal goal:
I like to control all fans, the printer is at home and I try to minimize noise if possible.

There are 3 patches:

1. Duct tape fix for race conditions with temperature combined sensor and other sensors during klippy initialization, where sensors can return 0. This may be by design, but it currently limits the minimum possible deviation between sensors. Like there, it is adc + i2c:

2. Temperature combined allows us to set "any" object in the sensor list, but with tmc2240, there is a corner case, it has method get_status() and temperature field, but it has no value. So, it will crush Klippy cause max/min can't handle None.

3. Proposal to allow monitoring of TMC2240 continuously, which in the end allows the use of it as combined sensor to control fans.

I can split PR if needed.

---

Should close: #6675

[KevinOConnor]

Thanks. In general the first two patches look fine. The new comments seem a little odd though - I'm not aware of any sensor that will return 0 - we should definitely fix any sensor doing that.

I'm not sure about the third patch. Why would one use temperture_combined on a tmc2240 temperature? What would they combine it with? If the goal is to use a tmc2240 temperature sensor as a regular sensor, maybe we should consider adding an internal mechanism to indicate to the tmc2240 that it always needs to provide temperature (that is, have the temperature code signal the tmc2240 instead of a user config option signalling tmc2240).

-Kevin

[nefelim4ag]

I'm not aware of any sensor that will return 0 - we should definitely fix any sensor doing that.

I think that about the first patch and the delay in initialization.
It will be nice, but all temperature sensors do that, the only exception is ADC ones.
They all initialized with the default value equal to .0:
https://github.com/Klipper3d/klipper/blob/master/klippy/extras/ds18b20.py#L20
(same for BME, HTU, SHT, LM75).

So, get_status which always just returns the last value of self.temp, can be called and will return .0
Of course, after the first query cycle, they will return the actual value or crash the system.

Maybe there is a more clever solution, like ... do pause() in get_status() to wait for initialization in every sensor?
I'm not so familiar with the whole call chain here, to suggest something sane here.
Looks like ADC is always making calls to MCU instead of providing cached property.

---

I'm not sure about the third patch. Why would one use temperture_combined on a tmc2240 temperature? What would they combine it with?

Maybe I misunderstood the goal of temperature combined and misused it, but I have only 2 use cases, like:

• use avg/max over drivers' temperatures to control temperature FAN (I use only one pin to control fan output to cool drivers)
• Duplicate sensor, because heater object can't have humidity or gas, and I use bme680 as the control for chamber temperature, and ADC + temperature fan to control PTC heater.

I do not see a reason to use any individual data from the drivers to control anything, AFAIK there is no external TMC2240 like TMC5160, which can be cooled individually.

If the goal is to use a tmc2240 temperature sensor as a regular sensor, maybe we should consider adding an internal mechanism to indicate to the tmc2240 that it always needs to provide temperature (that is, have the temperature code signal the tmc2240 instead of a user config option signalling tmc2240).

If there is a way to avoid adding strange config options and do some magic behind the scenes - I agree it is a better solution (like query always if there are consumers of data - it will be a better solution).
But I can't imagine how to do that without modifying every possible temperature consumer to add some callback for that. I even had thought about allowing to define something like temperature_sensor where type is tmc2240 stepper_* which will do something like that - change internal behavior/reload as a sensor. But it also feels weird a little.

My original intention here is to just allow us to use it somehow at least because right now it is only for graphs.
Graphs btw look strange to anyone who doesn't know why there is no data from the driver - if I can easily query it from the console.

[nefelim4ag]

I took another look at the code, ADC sensors were provided through the heater factory, and in the end have queried through the callback which is protected with a lock.
I'm not sure about global ordering here, there is still a possibility to query get_status before the first callback runs.
But it does not happen (get_status either waiting on a lock or running after callback).

As this is a callback any structure above it will receive data when they are ready, but combined sensor - query object directly, without a callback, so it can receive data before they are ready and between readings.

I think there are 2.5 possibilities to work with:

1. Migrate combined sensor to callback infrastructure, but it will break its flexibility and possible existing users, like I can define it over a heater, but I don't know if it is valid.
   1.5 Above + allows to setup of several callbacks for the sensor to handle that case (I think objects are unique and if there are two callback users one will steal a callback from the other).
2. Introduce a similar mechanism to get_status() for sensors, which will block till the first data is ready (I think that can break initialization of the combined sensor, there is a call to get_status on connect())
   2.5 Above + use event time for order, but that will make any users of get_status() lag till fresh data with query eventtime > get_status eventtime is available.

For me, it feels like callback is the correct way to go here, cause all internal users should use it, and it will automatically disallow the use of tmc2240, cause there is no setup_callback, and will update its own data on each update of sensors beneath (or maybe just cache values and still use internal 0.3s update cycle). But it is a lot of changes in different places.

I'm not sure I'm qualified here to choose the correct way to fix this, I will be pleased to hear your opinion on this, maybe I just have a strong misunderstanding there.

[freakydude]

I like to mention that the scenario is real 😉

* use avg/max over drivers' temperatures to control temperature FAN (I use only one pin to control fan output to cool drivers)

That is exactly my scenario. I do have a manta m8p board with 6 tmc2240 drivers and the cb1/cm4 pi on it.
It's below a silent running 120mm fan. The printer is next to my desk.

I would like to run the fan not faster than needed, defined by the max of all these temperature sensors.

At the moment I combined the manta and the cb1 temperature sensor only. It's OK - but could be better 😊

Best regards

[github-actions]

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.