casper-astro / mlib_devel

http://casper-toolflow.readthedocs.io/en/latest/jasper_documentation.html
56 stars 86 forks source link

RFDC block, DAC "Axis Clockign Invalid" locked error message #191

Closed jwkunz closed 1 year ago

jwkunz commented 1 year ago

I think this is the right repository to place this issue, but it could need posting in the casperfpga repository.

When using the ZCU111 with rfdc GUI block, there are some methods of input that leave the "DAC Tiles >> Required AXI4-Stream Block (MHz)" field with an error that says "Axis Clockign Invalid". This is good because it appears the GUI is trying to make sure there is a valid clock rate for the target sample rate, but there are two issues:

1) (minor) This is a typo in the error message. It should be "Clock[ing]". This was kind of confusing to figure out.

2) (moderate) When this DAC tile is disabled so that only the ADC portion is enabled, the error message will be greyed out (locked) and it is possible for the GUI to field to lock in the "Clockign Invalid" message even when the ADC portion has a valid clock configuration. If this field is locked with the error message, when the model is built with jasper, it will fail with a cryptic message about undefined aliases. This appears to be a library linking type issue on the surface, but upon deeper dive, you can see the line "t228_axis_stream_clk" in the generated jasper.per file will copy the error message from the GUI and cause the jasper parser to get confused. In order to get the locked error message to disappear, I had to enable the DAC tile, twidle the clock values with dummy values until the error disappeared, then disable the DAC tile again.

To fix this bug, I think the best approach would be to not have the jasper tools copy the fields from the DAC portion of the GUI if the tile is disabled. This may get complicated depending on how the Matlab GUI is set up. Another option is to run the error checking for both the ADC and DAC whenever the other side is updated.

mitchburnett commented 1 year ago

This is the right place. casperfpga is for client control of the fpga platform.

There is probably an issues with the RFDC mask here. The goal of the mask implementation though is as you say, ignore disabled tiles. I cannot seem to reproduce in MATLAB R2021a with latest m2021a branch. Screenshot attached. ADC tiles enabled, all ADC tile tabs have the same configuration. The DAC tiles are disabled. The two DAC tiles Required AXI4-Stream Clock (MHz) field shows an invalid configuration is applied. Click Apply in the mask, then run jasper, the toolflow does complete. Meaning the invalid configuration was ignored by the tools. To ignore this means that t228_axis_stream_clk and t229_axis_stream_clk had been given a compatible value in jasper.per to parse. It looks like it defaults to the value in slice 0. The other default value it is able to select is [] (empty list) but this looks like it is only used when a platform does not have four tiles (ZCU111 only has two DAC tiles).

Please provide your exact steps to reproduce? Or attach an .slx, or screenshot of the tile configuration for the tile that ends up with the error message in the jasper.per, or even the jasper.per that produces the error.

The RFDC mask is fairly involved, and the way Simulink callbacks work complicates how the mask is implemented and is a bit limiting on controlling what checks are made and how. Meaning the order parameters are toggled/changed usually reveals errors in the implementation as this might go through a control flow path that is unchecked. This is more likely where the real issue is.

In other words, the implementation of the mask needs to be hardened based on configurations so that when the tile is disabled, better defaults are restored and when each callback runs (callbacks have to run even when you do not care about the field) and a compatible value is stored. It is not possible to omit a field and not pass it on or to not copy the contents that are present. What ever is there must be copied and is passed along.

adctiles dactile228 dactile229

mitchburnett commented 1 year ago

This should be fixed in #205 to shortly merge. To re-open if similar issues emerge. Looks like it is fixed, for now.