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

Migrate from YARP_telemetry to robometry #155

Merged
merged 5 commits into from
May 23, 2022
Merged

Migrate from YARP_telemetry to robometry #155

merged 5 commits into from
May 23, 2022

Conversation

Nicogene
Copy link
Member

See robotology/robometry#173

Please review code.

@Nicogene Nicogene self-assigned this May 10, 2022
@Nicogene
Copy link
Member Author

I am not sure if the changes in ci.yml works

yarp::telemetry::experimental::BufferConfig bufferConfig;
yarp::telemetry::experimental::BufferManager<double> bufferManager;
robometry::BufferConfig bufferConfig;
robometry::BufferManager<double> bufferManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest removing the template

Copy link
Member Author

Choose a reason for hiding this comment

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

About this we are having this issue: robotology/robometry#174

Copy link
Member

Choose a reason for hiding this comment

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

Accordingly to robotology/robometry#175 we should now be able to remove the template, am I right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes exactly, I removed it

Copy link
Member

@lrapetti lrapetti left a comment

Choose a reason for hiding this comment

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

Overall looks good, but the CI seems to be failing on WIndows

@lrapetti
Copy link
Member

Btw, it might be worth mentioning this modification in the CHANGELOG.md

@Nicogene
Copy link
Member Author

Overall looks good, but the CI seems to be failing on WIndows

I think that the CI will be broken until the new robometry packets will be released am I right @traversaro ?

@traversaro
Copy link
Member

Overall looks good, but the CI seems to be failing on WIndows

I think that the CI will be broken until the new robometry packets will be released am I right @traversaro ?

Yes

@Nicogene Nicogene requested a review from lrapetti May 11, 2022 15:13
@Nicogene
Copy link
Member Author

Nicogene commented May 19, 2022

Here 9caf780 I removed the installation of robometry in the CI for windows since the conda packages.

This means that IWearLogger will be untested under windows until we have the packets
What do you think about @lrapetti @traversaro ?

@traversaro
Copy link
Member

Here 9caf780 I removed the installation of robometry in the CI for windows since the conda packets.

This means that IWearLogger will be untested under windows until we have the packets What do you think about @lrapetti @traversaro ?

I think it is a good idea.

@lrapetti
Copy link
Member

This means that IWearLogger will be untested under windows until we have the packets
What do you think about @lrapetti @traversaro ?

That's ok. We could open an issue after merging this PR in order to remember to add it back

@Nicogene if the PR is ready I would proceed merging.

mamba install cmake compilers make ninja pkg-config
# Actual dependencies
mamba install -c robotology yarp wearables idyntree matio-cpp yarp-telemetry
mamba install -c robotology yarp wearables idyntree matio-cpp
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but wearables should not be here

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it here 7190c9e

@Nicogene
Copy link
Member Author

This means that IWearLogger will be untested under windows until we have the packets
What do you think about @lrapetti @traversaro ?

That's ok. We could open an issue after merging this PR in order to remember to add it back

@Nicogene if the PR is ready I would proceed merging.

I opened the issue, I merge it!
#156

@Nicogene Nicogene merged commit 1021886 into master May 23, 2022
@Nicogene Nicogene deleted the feat/robometry branch May 23, 2022 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants