Skip to content

Fix AMDS data error#449

Merged
elsevers merged 27 commits intodevelopfrom
bugfix/adc-sign-extension
Apr 14, 2025
Merged

Fix AMDS data error#449
elsevers merged 27 commits intodevelopfrom
bugfix/adc-sign-extension

Conversation

@Known4225
Copy link
Contributor

@Known4225 Known4225 commented Feb 6, 2025

This PR closes #448.

It adds helper functions to the AMDS driver file that

  • allow users to read in AMDS measurements with the correct sign
  • factor in the specifics of the AMDS sensor cards to correctly interpret the digital values as an ADC voltage
  • provide support to convert ADC voltage measurements to the sensed signals along with default gain and offset values

Future work should update docs.amdc.dev to inform users about these functions.

@elsevers elsevers linked an issue Feb 6, 2025 that may be closed by this pull request
@codecubepi
Copy link
Contributor

Thanks for addressing this @Known4225 !

@Known4225
Copy link
Contributor Author

Known4225 commented Feb 7, 2025

From testing, it turns out that the data error issue only appears on the AMDS current card and AMDS high voltage card.

The branch has been updated to add two new functions to the amds driver, amds_get_voltage and amds_get_current.

These functions offer a more convenient way to get amds data. Rather than having to convert the raw data yourself, the amds driver can convert the data into volts or amps if given the card type that is being read from.

The functions amds_get_voltage and amds_get_current work similarly to amds_get_data (which has not been removed), but require an extra amds_card_t input and format their output as a double rather than an int32_t.

@Known4225 Known4225 marked this pull request as ready for review February 7, 2025 23:02
@Known4225 Known4225 requested a review from elsevers as a code owner February 7, 2025 23:02
@Known4225 Known4225 changed the base branch from v1.4.x to develop February 8, 2025 19:26
Copy link
Contributor

@elsevers elsevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Known4225.

I flagged a few things below.

Revising our measurement approach

In addition to that, I have one major comment for revision: all of our cards (current, low voltage, high voltage), we have analog circuitry that conditions the signal that is applied to the circuit board before the ADC sees it. This circuitry is sometimes customized on a per test stand basis. I think we should be thinking of measurements through the AMDS as a two step process:

  1. Step 1: read the raw voltage from the ADC. This should be voltage at the ADC terminals, not the voltage before the op-amp stage.
  2. Step 2: convert the raw voltage from the ADC into the measured quantity.

Step 1 function

I think your function amds_get_voltage should be our step 1 function.

Step 2 function

Some comments on this step 2:

  • For current measurement, this is indeed as you have implemented within amds_get_current(). However, in all of our research test stands, we have users measure their own sensor offset and gain as described in this AMDC doc. In addition to being more accurate, people often will wrap their phase cable around the hall sensor multiple times which means that their volts-to-amps gain will be scaled by a multiple of the number of wraps.
  • For low voltage measurement, I can see from the schematic that we should have out = (measured - 2.048)*10.0;
  • For high voltage measurement, it will be a similar expression to the current measurement, but the schematic indicates different resistors in the feedback network, so presumably different gains/offset (I have not checked these gains, but we can do that when we settle on our approach here).

So let's create a second function (or macro!) that converts the raw voltage obtained from amds_get_voltage into the measured quantity. Maybe amds_get_measurement. I think this function (or macro!) should go like this:

int amds_get_measurement(double v_in, double offset, double gain, double *out)
{
    out = (v_in - offset)*gain;
    return SUCCESS;
}

Note that I am formulating this function as (v_in - offset)*gain; to match the discussion here.

This should probably be a macro or inline function, right?

Set up some defines

I think we should have #defines for the default offset and gain set up for each of these boards.

We may also want to include a version of amdc_get_measurement that uses the default define values... hmm...

Remember

To test this all out and make sure it is working right.

@Known4225
Copy link
Contributor Author

Did some testing with the current code:

  • Tested with the low voltage and high voltage cards
image Left to right: analog output (AMDS ADC), low voltage card, high voltage card.

Ended up using 10.0 / 32768 as the gain factor for the low voltage card.

The high voltage card never read anything except 0, despite its supposed 0.015V resolution. This could be an issue with my setup.

Setup image:

20250211_125935

Also, the STM32F7 on the AMDS is running really hot. Maybe this is normal, I measured ~200 degrees Fahrenheit with the thermal camera:

FLIR0026

Will continue working on the high voltage card. One observation is that by disconnecting the card the raw value jumps to 2^16, so that's a good sign that the code is working

@Known4225
Copy link
Contributor Author

It turns out the BNC connector was an output not an input. But I also haven't had any luck with wires connected to the screw terminals

@elsevers
Copy link
Contributor

Thanks @Known4225. I am worried about how hot your ST MCU is running. Any resolution on this? Can you try measuring the temperature on a board we are using in a power cabinet?

@Known4225
Copy link
Contributor Author

Known4225 commented Feb 14, 2025

Update on heat situation. Daehoon believes a permanent short has occurred within the STM32 or in the components around it. This short is thought to be caused by plugging in the Low Voltage Card SN:03 into the card slot 1. We damaged another AMDS board (SN:15), possibly permanently, to find out this information.

Current draw (broken AMDS): 0.285 Amps

Current draw (working AMDS): 0.17 Amps

Normal AMDS STM runs at ~94 degrees F.

Broken AMDS can get up to 200 degrees F. But the hot AMDS isn't broken in the sense that it doesn't work, it still works just fine.

@Known4225
Copy link
Contributor Author

Known4225 commented Feb 17, 2025

Okay new mainboard, new Low Voltage card and new Voltage card. But we got data! And it looks pretty good!

image

Left to right: ADC (uInverter channelB), Low Voltage card (uInverter channelA), Voltage card (uinverter channelC)

Here's the code that generated this:

if (valid == 0xFF) {
    amds_get_raw_voltage(amds_port, AMDS_CH_1, AMDS_LOW_VOLTAGE_CARD, &out_ch);
    amds_get_measurement(out_ch, 0, 20.0 / 32768, &amdsOutput);
    LOG_amds_voltage_a = amdsOutput;
    amds_get_raw_voltage(amds_port, AMDS_CH_2, AMDS_HIGH_VOLTAGE_CARD, &out_ch);
    amds_get_measurement(out_ch, 0, 500.0 / 32768, &amdsOutput);
    LOG_amds_voltage_b = amdsOutput;
}

These "gain" values are somewhat sensible, though I would've expected the low voltage card's to be 10.0 / 32768 instead of 20.0.

You'll notice the Voltage card has an output offset of 500V. This means it requires an offset value of 32768. Here's what that looks like:

image
if (valid == 0xFF) {
    amds_get_raw_voltage(amds_port, AMDS_CH_1, AMDS_LOW_VOLTAGE_CARD, &out_ch);
    amds_get_measurement(out_ch, 0, 20.0 / 32768, &amdsOutput);
    LOG_amds_voltage_a = amdsOutput;
    amds_get_raw_voltage(amds_port, AMDS_CH_2, AMDS_HIGH_VOLTAGE_CARD, &out_ch);
    amds_get_measurement(out_ch, 32768, 500.0 / 32768, &amdsOutput);
    LOG_amds_voltage_b = amdsOutput;
}

Even still, it's not in complete agreement with the other two, peaking at around 2.2 volts rather than 1.5. I'm not exactly sure why. And also I'm not sure why the 32768 offset value is necessary.

Here's a picture of the setup, and yes I do need to hold the black wire against the BNC outer layer to record data on the Voltage card

20250217_121014

Final question: is it correct for the offset to be applied BEFORE multiplying by the gain in

*out = (v_in - offset) * gain;

or should the offset happen after as in

*out = v_in * gain - offset;

@Known4225 Known4225 requested a review from elsevers February 17, 2025 18:25
@Known4225
Copy link
Contributor Author

02/20/25:
There is a two step process for getting calibrated values from the AMDS:

  1. ADC conversion equation
  • Find reference voltage in the datasheet of the ADC component currently used in the AMDS card used.
  • Datasheet will contain equation for adc conversion
  • Formulate the equation Ex) adc_volts_out = (Vref) / 2^(# of Bits) * Vraw
  1. Gain and Offset
  • Measure the reference current and adc_volt_out and plot them.
  • Obtain the gain and offset from the plot

Step 1 will be done automatically by the function amds_get_converted_voltage, which takes in a port, channel, and card type.

Step 2 will be done by the function amds_get_calibrated_data, which takes in a converted voltage, an offset, and a gain.

@Known4225
Copy link
Contributor Author

Update low voltage and high voltage docs to provide user equation relationship between adc voltage and input voltage (like the current card docs have now). Equation should express input voltage as a function of adc voltage.

Review default values with Daehoon to ensure that they are based on the schematic.

Add default values for both revisions of current cards.

@Known4225
Copy link
Contributor Author

4.03.25: Meeting with Daehoon. The theoretical values for offset and gain based on this report (where the relevant section of the schematic is identical across the current card and high voltage card) do not agree with the experimental ones (offset is fairly similar, gain is off by a factor of 10). We found that the resistors on the high voltage card that I'm using (no SN on it) has the wrong resistor values for resisto R4, R6, and R8. All of the values of these resistors are 4.3k ohms when they should be 10k, 8.45k, and 4.64k respectively. Even with the incorrect resistor values we were not able to calculate a theoretical offset and gain that agreed with the experimental values.

@elsevers
Copy link
Contributor

elsevers commented Mar 5, 2025

@Known4225 -- curious....

I created this issue to update the docs:

Can you and @Daehoon-Sung work on adding comments with the expected expressions for each board?

@Known4225
Copy link
Contributor Author

I can confirm that the calculated experimental values for the low and high voltage cards are working. I was unable to confirm values for either current card because I once again broke the AMDS in the same way. The STM gets extremely hot immediately when powered. I'm not sure what causes this issue to happen but even with no cards plugged in, SN13 gets to extreme temperatures within 0.5 seconds of being powered, though the board is still fully operational.

Perhaps this happened because I unplugged the current card while the AMDS was capturing.

Either way, I feel like I should stop doing these experiments because I just keep breaking everything.

Also, should we use the default values for the high voltage cards according to the schematic, or our hardware? Since both high voltage cards that I have located have incorrect values for resistors R4, R6, and R8 (all of them are 4.3kΩ), I have put the default values according to our hardware and not the schematic, which calls for R4 = 10kΩ, R6 = 8.45kΩ, and R8 = 4.64kΩ.

@Known4225
Copy link
Contributor Author

@Daehoon-Sung One last modification from experimental data, it appears that the Low Voltage Card's experimental offset should be 0 rather than 2.048. Here is the plot with an offset of 0 when sweeping from 2.0v to 3.0v.

image

and here's the plot with a 2.048 offset:

image

@Known4225
Copy link
Contributor Author

Known4225 commented Mar 25, 2025

Test results (current cards)

The Rev B passed experimental tests using the default values with a 5v reference voltage.
The Rev C did not pass experimental tests using a 4.5v reference voltage, indicating the default values are incorrect.
I did a sweep from 2.0 to 3.0 amps and measured the outputs of the different cards using their default values as offset and gain.

Can confirm that the rev B (SN:004) current card values are experimentally accurate. Here's a sweep from 2.0 to 3.0 amps:

image

with code

amds_get_converted_voltage(amds_port, AMDS_CH_2, AMDS_CURRENT_CARD_REVB, &out_ch);
amds_get_calibrated_data(out_ch, AMDS_CURRENT_REVB_DEFAULT_OFFSET, AMDS_CURRENT_REVB_DEFAULT_GAIN, &amdsOutput);
LOG_amds_channel_2 = amdsOutput;

rev C (SN:033) does not seem to be working though, here's that same experimental sweep:

image

with code

amds_get_converted_voltage(amds_port, AMDS_CH_2, AMDS_CURRENT_CARD, &out_ch);
amds_get_calibrated_data(out_ch, AMDS_CURRENT_DEFAULT_OFFSET, AMDS_CURRENT_DEFAULT_GAIN, &amdsOutput);
LOG_amds_channel_2 = amdsOutput;

Here's the raw int32_t value from the AMDS:
image

And here's the values after converting over 16 bits and with a 4.5V reference voltage:
image

Compare that to the raw values of the rev B card:
image

And the values after converting (using a 5.0V reference):
image

@Known4225
Copy link
Contributor Author

Known4225 commented Apr 2, 2025

02.04.25:
Voltage measured from TP7 to TP2 with different current values through sensor

current voltage (multimeter) voltage (amdc)
0A 2.516V 2.518V
1A 2.533V 2.534V
2A 2.550V 2.551V
3A 2.567V 2.568V

Measured Resistor Values:
R4 - 4.750 kOhms
R5 - 148.8
R6 - 4.086 kOhms
R8 - 3.592 kOhms

@Known4225
Copy link
Contributor Author

@Daehoon-Sung

We cannot assume that our measurement of R4 was in isolation, because of this parallel path.

image

I believe that some math can be used to get the isolated resistance values of these resistors given the information that we have measured at the different points, but I'm not sure how to do that

@Daehoon-Sung
Copy link

Hello Professor @elsevers.
The current card which was used for this result was wrong. The offset should be around 2.5V but it shows 0.15V which is a way off from the expected value.

I am not sure which part is wrong but after replacing it to other RevC current cards, the offsets show correct values. However, with the replaced board, the gain is 1/2 of the expected gain from the math I did based on the schematic. Measuring resistance using multimeter seems not a good idea since R4 terminals are sharing parallel nodes that include other circuits. We are currently in the stage of how to figure out the resistance value and I have a plan for this.

@Daehoon-Sung

We cannot assume that our measurement of R4 was in isolation, because of this parallel path.

image I believe that some math can be used to get the isolated resistance values of these resistors given the information that we have measured at the different points, but I'm not sure how to do that

Hello @Known4225.
I already did this and I found out it was hard to calculate. I think there are other paths that contribute the resistance of two nodes not only two red lines you have drawn. Therefore,I think it is hard to calculate it. I have new plan on this and I can help you today. There are resistance values written on the resistor. They are small but I think we can read it using the magnifying glass in the soldering table. We can try this.

@Daehoon-Sung
Copy link

Daehoon-Sung commented Apr 6, 2025

Hello , Professor @elsevers, @Known4225.

I found out that the current sensor on RevC we used for the experiment is not LA 55-P but it is LA 55-P/SP9.
Those two are different models and they might have different turns ratio.

image

  • This is from AMDC current card article. If the turns ratio (N1/N2) is different, then the gain can be different. Currently, LA 55-P has turns ratio N1/N2 of 1/1000 according to its datasheet you can find here in LEM website.
  • Unfortunately, the manufacturer does not currently providing the datasheet for LA 55-P/SP9. If you see the datasheet of other LA 55 current sensor series such as LA 55-P/SP1, N1/N2 is 1/2000. This shows the possibility that if LA 55-P/SP9 has turns ratio of 1/2000, the gain will be half compared to the case of LA 55-P that has turns ratio N1/N2 of 1/1000. If the turns ratio of LA 55-P/SP9 is indeed 1/2000, then our experimental results (correct offset, half gain) make all sense.
  • I was unable to find the turns ratio of LA 55-P/SP9. So, I have reached out to the manufacturer to get the information and currently waiting for their response. I recommend you to find one if you can.
  • In addition, the issue is that some of our RevC current cards use LA 55-P/SP9 and the other use LA 55-P. If the turns ratios are different, then the default value of the gain should be determined according to what current sensor (LA 55-P/SP9 or LA 55-P) is used on the user's RevC current card.

@Known4225
Copy link
Contributor Author

Known4225 commented Apr 11, 2025

@Daehoon-Sung Can you confirm that the gain and offset numbers used in the BP4 cabinet are these current card rev C default values:

GAIN        29.4117647059
OFFSET      2.5126

if so, this PR can be merged

@Daehoon-Sung
Copy link

Daehoon-Sung commented Apr 14, 2025

Hello @Known4225. I just mentioned this in person and forgot to make a comment on this. sorry.
Yes, I am pretty sure your gain and offset from the equation are correct, and they are also verified experimentally in BP4.

The equation here is correct. If the board has different values on its gains or offsets, I suggest looking into whether the board actually has the same components as the schematic. As long as the board is working well and uses the same values of resistance and the same current sensors, the experimental results should match the theoretical equation.

Copy link
Contributor

@elsevers elsevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this with @Known4225 and @Daehoon-Sung on 4/14/2025 and we agree it is ready to merge. @Known4225 has tested these out on his local AMDC. @Daehoon-Sung plans to upgrade his research testbench to use this code prior to us releasing it.

@elsevers elsevers merged commit bd8083c into develop Apr 14, 2025
1 check passed
@elsevers elsevers deleted the bugfix/adc-sign-extension branch April 14, 2025 16:57
@Known4225 Known4225 restored the bugfix/adc-sign-extension branch May 12, 2025 17:49
@Known4225 Known4225 deleted the bugfix/adc-sign-extension branch May 12, 2025 17:50
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.

AMDS Data Sign Extension Error

4 participants