search

paulscherrerinstitute / PsiIpPackage

TCL framework to package Vivado IP-Cores
Other
13 stars 6 forks source link

NEW PsiIpPackager Version #39

Open patrick-studer opened 2 years ago

patrick-studer commented 2 years ago

In Issue #33 a discussion about a new PsiIpPackager version started. Let me quickly summarize:

Overview

Vivado is changing very fast. New features are added or old bugs are solved (finally). This means that with every new version of Vivado, old workarounds are not necessary anymore (or produce new errors) and new features can be added to the IpPackager. The problem is, that we cannot guarantee backwards-compatibility back to Vivado 2017_2 what the current IpPackage2017_2_1.tcl filename lead us to think. The only possibility we have is now to freeze the old IpPackage2017_2_1.tcl file (all features tested back to Vivado 2017_2) and start creating a new IpPackage_yyyy_r.tcl from a new version on. So we can add a lot of new features, and remove/fix old features, without breaking backwards-compatibility.

Features

Let's collect a feature list what ideas come from the community! I will add proposed features from your comments to the header section, so feel free to mention every feature/function that you missed:

I would say mid of December we can start defining which features shall be implemented in what order and by whom. I would offer myself to implement some/most of them but if someone of you is interested in contributing/supporting I would be happy to offload some pieces 😄.

References: Issue #37 and #38 are related to the NEW version of IpPackager.

Coding Guidelines

No comprehensive guideline yet. @jonasppsi suggests to keep it simple for this Repo and put it in the Readme (or a separate CodingGuidelines.md).

Botnic commented 2 years ago
  1. Define some Coding (-style) guide lines
    • Shall tabs or spaces be used?
    • Is a maximal line length defined?
    • ....
  2. Allow interfaces to be of kind "_int.xml/.xml" instead of ".xml/_rtl.xml" (I'm investigating this topic at the moment)
patrick-studer commented 2 years ago

  1. Define some Coding (-style) guide lines

    • Shall tabs or spaces be used?
    • Is a maximal line length defined?
    • ....

@jonasppsi do you already have defined Coding/Style Guidelines that we should follow? Otherwise, we will define them and store them in the repo (e.g. in the doc folder).

jonasppsi commented 2 years ago

13. Coding style No comprehensive guideline yet. I suggest keep it simple for this Repo and put it in the Readme.

15. Command Reference The CommandRef.md must be extended with a version info. Which command is supported in which version.

As Patrick started, collecting the features and putting it in an order, sounds good. I can also support the development at some point.

So, we will copy the IpPackage2017*.tcl to a new version, which we have to agree on, and adding/removing stuff on the new one, right? The old Version will remain as it is. But I think if someone needs to downgrade a certain feature to the older Vivado Version, it can be done when its compatible.

patrick-studer commented 2 years ago

@jonasppsi Regarding CommandRef.md. The question is if it is cleaner to define a separate CommandRef.md for each version => CommandRef2017_xxx.md and CommandRef2021_xxx.md, ...

For example if we keep a function from the old version, but rename it for being consisted with other functions, this would be very confusing in the CommandRef...

So, we will copy the IpPackage2017*.tcl to a new version, which we have to agree on, and adding/removing stuff on the new one, right? The old Version will remain as it is. But I think if someone needs to downgrade a certain feature to the older Vivado Version, it can be done when its compatible.

I would say yes, that's the simplest way to still keep the concept of the current implementation (so users do not have to learn new stuff...). I would appreciate if renaming of functions is also allowed to give the possibility to make the functions consisted to new added features.

patrick-studer commented 2 years ago

I checked what are the current "Naming-Style Rules" that were followed in the IpPackager.tcl. It is not consistent everywhere but thats my feeling. The other rules are copied from you or added by me.

  1. No tabs - replace TAB by 2 space
  2. Indentation done by 2 spaces
  3. Max. line length is 99
  4. Trim trailing white spaces on every line
  5. Only 1 empty line for separation
  6. Naming-Style:
    • Procedure names: lowercase and words separated by underscores (lower_snake_case)
    • Global Variable names: first letter of every word capitalized and no spaces/separators (PascalCase)
    • Local variable names: first letter lowercase and every word capitalized and no spaces/separators (lowerCamelCase)
    • Dictionary Keys: uppercase and words separated by underscores (UPPER_SNAKE_CASE)
  7. Comments have their own line. No inline comments.
  8. Indent comments to the current level. Add a space after #.
  9. Apply common indentation on functional groups. Align code on square brackets [...] basis.

Feedback is welcome.

patrick-studer commented 2 years ago

@Botnic 14. Allow interfaces to be of kind "_int.xml/.xml" instead of ".xml/_rtl.xml" (I'm investigating this topic at the moment)

This feature was added by me. Can you explain me what _int.xml is? So far I only saw the .xml/_rlt.xml versions of interface definition/abstraction files (up to Vivado 2020.1).

Botnic commented 2 years ago

@Botnic 14. Allow interfaces to be of kind "_int.xml/.xml" instead of ".xml/_rtl.xml" (I'm investigating this topic at the moment)

This feature was added by me. Can you explain me what _int.xml is? So far I only saw the .xml/_rlt.xml versions of interface definition/abstraction files (up to Vivado 2020.1).

We are working on a IP interfacing the Xilinx xxv ethernet subsystem. The status and control bus are defined via interfaces. You may have a look at them in your Xilinx installation folder (<Vivado>/data/ip/xilinx/xxv_ethernet_v4_0/interfaces/).

The interfaces have two files (_int.xml and .xml). The _int.xml corresponds to the .xml file in ".xml/_rtl.xml" and the .xml file is the abstraction _rtl.xml. I was not able yet to figure out any differences apart from the naming and have no clue why it is done this way. This files can be seen in different Vivado IP (usxgmii_v1_2, mrmac_v1_4, l_ethernet_v3_2....)

I added a branch https://github.com/Botnic/PsiIpPackage/tree/interface_int that allows to handle this system (based on your code)

patrick-studer commented 2 years ago

@Botnic BTW: Interfaces defined by Xilinx directly are always known inside Vivado. So you can just use the VLNV naming to use this interface definition in your IP. (The only difference is, that it will not copy the xml files to your IP).

Botnic commented 2 years ago

@Botnic BTW: Interfaces defined by Xilinx directly are always known inside Vivado. So you can just use the VLNV naming to use this interface definition in your IP. (The only difference is, that it will not copy the xml files to your IP).

@paedu92 That's what I thought as well. But we had troubles with these type of interfaces. Maybe we should open a new tread for this topic to keep this one clean ;-)

Botnic commented 2 years ago

I checked what are the current "Naming-Style Rules" that were followed in the IpPackager.tcl. It is not consistent everywhere but thats my feeling. The other rules are copied from you or added by me.

1. No tabs - replace TAB by 2 space

2. Indentation done by 2 spaces

3. Max. line length is 99

I would propose a little more space. Maybe 150 chars. If I look at the code at the moment, there are allmost all lines longer 99 chars. (of course they will shrink a little with 2 space intention)

  1. Trim trailing white spaces on every line

  2. Only 1 empty line for separation

  3. Naming-Style:

    • Procedure names: lowercase and words separated by underscores (lower_snake_case)
    • Global Variable names: first letter of every word capitalized and no spaces/separators (PascalCase)
    • Local variable names: first letter lowercase and every word capitalized and no spaces/separators (lowerCamelCase)
    • Dictionary Keys: uppercase and words separated by underscores (UPPER_SNAKE_CASE)

  4. Comments have their own line. No inline comments.

  5. Indent comments to the current level. Add a space after #.

  6. Apply common indentation on functional groups. Align code on square brackets [...] basis.

Feedback is welcome.

Botnic commented 2 years ago

@jonasppsi @paedu92 Would it be easier to keep the overview if we create a thread for every wish? Maybe we can use the 'label' function to sort the a little.

jonasppsi commented 2 years ago

@jonasppsi @paedu92 Would it be easier to keep the overview if we create a thread for every wish? Maybe we can use the 'label' function to sort the a little.

Hm, because there are already a lot of features I would rather try a single thread with a task list which can be edited by anyone. like this:

Botnic commented 2 years ago

Fine for me, if this works. I don't know how I can modify the list or create an "public" one.

patrick-studer commented 2 years ago

@jonasppsi I guess only you have the possibility of editing "others" comments. That's why I proposed, that I will copy all common-sense information to the top.

But the input to use checkboxes instead of bullets/numbers is for sure good :).

patrick-studer commented 2 years ago

@Botnic, @jonasppsi

Hello everyone and happy new year :). I hope you had no "overflows" like the Xilinx-Revision tags (uint32) [see here and here]...

I would like to start the "NEW-VERSION" project by creating a new branch and adding the agreed folder structure and some dummy files. Further, I would like to update the definitions of user-procedures (just the body, no functionality implemented in the package_ip procedure) that we have a new discussion base.

But before I start, did we already define what will be the Version number of the "NEW-VERSION"? If there are no strong opinions on that topic, I would propose Vivado 2020.1. AFAIK, 2019.2 was the first version including VITIS and full of bugs, so most people upgraded to 2020.1. The latest released version is 2021.2 and has some new features (we found out that there are several bugs in the new features as well). So I expect 2022.1 a next "stable" release...

So is it fine to create PsiIpPackager2020_1.tcl

(And if we need new features from the new 2021/2022 versions we can create a new version called PsiIpPackager2022_1.tcl)

Botnic commented 2 years ago

Hi @paedu92 Happy new year! Thank you for addressing this task. Using version 2020.1 is OK for me.

jonasppsi commented 2 years ago

thx, same! haha, embarrassing bug :)

2020.1 is okay.

Do you know if I can provide specific access to users for e.g. creating branches, editing comments individually? I would need create specific roles on github...