Severson-Group / AMDC-Firmware

Embedded system code (C and Verilog) which runs the AMDC Hardware
http://docs.amdc.dev/firmware
BSD 3-Clause "New" or "Revised" License
30 stars 5 forks source link

Verify that the cumulative timing statistics for each sensor is being computed correctly #357

Closed annikaolson closed 2 months ago

annikaolson commented 6 months ago

Verify that the cumulative timing statistics for each sensor is being computed correctly, i.e., check the mean, std dev, min/max etc and make sure it resembles what we are seeing on the hardware oscilloscope.

npetersen2 commented 3 months ago

@annikaolson Can you post onto this issue the results of the timing stats once you get it working? I am curious to see how fast each sensor is.

annikaolson commented 2 months ago

Current issue with this:

Before, the eddy current sensors were all working fine, I read the clock count directly to ensure the time in us was computing correctly and checked on the hardware as well, and when a sensor was not enabled, the time reported 0 when I called the command.

Now, the time is not reporting 0 even when the sensors are not enabled. And the ADC and encoder are both reporting very incorrect times.

I have plenty of ideas/ways to debug/troubleshoot this, but just wanted to document what the issue is!

Example:

Image

annikaolson commented 2 months ago

interestingly, the cycles look fairly reasonable (from what i can tell), the time is just not being computed correctly. here, i just added to the command such that it reads in the value of the clock cycles it took and prints those out. to read in the clock cycles, i used the exact code i used in the timing manager to ensure i'm getting the same value that is being read in there.

Before enabling sensors:

After enabling sensors:

with these cycles, the values should be: eddy: .27 us encoder: 0 us (which takes one clock cycle due to the nature of the sensor) adc: 5.49 us

I will note that the ADC is fluctuating a lot in cycles, whereas the eddy current sensor pretty much stays the same... image

annikaolson commented 2 months ago

partially resolved! when i made the new function to clear the ISR, i forgot to actually call it in the ISR. here are results: ADC_RESULTS

now, the issue is when enabling the eddy current sensors after the adc, the time reports 0. i think there might be some flawed logic in copying over the values/triggering the ISR in the verilog code when multiple sensors are enabled.

when just an eddy current sensor is enabled, it does print out .27 us

npetersen2 commented 2 months ago

@annikaolson interesting to see such large variation in the ADC timing. Are these values in the screenshot above the latest value, or the mean value from the stats module?

annikaolson commented 2 months ago

Latest. I also think its really strange how large the variation is, I can't make it in to test this evening, but I think it might have something to do with how the timing isn't quite working when multiple sensors are enabled - i added a print statement to the ISR and when I enabled the ADC after the eddy current sensor, the ISR was called a lot faster/more frequently - it should only call the ISR once all sensors are done, so it should at the very least be at the same amount as the eddy if not less.

I'll look more into the Verilog logic in both timining_manager.v and its interaction with the C code in the AXI file tomorrow since I think it probably lies there.

npetersen2 commented 2 months ago

Per our meeting, we expect the following times (approximately):

annikaolson commented 2 months ago

I decided to spend some time today simulating this to take a closer look at the logic and expected values; I modified a test bench I previously used, and slightly changed some logic in the timing_manger.v. Interestingly, here is the test bench with just eddy_0 enabled. The clock cycles were always 822 (since its going to be consistent in simulation), and so 822/200 MHz gives 4.11 us.

Image

Going to test some other things out, include some other sensors (since I just have amdc_spi_master hooked up right now) to make sure there aren't any obvious flaws in logic when in simulation. Then I'll go ahead and test it again on the board.

One last thing to note is that currently, each sensor's done signal stays high until the trigger signal starts the sensor over. This means that the sensor can't start over again until the trigger is asserted to signify the next acquisition. I have a positive edge detector for each sensor, so the first cycle that the sensor is high is when the clock cycle count is copied over, which seems to be working.

There are also these four signals: sclk_cnt, shift_index, miso_x, miso_y that I'll have to see if I can find the correct values for, because I can't remember if the values I assigned to them from when I made the tb in December are accurate to what's running on the ADC, and this will change the cycle count.

Edit: as per eddy current sensor driver:

So these correctly reflect the above timing.

At the very least, the current logic should be counting the number of clock cycles correctly and storing them at the appropriate time; the interrupt looks to be generated when all_done is asserted and the sensor's register holds the correct value until its time to update again.

annikaolson commented 2 months ago

ok so as for the eddy current sensor, turns out it was doing everything right and when i was reading from the lower half of slv_reg5 to get the value, I used 0x00FF as a mask because I for some reason thought FF is 16 bits. Changed it, and got the expected value from above of 822.

I made some changes to the ADC after realizing some flawed logic. Before, I was asserting done based on the data valid, but that wasn't working so I made a separate done signal. Also, in the SM_HANG state, it was sending it back to SM_CNV from before I added an idle state, which was causing a massive variation in time since it was never going back to idle, where it is based on the trigger.

Here are the results now:

time_sensors

All the eddy current sensors are the same. The one thing I'd like to talk about in the meeting is that they don't have any variation - the number of cycles is the same each time. Is this expected, or should it be varying more?

npetersen2 commented 2 months ago

@annikaolson awesome!! :) this is looking great and feels like it works now!

Regarding any variation, I would expect virtually zero variation between readings since the FPGA driver is running the same sampling. So, I think that is good. You can make it change by changing the driver speeds...e.g., by setting the ADC clock rate for SCK to like 25 MHz down from the default 50 MHz should make your reported number be slower, etc. There should be some driver functions in the drv/analog module to do this

npetersen2 commented 2 months ago

For example, here in the ADC init code it sets the SCLK to 50 MHz:

https://github.com/Severson-Group/AMDC-Firmware/blob/153dd3f76416b4752a0acfcac4cb20288b142683/sdk/app_cpu1/common/drv/analog.c#L13-L14

Somewhere in your code, you can reinit the ADC and change it to a different speed (slower, I believe the 100 MHz doesn't give valid data due to some FPGA issues... more things which could be solved haha)

npetersen2 commented 2 months ago

Excellent work! Glad to see this working :)