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.

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

Merged
merged 91 commits into from
Mar 22, 2024

Conversation

nicktrem
Copy link
Contributor

@nicktrem nicktrem commented Mar 4, 2024

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

@nicktrem nicktrem changed the title Rt capabilities p1 RT Capabilities Part 1 Mar 4, 2024
@nicktrem nicktrem changed the title RT Capabilities Part 1 RT Capabilities Part 1 - YARP Logger Device Mar 4, 2024
Copy link
Member

@GiulioRomualdi GiulioRomualdi left a 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.

bindings/python/YarpUtilities/src/VectorsCollection.cpp Outdated Show resolved Hide resolved
Comment on lines 181 to 183
void logData(const std::string& name,
const Eigen::VectorXd& data,
const double time);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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);

Copy link
Contributor Author

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.

Comment on lines 640 to 642
void YarpRobotLoggerDevice::logData(const std::string& name,
const Eigen::VectorXd& data,
const double time)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

Copy link
Contributor Author

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.

Copy link
Member

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!

Co-authored-by: Stefano Dafarra <stefano.dafarra@gmail.com>
@traversaro
Copy link
Collaborator

The macOS failure is:

2024-03-11T09:02:15.9093100Z �[31mError:�[0m The `brew link` step did not complete successfully
2024-03-11T09:02:15.9095690Z The formula built, but is not symlinked into /usr/local
2024-03-11T09:02:15.9099740Z Could not symlink lib/node_modules/npm/node_modules/@npmcli/disparity-colors/node_modules/ansi-styles/index.js
2024-03-11T09:02:15.9103080Z Target /usr/local/lib/node_modules/npm/node_modules/@npmcli/disparity-colors/node_modules/ansi-styles/index.js
2024-03-11T09:02:15.9105240Z already exists. You may want to remove it:
2024-03-11T09:02:15.9107510Z   rm '/usr/local/lib/node_modules/npm/node_modules/@npmcli/disparity-colors/node_modules/ansi-styles/index.js'
2024-03-11T09:02:15.9109480Z 
2024-03-11T09:02:15.9110010Z To force the link and overwrite all conflicting files:
2024-03-11T09:02:15.9111330Z   brew link --overwrite node@18
2024-03-11T09:02:15.9111960Z 
2024-03-11T09:02:15.9112310Z To list all files that would be deleted:
2024-03-11T09:02:15.9113650Z   brew link --overwrite node@18 --dry-run
2024-03-11T09:02:15.9114480Z 

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.

@GiulioRomualdi
Copy link
Member

Hi @nicktrem! Thanks for iterating! I'm going to merge the PR well done!

PS; sorry for the delay 😭

@GiulioRomualdi GiulioRomualdi merged commit 4e9a2ba into ami-iit:master Mar 22, 2024
10 of 12 checks passed
@DanielePucci
Copy link
Member

Great work all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants