search

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
31 stars 5 forks source link

Remove PWM Carrier High/Low Trigger logic from Eddy Current Sensor IP #380

Closed codecubepi closed 3 months ago

codecubepi commented 4 months ago

See the following remnant code in the Eddy Current Sensor IP's HDL:

https://github.com/Severson-Group/AMDC-Firmware/blob/d70ed83c1d96e3018c687eb137f22dfb09dba81d/ip_repo/amdc_eddy_current_sensor_2.0/hdl/amdc_eddy_current_sensor_v2_0_S00_AXI.v#L458-L473

I believe all the logic upstream of "trigger" is now taken care of by the timing manager IP, correct? I.E. The individual sensor IPs should not have to do any qualification using the PWM carrier and register configuration bits...?

It seems to me like we could remove trigger_on_high and trigger_on_low since they aren't used anymore, and the pwm_carrier_high and pwm_carrier_low input ports aren't needed anymore since they are now qualified in the timing manager, instead of in the sensor IP.

If @annikaolson can confirm this is all correct, I can make the change in my user/codecubepi/update-amds-interface branch, since I am already making FPGA changes. Else we can open another more-appropriately-named branch to fix this once the AMDS updates are done.

ALSO for whoever does get this done, don't forget to update the eddy current sensor IP README, specifically the fact that slv_reg3 is not longer used as PWM_TRIGGERS config register.

annikaolson commented 4 months ago

If its not being used for anything else in there, then it can be removed since I have the state machine starting on my trigger signal now

codecubepi commented 4 months ago

This issue is addressed by db1ad1c