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

fofb_shaper_filt: add module #30

Merged
merged 15 commits into from
Apr 22, 2024
Merged

fofb_shaper_filt: add module #30

merged 15 commits into from
Apr 22, 2024

Conversation

guilhermerc
Copy link
Contributor

@guilhermerc guilhermerc commented Sep 20, 2023

  • Shaper filters intended to equalize the open-loop response among all controllers.

@guilhermerc guilhermerc requested a review from ericonr September 20, 2023 20:33
@guilhermerc guilhermerc force-pushed the shaper-filt branch 4 times, most recently from b85a9f8 to 5be2152 Compare September 20, 2023 20:50
@guilhermerc guilhermerc changed the title IIR shaper filter IIR shaper filters Sep 20, 2023
@guilhermerc guilhermerc changed the title IIR shaper filters fofb_shaper_filt: add module Sep 20, 2023
@guilhermerc guilhermerc force-pushed the shaper-filt branch 21 times, most recently from fdef3ad to 9e7fe3f Compare September 25, 2023 16:15
@guilhermerc guilhermerc force-pushed the shaper-filt branch 2 times, most recently from c9c5fd1 to 627e7e6 Compare March 28, 2024 19:48
@augustofg
Copy link
Member

I've figured out how to expose the RAM interface instead of instantiating it internally using cheby, unfortunately the iogroup: wb_fofb_shaper_filt_regs_ifc attribute had to go:

memory-map:
  bus: wb-32-be
  name: wb_fofb_shaper_filt_regs
  description: Interface to FOFB IIR shaper filters regs
  x-hdl:
    busgroup: True
  children:
    - repeat:
        name: ch
        count: 12
        children:
          - memory:
              name: coeffs
              memsize: 200
              interface: sram
              description: |
                Coefficients for the ceil('max_filt_order'/2) IIR internal
                biquads.

                Each biquad takes 5 coefficients: b0, b1, b2, a1 and a2 (a0 = 1).
                The 'coeffs' array should be populated in the following manner:

                  coeffs[0 + 5*{biquad_idx}] = b0 of biquad {biquad_idx}
                  coeffs[1 + 5*{biquad_idx}] = b1 of biquad {biquad_idx}
                  coeffs[2 + 5*{biquad_idx}] = b2 of biquad {biquad_idx}
                  coeffs[3 + 5*{biquad_idx}] = a1 of biquad {biquad_idx}
                  coeffs[4 + 5*{biquad_idx}] = a2 of biquad {biquad_idx}

                This array acts like a 'shadow' for the real coefficients and is
                only effectivated when '1' is written to 'eff_coeffs' bit of
                'ctl' register.
                
                NOTE: This ABI supports up to 20th order filters, but only the
                coefficients corresponding to the first 'max_filt_order' filters
                are meaningful for the gateware.
              children:
                - reg:
                    name: val
                    width: 32
                    access: rw
                    description: |
                      Coefficient value using 'coeffs_fp_repr' fixed-point
                      representation. It should be aligned to the left.
    - reg:
        name: max_filt_order
        width: 32
        access: ro
        description: |
          Maximum filter order supported by the gateware.
    - reg:
        name: coeffs_fp_repr
        width: 32
        access: ro
        description: |
          Fixed-point signed representation of coefficients.
          The coefficients should be aligned to the left. The fixed-point
          position is then given by 32 - 'int_width' (i.e. one should divide
          this register's content by 2**{32 - 'int_width'} to get the
          represented decimal number.
        children:
          - field:
              name: int_width
              range: 4-0
              description: |
                Integer width (accounting for the signal bit).
          - field:
              name: frac_width
              range: 9-5
              description: |
                Fractionary width.
    - reg:
        name: ctl 
        width: 32
        access: rw
        description: |
          Control register.
        children:
          - field:
              name: eff_coeffs
              x-hdl:
                type: autoclear
              range: 0
              description: |
                Strobe for effectivating (i.e. updating) coefficients.
              comment: |
                write 0: no effect
                write 1: effectivates coefficients (this bit autoclears)

@ericonr
Copy link
Member

ericonr commented Apr 5, 2024

@augustofg does the iogroup affect anything in terms of ABI?

Also, does changing the interface to use RAM mean the eff_coeffs bit is no longer necessary?

@augustofg
Copy link
Member

@ericonr I deliver you good news, the generated ABI is exactly the same, except that eff_coeffs would not be necessary anymore.

@guilhermerc guilhermerc force-pushed the shaper-filt branch 2 times, most recently from 66ab9c4 to beefcc9 Compare April 11, 2024 13:16
Optimizations for easing timing closure.
Instead of instantiating a "shadow" RAM for holding the coefficients,
simply access'em through a RAM interface. This optmizes resource usage
and might ease timing closure.

NOTE: The ABI wasn't broken. The only thing that's changed is the
      removal of the control register (no need to effectivate the
      coefficients anymore).
@guilhermerc guilhermerc force-pushed the shaper-filt branch 3 times, most recently from c3e105d to 91b303d Compare April 15, 2024 20:10
In order to reduce the logic needed to decode coefficients' addresses,
align biquads' base addresses to a power of 2.

NOTE: This breaks the ABI.
Even with the optimizations from the latest commits, synthesis can't
close timing.

By now, the two options we found that solve this issue are:

  1. Reduce the number of supported channels from 12 to 8.
  2. Reduce the maximum filter order from 10 to 8.

We picked option 2:
Although current operation doesn't require more than 8 channels, we want
to keep those 4 extras as a backup for any malfuction. Also, we
currently don't have a real demand for {9,10}th order filters.

The testbench files changed accordingly.
Instead of defining the maximum order, use a generic to define the
number of internal biquads (the order is twice this value). This
simplifies things since we can use it directly (i.e. no intermediate
computation) for defining internal dimensions, number of iterations etc.

NOTE: The ABI was broken. The register 'max_filt_order' was changed to
      'num_biquads'.
Critial assertions should have its severity set to FAILURE so
synthesis/simulation breaks if it fails.

The argument '--assert-level=error' to ghdl isn't needed anymore.
We used to define constants in packages for constraining dimensions of
array types, which was required by VHDL standards older than 2008's. We
use VHDL 2008 now, so array types can be unconstrainedly defined and
these constants can be moved to the top level file.
@guilhermerc guilhermerc requested a review from augustofg April 19, 2024 18:52
Copy link
Member

@augustofg augustofg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@guilhermerc guilhermerc merged commit 64c5bf1 into devel Apr 22, 2024
1 check passed
@guilhermerc guilhermerc deleted the shaper-filt branch April 22, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants