-
Notifications
You must be signed in to change notification settings - Fork 18
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
Add OSQP block #209
Add OSQP block #209
Conversation
Per alignment with @CarlottaSartore, the issue that needs to be solved before we can even try to test this in the relevant codebase is KP2 . |
I think I found the issue behind
While the print of P and A are not working, I already noticed different numbers, that apparently influenced the success of the startup:
Not working:
|
Commit e9b61f7 solved KP6, by ensuring that the Eigen vector that contain the buffers do not go out of scope for the duration of OSQP Solver life, or at least until updateBounds is called again. The root issue may lay in the osqp-eigen interface docs, and I will open a separate issue for that. |
I did that, but actually the docs were there but I did not read them: robotology/osqp-eigen#97 . |
I fixed the KP5, implementing the support for @CarlottaSartore if you need the version from past week (perhaps because this improved version has some regression) you can find it in https://github.com/robotology/wb-toolbox/tree/fix_180_first_prototype . |
The PR is now ready for review. The only remaining problems seems to be "KP1: The library is not visualized anymore in the Simulink Library Browser". However, it seems that there is the same problem on |
The Codacy failure is a false positive, the error is:
But |
Do you have access to Codacy? This is a common problem for Eigen maps. When false positive like this one occur, the error can be manually ignored directly from Codacy. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nicely done @traversaro, this PR looks really good! Apart from some minor nits, I really need to check on how to add a workflow that enforces clang-format on our projects :)
return true; | ||
} | ||
|
||
bool wbt::block::OSQP::solverInitialization(const BlockInformation*blockInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space in parameter. Did your IDE pick clang-format?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I have to check how to do that on Windows, and who can provide clang-format there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that clang-tools
in conda-forge should have clang-format also on Windows.
if (solveReturnVal) { | ||
solution_colMajor = pImpl->sqSolver->getSolution(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe from here on we can assume that solveReturnVal
is true, right? This if seems redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, if stopWhenFails
is false you don't know if solveReturnVal
is true or false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok now I got the logic, makes sense!
I think the issue is this one:
From https://www.mathworks.com/help/simulink/ug/adding-libraries-to-the-library-browser.html . Even if it is done in wb-toolbox/matlab/export_library.m Line 23 in b776e49
|
on iRonCub we had sometimes issues related to our Simulink library not visualizing correctly in the library browser. I don't know if it can help, but the most common causes were:
|
@@ -1,7 +1,7 @@ | |||
Library { | |||
Name "WBToolboxLibrary_repository" | |||
Version 8.4 | |||
SavedCharacterEncoding "UTF-8" | |||
SavedCharacterEncoding "windows-1252" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems dangerous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. One thing I remember I was doing is, when possible, picking up only the chunks of the mdl that were directly related to the newly added / modified block (quite difficult from the command line, surprisingly easy from tools like GitKraken). I'm not sure if this could be done every time due to the re-numbering of the IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was fixed by calling slCharacterEncoding('UTF-8')
in MATLAB. In general while editing the library running:
set_param(gcs,'EnableLBRepository','on');
slCharacterEncoding('UTF-8')
is beneficial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It can definitely be added in the export script
Thanks a lot, this will help. A bit of in-line comments
By "not linked correctly" you mean that it was not in the directories that you obtain by running
For reference, all the tests for this PR were done by refreshing the library browser every time.
In WB-Toolbox case, there is only one library, so i think that should not be a problem as all the blocks are saved in the same library?
This is not relevant in this case as we only have one lib, but what do you mean in this context with "not linked correctly"?
"do not link correctly" in this context means that it has the wrong name?
Thanks. For reference, all the tests I reported were done on a clean run of Simulink and by deleting the install folder. |
I am not sure what happened, but after all the tests it seems that the version now on the PR works correctly and can be found by the Library Browser, so the PR is ready for merge. |
@CarlottaSartore when you want I can proceed with the merge and then I can prepare the v5.4.0 release. |
It's the magic of Simulink (cc @Giulero) |
My hypothesis is that I was missing some steps, this was the first time that I tried systematically, uninstalling every time, etc etc. |
@traversaro , I didn't have a chance to review the last version. It's ok because there are two other reviewers, but actually the model file |
What I mean is that I might be the only one still using MATLAB 2020b apparently. I'm always a bit fearful of the latest versions 😓 . |
Sure, but I guess this is related to the discussion in: #214 . I will comment there. |
Hello there: |
Hello @EricWang0306 , can you open a new issue in https://github.com/robotology/wb-toolbox/issues/new/choose explaining how did you installed the wb-toolbox? Thanks! |
I'm sorry to reply an old issue. I've open an new issue in #241. Looking forward your reply. |
This PR add an OSQP block based on osqp-eigen to fix #180 . It requires
OsqpEigen
to be installed and theWBT_USES_OSQPEIGEN
option to be set.Known problems:
Solved problems:
UseLb
andUseUb
options are not supportedUseLba
andUseUba
to true while the other to false, gives the wrong resultsComputeObjVal,
StopWhenFails` options are not supportedBasic Tests again qpOASES block:
Example model:
qpoases_vs_osqp_test_v3.zip