TECS based controller#52
Conversation
caiopiccirillo
left a comment
caiopiccirillo
left a comment
There was a problem hiding this comment.
Great job so far! Nice to see the controller taking shape 😄
| double jp_; // Moment of inertia of the pendulum | ||
| double jr_; // Moment of the rotating arm | ||
|
|
||
| const double g_ = 9.81; |
There was a problem hiding this comment.
I was thinking in 2 options to move this const:
- To config (maybe other classes can use?)
- As constant in a nameless namespace on .cpp file
What do you think?
There was a problem hiding this comment.
If there are going to be other shared constants I would say 2. for sake of simplicity.
| #include "ControllerInterface.h" | ||
| #include <chrono> | ||
|
|
||
| namespace tecscontroller { |
There was a problem hiding this comment.
I'd keep everything related to the controller lib in the same namespace controller
| namespace tecscontroller { | ||
|
|
||
|
|
||
| class TECSController : public ControllerInterface<double, double> { |
There was a problem hiding this comment.
I think you can remove this line (and its closing bracket) because you already defined the class in the header.
| public: | ||
| double kappa; | ||
| double ktheta; |
There was a problem hiding this comment.
If these variables are going to be used, move to header and initialize them in the constructor list (as you are already doing for the controller constants)
There was a problem hiding this comment.
From a quick re-read of my paper's summary I think ktheta and what I called kd_ in the code are the same thing and kappa should not even be there in the first place 😬
| TECSController(config::ConfigurationInterface& config, std::chrono::milliseconds cycle_time); | ||
| ~TECSController() override = default; | ||
| void Read(double error) override; | ||
| [[nodiscard]] double Write() override; |
There was a problem hiding this comment.
Not necessary for this moment: documentation. But we should create one before merging.
| public: | ||
| TECSController(config::ConfigurationInterface& config, std::chrono::milliseconds cycle_time); | ||
| ~TECSController() override = default; | ||
| void Read(double error) override; |
There was a problem hiding this comment.
I owe you some article reading, because if the error in .cpp needs those inputs, we sould rethink the controller interface. But I'll read the article and I'll come back to help you with a good structure for this.
There was a problem hiding this comment.
Thank you very much! :)
caiopiccirillo
force-pushed
the
49-investigate-possible-usage-of-controller-based-on-tecs
branch
from
688c1d3 to
aabd489
Compare
|
@LLagu I added some changes, but mostly aren't fully correct. The unit tests are failing and I need to figure out what's going on. |
Looking at the changes right now, mega job as always. No rush at all for the tests! |
2 participants
No description provided.