Support for Flashforge Creator Pro2

eazrael commented 2 months ago

Basic configuration for an unmodded Flashforge Creator Pro2 (FFCP2) IDEX printer.
Firmware and Klippy driver for Texas Instruments ADS1118 ADC chip. Required by FFCP2. Probably too uncommon for a standalone PR.

dockterj commented 2 months ago

Are you reading the cold junction temperature from the ADS1118 and using that to compute the actual temperature? The chip should be located close to where the thermocouple cable connects to the PC board, so the internal temperature of the ADS1118 should be very close to the cold junction temperature. It looks like you are assuming the CJ temperature is always 25c?

eazrael commented 2 months ago

There's a lengthy comment in the code:

# Open FFCP2, 25° ambient temperature, ADS1118 reports 33° which also
# pushes the extruder thermocouple to 33°C. Seems that 25°C is the real
# temperature. At least in this test case
# Especially for the FFCP2 you have multiple thermal zones, the
# extruder, the print chamber, the mainboard compartment and the room...
# so, what is the reference for the cold junction? Or where do we get
# this temperature?

How did you solve this? I will look into your code in the next days when I find time.

dockterj commented 2 months ago

The mcu code reports back the cold junction temp and the mv reading of either channel 0 or 1. The cj temp is converted to a mv reading, added to the thermocouple mv and then converted back to a temperature. This is done in python starting at line 84 https://github.com/dockterj/klipper/blob/master/klippy/extras/ads1118_thermocouple.py

This TI application note is a good reference https://www.ti.com/lit/ug/slau509/slau509.pdf?ts=1712687318583&ref_url=https%253A%252F%252Fwww.bing.com%252F

Looking at your config file I suspect you could use my code as is. The makerbots use software spi, but the spi communication is handled by generic klipper code so your hardware spi config should work fine.

eazrael commented 2 months ago

Oh, i did not find this. I read the Datasheed section and SLAU509 and sbaa274a from TI. But the issue is that you do not have the real cold junction temperature anywhere. In the FFCP2, the mainboard is in its own compartment below the printer which also gets the PSU heat. The ADS1118 reads 33°C, the MCU sitting 5cm away says 38°C, the hotbed in the print chamber says 25°C and the RasPi on the desk 40°C. The room has a temperature of 25°C. The thermocouple gives neutral 0V, So this is 33°C in this case 33°C from the ADS1118 + 0°C from the thermocouple. Took me some time to understand this and that is the reason for this comment. Other test, just the board and the thermocouple completely open on the desk. The ADS started at 25°C, I pressed my finger on the ADS1118 and with the ADS1118 temperature the temperature of the thermocouple went up while still reading 0V.

So for the FFCP2 this does not work, and usually when in doubt I make such things configurable, but then you need also an option for setting the ADC temperature as baseline or some override and then you have a configuration monster.

dockterj commented 2 months ago

I'm only guessing based on a picture of the main board I found online, but it looks like the ADC is located close to where the thermocouple connects, so the internal temperature of the ADC should be very close to the temperature of the cold junctions. Certainly they should reach an equilibrium where they are close to each other unless there is a major heat source somewhere near them. Is the ADC reading exactly 0? I'll try your code on my printer and see what it looks like.

eazrael commented 2 months ago

The thermocouples sit in the hotends, connected via PCB traces and simple cables to the Mainboard. Unless I total misunderstood thermocouples, the cold junctions is just outside of the extruder and before the PCB in the extruder.

So you need the temperature there, but there is no sensors. The FFCP2 is enclosed you have there 40-60°C , So the cold junction is hotter that the ADS1118. On the other side, before printing, or with an open printer, the compartment is hotter than the cold junction. and adding the ADS1118 temp would push the temperature higher.

I did it by the book and the results weren't feasible at least in the temperature range I could test them reliably (-20°C - 60°C).

dockterj commented 2 months ago

Weird design then. Any change there is a thermistor on the PCB where the thermocouple terminates? Without a way to measure the temperature there I don't see any way to make it accurate enough to use.

eazrael commented 2 months ago

Unfortunately no :-(

As I did not expect that the ADS1118 is used elsewhere I just set it at 25°C for the temperature calculation. I will change that back to the ADC temp adjustment.

eazrael commented 2 months ago

https://evilazrael.net/tmp/temps_at_25_fixed.jpg https://evilazrael.net/tmp/temps_adc_adjusted.jpg

1 sek between them. I doubt that you can get a reliable temperature.

Sineos commented 2 months ago

connected via PCB traces and simple cables to the Mainboard

The ADS started at 25°C, I pressed my finger on the ADS1118 and with the ADS1118 temperature the temperature of the thermocouple went up while still reading 0V.

If they are really using simple cables then you will never get a proper reading and it would explain your finger test. In expensive thermocouple measuring devices, you will have a calibrated temperature sensor at each terminal and the controller compensates it. In the cheaper scenario it is assumed that the temperature as measured by the ADC is used as reference for compensation.

If the cold junction is between the "normal cable" and the alloy cables, then you are probably measuring just "something". Seems especially catastrophic, since this printer is fully enclosed.

eazrael commented 2 months ago

Sorry for the thermocouple discussion. Have never seen thermocouples before this project. When I made my "finger test", the thermocouple was directly connected to the mainboard, without the extruder PCB and the long cables between them, which also carry heating & extruder power lines. And I doubt they have alumel/chromel PCB traces and cables. So there was only 20cm between them, but it still showed that the ADC is probably not a reliable cold-junction. And will probably never be as the temperature range of the ADC is neglectible when compared to the thermocouples (generally speaking).

I need to do some calculations, check what the error in the usual extruder temperature range is and how to how to fix that for the FFCP2.

Sineos commented 2 months ago

Basically, it is quite simple:

The measurable voltage is generated by the temperature delta between the "hot junction" and the "cold junction"
To compensate for linearity errors, the CJ is always assumed to be 0°C
Since 0°C at the JC is not practical, the temperature is measured there and used for mathematical compensation
Depending on the temperature curve of the probe (type), the error of a wrong JC measurement varies.

Thermocouple_EMF_Temperature

As the above diagram shows, a type B has a flat curve and will suffer less from wrong JC measurement compared to a type K with a relatively steep curve.

Personally, I think this type of sensor is a poor fit for 3D printing:

It has a huge temperature range that is not needed for 3D printing but with a low resolution of the temperature (Type K generates only 40 µV/°C)
It is quite susceptible to noise (due to the low voltage), e.g. you should keep it away from EMI and avoid touching GND on the hot end. Some measurement ADC even error out when you touch GND.

eazrael commented 2 months ago

I did a couple of tests, Without any modification the ADC temperature is the closest you can get to the cold junction temperature in the FFCP2. The regression tests are now all working.

github-actions[bot] commented 1 month ago

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:

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

KevinOConnor commented 1 month ago

Thanks. For what it is worth, I'm not familiar with the flashforge hardware and I didn't really understand the conversation leading up to this. I did look briefly through the code though, and it seems this code is invoking the spi transfer functions from irq context - that is not valid.

Could you take a look at PR #6584 and the coding approach taken there? What advantages/disadvantages are there to the approach here?

-Kevin

eazrael commented 1 month ago

Cutting the discussion above short: The FFCP2 cannot measure the extruder temperature correctly.

The ADS1018 & ADS1118 (ADS1X18) are the SPI variants of their ADS1X1X siblings with a simple SPI wire protocol, you send 16bits of configuration and receive 16bits with the last reading at the same time. That's all. Not so generic as reading I2C registers. The ADS1118 is a 16bit ADC and the ADS1018 a faster 12bit ADC, which could be supported with adding two shifts of the reading values. And their next line of TI's SPI ADC chip has another protocol, so that's really something special.

Scanning through #6584 code, one big difference is that Konstantin put all the configuration values as constants in the Python module, while I just put them in the configuration file, you have to look up in the data sheet that the setting "ads1118_pga: 7" means +-.256V.

I tried to make the sensor available as a virtual pin by looking at the other modules, but failed. It ended up as new sensor type :-.

Any short example or hint how to decouple the transfer from the interrupt (timer I assume)?

KevinOConnor commented 1 month ago

The ADS1018 & ADS1118 (ADS1X18) are the SPI variants of their ADS1X1X siblings with a simple SPI wire protocol

Thanks. At a high-level though, #6584 seems to just add python code. Even if the ads1118 can't share any code, is it feasible to implement it with just Python code?

I tried to make the sensor available as a virtual pin by looking at the other modules, but failed.

Ah, okay. What went wrong?

Any short example or hint how to decouple the transfer from the interrupt (timer I assume)?

One would wake a "task" to perform the spi actions. Take a look at src/thermocouple.c for an example.

For what it is worth though, I'd be reluctant to take on the long-term maintenance costs of new mcu code for one relatively uncommon printer that can't accurately measure temperatures even with that code..

-Kevin

eazrael commented 1 month ago

The ADS1018 & ADS1118 (ADS1X18) are the SPI variants of their ADS1X1X siblings with a simple SPI wire protocol

Thanks. At a high-level though, #6584 seems to just add python code. Even if the ads1118 can't share any code, is it feasible to implement it with just Python code?

Not for me. See next response

I tried to make the sensor available as a virtual pin by looking at the other modules, but failed.

Ah, okay. What went wrong?

To be honest, lack of documentation and defined interfaces of the core framework. Deducing everything from existing code in many variations is quite tedious. Even after tracing through a lot code I do not think I have understood the effects of "REPORT_TIME" correctly. And I have no problem with reading through code while looking for something you can use, extend, reuse, take as a template and so on. I hope I did not offend you.

Any short example or hint how to decouple the transfer from the interrupt (timer I assume)?

One would wake a "task" to perform the spi actions. Take a look at src/thermocouple.c for an example.

Ah, I see. The connection between thermocouple_wake and thermocouple_task is outside of the module. Could fix that.

For what it is worth though, I'd be reluctant to take on the long-term maintenance costs of new mcu code for one relatively uncommon printer that can't accurately measure temperatures even with that code..

I can understand this, same consideration on my side.

There also other printers using the ADS1118. Maybe @dockterj can start a new PR for his implementation.

KevinOConnor commented 1 month ago

Even after tracing through a lot code I do not think I have understood the effects of "REPORT_TIME" correctly.

The REPORT_TIME is used to notify the heater code of how frequently it should expect a sensor update. It's used to setup the corresponding PWM timing for a heater. Basically, it's the time between each sensor report.

I hope I did not offend you.

No offence taken. It's difficult with any large open-source project with so many different moving parts.

There also doesn't seem to be a good example. The closest example I can think of would be src/thermocouple.c and klippy/extras/spi_temperature.py . However, those chips are really temperature specific while the ads1118 seems more of a generic adc chip.

I guess the next step would be to see how #6584 proceeds. Although there doesn't appear to be any code reuse from it, hopefully it will provide a concrete example for similar chips.

Thanks again, -Kevin

quilt-jdockter commented 1 month ago

@KevinOConnor I was hoping that both of these 6584 and 6555 would be applicable to a better approach for the ADS1118 code that I wrote, but I think the big difference is that supporting the MakerBot Rep2 (and now this Flashforge Creator) requires "multiplexing" the ADC. Garethky had a good comment about some of the challenges in making this generic enough to be used anywhere https://klipper.discourse.group/t/my-pull-request-broke-the-ar100-build/15591/26?u=dockterj

The ADS1118 is weird, it has a mode that makes reading one sensor trivial using just python code (continuous conversion). However, a Rep 2x has 2 thermocouples hooked to it and then you have to read the internal (CJ) temp. This can only be done in one shot mode where you tell it to read the next channel at the same time you pull the reading for the previous channel. The synchronization to make that work seemed to me to naturally be handled on the mcu. Not saying it couldn't be done with just python but it seemed simpler to make a c module for it. Also, give that my target is specifically hotends I thought implementing the mcu safety checks for temp out of range errors was better (I didn't investigate if other hotends were implemented in only python or how the safety checks were handled). Feel free to take a look and I'm open to any feedback. https://github.com/Klipper3d/klipper/compare/master...dockterj:klipper:master Most of the code is for ADS1118 support - should be obvious which to ignore in that. Note that I used src/thermocouple.c as a starting point, so hopefully it isn't too far out.

Klipper3d / klipper

Support for Flashforge Creator Pro2 #6553