-
Notifications
You must be signed in to change notification settings - Fork 38
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
RT Capabilities Part 1 - YARP Logger Device #817
Conversation
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.
Thank you @nicktrem for the effort! I added some comments.
void logData(const std::string& name, | ||
const Eigen::VectorXd& data, | ||
const double time); |
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.
void logData(const std::string& name, | |
const Eigen::VectorXd& data, | |
const double time); | |
void logData(const std::string& name, | |
Eigen::Ref<const Eigen::VectorXd> data, | |
const double time); |
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.
Fixed in 0888c96. The change of just adding Eigen::Ref<> causes issues when saving the data via buffer manager. This is because the push_back method of the buffer manager uses a template for the data and is storing the raw data of the Eigen::Ref and not the Eigen::VectorXd. The solution was discussed and implemented with the help of @S-Dafarra in which we used a lambda function to still pass the Eigen::Vector by a reference.
devices/YarpRobotLoggerDevice/include/BipedalLocomotion/YarpRobotLoggerDevice.h
Show resolved
Hide resolved
void YarpRobotLoggerDevice::logData(const std::string& name, | ||
const Eigen::VectorXd& data, | ||
const double time) |
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.
void YarpRobotLoggerDevice::logData(const std::string& name, | |
const Eigen::VectorXd& data, | |
const double time) | |
void YarpRobotLoggerDevice::logData(const std::string& name, | |
Eigen::Ref<const Eigen::VectorXd> data, | |
const double time) |
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.
Fixed in 0888c96. The change of just adding Eigen::Ref<> causes issues when saving the data via buffer manager. This is because the push_back method of the buffer manager uses a template for the data and is storing the raw data of the Eigen::Ref and not the Eigen::VectorXd. The solution was discussed and implemented with the help of @S-Dafarra in which we used a lambda function to still pass the Eigen::Vector by a reference.
devices/YarpRobotLoggerDevice/include/BipedalLocomotion/YarpRobotLoggerDevice.h
Show resolved
Hide resolved
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.
Great job!
devices/YarpRobotLoggerDevice/include/BipedalLocomotion/YarpRobotLoggerDevice.h
Outdated
Show resolved
Hide resolved
Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
The macOS failure is:
it is unrelated to the PR, and kind the npm equivalent of actions/setup-python#577 . @nicktrem You can safely ignore this failure. @GiulioRomualdi @S-Dafarra For me we can safely disable homebrew CI. |
Hi @nicktrem! Thanks for iterating! I'm going to merge the PR well done! PS; sorry for the delay 😭 |
Great work all! |
The first part of the real-time capabilities functionality for the YARP logger device. This PR is a smaller portion of this original pull request for the logger device and just contains the bare-bone software required to get real-time logging up and running. The original pull request for the logger device will be broken up into this pull request and others to simplify the merge process