Add a vhdl PID for the PandA

[DiamondRC]

Created a PID block for use on PandA devices.

The PID accepts four parameters from input signals, allowing for custom tuning to fit the system. These include: a proportioal term, a derivative term, an integral term and a feed-forward term.

The PID takes a setpoint value and a measured value and returns a correction term for motion.

Currently the PID caps values at the signed limits, which are dynamically calculated from the size of the input/output signals. Whilst this provides flexability, if the values are fixed then this could be simplified to conserve board space.

[LeandroMartinsdS]

Shouldn't the in_error be updated inside the process?

[DiamondRC]

I think this would mean the derivative and integral term would need to wait until the next clock edge to use the in_error value? Keeping it outside the process allows the value to be immediately available.

[LeandroMartinsdS]

I agree that will be the behaviour.

However, I think calculating in_error outside the process makes it asynchronous. Since both in_signal and setpoint are synchronous, updating in_error outside the process could introduce timing issues, especially since the PID loop uses it in the same clock cycle.

In digital control systems, it's common for all control terms (proportional, derivative and integral) to be computed based on values sampled at discrete time intervals, not continuously. Therefore I would say that it is expected that the systems waits for the next clock edge before using updated values.

It might be safer to move the calculation inside the process to keep everything aligned with the clock. I’d also consider doing the same for out_signal, as that might help handle the mismatch between the Panda and PPMAC clocks.

I'm not an expert in FPGA design, so I might be missing something, but this seems like a more robust approach. I'm happy to hear other perspectives.

[Araneidae]

@LeandroMartinsdS, just think of in_error as an intermediate value in the calculation. From an FPGA timing point of view I'd be more concerned about the multiplies. A single tick 32x32 => 64 multiply with unregistered inputs ... you can get away with a lot on a 125 MHz clock, but even so I will expect serious timing problems with this design.

More to the point, I think I see the integral term being updated on every tick. Mmm. This is ambitious, and you're going to need a very small integral term for most sensible applications. Normally I'd expect the PID controller to update at a slower tempo than the FPGA clock; I'd recommend an input strobe to drive this.

[Araneidae]

Ohhhh. Oh dear. I just noticed: slo_clk_i is an ordinary signal being used as a fabric clock.

The FPGA doesn't work like that, I'll put a review comment in the appropriate place above.

[Araneidae]

A couple of extra thoughts:

• This needs to be marked as Draft as it has clearly never been synthesised
• There is no testbench, there really does need to be something

[Araneidae]

slo_clk_i is an ordinary signal derived from the bit bus (this is clear from its definition below in pid.block.ini). I'm afraid you simply cannot use this a "fabric" clock, which is what you're trying to do here. Instead you need to write:

process (clk_i) begin
    if rising_edge(clk_i) then
        if slo_clk_i then
            ... do stuff ...
        end if;
    end if;
end process;

Things then get more tricky, you'll need to figure out just how quickly everything proceeds, and you might need to internally create 1 and 2 tick delayed versions of slo_clk_i to help manage your calculations.

[LeandroMartinsdS]

It might be worth to have a look at the #188

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (master@61de7b0). Learn more about missing BASE report.

Additional details and impacted files

@@            Coverage Diff            @@
##             master     #230   +/-   ##
=========================================
  Coverage          ?   69.98%           
=========================================
  Files             ?       20           
  Lines             ?     1206           
  Branches          ?        0           
=========================================
  Hits              ?      844           
  Misses            ?      362           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[EmilioPeJu]

CI is failing because master is now using ipk packages instead of zpg, you can fix it by rebasing your branch on top of master