Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Edam Files format is incompatible with TerosHDL's file format #616

Merged
merged 2 commits into from
Jun 3, 2024

Conversation

ila-embsys
Copy link
Contributor

Edam's File field must contain the file_type in a format like vhdlSource-2008.

Latest TerosHDL's project structure changes have a different approach for version storage. It is done by storing a version information in a separate file_version field, while file_type still used as a file type. For example: file_type is vhdlSource and file_version is 2008.

export type t_file = {
    /** File type */
    file_type: LANGUAGE;
    /** File version */
    file_version: VHDL_LANG_VERSION | VERILOG_LANG_VERSION;
}

So that change pass the file information structure to Edalize in a wrong format. That is cause of loosing version information.

To fix that, the file type and the file version must be combined back. At least, I guess it must be for verilogSource-2005 and vhdlSource-2008.

However, I'm no sure what version Edam file assumes if file_type doesn't contain explicit version. So the fix is discussionable. Also, I didn't check other side effects of project structure change. This changes effectively fix recognition of vhdlSource-2008 and, maybe, verilogSource-2005.

@ila-embsys ila-embsys force-pushed the fix/edam_files_format branch 3 times, most recently from 1dda674 to 7b29730 Compare May 27, 2024 03:35
@qarlosalberto
Copy link
Contributor

Thanks!! But I think that it's better to do this conversion before to run Edalize: https://github.com/TerosTechnology/vscode-terosHDL/blob/dev/packages/colibri/src/project_manager/tool/edalize/edalize.ts#L80

In other case this can break save/load feature in global TerosHDL.

@ila-embsys
Copy link
Contributor Author

In other case this can break save/load feature in global TerosHDL.

I'm not sure that I clearly understand what exactly did you mean, but I checked mentioned 'save/load' feature and noticed that the changes affects saving project to the file. Did you mean that?

Now I'm confused why that happens. However, I guess that previously TerosHDL's project saving file was a kind of Edam format and now it is not. But it still uses function names like 'save_edam_yaml'. Am I right?

If so, that conversion should be outside 'get_edam_json' function. Did you mean something like that, when you said that the conversion is better to did before to run Edalize?

Edam's 'File' field must contain the 'file_type' in a format like
'vhdlSource-2008'.

Latest TerosHDL's project structure changes have a different approach
for version storage. It is done by storing a version information in
a separate 'file_version' field, while 'file_type' still used as
a file type.

For example: 'file_type' is 'vhdlSource' and 'file_version' is '2008'.
So that change pass thefile information structure to Edalize in a wrong
format. That is cause of loosing version information.

To fix that, a file type and a file version must be combined back.
@ila-embsys ila-embsys force-pushed the fix/edam_files_format branch from c1f3a69 to ef29cab Compare June 2, 2024 12:52
@ila-embsys ila-embsys force-pushed the fix/edam_files_format branch from ef29cab to 79305b8 Compare June 2, 2024 12:55
@ila-embsys
Copy link
Contributor Author

In other case this can break save/load feature in global TerosHDL.

This fixed by adding manual choosing whether edam json should be generated with standartized file_type or not. The default is TerosHLD's format.

@qarlosalberto
Copy link
Contributor

it looks perfect, thank you!!

@qarlosalberto qarlosalberto added this pull request to the merge queue Jun 3, 2024
Merged via the queue into TerosTechnology:dev with commit 9461dc8 Jun 3, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants