-
Notifications
You must be signed in to change notification settings - Fork 5
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 MPC #29
base: master
Are you sure you want to change the base?
Add MPC #29
Conversation
The commit that the vcpkg submodule was linked to depended on a version of zlib that was downloaded from the zlib website that is outdated. This commit updates the vcpkg to the newest version, which gets zlib directly from the github repository.
Roughly implemented the example code for MPC controller, based on the example from cloctools/lqmpc.
- The code for the MpcController is partially implemented (only necessary methods have been added, getters and setters need to be added) - Code still needs to be debugged (there are multiple memory leaks that need to be fixed)
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
Matrix Px = arma::kron(Matrix(N_, N_, arma::fill::eye), Q_); | ||
Matrix Pu1 = arma::kron(Matrix(M_, M_, arma::fill::eye), 2 * S_ + R); | ||
Matrix Pu2 = arma::kron(Matrix(eye_offset(M)) + Matrix(eye_offset(M, 1)), -S_); | ||
Matrix Pu3 = block_diag(Matrix((M_ - 1) * m_, (M_ - 1) * m_, arma::fill::zeros), -S_); | ||
Matrix Pu = Pu1 + Pu2 + Pu3; | ||
P_ = arma::trimatu(2 * block_diag(Px, Pu)); // Taking only the upper triangular part |
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.
Are these sparse, like in the Python lqmpc? That P matrix gets pretty big, right?
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.
They aren't sparse right now, but I think I should be able to make them sparse. Speaking of sparse matrices, is it okay if I define a type for sparse matrices in lds.h
like the Matrix
and Vector
types?
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, I think it would be fine to define a sparse matrix type in lds.h
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
|
||
private: | ||
/** | ||
* @brief Set the matrix for the OSQP solver from an Armadillo matrix |
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 doxygen comment (and for from_sparse and from_vec) is misleading; it sounds like it will directly set a matrix on the controller, but instead just does the conversion
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, I will fix that!
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
* | ||
* @return The identity matrix with an offset axis | ||
*/ | ||
arma::SpMat<data_t> eye_offset(size_t n, int k = -1) { |
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.
Not sure if it helps if its output is dense, but FYI arma has functions to help do this: https://arma.sourceforge.net/docs.html#diagmat, https://arma.sourceforge.net/docs.html#diag, https://arma.sourceforge.net/docs.html#diags_spdiags
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 didn't see that! I think that will make it easier to do.
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
// Implement methods | ||
|
||
template <typename System> | ||
inline MpcController<System>::MpcController(const System& sys, Vector u_lb, |
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.
might as well put inline functions above in the class definition
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
const Vector& u0, const Matrix& xr) { | ||
size_t n_sim = t_sim / sys_.dt(); // Number of points per simulation step | ||
|
||
OSQPSolution* sol; |
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.
Does the optimization problem get reinitialized on every step? That will probably be a lot slower than if parts can be reused and just the state is updated
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.
Yes, I think the problem does get reinitialized every step. I mostly was trying to stick to what the Python package had (except currently the fast update function is never called, which I haven't fixed yet because the slow update doesn't work correctly), but I can look more into how to keep some of the previous states to prevent reinitializing at every step.
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
// Solver, settings, matrices | ||
OSQPSolver* solver; | ||
OSQPSettings* settings; | ||
OSQPCscMatrix* A = from_matrix(Acon_); | ||
OSQPCscMatrix* P = from_matrix(P_); | ||
OSQPFloat* q = from_vec(q_arma.t()); | ||
OSQPFloat* lb = from_vec(lb_); | ||
OSQPFloat* ub = from_vec(ub_); | ||
|
||
// Set settings | ||
settings = (OSQPSettings*)malloc(sizeof(OSQPSettings)); | ||
if (settings) { | ||
osqp_set_default_settings(settings); | ||
settings->verbose = 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.
I see that this follow the example of Python lqmpc, where prob.setup() is called every step. But is there a way to keep the solver, settings, matrices, vectors, etc. around from one step to the next? I imagine this would help things go faster, and might help us keep memory leaks under control since pointers wouldn't be created on every step
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 didn't see this comment before I responded to the previous one, but I think that is probably what I should be aiming for and I will try my best to reuse as many of the variables as possible.
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
throw std::runtime_error(error_message); | ||
} | ||
|
||
OSQPSolution* sol = std::move(solver->solution); |
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.
is this recommended by OSQP? there aren't memory problems with osqp_cleanup later after moving the solution?
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'm not sure. I only did this because I was trying to avoid copying data over again and again, but now that I am looking at it it might cause memory leaks. Let me look into the code a little bit more and see what exactly is going on, and I will try to make sure that there are no memory problems between copying the OSQP solution and clean up and freeing up memory.
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
|
||
// Clean up | ||
osqp_cleanup(solver); | ||
if (q) free(q); |
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.
why free() instead of delete? (not saying delete is right; I'm not an expert on the finer points of memory management). Though these wouldn't be necessary if we reuse memory from step to step
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.
As far as I know, free()
is for freeing memory allocated with malloc
, whereas delete
is used for freeing memory allocated with new
, but I will verify that to make sure I'm not doing anything wrong.
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.
The follow-up question then is why malloc
instead of new
?
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 don't actually have a good reason for using either. I mostly just followed what the other controllers did and tried to align closely to that.
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
for (OSQPInt j = 0; j < mat->n; j++) { | ||
for (OSQPInt i = 0; i < mat->m; i++) { | ||
if (A(i, j) != 0) { | ||
mat->i[n] = i; |
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.
some comments would be good here to be able to follow what's going on with i, x, p, n, etc.
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
mat->nzmax = A.n_nonzero; | ||
mat->nz = -1; // -1 means the matrix is in CSC format (required for API) | ||
|
||
mat->p = (OSQPInt*)malloc((mat->n + 1) * sizeof(OSQPInt)); |
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.
again, hard to see if there's a potential memory bug here when I don't know what p, i, x are
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.
include/ldsCtrlEst_h/lds_mpcctrl.h
Outdated
throw std::runtime_error(error_message); | ||
} | ||
|
||
arma::sp_mat::iterator it = A.begin(); |
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'm guessing it
goes over elements and j
is the index of the element?
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.
Overall, awesome job @i3ta! It's amazing how you put together so much technical stuff on the first draft with so little direction
Defined and renamed arma::SpMat<data_t> to the Sparse shorthand
…apper for OSQP that interfaces with Armadillo matrices
…es if problem is updated
Update OSQP Installation Process
…utReference method
Added MPC control for Poisson system, and added example to show it in action
No description provided.