lucask07 / covg_fpga

FPGA and Python experiment code for the digital ion channel amplifier project.
GNU General Public License v3.0
6 stars 2 forks source link

modified endpoints and advance_endpoints so that REGBRIDGE_OFFSET end… #19

Closed lucask07 closed 2 years ago

lucask07 commented 2 years ago

…points do not roll over

The "roll over" feature of advance_endpoints was acting on REGBRIDGE_OFFSET bit indices when it should not. REGBRIDGE_OFFSET bit indices should always advance with the chip number independent of the MAX_WIDTH.

Modifications:

Ajstros commented 2 years ago

This seems like a larger issue that we should address with more than a register-bridge-specific solution. However, in this case, it seems like we should just use GEN_ADDR on these endpoints. @lucask07 could you clarify why GEN_BIT is being used?

lucask07 commented 2 years ago

@Ajstros: Agreed, this is not the cleanest architecture. The use here is that a SPIFifoDriven instance is accessed by the OpalKelly register bridge with addresses for each of the coefficients for the filter. The GEN_BIT seems more fitting since it allows for an advance per chip that is greater than 1 and has a WIDTH parameter. The register bridge address space has a specific width so that for example:

It may be preferred to incorporate the filter coefficient programming into the Registers.xlsx sheet as we do for the SPI Wishbone configuration. Yet, creating register names for each of the filter coefficients seems tedious and not that helpful.

Currently, we have a dictionary of filter coefficients like below with the key indicating the address advance beyond the REGBRIDGE_OFFSET address.

self.filter_coeff = {0: 0x009e1586,
                             1: 0x20000000,
                             2: 0x40000000,
                             3: 0x20000000,
                             4: 0xbce3be9a,
                             5: 0x12f3f6b0,
                             8: 0x7fffffff,
                             9: 0x20000000,
                             10: 0x40000000,
                             11: 0x20000000,
                             12: 0xab762783,
                             13: 0x287ecada,
                             7: 0x7fffffff,
                             15: 0x0000_2000}  

And then to write the coefficients:

def write_filter_coeffs(self):
    for i in np.arange(self.filter_offset, 1 + self.filter_offset + self.filter_len):
        if (i-self.filter_offset) in self.filter_coeff:
            addr = int(
                self.endpoints['REGBRIDGE_OFFSET'].bit_index_low + i)
            val = int(self.filter_coeff[i-self.filter_offset])
            # TODO: ian has first addr of 0x19, second as 0x1a
            print(
                'Write filter addr=0x{:02X}  value=0x{:02X}'.format(addr, val))
            self.fpga.xem.WriteRegister(addr, val)
Ajstros commented 2 years ago

@lucask07 Thanks for the clarification, I understand the problem now. I think it would be best to add a width parameter of some kind for addresses so we can use GEN_ADDR. Since most addresses only increment by 1, we can leave the parameter off of most Endpoints in ep_defines.v and default the increment width to 1. For these REGBRIDGE Endpoints and any others that may require specific address increments, we can specify them like with _bitwidth. Let's say we call this new parameter _addrstep (open to better naming), the REGBRIDGE Endpoints would look like this

`define ADS8686_REGBRIDGE_OFFSET 8'h00 // bit_width=32
`define DAC80508_REGBRIDGE_OFFSET_GEN_ADDR 8'h05 // bit_width=32 addr_step=8
`define AD5453_REGBRIDGE_OFFSET_GEN_ADDR 8'h2D // bit_width=32 addr_step=8

and write_filter_coeffs would use an address instead of a bit index.

def write_filter_coeffs(self):
    for i in np.arange(self.filter_offset, 1 + self.filter_offset + self.filter_len):
        if (i-self.filter_offset) in self.filter_coeff:
            addr = int(
                self.endpoints['REGBRIDGE_OFFSET'].address + i)
            val = int(self.filter_coeff[i-self.filter_offset])
            # TODO: ian has first addr of 0x19, second as 0x1a
            print(
                'Write filter addr=0x{:02X}  value=0x{:02X}'.format(addr, val))
            self.fpga.xem.WriteRegister(addr, val)

Also, now that I have moved pyripherals (formerly interfaces) to its own repository, whatever changes we make should be made there. I have made an issue for this in the new repository. We can finish discussing a solution here and then I will start a pull request in the new repository.

lucask07 commented 2 years ago

@Ajstros Sounds good. (The write_filter_coeffs example above seems to have a typo since the address is still being derived from _bit_indexlow). You can implement the changes that you see as best and then close this pull request. Note that the top_level_module.v will need updates since the ep_defines names are changing. It would be ok to make these updates without building a new bitfile.

Ajstros commented 2 years ago

This PR matches up with PR #5: "Add addr_step option to Endpoint" in pyripherals.