Traumflug / Teacup_Firmware

Firmware for RepRap and other 3D printers
http://forums.reprap.org/read.php?147
GNU General Public License v2.0
313 stars 199 forks source link

Config Tool: doesn't accept float numbers for temperature #176

Open youssefaly97 opened 9 years ago

youssefaly97 commented 9 years ago

I tried to use ConfigTool to build my thermistor table with the Steinhart-Hart Equation, one of the 3 temperature points is 75.5 degree Celsius but ConfigTool refuses the .5 which makes it inaccurate more than it already is.

jbernardis commented 9 years ago

Traumflug - your call. I can probably pretty easily accommodate floating point numbers, but I thought teacup did only integer math - hence the restriction.

The data type of the underlying table is uint16_t, so I assume floating point numbers are out.

Traumflug commented 9 years ago

Yes, Teacup does only integer. But this shouldn't hold back Configtool to accept such values for table generation. If I understand @youssefaly97 correctly, it's about the dialog for entering the SH-H numbers, not the generated table.

jbernardis commented 9 years ago

yes, but I would just round it to an integer to put it in the table - there would be no additional accuracy unless we scale the values as is done with steps per meter instead of millimeter.

I'd rather the user KNOW that they don't have the accuracy rather than THINK that they do.

triffid commented 9 years ago

I thought all the temperatures in teacup used 14.2 fixed point, in which case 75.5°C -> 302 ?

Rounding to the nearest 0.25°C should still be sufficiently accurate I think

Traumflug commented 9 years ago

Don't these formulas for generating the tables produce a lot of odd, non-integer values anyways? With input values just slighty lower or higher the output values could change by a digit.

jbernardis commented 9 years ago

Actually this discussion reminds me that the values entered in the GUI don't end up in the table - they go through a table generation algorithm. Let me look at the implications of taking floating point values into the argorithm

youssefaly97 commented 9 years ago

Before configtool, I used to generate my lookup table using an MS Office Excel worksheet with a macro. It would only round the final analog reading (0-1023) to the nearest integer, rounding at the beginning would make it less accurate. Excuse me if I sound foolish, but I love perfection. Teacup uses integer math only, I understand that, but even if it doesn't, microcontrollers can never report a floating point number for an analog reading.

On Jul 22, 2015, at 2:28 AM, Jeff Bernardis notifications@github.com wrote:

Actually this discussion reminds me that the values entered in the GUI don't end up in the table - they go through a table generation algorithm. Let me look at the implications of taking floating point values into the argorithm

— Reply to this email directly or view it on GitHub.

jbernardis commented 9 years ago

How's this for service - I just pushed up a new branch - floatinput - that contains changes to accept floating point values for temperature fields on this dialog box. Give it a try and let me know if it needs any adjustment. It's based on the master branch

youssefaly97 commented 9 years ago

Thanks Jeff, great job ! Working nicely ! It makes a bit of a difference than without it, considering that I use 100 values in the table. Thanks a bunch.

On Jul 22, 2015, at 2:53 AM, Jeff Bernardis notifications@github.com wrote:

How's this for service - I just pushed up a new branch - floatinput - that contains changes to accept floating point values for temperature fields on this dialog box. Give it a try and let me know if it needs any adjustment. It's based on the master branch

— Reply to this email directly or view it on GitHub.

youssefaly97 commented 9 years ago

I think configtool shouldn't generate entries with the same analog value, so for example: For 25 readings entered in configtool settings = NUMTEMPS, it should divide the 0-1023 analog value range to 1024/25 = 40.96 values between every 2 entries so that it will use the 25 entries better than having the microcontroller read say 10 and that might correspond in the table to a range of temperatures.

So it's better IMHO to have every reading correspond to a single temperature. Also the readings should be limited to end at a specified temperature.

On Jul 22, 2015, at 2:53 AM, Jeff Bernardis notifications@github.com wrote:

How's this for service - I just pushed up a new branch - floatinput - that contains changes to accept floating point values for temperature fields on this dialog box. Give it a try and let me know if it needs any adjustment. It's based on the master branch

— Reply to this email directly or view it on GitHub.

jbernardis commented 9 years ago

Do you have a set of values that produce such a table? Not that I doubt you, but it's easier to troubleshoot something if you can replicate it.

youssefaly97 commented 9 years ago

Yes I do, Rb 100000 T0 25 R0 100000 T1 75.5 R1 11270 T2 100 R2 4520

NUMTEMPS 25

On Jul 22, 2015, at 5:30 AM, Jeff Bernardis notifications@github.com wrote:

Do you have a set of values that produce such a table? Not that I doubt you, but it's easier to troubleshoot something if you can replicate it.

— Reply to this email directly or view it on GitHub.

jbernardis commented 9 years ago

I have just added a commit to the floatinput branch. I had to change on of the regular expression strings that I use to parse temperature tables. The old string did not handle floating numbers properly so the data for those tables was not loaded properly.

I am still working on the fix to the above suggestion about duplicate adc values. I had to invert the logic of the S-H algorithm so that I could go from adc value to temperature. I think I have it working, but I want to test a bit.

I didn't want to wait for this fix though.

youssefaly97 commented 9 years ago

I didn't back-calculate the produced values in the table so I was wrong about completely testing it. I thought it would require inverting it but I only know C and C++ not python so I didn't bargain about it.

On Jul 22, 2015, at 10:30 PM, Jeff Bernardis notifications@github.com wrote:

I have just added a commit to the floatinput branch. I had to change on of the regular expression strings that I use to parse temperature tables. The old string did not handle floating numbers properly so the data for those tables was not loaded properly.

I am still working on the fix to the above suggestion about duplicate adc values. I had to invert the logic of the S-H algorithm so that I could go from adc value to temperature. I think I have it working, but I want to test a bit.

I didn't want to wait for this fix though.

— Reply to this email directly or view it on GitHub.

jbernardis commented 9 years ago

I just made a commit to floatinput branch. I changed the logic of thermistor table generation to generate temperatures based on equally spaced ADC values rather than generating ADC values for equally spaced temperatures. As yousefaly97 pointed out, the latter caused tables with duplicate ADC values to be generated. I think I tested it pretty well, but the more people testing it the better.

Traumflug commented 9 years ago

Thanks, @jbernardis. These commits look good to me. So i made yet another branch, inputfloat2, which is the same, but rebased to experimental. @youssefaly97, could you please confirm this inputfloat2 branch works for you? Then I'd fully forward it onto experimental, making this enhancement permanent.

youssefaly97 commented 9 years ago

@Traumflug I can't seem to find the inputfloat2 branch but nevertheless, I have done some more testing on the floatinput branch.

@jbernardis I'm not sure that it's working correctly, I've found differences in the most important part of the temperature range (180 to 260) when I compared the table generated by configtool and the table generated by the excel sheet I mentioned earlier so I graphed both tables against each other.

image

Blue is configtool Red is excel sheet

youssefaly97 commented 9 years ago

Also, there is repeated temperature values so I think we should only keep the entries where the temperature is not rounded up or down because they are already oversampled,

for example, (120,101) (110,100) //might be 100.4 so rounded down, remove (100,100) //this entry is the entry nearest to 100 even if not 100.0, take this entry in the table (90,100) //might be 99.5 so rounded up, remove (80,99)

and modify the NUMTEMPS accordingly.

youssefaly97 commented 9 years ago

Also, is the Rp value entered in configtool the fixed resistor in the fixed-resistor--thermistor potential divider network ? If so, make it clearer in the hover menu.

Traumflug commented 9 years ago

I can't seem to find the inputfloat2 branch

D'oh. Forgot to push it. Should be better now.

youssefaly97 commented 9 years ago

@Traumflug Thanks, both branches work exactly the same regarding only configtool off course.

Traumflug commented 9 years ago

If so, make it clearer in the hover menu.

No commanding, please.

Blue is configtool Red is excel sheet

These two curves look like a perfect match to me. A nice proof for a correct implementation. Thanks for doing this math.

Is it the part to the left, which bugs you? This is near room temperature, isn't it?

Changing NUMTEMPS can have implications, all tables have to have the same number of entries.

Seeing that you have just 10 digits between ADC values I assume you use something like 102 table entries. Having double temperatures is the natural result of such a large table. There is only so much precision one can achieve.

Regarding plausibility: thermistors are usually specified to have +- 3%, so at 250°C you have 7.5°C deviation. Plus deviation due to wiring resistances, deviation of Rp, deviation of the ADC (19 bits). Putting this together, accuracy is ... uhm ... way to coarse to make a headache about a 0.1% deviation in the value calculation.

That said, there are the branches thermistortableless, -2 and -3, which calculate the value directly, without any table. If you're that keen on accuracy, this might be the right way for you. Not sure about current status of these branches. It's certainly pre-Configtool, so you have to hand-edit the config file. IIRC it worked already, I laid it aside at the cleaning stage.

youssefaly97 commented 9 years ago

I'm sorry if I sounded commanding.

The part on the left is the extruding temperature range (180C-260C) (X axis is ADC value, Y is 4 x the real temprature, on the far left it's 1200/4=300C) so it's the important part that bugs me.

I'm using a uhmm (excuse me) 1000 entries just for the sake of graphing.

I get what you are saying, didn't put some of that in mind. No, lookup tables is much faster.

Traumflug commented 9 years ago

No, lookup tables is much faster.

Not much. For all 1024 possible values, table lookup took 1800 clock cycles on average, direct calculation about 2450. There are many performance measurement results in the commit messages. Was a nice exercise in writing fast math, lending algorithms from the legendary HP-35 pocket calculator.

Aside from this, if you want to improve further, I think we need:

BTW., I just see you're not a collaborator, yet. If you want write access, let me know.

youssefaly97 commented 9 years ago

I thought it would differ much but it came out to be 40 microseconds, no biggy. I've never measured it's performance, yet. LOL.

-The Rp tooltip is easy I figured, just like any other language. It's a bit miss leading that's all. -I'll try to figure something out for the spreading matter. -We need to compare both steinhart-hart formulas most probably although it won't be a direct approach.

While it's not impossible, it would be somehow difficult for me to develop with you on configtool. I would really appreciate it to have write access as I'm also testing the lcd3 branch, it wouldn't compile so I made some definition improvements in the config_wrapper.h as well as consistency for the whole branch. Works now. More on that later.

youssefaly97 commented 9 years ago

Here are the excel sheets if you want to take a look: https://www.dropbox.com/sh/q444on8p62blc2c/AAArnPqGShQSHmUgLor6Huoma?dl=0

Traumflug commented 9 years ago

Nice. Just picked these three commits to experimental and removed these inputfloat* branches. Thank you for your contributions.

Traumflug commented 9 years ago

BTW., welcome the Teacup development, @youssefaly97. You have now write access to this repository. The rules are simple: you can create as many branches as you find useful, but keep fingers off branches you didn't create yourself, unless you discussed your plans with the branch creator before. About the strategy in use here, see issue #81 (which means cherry-picking and rebasing are favored over merging).

Happy hacking!

youssefaly97 commented 9 years ago

Great Job @jbernardis

Thanks for adding me @Traumflug :D Will stick to the rules.

Happy hacking XD

Traumflug commented 9 years ago

Looks like this is solved?

youssefaly97 commented 9 years ago

Not completely

On Aug 6, 2015, at 12:16 AM, Traumflug notifications@github.com wrote:

Looks like this is solved?

— Reply to this email directly or view it on GitHub.

youssefaly97 commented 9 years ago

A better wording for the Rp tooltip. Easy to implement, just changing a string. A better formula for spreading the table entries. I mean better than spreading them evenly. Find out why Excel brings values different from Teacup.

On Aug 6, 2015, at 12:16 AM, Traumflug notifications@github.com wrote:

Looks like this is solved?

— Reply to this email directly or view it on GitHub.

youssefaly97 commented 9 years ago

Fixed hotend, tried to upload inputfloat2 branch but board freezes. It sends Start, ok and refuses to react to any G-Codes or M-Codes. The thermistortable.h has NUMTEMPS different from the number of entries. I'm not sure if that's the only problem.

Sent from my iPhone

On Aug 6, 2015, at 12:16 AM, Traumflug notifications@github.com wrote:

Looks like this is solved?

— Reply to this email directly or view it on GitHub.

Traumflug commented 9 years ago

Branch inputfloat2 no longer exists. Please try with experimental. (git fetch --prune removes branches gone)