AMDS Data Sign Extension Error

[codecubepi]

I think there may be an error with how the FPGA layer of the AMDS driver handles incoming data from the AMDS mainboard.

The Texas Instruments ADS8860 analog-to-digital converter on the sensors cards requires that the analog voltage measured is prescaled to a 0.0 - $$V_{REF}$$ range (we use 5V for $$V_{REF}$$, see sensor card schematics here in the AMDS repo). The ADC then outputs over SPI (to the AMDS board's STM MCU) an unsigned 16-bit number. The following chart shows how the card input voltage is prescaled for the ADC and then mapped to a 16-bit number (see page 21 of the ADC datasheet to verify).

Card Input Voltage	ADC Analog Input Voltage	ADC Output (16-bit unsigned)
500V	5V ($$V_{REF}$$)	0xFFFF
250V	3.75V	0xC000
0V	2.5V	0x8000
-250V	1.25V	0x4000
-500V	0V	0x0000

This 16-bit unsigned number is then sent from the AMDS STM MCU to its AMDC FPGA driver over UART as described here. It is at this point I believe the error occurs. The FPGA performs a sign-extension to 32-bits when placing the unsigned value into the output register, instead of a zero-extension.

AMDC-Firmware/ip_repo/amdc_amds_1.0/hdl/amdc_amds_v1_0_S00_AXI.v

Lines 756 to 767 in 03bc92a

	always @(posedge S_AXI_ACLK) begin
	if (~S_AXI_ARESETN)
	adc_dout0 <= 32'b0;
	else if (adc_uart0_done)
	adc_dout0 <= {{16{my_adc_data0[15]}}, my_adc_data0};
	end
	always @(posedge S_AXI_ACLK) begin
	if (~S_AXI_ARESETN)
	adc_dout1 <= 32'b0;
	else if (adc_uart0_done)
	adc_dout1 <= {{16{my_adc_data1[15]}}, my_adc_data1};
	end

The C code layer of the driver (amds_get_data()) then reads the full 32-bit signed value. This will cause any value over the middle of the range (unsigned numbers with a leading 1) to be treated incorrectly treated as negative, and the negative values (which in unsigned have a leading 0) to be treated as positive.

AMDC-Firmware/sdk/app_cpu1/common/drv/amds.c

Lines 56 to 71 in 03bc92a

	int amds_get_data(uint8_t port, amds_channel_e channel, int32_t *out)
	{
	uint32_t base_addr = amds_port_to_base_addr(port);
	if (base_addr == 0) {
	// This means an invalid port argument was passed
	return FAILURE;
	}
	if (!is_amds_channel_in_bounds(channel)) {
	return FAILURE;
	} else {
	*out = (int32_t)(Xil_In32(base_addr + channel * sizeof(uint32_t)));
	return SUCCESS;
	}
	}

If all this is the case, I'm not sure how things have ever worked. As far as I can tell, this sign-extension has been performed since the first release revision of the "motherboard" driver back in AMDC-Firmware v1.0.0:

AMDC-Firmware/ip_repo/amdc_motherboard_1.0/hdl/amdc_motherboard_v1_0_S00_AXI.v

Lines 693 to 701 in b02347a

	else if (is_dout_valid0 & is_dout_valid1) begin
	adc_dout0 <= {{16{my_adc_data0[15]}}, my_adc_data0};
	adc_dout1 <= {{16{my_adc_data1[15]}}, my_adc_data1};
	adc_dout2 <= {{16{my_adc_data2[15]}}, my_adc_data2};
	adc_dout3 <= {{16{my_adc_data3[15]}}, my_adc_data3};
	adc_dout4 <= {{16{my_adc_data4[15]}}, my_adc_data4};
	adc_dout5 <= {{16{my_adc_data5[15]}}, my_adc_data5};
	adc_dout6 <= {{16{my_adc_data6[15]}}, my_adc_data6};
	adc_dout7 <= {{16{my_adc_data7[15]}}, my_adc_data7};

When I was updating the "motherboard" driver to the Timing Manager-compatible "AMDS" driver in Summer 2024, I did not catch this issue as I was more concerned with the timing of sampling and data transmission, not with the data itself. So the issue persists into the current release (v1.4.x).

For now, a very simple quick-fix that we've applied locally in the C code layer of the AMDS driver is to mask the value read from the register so that only the last 16 bits are pulled into the C space, and then treat the value as unsigned.

int amds_get_data(uint8_t port, amds_channel_e channel, uint32_t *out)
{
    uint32_t base_addr = amds_port_to_base_addr(port);

    if (base_addr == 0) {
        // This means an invalid port argument was passed
        return FAILURE;
    }

    if (!is_amds_channel_in_bounds(channel)) {
        return FAILURE;
    } else {
        *out = (Xil_In32(base_addr + channel * sizeof(uint32_t))) & 0xFFFF;
        return SUCCESS;
    }
}

A better long-term fix would be to update the FPGA IP to do zero-extension instead.

 always @(posedge S_AXI_ACLK) begin 
     if (~S_AXI_ARESETN) 
         adc_dout0 <= 32'b0; 
     else if (adc_uart0_done) 
         adc_dout0 <= {16'b0, my_adc_data0}; 
 end 
 always @(posedge S_AXI_ACLK) begin 
     if (~S_AXI_ARESETN) 
         adc_dout1 <= 32'b0; 
     else if (adc_uart0_done) 
         adc_dout1 <= {16'b0, my_adc_data1}; 
 end 
// ... six other adc data channels ...

This issue was found by @psflann while prepping for the ECE 504 Drive Lab course at UW-Madison.

[npetersen2]

See #426

[codecubepi]

Ah, thanks @npetersen2 . I figured this had to be a known issue, so I searched the existing issues for "AMDS" and came up short. #426 did not appear because it's a discussion... whoops on my part 🤷‍♂️

[Known4225]

I'm attempting to recreate this bug before applying the suggested fix.

I'm not able to recreate using the Low Voltage card

Top is voltage according to ADC, bottom is raw value from AMDS.

Here's the result when the "quick fix" is applied (taking the raw value from the AMDS and doing & 0xFFFF to it.

The schematic uses a slightly different ADC part, the ADS8865IDGSR, perhaps this could be why?

Does this bug only occur on the high voltage card and current card?

[codecubepi]

Hmm yeah it looks like the differing ADC parts may be an issue.

For the current and HV cards, reference page 21 of the ADS8860 datasheet for the ADC transfer function. That ADC outputs a 16-bit unsigned number.

For the LV card, reference page 20 of the ADS8865 datasheet. That ADC outputs a 16-bit signed value.

This means we can't treat all data coming from the AMDS the same. Somewhere in the chain, either on the AMDS or AMDC (probably the AMDC), the user should be able to tell the amds_get_data() function what card they are reading from, and then the function will read the 16-bit number accordingly.

[Known4225]

This has been completed by #449