ads1x1x: added support for ADC chip #6584
Conversation
|
Thanks. As high-level feedback, this sounds like useful functionality. I have some comments and questions:
-Kevin |
|
I should also note that most temperature sensors in Klipper today are implemented with mcu code that can check the min/max temperature setting and directly invoke a shutdown if the range is exceeded. This code would be a different in that only the non-realtime host python code would check the min/max temperature range. I don't think that is a "blocker" but something to be aware of. -Kevin |
|
|
I'll check the min max temperature part. |
|
Actually, the ADS1118 has a similar format, but supports a couple different commands. Maybe they are also different enough that it wouldn't make too much sense sharing the code. |
Can you elaborate on when/why a user would use
I don't know of a "one-to-one" example. You can look at -Kevin |
|
Also, as recently discussed in #6553, src/thermocouple.c and klippy/extras/spi_temperature.py could also be looked at as an example - though as mentioned there it may not be the best example. -Kevin |
That's the config I'm using for my setup. There are sensors like HTU21D, where this would not be compatible with, they define their own communication protocol and would be incompatible with the ADS just reading out the voltage on the pin. I thought that was what you originally meant with sensor_type, but there are other sensor_types which are equivalent to the probe_type here. I guess I could have a look in how to make the thermistor and adc_temperature objects talk with the ads chip object to do the voltage to temperature conversion, then the probe_type property would not be necessary anymore and would become the sensor_type instead. |
|
I'm not sure I understand. To take a step back, on one of my printers, I have the following: I guess my question is, if I had an ads1115, could I just do: That is, if -Kevin |
|
As the code currently is, that would not work, the pin is missing the setup_minmax function. Apparently that's needed for an adc pin. Additionally to that, I have the code for requesting the voltage currently on the sensor instead of the pin, I'll have to move some stuff around from the sensor to the pin. A config that currently would work is this: When I move the code around instead of using probe_type, it should be possible to use the NTC for the sensor_type instead. |
KevinOConnor
removed
the
pending feedback
|
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:
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, PS: I'm just an automated script, not a human being. |
|
Thanks for your work on this! I'm running into a couple of issues. The first is, when using all 4 ADC pins, the readings are jumping from one pin to another. For example, AIN0 might be 35c, and AIN2 50c, but then it might suddenly show both AIN0 and AIN2 as 50c, and then next AIN2 as 35c and AIN0 as 50c. The second issue is (and it might be a wiring issue on my end), I'm seeing |
|
@KevinOConnor Concerning the sensor setup, I have a question on how this could be solved.
The problem is, the pullup resistor is technically not part of the chip, when someone would use the thermistor, there has to be a little circuit that could theoretically be different every time. It would be unusual for the pull up resistor to be different at different parts of the machine, but the voltage offset might be different, if you want an accurate reading, a different length of the wire could result in a different voltage offset that someone might want to account for. On the pin, I don't have access to that information. The pins provided by the chip object, can also only be really used in connection with the ads1x1x sensor object, at the moment, they are not general pins that can be reused in other contexts. Is there a way to have the thermistor as the sensor_type under these circumstances? I don't see how I could make that work with the current architecture.
|
|
Ah, the simple way to handle that is to not implement So, something like: Said another way, just invoke the adc callback with the raw adc value (eg, see -Kevin |
|
will this PR include support for a ADC chips connected to the printer MCU? |
Added a temperature sensor configuration for ADS1103, ADS1104, ADS1105, ADS1113, ADS1114 and ADS1115 chips that can be used to add Analog to Digital Conversion capability to machines that don't have that on their own. Like Raspberry Pi's or if more analog input pins are needed than the chip provides like for RP2040. Generally they can be used for any analog input, but the typical use case is for temperature measurement. This code also has been written with temperature measurement in mind and not as a general ADC. Signed-off-by: Konstantin Koch <korsarnek@gmail.com>
Signed-off-by: Jack Wakefield <jackwakefield@protonmail.com>
…d try to be more accurate on the sample timing Signed-off-by: Konstantin Koch <korsarnek@gmail.com>
@legend069 I did not test a setup with an ADC chip connected to the printer MCU. I have the I2C pins needed for this connected to my linux mcu. From my understanding, Klipper should abstract a proper printer MCU and a linux MCU enough that they should behave the same. My setup: The setup that I think, you mean: Both should be working fine. What's a bit special about this implementation is that most sensors have their loop to check if the temperature is out of bounds on the mcu, this implementation has it only in the python code. It might not be quite as real time as other sensors. I'm currently using the chip only to measure the chamber temperature, so that setup is sufficient for me. For the final version the config will change a bit. I've already tested a new version that doesn't use sensor_type: ads1x1x anymore but allows using the normal thermistors. The pushes from yesterday were just a rebase, the code wasn't quite compatible with the most recent version of klipper. I'll probably finish it up next weekend and then push the newest version. |
…r instead of a custom ads1x1x sensor. Signed-off-by: Konstantin Koch <korsarnek@gmail.com>
|
@KevinOConnor I have added a new version that can now use thermistors or adc_temperature as the sensor instead of having a custom ads1x1x sensor. [ads1x1x adc]
chip: ADS1115
pga: 6.144V
samples_per_second: 16
i2c_mcu: host
i2c_bus: i2c.1
address_pin: GND
[adc_temperature Chamber Sensor]
temperature1: ...
voltage1: ...
...
[temperature_fan chamber_fan]
pin: host:gpio17
min_temp: 0
max_temp: 80
max_power: 0.8
off_below: 0.1
shutdown_speed: 0
control: watermark
sensor_type: Chamber Sensor
sensor_pin: adc:AIN0
gcode_id: C |
| ADS1013, ADS1014, ADS1015, ADS1113, ADS1114 and ADS1115 are I2C based Analog to | ||
| Digital Converters that can be used for temperature sensors. They provide 4 | ||
| analog input pins either as single line or as differential input. | ||
|
|
There was a problem hiding this comment.
I think it might be useful to add a warning about using the sensor for heaters - maybe something like:
Note: Use caution if using this sensor to control heaters. The heater min_temp and max_temp are only verified in the host and only if the host is running and operating normally. (ADC inputs directly connected to the micro-controller verify min_temp and max_temp within the micro-controller and do not require a working connection to the host.)
docs/Config_Reference.md
Outdated
| ``` | ||
| [ads1x1x my_ads1x1x] | ||
| chip: ADS1115 | ||
| #pga: 6.144V |
There was a problem hiding this comment.
If the default is 4.096V then please use the comment #pga: 4.096V.
docs/Config_Reference.md
Outdated
| #mode: single | ||
| # Default value is single. Turn off the chip after a single reading or keep | ||
| # it running. Turning off saves power but turning it back on will be slower. | ||
| # Options are single and continuous. | ||
| #samples_per_second: 128 | ||
| # Default value is 128. The amount of samples that the ADC can provide per | ||
| # second. A lower value makes the samples more accurate, but it takes longer | ||
| # until a new value is available. | ||
| # ADS101X's support 128, 250, 490, 920, 1600, 2400, 3300. | ||
| # ADS111X's support 8, 16, 32, 64, 128, 250, 475, 860. |
There was a problem hiding this comment.
Just out of curiosity, why would a user ever configure these two settings?
Since the code knows the report_time from setup_adc_callback() could the code just choose an appropriate value?
There was a problem hiding this comment.
Someone might want to change it for higher accuracy, how many samples_per_second you can get away with also depends on how many inputs of the chip are used as one sampling would have to be done after the other.
It would make more sense to try to calculate that automatically, someone would need to know too much about how klipper and the ADS chip works to choose a good value.
A report_time in setup_adc_callback of for example 0.010s, would be 100 samples per second. On a single input I would choose 128. If 4 inputs are used, I would need to choose 920 (128 X 4 = 512, next best is 920) for ADS101X and 860 for ADS111X, reducing the accuracy but fitting in the time frame. I would then output a warning if the sample_rate needs to be higher than the chip can support.
It's a bit of extra logic finding the next best value and counting the used inputs, but then I could kick out the samples_per_second.
Concerning the single or continous mode, there is a theoretical power saving on single mode. I just wanted to include it for completeness.
If I compute the sample_rate anyway, this could be hard coded to continous.
There was a problem hiding this comment.
Okay. Just to be clear, my comments here are not "blockers" to merging.
As for "power savings" - I can't see anyone caring, so it sounds like continuous is a better default and one can use a code constant to make it possible to enable sleep mode.
As for auto-calculating wakeup times - I'll defer to you if that's something worthwhile to implement.
There was a problem hiding this comment.
I have now added a calcuation for the sample rate. I made single mode the default though, it turns out that the idle state that we check on to see if the sampling is done, only gets set on single mode, not on continuous mode.
klippy/extras/ads1x1x.py
Outdated
| def _handle_ready(self): | ||
| self.reset_all_devices() |
There was a problem hiding this comment.
It is not valid to communicate with an mcu in the "klippy:ready" callback. That callback can't be used to call anything that could report an error (nor anything that can block, or run for a long time).
You could kick off a reactor callback from ready, but it sounds like you want to do this work in the connect callback.
There was a problem hiding this comment.
Sounds good, I wasn't quite sure which one was the correct one for this.
klippy/extras/ads1x1x.py
Outdated
| self._printer.register_event_handler("klippy:shutdown", \ | ||
| self.reset_all_devices) |
There was a problem hiding this comment.
On a shutdown, the mcus wont accept any i2c commands. Maybe I'm missing something, but registering this event handler looks odd.
There was a problem hiding this comment.
I'll remove it, reset on initialize should suffice anyway.
klippy/extras/ads1x1x.py
Outdated
| except Exception: | ||
| logging.exception("ADS1X1X: error while sampling") | ||
| return None |
There was a problem hiding this comment.
Can you provide a little more explanation for this error handling? If I understand the code correctly, if there is some error in this interaction the code will just stop sending temperature updates to the higher-level code. That sounds like that could cause a subtle failure (the higher level code will just assume the temperature hasn't changed for potentially extended periods). I'm not sure what the best solution is, but perhaps invoking a shutdown should be considered.
There was a problem hiding this comment.
I could adjust it so that a failed sampling adds to the invalid_count and invokes a shutdown if it happens often enough in a row.
klippy/extras/ads1x1x.py
Outdated
| self._sample_timer = \ | ||
| self._reactor.register_timer(self._timer) | ||
| self._reactor.update_timer(self._sample_timer, self._reactor.NOW) |
There was a problem hiding this comment.
FYI, you can do these two things directly with reactor.register_timer(self._timer, reactor.NOW)
klippy/extras/ads1x1x.py
Outdated
| if sample is not None: | ||
| self._process_sample(sample) | ||
|
|
||
| return self._reactor.monotonic() + self.report_time |
There was a problem hiding this comment.
It seems odd that this does not use eventtime + self.report_time. Rescheduling based on the current time will result in an ongoing drift in the period between readings.
klippy/extras/ads1x1x.py
Outdated
| else: | ||
| target_value = sample / ADS111X_RESOLUTION | ||
|
|
||
| if self.maxval > self.minval: |
There was a problem hiding this comment.
The calling code should verify maxval > minval and always provide values - so this looks like an odd check.
There was a problem hiding this comment.
Ah, I wasn't sure if it does for certain, so I just added it to be safe. I'll remove that part.
Signed-off-by: Konstantin Koch <korsarnek@gmail.com>
Added support for ADC chips ADS1103, ADS1104, ADS1105, ADS1113, ADS1114 and ADS1115.
The PR is rougly based on #5618 but with a different architecture.
I've tested it with an ADS1115 and a linux mcu host.