Add trigger qualifying on all_done in timing_manager?

[codecubepi]

@annikaolson @npetersen2

When reviewing the timing manager behaviour while adding AMDS support, I found the following and am unsure if it is the desired implementation:

Currently, in timing_manager.v, the trigger output signal is not qualified on _alldone, but only on the PWM carrier and user ratio. So if the user ratio is 1, on PWM high and PWM low, the timing manager will send out a trigger to all sensors on every peak and valley, as shown below.

https://github.com/Severson-Group/AMDC-Firmware/blob/24afb777ab3f94c97bf4a5c915ebaa6a8c7b38e2/ip_repo/amdc_timing_manager_1.0/hdl/amdc_timing_manager_v1_0_S00_AXI.v#L603

https://github.com/Severson-Group/AMDC-Firmware/blob/24afb777ab3f94c97bf4a5c915ebaa6a8c7b38e2/ip_repo/amdc_timing_manager_1.0/src/timing_manager.v#L66-L84

Any "fast" sensors like the encoder will receive the trigger, clear their done signal, update their values quickly, and then reassert that they are done, almost immediately after being triggered.

However, "slow" sensors like the AMDS or Eddy Current Sensor will take more time after receiving the first trigger to update their values and assert done.

But because trigger coming from the timing manager is not qualified on _alldone (specifically, the "slow" sensors being done as well), it is possible that fast sensors will trigger and assert done multiple times while waiting for the slow sensors. See this image:

Is this, described and shown above, the desired behaviour? When referencing the diagram shown in #323 , I thought we intended behavior more like this:

I could be wrong though, just making sure.

[annikaolson]

So right now, you're right that the trigger isn't based on the all_done signal. So the second image is the desired behavior, and when the ratio is high enough, that is what's occurring (the encoder done signal is held high until the next trigger). I'm going to modify my old testbench to simulate the specific behavior, because when I tested it on the AMDC it gave the expected times, but you're right that this could be an issue since it simply resets on a trigger rather than also waiting for all the sensors to be done.

The trigger currently gets asserted when the amount of times the event qualifier (PWM high, low, or both) has occurred is the ratio, could we simply add something like:

else if (count == user_ratio & all_done) begin 
         count <= 0; 
         trigger <= 1; 
end

and right now, all_done is held high until the next trigger (since its based on the individual done signals which are also only de-asserted on trigger), so I believe that should work fine there.

[codecubepi]

Okay, that's all what I thought. I figured it was a pretty quick fix. And I'm willing to put any FPGA changes in my branch to avoid merge conflicts, I just want to make sure we're on the same page.

[annikaolson]

The only thing is that we'll need to make sure what the ratio does is clear in any documentation on it - so a ratio of '1' in this case would really mean it triggers every carrier high or low only when all of the sensors are done, i.e. it can't assert a trigger if the sensors aren't all done; so technically the sensors like the encoder aren't going every high or low. Here's the default eddy frequency, which does complete in a cycle:

versus a slower frequency, ~3000kHz:

since the count_time is getting reset upon a trigger, the incorrect time is getting copied over. I tried adding the all_done in determining the trigger but that's not working either, I'll try and figure out the logic to add.

[annikaolson]

So I believe I have the desired behavior here now (tested at eddy SCLK ~3000kHz):

Here, I'm asserting the individual sensors' done signals upon reset:

// encoder done
always @(posedge clk, negedge rst_n) begin
    if (!rst_n) done <= 1;
    else if (trigger) done <= 0;
    else if (set_done) done <= 1;
end

So now trigger can only be asserted as long as all the sensors are done to ensure that if a sensor takes longer than a pwm cycle, it doesn't try and collect data again. I think it should be fine if the sensors are initially set to done, since it doesn't change how they work, and the interrupt is only sent on the rising edge of all_done anyway. Also the time counter will be counting, but it resets upon trigger so that also should be fine.

Here's the code in timing manager I needed to change:

always @(posedge clk, negedge rst_n) begin
        if (!rst_n) begin
            count <= 0;
        end
    else if (count == user_ratio) begin
        count <= 0;
    end
        else if (event_qualifier) begin
            count <= count + 1;
        end
    end

always @(posedge clk, negedge rst_n) begin
    if (!rst_n) begin
        trigger <= 0;
    end
    else if ((count == user_ratio) & all_done) begin
        trigger <= 1;
    end
    else begin
        trigger <= 0;
    end
end

Now, the ratio count will still reset when it hits the user ratio, so it still follows the ratio, but the trigger can't be asserted until we hit that user ratio and all the sensors are done (effectively just skipping some of the "triggers"). And then when trigger is asserted, done will get cleared.

@codecubepi does this look right to you?

[codecubepi]

Yeah this looks great, thank you! I will make this correction and address #383 when I'm done with the AMDS driver.

[codecubepi]

This issue is addressed by 070de51