-
-
Notifications
You must be signed in to change notification settings - Fork 725
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
Major issue in TS100 code - Chapter 1. ADC #1038
Comments
Hello, Okay there is a fair bit here to unpack, so I will probably be back later on with more to this reply as I have time to check things out. But I will address the things I can now. a) Timer+ADC setup may be wrong Honestly back when this was coded forever ago I did do a fair bit of validation on this, but it could absolutely be wrong. Would definitely love to see a PR if you feel like making one 😂 But, I absolutely will dig into this as you are probably right here. b) Dual sampling of two the two adc's at once which may be bad Could you point to a reference for this? Back when this was written it was written in reference to the STM32F1 reference manual. So would love to know where this from :) c) Input adc readings are not sampled inline with PID So the intent of the current code is that we want to measure a mix between voltage during sag and at idle to allow for showing both to the user, though this is probably pointless at this point in time. d) Not certain off the top of my head, will need to look at this again but most likely it can be optimised e) That code was once upon a time from an Adafruit library, though we do not use the interrupts as one some irons they do not work (not soldered correctly). The highpass was turned on to be used for activity detection, with orientation based on its internal orientation engine. f) Handle all ADC sampling in one place only Honestly it sounds like you have done a fantastic job of re-writing software for these devices, if you would like to contribute anything back to this software that would be fantastic, or even open source your software so that others could use it / community could share resources and ideas back and forth. Obviously there is no pressure to do this though; this is just a tiny project in the corner of the internet :) This project does need some love and a little life in innovations, so anything you would want to release would be excellent to see, though no pressure. There has been a lot of hands in this codebase over the years and some things have evolved more than others. It is probably worth doing a re-evaluation from the ground up of some of the code that has been around for ~5 years. Definitely appreciate the effort in writing this up for the community, and will be back soon (i hope) with some changes based on this, I think you are absolutely correct on these and integrating some or all of these will take time but should be worth it. |
@sandmanRO Your work will prove to be good on its own if it works on other TS100s as well. On the other hand, you did a great job posting this. Especially this part made me prick up my ears:
Because there were discussions in the past about the temp readings being all over the place. The graphic mode got my attention as well. And maybe the code for the Looking forward to the next chapter. 😁 @Ralim It would be exciting witnessing a collaboration between the two of you. |
@discip Honestly, all of these changes here sound like exactly the sort of tweaks and changes I would love to be able to pull in to improve the state of things for everyone. I wasn't surprised that there were bugs in the setup of the DAQ code, some of this code goes back to when I was first using STM's and hasn't ever really been touched since then. If the code for this was open sourced or even chunks of it posted here or as a PR It would be greatly accepted and integrated. Would be huge appreciation for graphical improvements given that some people really love flashy GUI's :) That said, guessing by the screenshot this code is not based off IronOS, so assuming as such PR's are unlikely. That said, the writeup is highly appreciated, have always wanted someone to audit this code and I think I finally have that 🙃 For the first point I'm not 100% certain the code is exactly wrong as the TRGO output is not being used, but rather the direct link from T2 CC1 to the ADC, and the CC1 event is triggered on match, ignoring the polarity of the PWM configuration. My validation for thus was to move the ADC sample time well into the PWM driver area (pull it back into the "power" area) and test both methods, and both methods resulting in similar disturbances depending on where the value for channel 1 of timer 2 is set to. Noting sample size here is only two units as thats all I have on me at the moment. Could be something else going widly wrong 😁 Did also disable the fast/slow PWM code to make this a fair test. That said, this testing has shown that we could easily have a longer adc sampling time, so I probably need to dig into why it is so short. I think it was done to keep short the settling time to the ADC cap was quite fast with the nearly direct drive of the op-amp, but might need to get out the scope or datasheets to validate that. Though I do think your setup is more logical of a way of going about it though, so inclined to change to it anyway just since it is far more clear of a setup. Would be curious to see the actual setup you are using on the timer to have these scary stable readings, if you are willing to share of course. Testing just one injected channel seems to perform about the same as the pair, though not particularly noiser than the other from what I've seen and I do like the idea of moving the power sampling to be in sync in the ADC2 injected time slot. I'll have to do the maths about the sampling time, the 8pF internal capacitance is quite small in the stm32; but also this will heavily depend on the speed of your ADC :) I agree the fast and slow pwm is a bit odd, I believe the authors intent was that it preserves the same delay+sample time, even with the speed change. As this part should be fairly constant, rather than having just a change of the divider, as this would effectively half (or double; your choice) this time period the ADC is sampling in. Anyways, definitely some good food for thought in this; and also a huge thankyou for your interest in the first place :) |
Hello Gentlemen, English is not my first language (that I suppose is obvious) so please don't take my bluntness as lack of respect. I took the time to write down my findings and that by itself is anything but lack of respect for your work. Few questions have been raised. I will address them. First the use of the same channels on dual mode. |
On Wed, Sep 08, 2021 at 03:22:11AM -0700, Ben V. Brown wrote:
I agree the fast and slow pwm is a bit odd, I believe the authors intent was
that it preserves the same delay+sample time, even with the speed change. As
this part should be fairly constant, rather than having just a change of the
divider, as this would effectively half (or double; your choice) this time
period the ADC is sampling in.
I do not remember it clearly by now but it feels like your description
is pretty accurate. Guess the idea was that we want to spend as little
time as possible on ADC (when going full power), and your code ("slow
PWM) was already doing that, so using higher PWM frequency when
_lower_ power was asked for requires changing the ADC times as it's
expressed in PWM clock ticks or else we would be trying to measure too
fast for the hardware.
…--
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
***@***.***
|
As my code is fairly different I will right down some snippets of code that would follow the naming used with your code (if/when possible) so it could integrated with yours with minimum effort if you decide to do so. I should have them in a couple of days or so. I hope it's ok if I will attach them as zip file here. |
No problem, honestly your English is good enough that it isnt super obvious its not your first language. Out of curiosity, what is your native language? (As a sad single language user I find it super interesting to hear about)
Ouch, yep I never saw that note. I've not really noticed the impact of this but definitely will get a patch to work around this; will follow your suggestion to sample vin at the same time.
Honestly, so long as the code is readable in English (or near enough, online translators get me quite far) to figure out intent, dont stress too much about restructure, I've worked with some terrible things, I'm sure your code is perfectly understandable. Attaching a zip is completely fine, anything you are willing to share I would love to be able to see, and integrate what can be integrated :) |
As for the 'precise' time when the injected ADC is triggered you could try this. Get the ticks from TIM2 CTR1 Period Complete and compare them with the ticks right after the PID triggered. If the injected channels starts on TIM2 CTR1 Pulse Complete the difference should be at least the equivalent TIM2 CTR1 pulse width. Otherwise (and you will notice that) if the difference is like 0-1 ticks then the injected ADC is triggered by TIM2 CTR1 Period Complete and not by pulse complete as intended. The PID loop wakes up triggered by injected ADC conversion complete interrupt so measuring the time from PID loop would give us a fairly accurate (within 1 tick) time when the injected ADC is triggered. |
As I assumed, your getting along quite well already. 😁Can't wait to see the results of this teamwork of yours! |
Sorry I missed that question, possibly others. I'm Romanian. For the past 15 years I've been running a small business in the field of data acquisition (www.roscientific.com). Before that I was employed as software engineer for the top dog in the field. That would be National Instruments. However, I'm a programmer by trade. My actual background is physics. Nevertheless, it turns out that having a decent physics background is a good assed in this business as it allows to interface with customers from various engineering fields...and that would be about me. |
|
The powerStore is based on this template:
|
I keep all the internal temperature related variables in Celsius multiplied by T_FACTOR (this is 9000). This way we can switch back and forth between C and F units without changing the actual internal value. |
the powerLimit is minimum between the hardware limit (with TS100 this would be 65W) and the user specified limit |
getPwmCycleRate() would return the actual frequency (in Hz) of the PWM cycle (in my case this switches between 5Hz and 10Hz only, depending on the conditions) |
Now let's go the configurations for ADCs and TIMs |
This is ADC1. The regular ADC1 reads the handle temperature, injected ADC1 reads tip thermocouple static void ADC1_Init(void)
} |
This is ADC2. Only injected for reading the voltage input static void ADC2_Init(void)
} |
This is TIM2 static void TIM2_Init(void)
} |
static void TIM3_Init(void)
} |
Man you are fast! 🤯I hope @Ralim is able to keep up. 😁 |
And this is the master init sequence: HAL_ADCEx_MultiModeStart_DMA(&hadc1, ADCBuffer, ADC_SAMPLES); |
And now the actual data grabbing: // Start of ADC Section ***************************************************************************************************
|
Now how do I set the PWM (the call is made from PID loop and the power is the powerOut as seen above). getPwmCycleTicks() would always return 199 as I'm using a fixed PWM cycle period.
|
And this is the full PWM handling
|
This should cover pretty much everything. Maybe just a small note on the PWM switch: in real terms, I switch from slow PWM to fast PWM only when the ON time in slow PWM cycle is less than the actual ON time in fast PWM cycle. As the tick rate in slow PWM is half the rate of fast PWM we divide pwmPowerTicksLimit by 2. The added (pwmBaseWaitTicks + pwmBaseDaqTicks) works as hysteresis. These are constants used: |
Now about movement:
|
And this is how the orientation returned by LIS chipset is handled:
|
Hello paulfertser, The issue does not lie in the way the PWM cycle is segmented. That is absolutely fine. Moreover, changing the PWM cycle parameters is not necessarily bad in itself as long as you keep some tracking of the actual cycle time. The real issue with the current implementation is that there is no real correlation between PWM cycle vs, feedback data & PID output. I mean, obviously there is an arithmetic correlation but not necessarily a sound one. Not sure how to put this so it would be clear: the POWER by itself is an abstract notion...it speaks about the ability of a system to deliver/absorb a certain amount of energy in a certain amount of time. However, what heats up the iron tip is not the "POWER" but the amount of ENERGY we are transferring to the tip (in this particular case the transfer is done by converting electrical energy into thermal energy by resistive effect). We can arithmetically work with POWER but in that case we need to keep in mind that the ENERGY is POWER times TIME. That is, the maximum amount of energy we can transfer to the tip in one cycle is the available power (this is given by the used power supply) times the cycle period. Obviously, the closer you get to the setpoint you might need less than this maximum amount and that is the purpose of the feedback loop, to constantly adjust the demand. The current implementation does not really consider the time aspect. Looking over the comments in the code I see that there is some awareness regarding the issue but nothing implemented consistently. Otherwise, a feedback loop, PID or alike, would get you to the setpoint eventually, even if not set properly, but usually it would either (sometimes severely) overshoot then oscillate up and down setpoint or reach setpoint after longer than necessary time. With the above suggested modifications you will not have such issues disregarding your target is set to 40 or 400 Celsius (and any value in between for that matter) and finally have a TS100 that works as it was meant to. The code could be perfected no doubt but it's the simplest code I could think of, that would still do the job decently. |
I've been working through the ADC setup against yours, and a few notes:
Given this, I think the stability you are seeing is mostly a result of just having the better filtering layout in the sampling. By just applying your changes to (wisely) filter the adc readings at the source, and lock sampling time to the PID iterations the values far more stable. I've refactored to use this wisdom with the sampling and filtered and its now in the Going to do this as separate items to keep the changes distinct. This is just the ADC refactor to handle sampling locked in time with the PID loop. Then after that will look at fast/slow PWM, and the PID |
Ralim, what the are you talking about? It's exactly the other way around you say it is. ADC_EXTERNALTRIGINJECCONV_T2_CC1 means trigger on Timer2 Capture/Compare 1. In the current setup is occurs on rising edge of TIM2 Ch1. It is true that you can configure it to occur on rising, falling or both edges but the default is rising edge. That is exactly what my diagram, and "claim" said. I have suggested you a simple test. You could try it unless you prefer to revisit STM32 documentation. As for the presumed better filtering, I'm averaging on 4 times less samples than your code does so that statement of yours does not stand either. The stability does not come from better filtering but from the sampling the input voltage at the right time, as stated with my first message. Look, it's not my purpose to convince you guys of anything nor persuade you change your code. You do whatever it feels right with it. I honestly don't care. |
I said that's as in my testing having the timer polarity high or low gave me a trigger time of 283+-1 on my test unit. I've been testing around and the most stable for me in terms of testing has been to use trgo rather than cc1 directly. I was asking as I was not seeing the results I would have expected and thus was curious. Could also be this TS80P, it's had its stm32 replaced a few times. |
So, looking at your PID I love how simple it is.
As you probably noticed the output of that timer is AC coupled. I'm wondering if you have done any investigation into this, and if so what your results were? ( If you have an opinion that is, you dont have to have one 🙃 ). |
In regards to the orientation handling; If so, did you find 10.5 degrees was a good number (was it tested for, picked semi-randomly or one you already knew) ? |
10 degrees felt right...it would mean something around two thousand nine hundred something counts....so I picked to closest "clean" integer that happened to be 3000...that in real means about 10.5 degrees. |
I thought about getting rid of the internal orientation as well. But then again, I spent a whole day traying to make sense of LIS documentation and kind of felt bad about giving up on all that wasted time. Otherwise, there is no real benefit coming out of it. Working with x, y, z accelerations would do just fine. |
I feel a little better to not be the only one that doesnt like their docs. |
If you fill that a different threshold is right, the math is like this: in +/-2G scale, 1G means about 16384 (that would be Earth acceleration at 90 degrees in respect to the horizontal). For a random alpha angle the threshold would be something like 16384 * sin(alpha). |
"feel" |
Yeah unit conversions are fun with the five different accelerometers... They should all be scaled to be similar scale already so will mostly be validation time I fear. Also if you know of a "good" way of doing it other than just checking the gravity vector in the averaged data and masking movement I'm all ears |
that simplified "standard deviation" is best way I could think of. It gives an indication on how close the data in the collection is to the average value. A small std means the values are close to the average, which in turn means the acceleration values are static. A large std would imply a large spread of data in respect to the average value, specific to dynamic accelerations. |
the true standard deviation would have implied to sum up the squared differences then do the root mean square over the result. |
so that "std" could be used to discriminate both movement (if std above a specific threshold) and static (if std below a specific threshold) accelerations. The threshold for static and movement discrimination do not need to be the same. |
Most likely there must be a more efficient way doing it but I just could not think of any other right now. |
@sandmanRO |
Well, this is the trade...you will let me use your soldering menu animated icon. That was a stroke of a genius. No matter what I tried, it did not produce the same visually suggestive effect. It would be for internal use only...that is four TS1000 units that I purchased sole for our own needs. Let me stress I'm neither in the business of selling soldering irons nor have any intention of releasing this work to public. |
Hi, |
Hello Firebie, I can not speak about Pencil (and honestly my interest does not extend beyond what I'm using, that is TS100) but I have a whole bunch of TS100 tips, various designs. Their thermal mass varies within a range of at least +/-2J/C. 6.5J/C would be a good average value. Using a smaller than real thermal mass would make the PID reach the setpoint slightly slower. Using a largen than real thermal mass would probably make the PID slightly overshoot (especially if the starting temperature is close to the setpoint). However, it's not a critical but rather a guiding value for the PID. Of course, having a spot on value would be ideal but it would imply to know / set precisely the tip you are using. Personally I find this complications unpractical in respect to the outcome, but this is me. |
My view on you using it is that it's fine, since it's both personal use (which main reason I went with the licence was to stop it being "sold", a company is free to use it be default so long as they maintain what it is / release their source). Also you are keeping the main intent of upstreaming your improvements, so to me it's fine. That said; is love to see your other improvements dumped whenever you have the time if you wished. @Firebie good question, smells like a bad merge. Going to revisit all of those numbers soon (when time permits) so will rectify that too. Most likely the old algorithm was miss tuned enough it wasn't noticeable in practical use. |
...and if I manage to do something that I fee would add value to your project most certainly I will post it up. That is a promise. |
Three weeks ago, while in the look for a portable iron I ended-up purchasing a Miniware TS100 unit. I was petrified by the crudeness of delivered factory firmware and I was looking for alternatives. I came across your firmware, but honestly I was not significantly impress as compared to factory firmware. Left without alternatives I had no choice but build my own. Nevertheless, I though I would share with you some of the issues I noticed in your code. No offense gents, but let me just say that your code works by pure accident. In fact, it's one tick away from failure. Where I should start? Oh well...there we go:
a. The injected ADC1 channel is not triggered when you believe it does. In the way the TIM2 CTR1 was configured, the injected ADC1 (temperature reading) is triggered on period complete (as opposed to pulse complete) of TIM2 CTR1, that is, right when the heating is about to begin. Only the use of the smallest sampling time and the delay induced by software starting of TIM3 CTR1 (the actual power PMD control) from TIM2 CTR4 period complete interrupt saves the execution from total failure, that is reading the temperature while the tip heater is on.
b. All the STM32 documentation specifically state not to simultaneously sample the same channels with both ADC1 and ADC2 when dual ADC mode is used as this could lead to conversion errors, and yet, the code does exactly just that, on both regular and injected channels (ADC2 duplicates the readings of ADC1, same channels at the same time).
c. The input voltage readings are not synchronized with the PID. As the input voltage drops during heating window, you get a collection of random values between maximum voltage (voltage at idle) and minimum voltage (voltage during heating window). No wonder the voltage readings, even with the 16 sample DMA buffer averaging + extra post acquisition (8 samples deep) average filtering, jump all over the place.
d. During the switch between 'fast' and 'slow' PWM, for some unknown reasons you took the approach of changing everything, tick rate, total tick count and the pulse count of TIM2 counters. This leads to variable PID cycle rates which in turn makes the full scale synchronization with all ADCs virtually impossible.
e. The LIS2DH (the accelerometer with the latest revision of TS100) is not configured properly (if at all - the code is attempting to configure the interrupt registers using a wrong address with unknow side effects). No wonder your code ended up getting the accelerometer data in pulled mode. Otherwise getting the accelerometer data in pulled mode is not necessarily a bad thing as it allows to do some extra processing at your won pace. However, you activated the high pass filter which means you are eliminating exactly the static acceleration (Earth gravity) that is critical for orientation discrimination. No doubt, LIS2DH orientation data sucks. I agree. As the LIS2DH does not provide low pass filtering (only high pass filter), in my implementation I ended up using a simplified STD (standard deviation) filter, to determine if an orientation change was triggered by a temporary acceleration or by static acceleration (Earth gravitational field). Only the last one would be consider for an orientation change as I don't want the display to flicker every time I move the iron left or right. Moreover, LIS2DH generates orientation data only for the axis with maximum acceleration. So if the tip of the iron is oriented significantly upwards / downwards (accelerometer X axis) you will no longer get orientation for the axis of interest (accelerometer Y axis) in respect with left/right handed display orientation so I'm compensating this by using a software algorithm that considers the actual X, Y, Z accelerations as provided by the accelerometer. Now I'm keeping the iron only in automatic orientation mode by default as finally it's working consistently.
Well, these were the DAQ (data acquisition) problems. Here are my solutions. For start let's review the correct daq sequence:
As the injected ADC channels are triggered by the RISING edge of the TIM2 CTR1 counter (and NOT by the falling edge of the counter output), to get the timing right, it is critical then that TIM2 CTR1 is configured as ACTIVE LOW (sConfigOC.OCPolarity = TIM_OCPOLARITY_LOW;)
Moreover, to avoid data conversion errors, I use the ADCs as follow: ADC1 regular, rank 1 through 4 for reading handle temperature, injected ADC1 ranks 1-4 for tip thermocouple, injected ADC2 ranks 1-4 for voltage input readings, both injected channels with a post acquisition (8 slots deep) average filter. Anything, regular and injected, use the same sampling time of ADC_SAMPLETIME_28CYCLES_5 (anything lower than this would not be enough to correctly measure a 'large' signal like when the input voltage has a value of 24V, even if internally divided by the R20 (75kOhm) and R21(10kOhm) resistive divider). Moreover, the samples are updated in ONE single place, that is within the PID loop. All other calls within app would call for filtered (average) result. This way we keep everything synchronized. You guys do that and you will be amazed how stable everything becomes (so stable that several times during development I though the STM32 just hanged on me...nope...only scary stable readings). For PID I'm using a simple, yet fully effective self-decaying integrator and nothing else (no proportional, no derivative term). When integrating I'm taking into account the PID cycle period as integration time, and as decaying factor I'm using the thermal inertia of the tip (thermal mass) that makes perfect physical sense. Despite its simplicity, it turned out to be so accurate that I even had to include a gain of 2 when integrating for compensating for the fact we are actually pumping current through the tip only half the time of TIP2 CTR4 pulse as the actual TIM3 CTR1 that drives the power has 50% duty cycle.
Regarding timers, in my implementation I keep the TIM2 period (total ticks) fixed at 199 ticks and change only the TIM2 frequency (TIM2 prescaler at 4000 and 8000 respectively) and the pulse width of TIM2 CTR4 as requested by the PID. The TIM2 Period / PID Cycle will switch only between fixed 5Hz and 10Hz rates so everything stays synchronized.
I would stop here. If I start talking about user interface, it would take me days.
Below it's a glimpse of my implementation. All sort of bells and whistles like OLED contrast, enhanced graphics, double click button events, single / dual language support + spot on data acquisition w/ augmented automatic display orientation. Despite my best efforts I could not manage to get more than two languages fit in at a time. In my implementation each language adds about 10k to the hex file. With one language the hex is about 113k, two languages about 123k, three languages in excess of 132k so it would no longer fit in.
Idle Screen (graphic mode)
Buttons pop-up with animation (they 'inflate' with animation from the small buttons) when sensing movement so the user would know which button does what
No tip display
Tip hot notification
The text was updated successfully, but these errors were encountered: