ambrop72 / aprinter

3D printer firmware written in C++
Other
143 stars 42 forks source link

Ad849x support #31

Open rmie opened 6 years ago

rmie commented 6 years ago

not working (yet), but compiles :)

Is there a way to get the scaling of the adc? It is important in this case as the output of these modules = temperature*5mV/K + 1.25V. Hardcoding to 3.3V seems a bit ugly (in case AREF isn't yet available) as it will not work for systems that use 5V for AREF.

Adding another parameter to the module would be somewhat a middle ground between the effort of adding it to each HAL and hardcoding.

BTW: I think that InterpolationTableE3dPt100 has the same issue.

ambrop72 commented 6 years ago

Hey,

About the voltage issue, I would add a setting to Ad849xFormula to make it more user friendly. I don't think it makes much sense to add voltage reference awareness to the ADC HAL since the drivers generally wouldn't know the voltage, you'd just be forced to configure it statically (HAL layer cannot use the runtime configuration system). I agree that InterpolationTableE3dPt100 has this issue, I will fix it shortly.

You say it does not work yet, in what respect? Are you sure that NegativeSlope=false is correct?

Perhaps this should be renamed to be more generic like LinearTempFormula, but then NegativeSlope should be (statically) configurable. Unfortunately the abstraction used does not currently support configuration-dependant slope.

rmie commented 6 years ago

It is 'not working' in the sense of returning the wrong temperature.

I understood NegativeSlope=false as the adc reading increases with the temperature, if so, then this is correct. I will add an assertion to ensure that the slope parameter is actually positive.

I agree that the more generic name makes sense, btw. this could be implemented as an InterpolationTable but it would consume considerable more CPU cycles per measurement.

ambrop72 commented 6 years ago

Yes your understanding of NegativeSlope is correct. I don't see any issue with the code itself, I suspect the issue is some sort of misconfiguration or misunderstanding on your side. Do you know that the 'adc' floating point value you get in adcToTemp() is in the range [0,1] (so that its meaning does not depend on the bit width of the ADC)?

I don't like the assertion with NegativeSlope since assertions are meant for logically impossible things, perhaps just a warning; if the message constant this can be done like this:

#include <aprinter/base/ProgramMemory.h>
Context::Printer::print_pgm_string(c, AMBRO_PSTR("//Error:BlaBlaBla\n"));

But maybe it's better to just implement support for runtime-determined NegativeSlope. I'll probably have some time tomorrow to do that as well as fix the InterpolationTable thing to have configurable voltage.

ambrop72 commented 6 years ago

You can use M921 command to see ADC values without conversion to temperature, the ones adcToTemp() gets (actually not exactly the same ones, because one half of the ADC resolution is added before calling adcToTemp to improve precision under the assumption the ADC rounds voltages down and not to the nearest value).

rmie commented 6 years ago

tested with Teensy3: M105: ok T:18.1328 /nan M921: ok TA:0.407669 touching the nozzle raises the temperature.

rmie commented 6 years ago

I couldn't figure out how to create the warning message during startup and/or reload of an invalid configuration.