Xilinx / RapidWright

Build Customized FPGA Implementations for Vivado
http://www.rapidwright.io
Other
284 stars 109 forks source link

NullPointerException when running LogicalNetlistExample #136

Closed reillymck closed 3 years ago

reillymck commented 3 years ago

This may be related to the other issue I opened concerning PhysicalNetlistExample. However, I have more information on this one. The exception is cause by n.getDevice() returning null on line 35. I am going to open a pull request that I think partially solves the issue: I noticed that the getters for the device and design members of an EDIFNetlist object do not return correctly. However, this does not solve the problem. Upon examining the EDIF parser, I do not think that there is any place where the device for the edif object is actually set (although I believe that the parser accurately picks up the device name string from the first line of the EDIF file). I noticed that in parseEDIFNetlist(), on line 398 the design is set. I think that this function would be the appropriate place to call setDevice() as well. I am not very sure specifically how/where to add that, but I think it will fix the problem.

clavin-xlnx commented 3 years ago

The problem is that RapidWright cannot auto detect the device being targeted by the EDIF file and looks for the device partname attribute. It is likely that the EDIF file does not contain this attribute and thus doesn't know what device to load. For example, in pblock0.dcp (from #134), the EDIF has the following at the end of the file:

  (design picoblaze_top
    (cellref picoblaze_top (libraryref work))
    (property XLNX_PROJ_DIR (string "/home/clavin/testRW/picoblaze"))
    (property PART (string "xcvu3p-ffvc1517-2-i"))
  )
)

Check your EDIF file to see if it has the PART property and if it is set. If not, you could try adding it. Unfortunately, the PART property is not a mandatory field and is not always written by Vivado.

reillymck commented 3 years ago

Thanks for pointing this out. The EDIF that I want to use with this example does not have that PART property set. However, before I raised this issue, I did try running this with the pblock.edf file that your provide for the PhysicalNetlistExample, and it still gets the NullPointerException.

reillymck commented 3 years ago

At the very least, maybe there should be a conditional that sees if the EDIF has the part declared, and if it does, updates the device property of the netlist? It looks like the example expects this property to be set on the EDIFNetlist object, but that is not happening, even with an EDIF file that has the PART declared.

litghost commented 3 years ago

I'd argue this is a driver issue, e.g. LogicalNetlistExample should detect if the PART was present in the incoming EDIF file, and if the part is not supplied, require that a PART be supplied as needed? Yes? Or maybe I missed the problem

reillymck commented 3 years ago

I agree that this should be added. However, even when running with a PART supplied, it is not correctly imported into the EDIFNetlist object, so calling EDIFNetlist.getDevice() still returns null

reillymck commented 3 years ago

By not correctly, I mean the code that takes the PART string, then loads a Device, and then sets EDIFNetlist.device is missing.

litghost commented 3 years ago

I agree that this should be added. However, even when running with a PART supplied, it is not correctly imported into the EDIFNetlist object, so calling EDIFNetlist.getDevice() still returns null

Sure, but that still makes this a driver issue, e.g. the problem is in https://github.com/Xilinx/RapidWright/blob/master/src/com/xilinx/rapidwright/interchange/LogicalNetlistExample.java

clavin-xlnx commented 3 years ago

I've committed two fixes, one to load the device upon calling EDIFNetlist.getDevice() if it is found to be null by checking the PART property on the design. If the PART property is not populated it will print a warning.

The other commit fixes an issue with random population of allInsts in the EDIF interchange parser.

reillymck commented 3 years ago

I reviewed the commits, specifically regarding the new code for getDevice(). Just curious, is getDesign() still supposed to just return design instead of this.design? If so, then I'll close my pull request.

clavin-xlnx commented 3 years ago

It is my understanding that design and this.design are the same thing (See this documentation). So I don't think there is a material change in functionality.

reillymck commented 3 years ago

Sorry, I have been doing a lot of Python coding. You're right; it does not make a difference. Also, I am closing this issue because I tested the example and it is working now for the pblock0.edf example file.