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.

Sign up for GitHub

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

Jump to bottom

Re-implement the Holder block to make it compatible with code generation and 'treat as atomic unit' #211

Merged
merged 1 commit into from
May 22, 2021
Merged

Re-implement the Holder block to make it compatible with code generation and 'treat as atomic unit' #211

merged 1 commit into from
May 22, 2021

Conversation

nunoguedelha
Copy link
Collaborator

Fixes #205 .

@nunoguedelha
Fixes #205: Re-implement the Holder block to make it compatible with …
8cb2b71 
…code generation and 'treat as atomic unit'
@nunoguedelha nunoguedelha self-assigned this May 18, 2021
@nunoguedelha nunoguedelha requested a review from traversaro as a code owner May 18, 2021 21:33
nunoguedelha added a commit to robotology/whole-body-controllers that referenced this pull request May 18, 2021
@nunoguedelha
Replace 'holder' blocks in +floatingBaseBalancingTorqueControlWithSim…
e21f7e5 
…ulator

- Restore the link of the 'holder' blocks in the controller model to the
  WBToolbox (utilities) library -> after the new holder implementation was
  (robotology/wb-toolbox#211).
- Remove the 'holder' block (not required anymore) from the 'utilityBlocks_lib.slx'
  library.
@nunoguedelha nunoguedelha mentioned this pull request May 18, 2021
Merged
@traversaro
Copy link
Member

traversaro commented May 19, 2021 •
edited
Loading

@nunoguedelha do you think we can wait a few days for this? For some internal projects, #209 may need to have an higher priority and I guess there are incompatibilities between this two PRs.

@traversaro
Copy link
Member

traversaro commented May 19, 2021 •
edited
Loading

@nunoguedelha do you think we can wait a few days for this? For some internal projects, #209 may need to have an higher priority and I guess there are incompatibilities between this two PRs.

Actually I think we can wait a few more days for #209, and in practice it is blocked by robotology/robotology-superbuild#719 . So I think we are good to do on this, not sure if @Giulero is able to quickly check and approve the PR or he is too busy with the experiments (otherwise I can review myself).

@nunoguedelha
Copy link
Collaborator Author

I guess there are incompatibilities between this two PRs.

Hi @traversaro , when you say "incompatibilities" you just mean that both PRs are changing the same .slx file, and not real incompatibilities in the implementation, right?

@traversaro
Copy link
Member

I guess there are incompatibilities between this two PRs.

Hi @traversaro , when you say "incompatibilities" you just mean that both PRs are changing the same .slx file, and not real incompatibilities in the implementation, right?

Exactly.

@nunoguedelha
Copy link
Collaborator Author

Exactly.

For the record, in this case the merge should be easy.

@Giulero
Copy link

Giulero commented May 20, 2021

Today I could check the PR, testing some of our simulink models. They have a lot of holder blocks!

@nunoguedelha nunoguedelha requested a review from Giulero May 20, 2021 13:57
@Giulero
Copy link

Giulero commented May 21, 2021

I run a model with the simulink simulator. The controller block (containing some holders) is set as an atomic unit running 10 times slower than the simulink solver.
The only error was related to the use of the clock (simulink) block (that I guess was the source of the issue also for the wbt holder block). Once that block was removed everything run smoothly!

Thanks Nuno!

@traversaro
Copy link
Member

@nunoguedelha I think we can merge this, what do you think? Then I would probably need to rebase #209 on top of the new master after this PR is merged, not sure if you have any advice on how to do this.

@nunoguedelha
Copy link
Collaborator Author

@nunoguedelha I think we can merge this, what do you think? Then I would probably need to rebase #209 on top of the new master after this PR is merged, not sure if you have any advice on how to do this.

Ciao @traversaro , sorry just saw this, had an afternoon a bit problematic. Yes we can merge this now. For rebasing your branch fix/180 impacting the same library, it shouldn't be a problem. Here's what I would do:

  1. rebase your branch on top of the new master, resolving the conflict on the lib files WBToolboxLibrary_repository.mdl and WBToolboxLibrary.slx choosing your version only, discarding mine.
  2. create a new commit after coverting the libraries slx and mdl to version 2014b.
  3. merge the libraries (manually or through the Simulink tool visdiff), then git commit --amend.

I guess you main concern was more precisely how to merge the mdl and slx files. It should be trivial since the blocks impacted by the 2 branches are distinct. I only changed the "holder" block in the "utilities".

  • Manually:
    1. create a copy of my library version to an untracked file that you can still see when you go back to to your branch.
    2. replace your "holder" block in the library by mine.
  • With Simulink tool visdiff:
    1. After the initial merge (in favour of your version), and while still on your branch checkout, run a diff of the two versions of each file.
    2. A diff GUI opens in anew MATLAB process. There is a 'merge' button in the diff GUI you can use to merge the files.
  • Using "il Nuno" for doing the merge (this is a joke for @Giulero): I already have the tool visdiff integrated with Git, so it's really easy for me. Ideally I should integrate it in WBToolbox and present it to the team, it's really useful.

@nunoguedelha
Copy link
Collaborator Author

The only error was related to the use of the clock

Yes @Giulero , indeed the clock used before this PR was the only thing preventing the "set as atomic unit", not the persistent instruction. The problem with the persistent instruction is the following:

  • You use it in a MATLAB System function.
  • You then can only use the "MATLAB Inerpreted" compilation and not "Code Generation".

But in general I think it's a good practice to avoid persistent as it tends not to be compatible with other features in Simulink, and most of all is error prone as it is a static variable that needs to be cleared via clear functions (if I'm not wrong), and for which clear all is uneffective.

@nunoguedelha
Copy link
Collaborator Author

I'm not authorized to merge this PR. @traversaro I let you do it. I suggest a "Rebase and merge" since there is a single commit in my branch.

@traversaro traversaro merged commit d9b0633 into master May 22, 2021
@traversaro
Copy link
Member

I'm not authorized to merge this PR. @traversaro I let you do it. I suggest a "Rebase and merge" since there is a single commit in my branch.

Probably it was an artifact of broken CI.

@traversaro
Copy link
Member

@nunoguedelha I think we can merge this, what do you think? Then I would probably need to rebase #209 on top of the new master after this PR is merged, not sure if you have any advice on how to do this.

Ciao @traversaro , sorry just saw this, had an afternoon a bit problematic. Yes we can merge this now. For rebasing your branch fix/180 impacting the same library, it shouldn't be a problem. Here's what I would do:

1. rebase your branch on top of the new master, resolving the conflict on the lib files `WBToolboxLibrary_repository.mdl` and `WBToolboxLibrary.slx` choosing your version only, discarding mine.

2. create a new commit after coverting the libraries slx and mdl to version 2014b.

3. merge the libraries (manually or through the Simulink tool `visdiff`), then `git commit --amend`.

I guess you main concern was more precisely how to merge the mdl and slx files. It should be trivial since the blocks impacted by the 2 branches are distinct. I only changed the "holder" block in the "utilities".

* Manually:
  
  1. create a copy of my library version to an untracked file that you can still see when you go back to to your branch.
  2. replace your "holder" block in the library by mine.

* With Simulink tool `visdiff`:
  
  1. After the initial merge (in favour of your version), and while still on your branch checkout, run a diff of the two versions of each file.
  2. A diff GUI opens in anew MATLAB process. There is a 'merge' button in the diff GUI you can use to merge the files.

* Using  "il Nuno" for doing the merge (this is a joke for @Giulero): I already have the tool `visdiff` integrated with Git, so it's really easy for me. Ideally I should integrate it in WBToolbox and present it to the team, it's really useful.

Thanks @nunoguedelha ! This is quite interesting (and quite documented it seems: https://www.mathworks.com/help/simulink/ug/resolve-conflicts.html), I will try myself to familiarize with the tooling. fyi @pattacini

@traversaro traversaro deleted the fix/holder-block-compatible-with-code-generation branch May 22, 2021 08:49
nunoguedelha added a commit to robotology/whole-body-controllers that referenced this pull request May 22, 2021
@nunoguedelha
Replace 'holder' blocks in +floatingBaseBalancingTorqueControlWithSim…
9663c20 
…ulator

- Restore the link of the 'holder' blocks in the controller model to the
  WBToolbox (utilities) library -> after the new holder implementation was
  (robotology/wb-toolbox#211).
- Remove the 'holder' block (not required anymore) from the 'utilityBlocks_lib.slx'
  library.
@pattacini
Copy link
Member

pattacini commented May 23, 2021 •
edited
Loading

Thanks @nunoguedelha ! This is quite interesting (and quite documented it seems: https://www.mathworks.com/help/simulink/ug/resolve-conflicts.html), I will try myself to familiarize with the tooling. fyi @pattacini

As @nunoguedelha was mentioning, this tooling helps quite a lot when the changes impact different parts of the model. When changes impact the same block instead, for example, vsdiff starts quickly becoming difficult to employ, depending on the complexity of the change and/or the impacted block.

This was referenced May 23, 2021
Merged
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Giulero Giulero Giulero approved these changes

@traversaro traversaro traversaro approved these changes

Assignees

@nunoguedelha nunoguedelha

Labels
complexity::medium component:simulink issue::resolution::fixed priority::normal type::enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-implement the Holder block to make it compatible with code generation and "treat as atomic unit"
4 participants
@nunoguedelha @traversaro @Giulero @pattacini