Pinecilv2 fix adc scaling #1529
Conversation
…C conversion speed))
This change enables the usage of the ADC trim value, and adjusts configuration constants around it, to make the new trimmed value work correctly. This changes should have the following effects: 1) Vin measurement is unchanged on systems on which the previous scaling was already accurate. Vin measurement accuracy improves on some systems. (like on my iron) 2) Tip thermocouple measurement is unchanged on systems on which the previous scaling was already accurate. Thermocouple microvolt accuracy should improve on systems on which the previous constant numbers were not perfectly accurate. 3) NTC handle measurement is affected, because the NTCHandleLookup[] table has not been changed. It's not clear if affected in a positive or negative way. Need to discuss with Ralim about where the NTC numbers come from, and if they need to be adjusted.
…decided to leave the Mic2Diff modification disabled cause it seems to make small voltages slightly worse.
(Note: my Pinecil's coe reads as 0.9629)
|
Note: there is also a new debug menu showing the ADC coe read from efuses. |
| #define ADC_VDD_MV 3300 // ADC max reading millivolts | ||
| #define ADC_MAX_READING (62000 >> 1) // Maximum reading of the adc | ||
| #define ADC_VDD_MV 3200 // ADC max reading millivolts | ||
| #define ADC_MAX_READING ((1 << 16) >> 1) // Maximum reading of the adc |
There was a problem hiding this comment.
Is max reading truely 32 768 rather than 2^16-1 or 2^15-1 ?
There was a problem hiding this comment.
ADC_MAX_READING is the wrong name for what it's used like in the rest of the code.
For calculating to millivolts, it's reasonable to multiply by 3200, and divide by ((1 << 16) >> 1), (and maybe we can call the number something else, but not sure what to call it)
Somewhere else in the code it also does uint32_t maximumTipTemp = TipThermoModel::convertTipRawADCToDegC(ADC_MAX_READING - 1);
Now that's wrong, cause in the case of PinecilV2 it will really be:
uint32_t maximumTipTemp = TipThermoModel::convertTipRawADCToDegC(((1 << 14)-1) << 2);, which is the actual maximum value the ADC can ever return. Yes there's a -1 in there, but a number of LSB bits will be zero, and exactly how many depends on how the ADC is configured, and the platform.
So maybe we need two values:
#define ADC_MAX_READING_PLUS_1_LSB ((1 << 16) >> 1)
#define ADC_MAX_READING ((1 << 14)-1) << 2
And have the milivolts calculation use the first one, and the max tem calculation use the second one, and of course will have to figure out these numbers for the other platforms too. Do you want it in this PR?
There was a problem hiding this comment.
ADC_MAX_READING should be the maximum possible ADC reading returned from functions such as the get raw functions. It should (ideally) also be the reading that matches ADC_VDD_MV.
uint32_t maximumTipTemp = TipThermoModel::convertTipRawADCToDegC(ADC_MAX_READING - 1);
I dont follow why this would be wrong; if the op-amp is outputting our max (3.2V); then getTipRawTemp(1) should return ADC_MAX_READING.
There was a problem hiding this comment.
- the ADC:
Let me go back to basics. Let's ignore oversampling and averaging for now, and assume we're only doing basic 12-bit conversions. We have a SAR ADC (Successive Approximations: https://en.wikipedia.org/wiki/Successive-approximation_ADC ). It is basicly composed of a DAC, and a comparator, and it goes step by step from MSB to LSB basicly doing something similar to a binary search on the input signal. On first step it tests 0b1000.... If the result is high, then the signal is above 0b1000..., and then it will test 0b1100... if the result is low, then the next value it will test is 0b1101..., etc... this goes on until all the 12bits of the ADC have been exhausted. So in general if we get an integer value of X, that means that the voltage of the signal we measured was between DAC(X) .. DAC(X + LSB).
LSB in our case is 1 << (16 - 12), because it's left-justified, but you can also think of it for the sake of simplicity as LSB being 1, and the number being a 12-bit number.
In the case of an all-ones output all we know is that the signal is > DAC(0b1111....111), and in the case of all-zeros, all we know is that the signal is < DAC(0b00....00001).
- The DAC in the ADC:
So lets look at the DAC, see a basic R2R dac here:
https://en.wikipedia.org/wiki/Resistor_ladder#/media/File:R2r-ladder.png
Imagine if all of those "a_i" inputs were tied to GND, then the output signal is exactly 0V.
Hoever if all of those "a_i" inputs were tied to VREF, then the output signal would not be equal to Vref, cause there's still a big resistor network to solve there, and if you try to solve it, you will see that the maximum possible output of the DAC will be VREF * ((2 ^ N)-1) / (2 ^ N), the maximum possible value is not VREF, but slightly less than that. (for an N-bit DAC)
- So applying the above information:
- The voltage VREF * ((2 ^ N)-1) / (2 ^ N) corresponds with the DAC value of (2^N)-1, aka 0b111...111, aka the maximum possible value
- VREF corresponds with the number 2^N, aka 1 << N, aka 0b10000...000, that value is 1 LSB higher than the maximum possible reading.
- So when converting from reading to voltage, you can do:
Volts = raw_adc_reading * maximum_possible_value_in_volts / maximum_possible_value_as_raw_reading, which if you expand becomes:
Volts = raw_adc_reading * (VREF * ((2 ^ N)-1) / (2 ^ N)) / ((2^N)-1)
Volts = raw_adc_reading * VREF / (2 ^ N)
Makes sense?
So we really need two values here to model both usecases:
- converting from raw to volts
- calculating max possible voltage or max possible temp reading
- I was slightly wrong about the max value. The ADC is configured to do averaging over 4 samples, but if the original max value is (2^12)-1 then the average of that is still gonna be (2^12)-1, so the constants for PinecilV2 should be:
#define ADC_RAW_VALUE_EQUIVALENT_TO_VREF ((1 << 16) >> 1)
#define ADC_MAX_RAW_VALUE ((1 << 12)-1) << 4
Note: I changed the names again, I don't want to reuse the old name. It's better to not reuse, cause then a compilation error will alert us of any old usage of the old name, and we have a chance to fix it.
We might also define:
#define ADC_REAL_BITS (12)
#define ADC_LEFT_JUSTIFIED_BITS (16)
#define ADC_OVERSAMPLING_BITS (14)
And then have the rest be calculated automatically:
#define ADC_RAW_VALUE_EQUIVALENT_TO_VREF ((1 << ADC_LEFT_JUSTIFIED_BITS) >> 1)
#define ADC_MAX_RAW_VALUE (((1 << ADC_REAL_BITS)-1) << (ADC_LEFT_JUSTIFIED_BITS - ADC_REAL_BITS))
#define ADC_LSB_WITH_OVERSAMPLING (1 << (ADC_LEFT_JUSTIFIED_BITS - ADC_OVERSAMPLING_BITS))
#define ADC_LSB_REAL (1 << (ADC_LEFT_JUSTIFIED_BITS - ADC_REAL_BITS))
right now they have no use for it but it may simplify the code in the future.
- In the future we might also want to add 0.5LSB_REAL to the calculated volts, that is rounding to the closest volts rather than the minimum of the range the signal is in, i.e.
Volts = raw_adc_reading * VREF / (2 ^ N) + 0.5 * 1 / (2 ^ N)
In other words to balance the quantization error to -0.5LSB..+0.5LSB, rather than have it be 0..+1LSB
But that also depends on the design of the ADC, and also it's a very small offset, we might never notice, it's gonna be much less than the offset error of the opamp when it comes to measuring tip temperature. And it's gonna be lost in the 1% error of the resistor divider networks used to measure VIN and NTC. Anyway, that 0.5LSB add is not really in the scope of this change, just wanted to mention it for completeness.
There was a problem hiding this comment.
So my understanding is that in the design of a switched capacitor ΔΣADC is that once you are ~= Vref you will saturate at the maximum ADC reading. (Ignoring Offsets/INL/DNL).
Since in this case we would have the state where all caps charge to the input (which is >= Vref).
Then when all the caps are toggled across to vref and the comparitor checks the sample bus; its going to return 1 in all cases.
(10 bit example from ST AN2834 docs; as its what I have locally).
Following the switched cap model through; I think it works out that there is not this 1 bit offset.
I've not seen an R2R SAR design in a long time due to the large size of resistors in the silicone die. So I always assume they are switched cap designs. I havent been able to spot anything in the BL70x docs to suggest its an R2R design?
Please tell me if I missed an offset in that though.
(And yes I'm aware that all this discussion around effectively 1 bit is a mess).
I think we have a switched cap design; and that we can assume Vref == full scale reading, which also makes all the maths all over the place nicer to work with.
The 64x averaging is mostly to remove very high frequency noise. But also to help us to measure a better value; since during the time we are taking this measuring we have both (1) The tip cooling and (2) any drift from the handle temperature changing (often cooling). This smooths out these two a little as well.
I'm happy with name changes to the #defines. Totally agree in using that to make sure all usages are found. Either way on this discussion it may be worth the name change anyway; especially if you find that more clear :).
There was a problem hiding this comment.
once you are ~= Vref you will saturate at the maximum ADC reading
I think all ADCs will saturate their output to their maximum value once above the last transition voltage. It doesn't really matter to me what happens when you give it a voltage that saturates it, what matters is what is the last transition voltage, i.e. what is the highest voltage at which it switches from maximum-1 to maximum?
The image you attached from AN2834 is a switched capacitor SAR adc (not ΔΣ). It's cool, I didn't realize it can be implemented this way. I agree that this implementation has the physical ability to determine if the input voltage is above VREF or not, but I don't think this ability is used. Take a look at the last two capacitors at: S10 and S11, they have the same value, C/512! That means that S10 is the LSB bit, and S11 is only used to sample and hold the VIN, and to make sure that the sum of the capacitances always equals 2*C.
So in the above image, if we were to consider S11 as part of the code I will write it like this: S1 S2 .. S10 (S11)
- code 0000000000 (0) = 0V
- code 1111111111 (1) = Vref
- code 1111111111 (0) = Vref * ((1 << 10) - 1) / (1 << 10)
- code 1111111110 (1) = Vref * ((1 << 10) - 1) / (1 << 10) // This is the same as the one above it!
So switch S11 is not the LSB, this is really a 9 bit ADC that is pictured. Having switch S11 on would add "1" to the ADC code (not extend its precision by one more bit). So a hypotetical ADC might make use of this S11 bit, by adding it to the number (S1 ... S10), resulting in a value that is in the range that is inclusive on both ends [0000000000 .. 10000000000], I don't think I've seen an ADC that reports its results like that, so I think S11 is simply ignored when calculating the ADC code, and is only used for sampling the input signal. (or perhaps it could be used for the positive saturation interrupt that the bl70x ADC has, but the datasheet is not clear if that gives extra information, or is it just a binary compare to all-ones...)
Furthermore if you scroll a little more in AN2834, you will see the description of gain error, where:
Yeah you see digital output is 4095, so it's not the exact same ADC as in the previous drawing, but ignoring that, see 4095 is the maximum value, and it is reached at the voltage 4094.5 LSB
where LSB is defined as:
So 4094.5 LSB is the lowest voltage that can generate a 4095 reading
And in the description of the offset error:
You will see that 0.5LSB is the lowest voltage that can generate a code of "1"
So even in this application note Vref does not match the maximum output value of 4095!
I actually think that the math is more intuitive this way, if the ADC can never output a code that matches Vref.
I think of it kind of like a fixed-point fractional-only number, where Vref would be the integer "1".
If you had a fixed point number with 1 integer bit and 12 fractional bits, then:
- 1.000000000000 would be the integer "1", or 1 * Vref in voltage
- 0.111111111111 would be the maximum value of the ADC that is a little smaller than Vref.
Except the integer part is never shown. The only complication is, what is the most intuitive name for the 1.000000000000 code, that is equivalent to Vref, that can never be output by the ADC? I'm gonna go with ADC_RAW_VALUE_EQUIVALENT_TO_VREF for now, but if you have better ideas then let me know.
I am not very familiar with the workings of actual ΔΣ ADCs, so I can't say with 100% confidence that they also behave the same as I've described above, but my fundamental expectation would be that they behaved the same, because I actually think the math is nicer this way.
There was a problem hiding this comment.
Okay I think we are both on the same track here; and I was very off track to a poor define name <-> usage (and general lack of sleep).
Apologies for that massive side-track and a half.
To come back to the original cause of all of this; yes update the #defines to your preference.
Keeping in mind that we do, do oversampling on all devices as it removes a bunch of noise sources (and gives maybe some increased resolution, not we need it).
There was a problem hiding this comment.
@purdeaandrei
Do you want to refactor to use nicer #define names or would you prefer I merge this as-is?
There was a problem hiding this comment.
Hey, these are my notes of remaining todo-s, not necessarily blocking the PR:
- Refactor naming of macros for ADC range calculation
- Solve the possible overflow due to coe < 1.
- Improve upon the calculation of maxTipInC, if coe is > 1.
- Add debug menu to read millivolts at VIN sense net. This can then be used by supplying various voltages at VIN, and measuring with both a multimeter, and with the iron the voltage on the VIN sense line, and comparing the results. This should bypass any inaccuracies coming from the voltage divider. The voltage divider can be up to 1.9% inaccurate (with 1% resistors), but most should be closer to the ideal value, and not on the edge of accuracy. In any case, bypassing that source of inaccuracy can give more confidence in the correctness of this PR. This debug menu doesn't necessarily need to be kept forever.
- Use the above debug menu to check for the accuracy of the rescaling work done in the PR on at least a couple more irons.
- I would also like to see some temperature measurement accuracy checks, and comparing with and without this PR (but both should probably include your PR 1535 , which I haven't tested yet) I don't really have an iron temperature measurement tool. I ordered an off-brand one from Aliexpress. But maybe someone in the community has a calibrated high quality measurement tool we can ask to do some checks...
Note that so far I have checked my own iron's millivolt accuracy, and the error is around -0.34%..-0.71%.
I have not yet done any checking of:
- the fresh NTC tables I have calculated. The result looked okay-ish, but I don't actually know if it works right. I did no measurements to compare against.
- I did not check if the conversion from opamp microvolts to thermocouple temp differential is still correct or not. I assume it should be cause before this PR the ADC output value was kind of similarly scaled, just hardcoded. But again, i didn't actually do any checking on this. And I'd feel much better about merging this PR only after knowing for sure that it's not making things worse than before.
If you want to merge this PR without waiting for some more work to be done on the above points, I'm okay with that. In any case I'll try to set aside some time to look into this PR some more over the next weekend.
There was a problem hiding this comment.
- I would also like to see some temperature measurement accuracy checks
@purdeaandrei
I have a 191 tip temperature measurement device and both a V1 and V2 if you need help checking temperatures. just need to tell me exactly the steps. similar to this device https://a.co/d/fzVQAR6 (it's a cheaper kind so it's only so accurate, many YT people seem to use this same one).
just point me to which beta you want installed and then how you want the measurements done if this device is enough.
…start accidentally using the wrong VOLTAGE_DIV
source/Core/BSP/Pinecilv2/Setup.cpp
Outdated
| @@ -80,26 +80,47 @@ void setup_adc(void) { | |||
| adc_cfg.vref = ADC_VREF_3P2V; | |||
| adc_cfg.resWidth = ADC_DATA_WIDTH_14_WITH_64_AVERAGE; | |||
There was a problem hiding this comment.
Any chance; you could maybe try and add a block comment before these with a TL;DR of your findings and ∴ why these were selected.
For the future reader :)
There was a problem hiding this comment.
I added a note to explain.
This made me think a little bit about it, and I just realized there is a risk here that the output value will overflow, and it depends on the coe in the efuse. In the worst case (smallest coe we measured on our combined 3 irons), the input code will be int((4095 <<2) / 0.9438) << 2 = 69420 which overflows the 16-bit value of the filter in the current code.
So there's a question of how we handle this overflow.
I'm leaning towards:
- reducing the shift count by 1, for all values, to avoid overflow.
- getMaxTipInC should still be based on the 3.2V limit, because the datasheet says max input to the ADC is 3.2V. While I think there are some temperatures that we could measure above 3.2 but below 3.3V, I don't think we should rely on this behaviour.
- for getMaxTipInC I need to write some code, such that in case some Irons will have a coe > 1.0, it can calculate the lower possible maximum temperature. So I think ADC_MAX_RAW_VALUE needs to become a function, not a macro, cause it depends on the "coe" value.
There was a problem hiding this comment.
because the datasheet says max input to the ADC is 3.2V
I do love this addition to the datasheet not being in the first one I had 🙃 It turned up a while later.
getMaxTipInC should still be based on the 3.2V limit, because the datasheet says max input to the ADC is 3.2V.
I agree
So I think ADC_MAX_RAW_VALUE needs to become a function, not a macro, cause it depends on the "coe" value.
Yep I think so too; It will need to be pulled out to a function and can be a const for other devices.
Are you still planning to complete your self-imposed tasks so that this PR can be merged? |
|
I'm still planning to do those todos. |
|
Then we'll have to wait until you have time again, I guess. 😊 |
corrected style
corrected style
corrected style
corrected style
corrected style
corrected style
corrected style
|
@purdeaandrei |
|
Yeah, should be closed automatically when I merge that one |
This PR changes the ADC settings to be more in line with latest bl SDK (except for 1 bit called mic2diff, which I found to make things slightly less accurate), and makes it use the ADC trim factor in the EFUSE of each chip. There is no more magic "62000" number to scale things, and the default voltage div of "630" is not magic either anymore
That is VOLTAGE_DIV=630 comes from: (1 << 16) * 4 / 10. / 3.2 / 13, Where: Where
ADC_VDD_MV=3200 - this is the actual ADC Vref
ADC_MAX_READING ((1 << 16) >> 1) This is because we first left-justify to 16-bits, then at some point we shift it to the right by 1.
The changes should have the following effects:
1) Vin measurement is unchanged on systems on which the previous magic number scaling was already accurate. Vin measurement accuracy improves on some systems. (like on my iron)
2) Tip thermocouple measurement is unchanged on systems on which the previous scaling was already accurate. Thermocouple microvolt accuracy should improve on systems on which the previous constant numbers were not perfectly accurate.
3) NTC handle measurement is affected, because the NTCHandleLookup[] table has not been changed. Currently it reports a too high handle temperature, this still needs to be addressed.
Note: since VOLTAGE_DIV is a saved setting, and the default now changed, you may need to reset to factory settings!