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

AMC–BLDC: Add the copier for codegen #327

Merged
merged 7 commits into from
Dec 19, 2022

Conversation

sgiraz
Copy link
Contributor

@sgiraz sgiraz commented Dec 9, 2022

What's new

  • Add the Copier.py script within the icub-firmware\emBODY\eBcode\arch-arm\mbd\utils folder
  • The copier now does more checks in order to avoid mistakes when copying the generated code from icub-firmware-models
  • The dictionaries.json file contains 2 templated strings that any user has to replace with the path to the parent folder of the repositories (icub-firmware-models and icub-firmware)

Note

  • The proposal is that each board that may be affected by the generated code from Simulink should contain a copy of this copier with the dictionaries.json file properly updated.
  • As an input argument, the copier script expects the path to the directories.json file, so each board contains its own configuration file.

cc @pattacini

@sgiraz sgiraz self-assigned this Dec 9, 2022
@valegagge
Copy link
Member

Hi @sgiraz,
first of all, I wanted to thank you for taking care of this activity. 🔝

You did a good job!

I have only two small suggestions:

  1. to make the readme.md clearer, you could add the link to the directories.json at the beginning of the file; in this way, the user has the entire overview of the directories-configuration file before diving into each section.
  2. specify that the script doesn't work in Ubuntu because of \\. (I did some tests on ubuntu).
    Currently, I think it is not strictly necessary to make this script run on Ubuntu, so it is sufficient to update the readme file.

Then I don't agree with you about this idea completely :

The proposal is that each board that may be affected by the generated code from Simulink should contain a copy of this copier with the dictionaries.json file properly updated.

I think it is better (from a maintainer and bug fixer point of view) to maintain the source code of the script in only one place and put the directories-configuration file in each board folder.
What do you think? Is it possible to make this change?

@sgiraz
Copy link
Contributor Author

sgiraz commented Dec 12, 2022

I have only two small suggestions:

  1. to make the readme.md clearer, you could add the link to the directories.json at the beginning of the file; in this way, the user has the entire overview of the directories-configuration file before diving into each section.
  2. specify that the script doesn't work in Ubuntu because of \\. (I did some tests on Ubuntu).
    Currently, I think it is not strictly necessary to make this script run on Ubuntu, so it is sufficient to update the readme file.

Hi @valegagge, as of now, the script is intended to be executed only on Windows, we may extend the compatibility to Ubuntu, but it may be a task for another issue, not for this one.
I'll update the readme according to your suggestions 👍🏻.

I think it is better (from a maintainer and bug fixer point of view) to maintain the source code of the script in only one place and put the directories-configuration file in each board folder. What do you think? Is it possible to make this change?

Yep I think is it. There are 2 things to consider:

  • The user has to specify the path to the .json file each time the script is launched as a parameter or as input from the user (let me know which is better).
  • Moreover, we have to decide where to put the python source file. Probably the path: icub-firmware\emBODY\eBcode\arch-arm should be fine.

@sgiraz
Copy link
Contributor Author

sgiraz commented Dec 13, 2022

Hi @valegagge,

I applied the changes requested. Let me know if it's ok.

@valegagge
Copy link
Member

Excellent!!!
Thanks very much!
As we discuss f2f, a small snippet of the directories-configuration file is missing.
In this way, the user has an overview before deepening the structure.

Copy link
Member

@valegagge valegagge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

- now it takes 3 args:
  - path to codegen source folder
  - path to model-based-design source folder (within icub-firmware)
  - path to directories.json
@sgiraz
Copy link
Contributor Author

sgiraz commented Dec 13, 2022

Ok, guys, I applied the changes suggested by @pattacini during the review meeting.
In particular, now the scripts expect 3 arguments as follows:

 Python3 copier.py <source> <destination> <path_to_directories_json>.

Where:

  • source: It's the directory where the generated code by Simulink resides. This should point to the codegen directory of the Model repository.

  • destinantion: It's the directory where to copy the code. This should point to the src/model-based-design directory of icub-firmware repo of the respective board.

  • path_to_directories_json: It's the path to the directories.json file. This file should be placed, and properly configured, for each board that includes parts of mbd code.

After these changes, the directories.json does not contain the source and target fields anymore as they will be passed as input from the user.

@marcoaccame @valegagge

@pattacini
Copy link
Member

pattacini commented Dec 13, 2022

Yep, my point was that, this way, the json file contains info that are independent from the particular user system and we will get around the risk of pushing system specific json files to the repo.

Copy link
Contributor

@marcoaccame marcoaccame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is ok for me

@marcoaccame marcoaccame merged commit 8eac2ae into robotology:devel Dec 19, 2022
@sgiraz sgiraz deleted the feat/mbd-copier branch December 20, 2022 09:39
valegagge pushed a commit to valegagge/icub-firmware that referenced this pull request May 5, 2023
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.

4 participants