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

High wattage means tip temp is low, compensate #726

Merged
merged 2 commits into from Dec 28, 2020

Conversation

dhiltonp
Copy link
Contributor

@dhiltonp dhiltonp commented Dec 5, 2020

I just figured something out.

Long story short, the actual tip temperature is lower than the tip read temperature by some value, and I think that value may be linear with the I term.

Expected behavior:

We want to see power be high until the element is fully heated, at which point watts drop to maintenance levels very quickly without overshooting.

When touching the tip to a large thermal mass, we want to see power output very quickly jump up/jump down.


I've tested this minimally, but haven't really tuned it - we might want to adjust the P and I gains, I haven't reviewed the exponential moving average filter on I, I haven't looked at the effects on calibration etc. etc.

From

PR
time (s) 0 12 14 17
temp (C) 26 320 320 320
power (W) 0 49 6.5 4.1
master
time (s) 0 11 19 21
temp (C) 30 320 320 320
power (W) 0 49 6.5 4.5

I also tested this against a mass, dumping heat into a washer.

Time to stabilize power output was around 2s for this PR, 6s against master.

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Dec 5, 2020

I haven't touched temp in F, I should probably do that...

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Dec 5, 2020

Updated getTipInF as well.

@Ralim
Copy link
Owner

Ralim commented Dec 6, 2020

This makes sense!

Thinking it would be good to pull out the magic running variable to a #define as a setting somewhere so it can change for different models of iron.

I'll give this a test on the smaller tips too before I merge :)

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Dec 7, 2020

Thanks! It really could use more testing.

I think there might be a magic number that just works? But I haven't done any math to correlate this with a physical model, and I've only tested it on a BC2.

BTW, the current PID loop seems to pulse a bit, I'm surprised we're seeing stable temps. I just revived some old code to see instantaneous wattage. https://github.com/dhiltonp/ts100/tree/instant-wattage

With the pulsing in mind I'm almost surprised this PR works - it barely effects the stability of the temperature readings even at 400C when the wattage is bouncing from 0-15W.

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Dec 7, 2020

I guess the ideal temperature compensation would compensate the thermocouple temperature to tip temperature even when the wattage applied is oscillating at various rates 🤷‍♂️

I don't recall there being any smoothing on the thermocouple other than taking 8 readings and averaging them. I'm not seeing anything that's changed. Let me know if I'm wrong.

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Dec 7, 2020

Oh, one thing to note - I have not tested this against external thermocouples due to a move+COVID. I'm reluctant to go through the effort of getting my testing equipment shipped to me or buying it again when I'll get easy access to it in 6-9 months.

@Ralim
Copy link
Owner

Ralim commented Dec 13, 2020

I will do a bit more testing on this, but first pass seems quite good, and also love simplicity of implementation ❤️

Will need to find my darn thermocouple before release.

Pulsing is a slight issue, which came up from having dynamic PWM<->ADC speeds, so that we can have more "on time" during the bulk heat up.

Possibly need more filtering somewhere in the PID-ish control loop but honestly haven't had the time to dig into it, since it still works quite well 🤣

@Ralim
Copy link
Owner

Ralim commented Dec 17, 2020

FYI:
This is looking good in my inital testing.
Need to double check with a thermocouple first but should be ok to merge otherwise I think :)

@Ralim
Copy link
Owner

Ralim commented Dec 17, 2020

@dhiltonp
Any chance you can merge master back into this branch, just so I can check that I didnt magically break this :) 🤣

@dhiltonp
Copy link
Contributor Author

Updated.

I haven't tested the merge, but I expect it will be fine.

I'm really interested in what the tip temp looks like via thermocouple!

@Ralim Ralim merged commit 768d36c into Ralim:master Dec 28, 2020
@Firebie
Copy link

Firebie commented Dec 30, 2020

May be we should do this compensation only from PIDThread:
if (sampleNow) currentTipTempInC -= x10WattHistory.average() / 25;

@dhiltonp
Copy link
Contributor Author

That would look pretty odd - users would see the temp reading higher than their set point, which it isn't.

@Firebie
Copy link

Firebie commented Dec 31, 2020

You are right.
Sometimes I have another issue with latest build - temp (target - 350C) suddenly goes higher (427C) than max for the tip (413C), and stays there for a while (power consumption becomes almost 0 - which is correct, as iron thinks that there is a big overshoot).
TS100, pcb ver 1, 22V 4A

@Ralim
Copy link
Owner

Ralim commented Dec 31, 2020

When you say max for the tip, what number are you referring to ?

@Firebie
Copy link

Firebie commented Dec 31, 2020

I dropped part of code for 'if (tipTemp > tipDisconnectedThres)' and always show tip temp. So, for disconnected tip it shows now 407C (ambient is around 20C).

@Ralim
Copy link
Owner

Ralim commented Dec 31, 2020

@Firebie This is not related to this pr, so can you move this into an issue instead?

@Firebie
Copy link

Firebie commented Dec 31, 2020

Ok, I will check something and will open a new issue, if any.

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Jan 1, 2021 via email

@Ralim
Copy link
Owner

Ralim commented Jan 1, 2021

The tip reading should_ be independent of supply voltage.

But tuning may not be 😂

@Firebie
Copy link

Firebie commented Jan 1, 2021

Now I can't reproduce.
Before, I was also using your another patch - 'show instantaneous wattage', I suspect that this may be related to this code:

--- a/workspace/TS100/Core/Inc/history.hpp
+++ b/workspace/TS100/Core/Inc/history.hpp
@@ -20,11 +20,10 @@ struct history {
 
 	void update(T const val) {
 		// step backwards so i+1 is the previous value.
-
+		loc = (loc + 1) % size;
 		sum -= buf[loc];
 		sum += val;
 		buf[loc] = val;
-		loc = (loc + 1) % size;
 	}

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Jan 2, 2021

Thanks for your testing, @Firebie.

You are right that that is the problem with the instant-wattage branch. It's somehow causing pulsing of temperature, which is really weird, given that only x10WattHistory_orig uses the bracket notation.

I have no idea how that's the problem, but I do know how to work around it, I've pushed a fix.


Notes:

Using the "instant-wattage" branch, I saw pulsing of both wattage and temperature:

  • 2x/sec at 10W/9V
  • 1x/sec at 20W/12.5V
  • 1x every 3.5 seconds at 50W/20V.

The pulsing affects both the wattage and temp reading, which is confusing because the temp-correction branch shows mostly stable temperature, and the instant-wattage branch should only have effected the display of wattage, not temperature.

x10WattHistory_orig uses the history class, as do rawTempFilter and tempError. We only use the average and update methods for those 2 variables, never the array index notation, so somehow that update causes the averaging to break?

That's really weird, because {0} should initialize all array locations to 0, so it shouldn't matter where we start...

I'm rusty at C, but not that rusty... this is weird.

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Jan 2, 2021

With this fix I do still see some pulsing/instability in the instantaneous wattage but it's not as bad as I had thought when I wrote #726 (comment).

The wattage jumps from 5w to 10w for a fraction of a second a couple of times a second, the highest I've seen it jump was to 15, sometimes it drops to 0 for a fraction of a second.

@Firebie
Copy link

Firebie commented Jan 2, 2021

Out of curiosity - why you have used 'history<uint32_t, oscillationPeriod> x10WattHistory_orig' instead of simple 'uint32_t actualX10Watts' for instantaneous wattage?

@dhiltonp
Copy link
Contributor Author

dhiltonp commented Jan 3, 2021

That was so I could easily do back-to-back comparisons of the exponential average vs running average :)

@Firebie
Copy link

Firebie commented Jan 6, 2021

With latest build, during soldering process, with target temp 320 C, sometimes, on screen, I can see temps over 400. (for half second).
PSU set to 24V, 4A.

@Ralim
Copy link
Owner

Ralim commented Jan 6, 2021

@Firebie which build do you refer to here?

@Firebie
Copy link

Firebie commented Jan 6, 2021

Latest master

@Firebie
Copy link

Firebie commented Jan 6, 2021

Sorry, used version with some changes in sources
Vanilla master doesn't have such issue

@Ralim
Copy link
Owner

Ralim commented Jan 6, 2021

If you have a public branch I can try and help find the issue

@Firebie
Copy link

Firebie commented Jan 6, 2021

Thanks for help
I think that I found problematic piece of code - it was bug from my side
Sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants