ads1x1x: added support for ADC chip

[korsarNek]

Added support for ADC chips ADS1103, ADS1104, ADS1105, ADS1113, ADS1114 and ADS1115.

The PR is rougly based on https://github.com/Klipper3d/klipper/pull/5618 but with a different architecture.

I've tested it with an ADS1115 and a linux mcu host.

[KevinOConnor]

Thanks. As high-level feedback, this sounds like useful functionality. I have some comments and questions:

1. I'm confused why this new module ties into the temperature system (and is described in the temperature section of the documentation). I would have thought that one could add a new adc pin via the setup_pin() mechanism, and then users could define temperatures sensors (or adc buttons) using the existing my_ads_chip:pin1 mechanism. That is, wouldn't the generic adc pin system be able to handle existing thermistors, analog buttons, and other features?
2. How does this module relate to (if at all) the ads1220 code proposed in #6555 and the ads1118 code proposed in #6553?

-Kevin

[KevinOConnor]

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

[korsarNek]

1. Users can define temperature sensors with the my_ads_chip:pin1 mechanism. I have one definition for the chip in general that controls stuff like the i2c definition and then there is a separate definition for temperature sensors where the sensor_pin and sensor_type get used with the option to add a couple extra config values. The chip object also provide a setup_pin function. I thought that is the same system as the other temperature sensors. The separation of chip, sensor and pin and not including it all in the sensor is one of the things I've changed compared to the original PR where everything was defined on the sensor directly. One ADS chip can support up to 4 input pins, so 4 thermistors. Do you maybe have an example where I can see how it is supposed to look like?
2. The ADS1118 is very similar to the ADS chips I've implemented here and seems support the same state machine, the big difference would just be that SPI is used instead of I²C. It could make sense to try to unify them. Concerning the ADS1220, from skimming through the data sheet, it looks like there are too many differences between that and the other ADS chips to reasonably try to unify them.

[korsarNek]

I'll check the min max temperature part.

[korsarNek]

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.

[KevinOConnor]

I have one definition for the chip in general that controls stuff like the i2c definition and then there is a separate definition for temperature sensors where the sensor_pin and sensor_type get used with the option to add a couple extra config values.

Can you elaborate on when/why a user would use sensor_type: ads1x1x instead of just using sensor_pin: my_ads1x1x:AIN0 with an existing sensor_type?

Do you maybe have an example where I can see how it is supposed to look like?

I don't know of a "one-to-one" example. You can look at klippy/extras/adc_scaled.py and config/generic-duet2-maestro.cfg for an example of a pseudo-adc chip. You can look at klippy/extras/sx1509.py and config/generic-duet2-duex.cfg for an example of an i2c digital gpio expander chip.

-Kevin

[KevinOConnor]

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

[korsarNek]

[ads1x1x adc]
chip: ADS1115
pga: 6.144V
samples_per_second: 16
i2c_mcu: host
i2c_bus: i2c.1
address_pin: GND
adc_voltage: 5

[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: ads1x1x
sensor_pin: adc:AIN0
probe_type: Generic 3950
gcode_id: C
target_temp: 0
voltage_offset: -0.2
pullup_resistor: 4700

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.

[KevinOConnor]

I'm not sure I understand.

To take a step back, on one of my printers, I have the following:

[heater_bed]
heater_pin: PC9
sensor_type: NTC 100K MGB18-104F39050L32
sensor_pin: PC4
control: pid
min_temp: 0
max_temp: 130
pid_Kp: 57.006
pid_Ki: 3.334
pid_Kd: 243.700

I guess my question is, if I had an ads1115, could I just do:

[ads1x1x my_adc]
chip: ADS1115
pga: 6.144V
samples_per_second: 16
i2c_mcu: host
i2c_bus: i2c.1
address_pin: GND
adc_voltage: 5

[heater_bed]
heater_pin: PC9
sensor_type: NTC 100K MGB18-104F39050L32
sensor_pin: my_adc:AIN0
control: pid
min_temp: 0
max_temp: 130
pid_Kp: 57.006
pid_Ki: 3.334
pid_Kd: 243.700

That is, if setup_pin() is implemented and returns a value between 0.0 and 1.0, would the existing temperature code work as is?

-Kevin

[korsarNek]

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:

[ads1x1x my_adc]
chip: ADS1115
pga: 6.144V
samples_per_second: 16
i2c_mcu: host
i2c_bus: i2c.1
address_pin: GND
adc_voltage: 5

[heater_bed]
heater_pin: PC9
>sensor_type: ads1x1x<
>probe_type: NTC 100K MGB18-104F39050L32<
sensor_pin: my_adc:AIN0
control: pid
min_temp: 0
max_temp: 130
pid_Kp: 57.006
pid_Ki: 3.334
pid_Kd: 243.700

When I move the code around instead of using probe_type, it should be possible to use the NTC for the sensor_type instead.

[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.