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

Latch encoder state synchronous with PWM carrier #313

Closed npetersen2 closed 9 months ago

npetersen2 commented 12 months ago

In the v1.0 firmware, the amdc_encoder_v1.0 IP core interfaces to the "ABZ" encoder hardware. Read about encoders here. It keeps track of each edge of the A and B signals, and when they change appropriately, the state machine updates the steps and position registers. Note that these two registers are basically the same, but one wraps to 1 rev and one keeps accumulating until it overflows the 32-bit value.

In the user C code, these registers can be read to get the current state of the encoder.

Problem

The issue here is that the register updates (steps and position) are asynchronous to the control code---they happen relative to the encoder, not to the AMDC-Firmware timing. This creates problems for high-speed motor control since most control code assumes the encoder is sampled synchronously with the current feedback.

Solution

The solution to this is simple:

  1. Create two more registers: steps_synced and position_synced
  2. Pass the pwm_carrier_low and pwm_carrier_high signals from the top-level FPGA module into the encoder IP core
  3. When either of the carrier high or low signals is high, copy the value from the instantaneous steps into steps_synced. Same with position to position_synced
  4. Add associated C code to allow the user to read these new registers
    1. The new default behavior should be to read the synced values, NOT the instantaneous values...
    2. If a user wants the instantaneous values (i.e., steps or position), they need to call a different C code API function: encoder_get_pos_instantaneous()

Validation

Work with @anirudhupadhyaya to test on a real motor test bench. At a minimum, the encoder interface should work about the same... But at high speeds, it should be improved.

npetersen2 commented 11 months ago

@annikaolson after some thinking, I'd like you to create a testbench to validate your changes here. You can run it via ModelSim, or directly in the Vivado Simulator (it acts like ModelSim).

You should model the pwm_carrier_high and pwm_carrier_low signals and then show how your new changes latch in the right value at the right time. You can create your own carrier high/low signals to model ~100kHz triangle carrier, or you can use the triangle_carrier.v module directly in your test bench.


The reason for validating in the testbench is that, in addition to the changes above (the synced registers), I'd like you to take over this stale PR #263 from @ngadiyar93 and integrate his changes into your PR. This should be fairly trivial.

Once you integrate your changes + the changes from #263, I'd like you to test in the test bench with arbitrary encoder total counts and make sure everything works as expected. For example, try these total lines: 10, 99, 100, 1024, 8196, 10000.

We can discuss approaches to automating the testbench if you'd like. I recommend you model the ABZ encoder as its own module, them hook it up to the receiver in the AMDC-Firmware. This should be very similar to testbenches in ECE 551. Your test bench should "spin the encoder" forwards and backwards, then make sure the steps and position register outputs match the actual movement.