Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QUAD_TYPE borked #44

Open
jschoch opened this issue Dec 31, 2024 · 9 comments
Open

QUAD_TYPE borked #44

jschoch opened this issue Dec 31, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@jschoch
Copy link

jschoch commented Dec 31, 2024

So, to sniff out the bug a bit further I added a pin to show the delta counts for every run of the rps calculation. It showed that delta counts was always 32 no matter what the actual speed was of the pulses. This didn't match up with the UDP latency and would basically only calculate the velocity every (pulse_duration * 32), this means that for slow updates the data was sparser and sparser thus requiring filtering. I did a bunch of digging and couldn't find anything obvious so as a last resort (and not to bug you olli) i asked gemini. Amazingly it was correct. Removing the QUAD_TYPE bitshift gave the expected updates for every udp packet!

simple change

//assign position = $signed(count>>>QUAD_TYPE);
assign position = count;

I asked gemini "why would <quadencoderz.v> only send counts when the counts diff was 32?"

it said:

The issue lies in the line:

Verilog
assign position = $signed(count>>>QUAD_TYPE);

You're using a right arithmetic shift (>>>) with the parameter QUAD_TYPE. This is intended to divide the count by 2 raised to the power of QUAD_TYPE.

If QUAD_TYPE is set to 5 (2^5 = 32), then the position output will only change when the count changes by 32. Let's break down why:

  • count>>>QUAD_TYPE: This performs an arithmetic right shift. It shifts the bits of count to the right by QUAD_TYPE positions. For each shift, the leftmost bit (the sign bit) is replicated, preserving the sign of the number.
  • Effect of the Shift: When QUAD_TYPE is 5, the shift effectively divides count by 32. So:
    • If count is 0, position is 0.
    • If count is 1, position is 0 (1 >>> 5 = 0).
    • ...
    • If count is 31, position is 0 (31 >>> 5 = 0).
    • If count is 32, position is 1 (32 >>> 5 = 1).
    • If count is 63, position is 1 (63 >>> 5 = 1).
    • If count is 64, position is 2 (64 >>> 5 = 2).

As you can see, position only increments when count has increased by a multiple of 32. The same applies to decrements.

How to Fix It:

If you want position to reflect the raw encoder count, you should remove the shift entirely:

Verilog
assign position = count; // Assign the count directly

If you intend to divide the count by a fixed value (e.g., to implement quadrature decoding where you get 4 counts per encoder cycle), you should use a division or a right shift by a constant value, not a parameter that can be easily misinterpreted:

Verilog
// For quadrature decoding (4 counts per cycle):
assign position = count >>> 2; // Right shift by 2 (equivalent to division by 4)

// Or, for division by a specific value (e.g., 10):
assign position = count / 10; // Use division (more resource-intensive in hardware)

Example demonstrating the shift:

Let's say count is a 6-bit number and QUAD_TYPE is 2:

count (Binary) | count (Decimal) | count >>> 2 (Binary) | count >>> 2 (Decimal) -- | -- | -- | -- 000000 | 0 | 000000 | 0 000001 | 1 | 000000 | 0 000010 | 2 | 000000 | 0 000011 | 3 | 000000 | 0 000100 | 4 | 000001 | 1 000101 | 5 | 000001 | 1 ... | ... | ... | ...

This table clearly shows how the right shift with QUAD_TYPE = 2 effectively divides the count by 4.

By making the appropriate change (either removing the shift or using a constant value), you'll get the desired behavior from your quad encoder module. If you intended to divide by 32 based on some external configuration, consider using a separate signal and a conditional assignment instead of relying on a parameter that affects the core counting logic.

@multigcs
Copy link
Owner

multigcs commented Jan 1, 2025

default QUAD_TYPE is 2, this means that the ppr are exactly as indicated on the encoders.
but since the plugin can count every edge, we can achieve 4-fold resolution, which is achieved with QUAD_TYPE = 0
I don't know why you have a 5 configured.

the plugin does exactly what it is supposed to do and can also be checked in the test-bench, a QUAD_TYPE=2 divides all pulses by 4, so that in principle you only count one edge of a signal

in order to use the highest resolution cleanly, the pulses should also come cleanly and symmetrically from encoder,
~50% duty cycle at constant speed, which normal encoders also do.

what i can well imagine at the moment is that your esp32-test-encoder-tool does not output symmetrical pulses and therefore produces strange side effects

@jschoch
Copy link
Author

jschoch commented Jan 1, 2025

haha, it isn't my shitty AI addled code!

I had QUAD_ENC set to 0, but It only sent a value when count was 32. This is why RPS would drop to 0 without the smoothing/averaging even with a pulse every 1000 microseconds. When I removed that bitshift delta counts ( via hal scope) would be 15..18 with variation at 10RPS pulse generation. With the bit shift it was 32, 0,0,32 and with RPS of 1 it was more like 32, 0,0,0,0,0,32

@multigcs multigcs added the bug Something isn't working label Jan 1, 2025
@multigcs
Copy link
Owner

multigcs commented Jan 1, 2025

ok ok, but i don't see the error here, i have add 2 additional position outputs to the quadencoder .v

position: with shift 0
position2: with shift 2
position3: without shift

wve


module quadencoder
    #(
         parameter BITS = 32,
         parameter QUAD_TYPE = 0
     )
     (
         input clk,
         input a,
         input b,
         output signed [BITS-1:0] position3,
         output signed [BITS-1:0] position2,
         output signed [BITS-1:0] position
     );
    reg [2:0] a_delayed = 0;
    reg [2:0] b_delayed = 0;
    always @(posedge clk) a_delayed <= {a_delayed[1:0], a};
    always @(posedge clk) b_delayed <= {b_delayed[1:0], b};
    wire count_enable = a_delayed[1] ^ a_delayed[2] ^ b_delayed[1] ^ b_delayed[2];
    wire count_direction = a_delayed[1] ^ b_delayed[2];
    reg signed [BITS-1:0] count = 0;

    assign position = $signed(count>>>0);
    assign position2 = $signed(count>>>QUAD_TYPE);
    assign position3 = count;

    always @(posedge clk) begin
        if (count_enable) begin
            if(count_direction) begin
                count <= count + 1;
            end else begin
                count <= count - 1;
            end
        end
    end
endmodule

@jschoch
Copy link
Author

jschoch commented Jan 1, 2025

position is accurate, it is the interval a value get sent that is hte problem. I will send some screenshots, recreating the halscope setup

@jschoch
Copy link
Author

jschoch commented Jan 1, 2025

This is using the bitshift and QUAD_TYPE=0, signal emulation is 1RPS for PPR of 500. Pulse duration should be 1ms. Scope measures frequency of single channel 125hz

Green trace is duration, range +/- ~20 microsecond

Blue is RPS, unfiltered. 433 or zero

Red is delta counts (raw_value - last_raw_value)

My setup from yesterday is gone so I'm haveing a bit of trouble reproducing the results and i'm very confused by them.

7

New signals, RPS 1, PPR 500, pulse delay 5ms, scope measurs 25HZ. Notice the delta counts is still ~26.

8

Last setup: RPS 1, PPR 1000, pulse delay 500us, scope measures single chan frequency of 250hz, why are counts 26 or 52?

9

Weirder is that yesterday everything was 32 or zero.

@jschoch
Copy link
Author

jschoch commented Jan 1, 2025

i removed the 3rd stepdir plugin, recompiled, reflashed, re-added my delta_counts pin and deltacounts seem to be behaving. Really hard to test this.

@jschoch
Copy link
Author

jschoch commented Jan 1, 2025

pulling the timestamp header bug and retesting join following errors, maybe related to this tho?

@multigcs
Copy link
Owner

multigcs commented Jan 1, 2025

puhhh, yes, maybe, i think so, the frame size was to short and cut the last bits from the encoder value.

that's why i couldn't see the error, because my configuration was such that i didn't notice the bug.

i have also modified my test setup and now have a stepper with generator to generate 1rps.

without the filter i get +-0.2 rps fluctuations

with avg8 ~ 0.04 rps

i'll try calculating the rps every 10 ms again, then the rounding errors should be less.

@jschoch
Copy link
Author

jschoch commented Jan 1, 2025

we should set lower bound on RPS, like 1RPM or .016 RPS.

Also as per the DM the alpha beta filter I am using makes really nice results. May want to have it as a general purpose function and add to a "filter library"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants