Add rapidwright.util.VivadoTools #684
Conversation
Signed-off-by: Zak Nafziger <[email protected]>
Signed-off-by: Zak Nafziger <[email protected]>
plus some formatting fixes Signed-off-by: Zak Nafziger <[email protected]>
Signed-off-by: Zak Nafziger <[email protected]>
also overload runTcl s.t. to support running either a signle tcl command or a script. Signed-off-by: Zak Nafziger <[email protected]>
Signed-off-by: Zak Nafziger <[email protected]>
also some cleanup of javadoc Signed-off-by: Zak Nafziger <[email protected]>
also some formatting/comment clean up Signed-off-by: Zak Nafziger <[email protected]>
| * @author zakn | ||
| * | ||
| */ | ||
| public class VivadoTools { |
There was a problem hiding this comment.
Is the idea to have multiple classes going forward dedicated to each task? If so, I would suggest that we create a dedicated package for Vivado instead of putting them all into the same class. We have tried to avoid inner classes because they tend to complicate compilation and a few other areas.
Alternatively, the class could be converted into a static method and just return a Map<String,Integer> that contains the results. This would be simpler and would probably be the preferred method in my opinion.
There was a problem hiding this comment.
@eddieh-xlnx has been directing some of the architecture decisions here, but I think the idea is to add classes for future vivado tasks, so maybe a separate package is warranted.
Returning a Map<String,Integer> is perhaps little more awkward, since the user then needs to know exactly what vivado output log key phrase they want (i.e. "# of unrouted nets"). I was mostly following the proposed system from here: #684 (comment)
There was a problem hiding this comment.
@eddieh-xlnx has been directing some of the architecture decisions here, but I think the idea is to add classes for future vivado tasks, so maybe a separate package is warranted.
Ok, sure. Let's keep it the way it is for now and we can revisit it in the future if the file seems becomes unwieldly.
Returning a
Map<String,Integer>is perhaps little more awkward, since the user then needs to know exactly what vivado output log key phrase they want (i.e."# of unrouted nets"). I was mostly following the proposed system from here: #684 (comment)
Sure, I can see how that is a bit cumbersome and error prone. For simplicity, let's keep it as is.
There was a problem hiding this comment.
I've modelled this package under the "Tools" packages like DesignTools. It's also feels a little unweildy to do VivadoTools.<tool>.<method>(). I do agree that the inner class should be moved outside (though I don't necessarily agree they are a bad thing entirely -- they can be used for class-level (as opposed to package-level) encapsulation and do appear in the standard Java library too.
The problem with returning a Map is that I can't attach a method to it -- like a isFullyRouted() query.
Co-authored-by: Chris Lavin <[email protected]> Signed-off-by: zakn-amd <[email protected]>
| * @author zakn | ||
| * | ||
| */ | ||
| public class VivadoTools { |
There was a problem hiding this comment.
@eddieh-xlnx has been directing some of the architecture decisions here, but I think the idea is to add classes for future vivado tasks, so maybe a separate package is warranted.
Ok, sure. Let's keep it the way it is for now and we can revisit it in the future if the file seems becomes unwieldly.
Returning a
Map<String,Integer>is perhaps little more awkward, since the user then needs to know exactly what vivado output log key phrase they want (i.e."# of unrouted nets"). I was mostly following the proposed system from here: #684 (comment)
Sure, I can see how that is a bit cumbersome and error prone. For simplicity, let's keep it as is.
Signed-off-by: Zak Nafziger <[email protected]>
Small utility class that contains a method to spawn a vivado process that consumes a tcl script, and a method to parse vivado's stdout. Also includes a basic testcase to exercise these methods.
Similar to com.xilinx.rapidwright.ipi.BlockUpdater.runVivadoTasks, but more generic and could probably be used to replace this (and possibly others)