Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] MAX31865 Incorrect Calibration Resistance Value #18809

Closed
zervin opened this issue Jul 27, 2020 · 12 comments
Closed

[BUG] MAX31865 Incorrect Calibration Resistance Value #18809

zervin opened this issue Jul 27, 2020 · 12 comments

Comments

@zervin
Copy link

zervin commented Jul 27, 2020

Bug Description
The max31865 calibration resistance is set to 400 in the temperature.cpp. I am able to get the system to read temperatures, but the values are incorrect. The MAX31865 typically uses a 430 for reference. Modifying the temperature.cpp from 400 to 430 results in the correct calculation. The current ambient temprature in the space is 29.4c. At 400, the extruder reads 9c, and 430 it reads 30c.

My Configurations
RelatedConfigs.zip

@sjasonsmith
Copy link
Contributor

sjasonsmith commented Aug 9, 2020

The MAX31865 manual mentions 400 ohms, although I also see that AdaFruit installs 430 ohm.

Perhaps this needs a configuration option, rather than a simple value change?

@zervin
Copy link
Author

zervin commented Aug 9, 2020

Quite possible. It probably should as least be documented somewhere. I didn't find a clear rationale for one versus the other at the time. I only have 1 type, so I can't test if it matters when switching between pt1000 and pt100, or if it's maybe a different board revision.

@AnHardt
Copy link
Contributor

AnHardt commented Aug 9, 2020

I only have 1 type, so I can't test if it matters when switching between pt1000 and pt100, or if it's maybe a different board revision.

You can't simply connect a PT1000 to a module made for PT100. The modules for PT1000 differ in having a 10 times higher reference resistor. The Adafruit PT1000 modules have a 4300 Ohm resistor. (PCB and chip are identical - it's just the reference resistor what is different. For that the clones are often soled as PT100/PT1000-boards.)

The 400 Ohm value comes from the MAXIM-datasheet Page 20. It reads: "In the typical case of a PT100 RTD with a 400 Ohm high precision low drift reference resistor, ...". Its just a suggestion to get the maximum resolution over the complete range of temperatures. Higher values of the resistor (like 430) result in a reduced selfheating of the PT-resistor. As always you have to search for a compromise.

Is anyone aware of a MAX31865-module not being from Adafruit (or a clone)?

There is a sub-board for the DUET where i have not data for. But as long Marlin is not running on the DUET-Baoards - ... .

@wallaceowen
Copy link

All I needed to do to get a PT100 from Adafruit (with the 430 ohm resistor) was change the 400 to 430. But it seems like bad practice of the first order to have that raw 400 numerical constant in a .cpp file

I would think that simply the fact that you've got a hard-coded number with no docs would be sufficient warrant to make a change that creates a #define for an appropriately named constant in a header file. Then I can #define an alternate value in Marlin/Configuration.h, so I can override it and not have to go searching through the code for magic numbers.

@Quas7
Copy link
Contributor

Quas7 commented Sep 12, 2020

maybe adding another temperature probe number for MAX31856 with 430Ohm reference seems logical to me

@AnHardt
Copy link
Contributor

AnHardt commented Sep 12, 2020

image
It's unlikely a module with a 400 Ohm reference resistor is existent. If @Bob-the-Kuhn has not one he tested with, the 400 is just an error.

@github-actions
Copy link

This issue has had no activity in the last 30 days. Please add a reply if you want to keep this issue active, otherwise it will be automatically closed within 7 days.

@wallaceowen
Copy link

I've been asked if I want to add a comment to keep this issue alive.

I'm kind of discouraged that it's down to me.

I found the issue and have modified my local checkout of 2.0.6, so this doesn't impact me.

I'm discouraged because no one has stepped up to make a change to the code to either

  1. do the minimum and change the magic number 400 to the magic number 430 in two places
  2. do the right thing and #define a constant or an enumeration of the values used by various vendors (sounds like it rounds to one).

sad. not complaining, I know it's because lack of resources. and that's why I'm sad.

@sjasonsmith
Copy link
Contributor

I posted a pull request to allow this configuration. I don't use these parts, so did not actually test it, but it is a low risk change.

This change is not important to me personally, so if you with to see it (or something similar) merged please go share your support or concerns on that PR.

#19695

@boelle
Copy link
Contributor

boelle commented Oct 13, 2020

@zervin did the PR from jason help?

@zervin
Copy link
Author

zervin commented Oct 13, 2020

Yes, I think that is the proper way to define it from the looks of the commit that will do nicely. It will be much easier to change versus digging through source. Thanks!

@zervin zervin closed this as completed Oct 13, 2020
@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants