kanflo / opendps

Give your DPS5005 the upgrade it deserves
MIT License
878 stars 124 forks source link

Device Specific Calibration #17

Open geekbozu opened 6 years ago

geekbozu commented 6 years ago

Not sure if this is an actual bug. But I got my DPS today flashed it with OpenDPS and was playing with it. I currently set the voltage to 5.00 and when I turn it on it drops down to 4.74 even with out a load attached? Is this normal/expected behavior?

Xenoamor commented 5 years ago

You might want an isolated USB to serial adaptor

geekbozu commented 5 years ago

Only ground is wired up, So it may introduce some noise to the circuit/get some noise from the circuit but other then that it should be fine, I honestly don't plan on using it with USB plugged in in most cases anyway its really just for development and occasional data logging.

kanflo commented 5 years ago

@geekbozu I finally got around to test and I am impressed! Much looking forward to your PR.

geekbozu commented 5 years ago

I actually have been waiting for someone other then me to test this as I haven't had much time to put into it! So its been sitting on my backburner waiting for voltage calibration to be tested before I start working on current calibration!

Xenoamor commented 5 years ago

Will test now

Xenoamor commented 5 years ago

Okay so just an initial test. I have another DPS5005 supplying this one so I can easily vary in input voltages. I build your code from source

$ sudo python dpsctl/dpsctl.py -d /dev/ttyUSB0 -c
Previous Calibration Constants:
     A_ADC_K = 1.71300005913
     A_ADC_C = 1.0732764899e-08
     A_DAC_K = 1.0732764899e-08
     A_DAC_C = 288.610992432
     V_ADC_K = 0.0719999969006
     V_ADC_C = 1.85000002384
     V_DAC_K = 0.0719999969006
     V_DAC_C = 1.85000002384
     VIN_ADC_K = 16.7460002899
     VIN_ADC_C = 64.1119995117

Please ensure nothing is hooked up to the DPS before starting calibration

Perform Input Voltage Calibration? (Y/n): Y
You will need an accurate method of measuring voltage, Such as a multimeter.
You will need an accurate method of generating 2 stable input voltages.
please type results in in mV, EG 1V = 1000 mV
###########################################################
Please hook up the first lower supply voltage to the DPS now
         ensuring that the serial connection is connected after boot
Type input voltage in mV: 800
Please hook up the Second higher supply voltage to the DPS now
         ensuring that the serial connection is connected after boot
Type input voltage in mV: 1800
Unknown response 15 from device.
Input Voltage Calibration Complete
Perform Output Voltage Calibration? (Y/n): n
Perform Output Current Calibration? (Y/n): n
Perform Constant Current Calibration? (Y/n): n
$ sudo python dpsctl/dpsctl.py -d /dev/ttyUSB0 -q
Func       : cv (off)
  current  : 400
  voltage  : 1000
V_in       : 1.80 V
V_out      : 0.00 V
I_out      : 0.000 A

The voltage in reading appears to now be out by a factor of ten. Changing the input values shows this is linear and is always the case.

A minor point but some of the instructions could be clearer. For instance you can't apply 1V to the input voltage

geekbozu commented 5 years ago

I will gladly accept any input on how to make this program flow better. User side software is definitely not my strong point :)

Please ignore the Unknown response 15 I have not modified that portion of the code yet to handle the new response codes i have added.

Out by a factor of 10? Can I have an example? :)

Xenoamor commented 5 years ago

Ooops... disregard that I haven't had my evening coffee. I input the values out by a factor of 10.... It seems to have corrected the 140mV offset that the device had on the input with the default values so all is working well!

I'll try the rest now

geekbozu commented 5 years ago

Woo :) Definitely need to spruce up the instructions but for now "they work" The only other thing I'm worried about is how it handled over power cycles and state changes. I did some testing on my end but have not fully verified its ability to retain state.

Xenoamor commented 5 years ago

It seems to be working to me. I think with some code cleanup and some clearer instructions in the python tools this should be good to go. I'll start making pushes to your repo for these

geekbozu commented 5 years ago

That would be amazing, Thank you a ton. Ill work on tackling Current calibration soonish Should have time later this week.

kanflo commented 5 years ago

If you store the coefficients in the past they will survive power cycles. I see you store the K and C values in different units which in theory may cause problems with atomicity. I would recommend storing them in the same unit.

geekbozu commented 5 years ago

I can add the mangling to store them into 1 past unit later. Due to the fact that there is only one place where they are written and read they should always reflect the last written value.

Xenoamor commented 5 years ago

I think at some point we'll need to tackle handle_set_parameters and handle_set_calibration as they're fairly unwieldy. It's a bit unnerving having the _pos and _value variables defined in a macro and then directly manipulated

Also is it possible to change the way variables are passed to opendps_set_calibration? It seems like we should be using an enum here. Or perhaps even a lookup table so that the if/else tree can be removed

geekbozu commented 5 years ago

It very much is possible to re-work that, I am not super versed on how to effectively handle blocks of code like that so I defaulted back to If/Else. Thankfully it does work in its current state and at the least should not need to be expanded to much Honestly this code needs more love IMO https://github.com/geekbozu/opendps/blob/a8d7248c63a8b69777929b5a6959c5ae0520c9af/opendps/pwrctl.c#L55

Definitely agree those routines are a mess in both the Python file and the C source code. They work but they are not really beautiful code 👍

Xenoamor commented 5 years ago

There's not much we can do about that pwrctl_init function as the past_read_unit() function modifies a pointer to point to the right address. I don't think you can get around having an intermediate pointer *p. I've made a small push for it here anyway

Xenoamor commented 5 years ago

I didn't realise that the serial commands operate on variable=value. Please disregard my comments on changing opendps_set_calibration to use enums. It seems a bit overkill to send full strings but it's not like speed is an issue for calibration. I'll look at cleaning up handle_set_calibration/handle_list_parameters.

EDIT: I've just noticed that @kanflo made the handle_set_parameters function so these can remain as they are for now. There's now a decent size of duplicate code in protocol_handler.c now though...

Xenoamor commented 5 years ago

I've noticed a strange issue with when you calibrate the output voltage

$ sudo python dpsctl.py -d /dev/ttyUSB0 -c
To restore the device to factory default calibration please use dpsctl.py --init

Please ensure nothing is connected to the output of the DPS before starting calibration!

Perform Input Voltage Calibration? (y/n): n
Perform Output Voltage Calibration? (y/n): y
You will need an accurate method of measuring voltage, such as a multimeter

Proceed (y/n): y
DPS supply voltage in mV: 16000
Cal Point 1, 10% of Max
voltage: ok
current: ok
Measured voltage on output in mV: 1543
Cal Point 2, 90% of Max
voltage: ok
Measured voltage on output in mV: 13720
['V_DAC_K=0.0756343927076', 'V_DAC_C=0.29613205223', 'V_ADC_K=13.2214983713', 'V_ADC_C=-83.2442996743']
Unknown response 15 from device.
Output Voltage Calibration Complete
Perform Output Current Calibration? (y/n): n
Perform Constant Current Calibration? (y/n): n
$ sudo python dpsctl.py -d /dev/ttyUSB0 -cr
Calibration Report:
        A_ADC_K = 1.71300005913
        A_ADC_C = -118.510002136
        A_DAC_K = 0.652000010014
        A_DAC_C = 288.610992432
        V_ADC_K = 0.0756343901157
        V_ADC_C = 0.296132057905
        V_DAC_K = 0.0756343901157
        V_DAC_C = 0.296132057905
        VIN_ADC_K = 16.7714881897
        VIN_ADC_C = 201.257858276
        VIN_ADC = 942
        VOUT_ADC = 7
        IOUT_ADC = 70
        IOUT_DAC = 0
        VOUT_DAC = 0

It seems like V_ADC_K is being set to the value of V_DAC_K and V_ADC_C is being set to the value of V_DAC_C

Xenoamor commented 5 years ago

Okay I've found it. It's just a printing error here. I've pushed a commit for it to your repo @geekbozu

Xenoamor commented 5 years ago

Right I believe this is all good to go now. If anyone gets a chance to test it the repo can be found here

It's fixed the issues I had on my DPS5005 which were all way out before I calibrated it

geekbozu commented 5 years ago

I've been following, I just havent had time. Life and holidays are reeking havoc with my schedule

geekbozu commented 5 years ago

I looked at your repo, I was planning on doing current calibration differently, Instead of needing multiple resistors once we know that Output is correct we can use one nominal resistor value for the whole range of available current. With out any user interaction. Should be more user friendly to just give a measured Low value resistor and handle it that way.

Xenoamor commented 5 years ago

That seems like a good way of doing it. I guess you could first calibrate the current ADC and then straight after calibrate the constant current mode with the same resistor

geekbozu commented 5 years ago

that was the thought, It requires a strict in order calibration process. Input -> output -> current. But I feel its less burdensome on the user. It not like this is a nist traceable calibration after all :) Any other opinions @kanflo ?

Xenoamor commented 5 years ago

Isn't it already important that you do the output voltage calibration before you do the current calibration though as it works out the current from V/R? If that is the case then I don't see there being a much greater hinderance from having to do the input voltage first

geekbozu commented 5 years ago

Well the way it is programmed now its very important. We could in theory have the user generate all the values. But then what would be the point of calibration to begin with ;) Its also very important todo input calibration before all of those else we cant properly set a min/max operating voltage!

The current code for current calibration was a copy paste and honestly if it works I'm shocked FWIW. It was me setting up boilerplate for organization reasons. Bad habits die hard I know.

kanflo commented 5 years ago

@geekbozu I think "strict order calibration process" beats "burdensome for the user" ;) I will have a look at your work this Christmas.

geekbozu commented 5 years ago

Glad we are in agreement! And merry Xmas Everyone!

Xenoamor commented 5 years ago

Okay the calibration routine has been changed to have a fixed order. It got my device down to under 1% accuracy from ~5%. My multimeter doesn't have many counts so a better one would likely get it lower

The way I did it is the following:

  1. Remove everything from the output of the DPS5005
  2. Power the DPS5005 from a bench power supply set to ~7V
  3. Run python dpsctl.py -d COMX --init. If you have wildly wrong values calibrated you get crazy results so returning to the defaults is a good idea
  4. Run python dpsctl.py -d COMX -c
  5. Multimeter the input terminals and input the value read in mV
  6. Turn up the bench power supply to ~15V
  7. Multimeter the input terminals and input the value read in mV
  8. Multimeter the output terminals and input the value read in mV
  9. Multimeter the output terminals again and input the value read in mV
  10. Enter the max output current of your DPS device (e.g. 5000mA for the DPS5005)
  11. Enter your load resistor in ohms
  12. Connect your load resistor to the output of the DPS
  13. Proceed if your load resistor can take the required wattage
  14. The device will now iterate through 10 voltages and use this to calibrate the current ADC

Below is the output from this

python dpsctl.py -d COM7 --init

python dpsctl.py -d COM7 -c
For calibration you will need:
        A multimeter
        A known load capable of handling the required power
        2 stable input voltages

To restore the device to factory default calibration please use dpsctl.py --init

Please ensure nothing is connected to the output of the DPS before starting calibration!

Input Voltage Calibration:

Please hook up the first lower supply voltage to the DPS now
ensuring that the serial connection is connected after boot
Type input voltage in mV: 7090

Please hook up the second higher supply voltage to the DPS now
ensuring that the serial connection is connected after boot
Type input voltage in mV: 15030
Input Voltage Calibration Complete

Output Voltage Calibration:
Cal Point 1, 10% of Max
voltage: ok
current: ok
Measured voltage on output in mV: 1443
Cal Point 2, 90% of Max
voltage: ok
current: ok
Measured voltage on output in mV: 12910
Output Voltage Calibration Complete

Output Current Calibration:
DPS max Amperage in mA: 5000
Load resistance in ohms: 14.1
Load must be rated for at least 16.0 watts!
Please connect load to the output of the DPS, then press enter
voltage: ok
current: ok
voltage: ok
current: ok
voltage: ok
current: ok
voltage: ok
current: ok
voltage: ok
current: ok
voltage: ok
current: ok
voltage: ok
current: ok
voltage: ok
current: ok
voltage: ok
current: ok
Output Current Calibration Complete

Constant Current Calibration:
NOT IMPLEMENTED

I don't really understand how the constant current stuff works so I've left this unimplemented. Perhaps if someone could explain it to me I'll add it in?

The test repo can be found here: https://github.com/Xenoamor/opendps

Xenoamor commented 5 years ago

Okay I've now added constant current calibration (although it doesn't seem to be as accurate as I would like)

I've also made it so it restores whatever settings you had before the calibration routine so it doesn't wipe them. It's also less verbose now so you won't get the "current: ok" prompts

Let me know if anyone needs anymore details so they can test it. This seems all good to go on my side. If people are happy with it @geekbozu can either accept my pull request and make one himself or I can make one directly from my repo

Xenoamor commented 5 years ago

Okay I've been using this for a while now and it seems okay. However one of the parameters, A_ADC_C, is being corrupted on a power cycle. Could this be a bug in past.c? I don't think there's anything wrong in my code additions/changes..

python dpsctl.py -d COM7 -cr
Calibration Report:
        A_ADC_K = 1.70325279236
        A_ADC_C = -115.116912842
        A_DAC_K = 0.662139832973
        A_DAC_C = 238.729721069
        V_ADC_K = 13.2220907211
        V_ADC_C = -84.8741073608
        V_DAC_K = 0.0756310075521
        V_DAC_C = 0.419114351273
        VIN_ADC_K = 16.8350524902
        VIN_ADC_C = 59.6804122925
        VIN_ADC = 913
        VOUT_ADC = 7
        IOUT_ADC = 70
        IOUT_DAC = 0
        VOUT_DAC = 0

-- Power cycle the DPS5005

python dpsctl.py -d COM7 -cr
Calibration Report:
        A_ADC_K = 1.70325279236
        A_ADC_C = 1.0732764899e-08 <-- this is now erroneous
        A_DAC_K = 0.662139832973
        A_DAC_C = 238.729721069
        V_ADC_K = 13.2220907211
        V_ADC_C = -84.8741073608
        V_DAC_K = 0.0756310075521
        V_DAC_C = 0.419114351273
        VIN_ADC_K = 16.8350524902
        VIN_ADC_C = 59.6804122925
        VIN_ADC = 913
        VOUT_ADC = 7
        IOUT_ADC = 0
        IOUT_DAC = 0
        VOUT_DAC = 0
geekbozu commented 5 years ago

i'm honestly kinda stumped on this one for the moment, I see nothign glaring in the code. Seriously though thanks for all the help! Could you do some more power cycles and see if its just that param that corrupts or if others do eventually as well?

Xenoamor commented 5 years ago

The mystery deepens... it appears it sets the A_ADC_C value to this strange value even if I run a --init. Perhaps it's not being erased properly?

python dpsctl.py -d COM7 --init

python dpsctl.py -d COM7 -cr
Calibration Report:
        A_ADC_K = 1.71300005913
        A_ADC_C = -118.510002136
        A_DAC_K = 0.652000010014
        A_DAC_C = 288.610992432
        V_ADC_K = 13.1639995575
        V_ADC_C = -100.750999451
        V_DAC_K = 0.0719999969006
        V_DAC_C = 1.85000002384
        VIN_ADC_K = 16.7460002899
        VIN_ADC_C = 64.1119995117
        VIN_ADC = 913
        VOUT_ADC = 7
        IOUT_ADC = 0
        IOUT_DAC = 0
        VOUT_DAC = 0

-- Power cycle the DPS5005

python dpsctl.py -d COM7 -cr
Calibration Report:
        A_ADC_K = 1.71300005913
        A_ADC_C = 1.0732764899e-08 <-- this is now erroneous
        A_DAC_K = 0.652000010014
        A_DAC_C = 288.610992432
        V_ADC_K = 13.1639995575
        V_ADC_C = -100.750999451
        V_DAC_K = 0.0719999969006
        V_DAC_C = 1.85000002384
        VIN_ADC_K = 16.7460002899
        VIN_ADC_C = 64.1119995117
        VIN_ADC = 914
        VOUT_ADC = 6
        IOUT_ADC = 0
        IOUT_DAC = 0
        VOUT_DAC = 0
Xenoamor commented 5 years ago

Okay so I changed the enum values in pastunits.h and it's now working...

typedef enum {
    /** stored as [I_limit:16] | [V_out:16] */
    past_power = 1,
    cal_A_ADC_K = 4, <--- now starting from 4
    cal_A_ADC_C,
    cal_A_DAC_K,
    cal_A_DAC_C,
    cal_V_DAC_K,
    cal_V_DAC_C,
    cal_V_ADC_K,
    cal_V_ADC_C,
    cal_VIN_ADC_K,
    cal_VIN_ADC_C,
    /** stored as 0 or 1 */
    past_tft_inversion,
    /** stored as strings */
    past_boot_git_hash,
    past_app_git_hash,
    /** A past unit who's precense indicates we have a non finished upgrade and
    must not boot */
    past_upgrade_started = 0xff
} parameter_id_t;

@kanflo do you know what might be happening here? Is it possible I have a really b0rked past flash section that isn't being erased? I've checked the .map file and it looks like the linker is keeping the past section clear like it should. We call --init so I'd expect the flash section to be blanked

If anyone can test my code without this enum change and confirm this is a "me" problem then that would be great

Xenoamor commented 5 years ago

Okay the "1.0732764899e-08" erroneous value is stuck in the ID space 3. Is something else using this past ID? The specific commit fix is here

Xenoamor commented 5 years ago

Okay I've found the offending line here: https://github.com/kanflo/opendps/blob/2e1a8e686c8f3b910c00cb737a68af8efa043e21/dpsboot/dpsboot.c#L308

Looks like DPSboot is changing it and obviously it should remain this way so people don't have to reflash their bootloader. Now there's nothing wrong with this but I'll make it clear in pastunits.h and I'll also remove 'past_power = 1' as this is now not used

Xenoamor commented 5 years ago

Commit has now been made and I believe we're looking to be able to merge this in again

geekbozu commented 5 years ago

Your amazing. I spent my time checking the CV/CC modes for the same bug, Never thought to look in the boot-loader. Ill build and flash this tonight and give it a check. Really excited thanks for all the help!!!!!

Xenoamor commented 5 years ago

I noticed that when I converted that float value to hex and then to ASCII that it looked suspiciously like a string so I found it that way

The python tool could do with taking multiple ADC readings and averaging them but my python-fu isn't very good so I'm not too sure how to do this without cluttering the codebase

kanflo commented 5 years ago

I probably should add a comment regarding the past unit IDs stating that they must not be shuffled around. Glad it wasn't a bug ;)

geekbozu commented 5 years ago

Once this has been proven to work, I can take a stab at the python side. Sadly nothing will be pretty with this but I'm sure there is a way to clean up the code and make it "prettier" we shall see.

I think a better comment to be made would be to ensure that all pastId's are properly documented in said enum. however I think that is likely hard seeing as the boot-loader and opendps do have "seperate" code bases. Either way I'm glad it was caught as well, thinking back I think I had this issue as well and was not sure if I was just going crazy or not! probably why I wanted someone else to test so bad.

geekbozu commented 5 years ago

So I've tested it. We need to add a way for the DPS to ignore current overage during calibration. My unit refuses to calibrate up because it immediately overloads upon starting and it gets bad readings as a result. I think I might just be putting to much load on the unit. Everything seems good until its under load then the numbers go wonky. Im going to grab a larger resistor.

SCRATCH MOST OF THAT...i grabbed a .47Ω resistor....

Xenoamor commented 5 years ago

I'm actually thinking about removing the overcurrent completely in #65 and just having it limited rather then turned off. I too had the same issue though on occasion. The issue you're referring to is the same as seen in #6

I used a 14Ω myself so I'm no where near the limits but it still seems to have got pretty accurate results

Did you manage to get it to work for you?

geekbozu commented 5 years ago

I did manage to get it to work after disabling OCP and grabbing a 510Ω resistor. The constant current routine needs some love still My results not consistent. However I feel this may be to the voltage sag associated with the lack of PID or what ever we are missing there. Then the in accuracy of the routines.

I personally agree that #65 is the way it should be as well FWIW

Xenoamor commented 5 years ago

The only known voltage sag is due to the issue in #7 which won't be fixed by a PID. This is because the ADC value for the voltage is effectively wrong so there's no way to adjust for this until that issue is resolved. Once you have calibrated the open circuit output voltage using this routine you shouldn't see any discrepancy between the voltage set on the display and the voltage that is read back. When you read this with a multimeter though you will notice that this voltage is actually wrong under high current loads as a voltage is dropped across the shunt resistor. This in turn means it will be calculating/calibrating the current incorrectly at the top end

Perhaps we should calibrate it by having a multimeter in series with the device and do it much the same as we do the output voltage calibration? I'm less comfortable doing this though as most multimeters have low value fuses on some inputs

Saying all this though my calibrated device is now within 10mA in constant current mode where it used to be out by over 100mA: image The DPS5005 is set to 1.000A output. The top meter is reading 1.00A and the bottom meter is reading 14.347V (this is lower because of #7)

Xenoamor commented 5 years ago

Heres the DPS5005 in CV mode with an output of 10V set: image The top meter is reading 0.70A and the bottom meter is reading 9.997V

Xenoamor commented 5 years ago

Okay I've been thinking about this and I think what we need is a way of directly setting the DAC values and then working out the calibration coefficients from this directly. This current method relies on the default coefficients being in the right ballpark which is not great if it's a new model or a new hardware build.

This would be tricky however as you'd have to tell the calibration routine when the device tops out and use that as the maximum. This could be done by reducing the DAC from the maximum in 10% steps until the user sees the voltage drop

I believe it would also be better to have the current calibration work done by putting a multimeter in series. Now you'd have to attach it to the high amperage input on the meter to prevent any accidental fuses blowing but I think this is likely to be a more accurate method of doing it then by using a resistive load

geekbozu commented 5 years ago

I see no reason why the resistive load would not be "accurate" afaik that is the NIST standard, Just using HIGH power High tolerance resistors.

I agree having it handle DAC values would be better. Definitely going to ponder this! I think I would like CC/CV modes merged first however. I feel like that holds this unit back quite a bit.

also apologies for like vanishing. Life has gotten a bit hectic I'll be back to this soon! Thank you for all the help in the mean time!

Xenoamor commented 5 years ago

That's a good point. I think I'm worrying needlessly and when it worked for me it worked well.

I'll work on the DAC stuff which I've now labelled as #103 and then get an input and output voltage calibration routine implemented. I'd merge CC/CV modes like you said but I'd rather sort this out now for #100.

Don't worry about being around too much, we're all working on this for fun so feel free to drop in and out as you please!

Xenoamor commented 5 years ago

This would be tricky however as you'd have to tell the calibration routine when the device tops out and use that as the maximum

Actually the device can increase the DAC output until it sees the relevant ADC input level out. This would then be the maximum DAC level and the device can then calibrate in the range underneath this