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

Add the possibility to store the elements_names for each channel #161

Merged
merged 10 commits into from
Mar 29, 2022

Conversation

GiulioRomualdi
Copy link
Member

This PR adds a new entry for each stored variable called elements_names. The entry is a cell that will contain the name of each element of a variable.

BufferInfo is now a struct that contains the name, the dimension, and the elements_names.

cc @traversaro @S-Dafarra @Nicogene @AlexAntn

@GiulioRomualdi GiulioRomualdi force-pushed the elements_names branch 2 times, most recently from fe2b444 to 7571c96 Compare March 24, 2022 13:15
Copy link
Member

@Nicogene Nicogene left a comment

Choose a reason for hiding this comment

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

If this has been tested both with and without elements_name is ok for me 👍🏻

Could you please add a test for this new feature added? Thanks!

@GiulioRomualdi
Copy link
Member Author

GiulioRomualdi commented Mar 25, 2022

Hi @Nicogene I updated the documentation 31eba39 and I added a test 9266fd2

I've also noticed that the project version is 0.2.0, even if the current release is 0.4.0. Do you think it is possible to bump the CMake project version to 0.4.100 as soon as this PR is merged so the consumer application can ask for yarp_telementry 0.4.100 to use the new features implemented?

https://github.com/robotology/yarp-telemetry/blob/f291b7dc0d32f9d217a8a14e2eaf13ad3b3469d8/CMakeLists.txt#L9

@GiulioRomualdi
Copy link
Member Author

I'm not able to figure out why the conda ci on windows is failing

@traversaro
Copy link
Member

I'm not able to figure out why the conda ci on windows is failing

I was able to reproduce the failure locally.

@traversaro
Copy link
Member

traversaro commented Mar 25, 2022

I'm not able to figure out why the conda ci on windows is failing

I was able to reproduce the failure locally.

On Windows yarp-telemetry manages the visibility of functions manually, different from most of our project were we rely on CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS. Now that ChannelInfo is a proper type, we need to mark it is as visible by declaring it as:

diff --git a/src/libYARP_telemetry/src/yarp/telemetry/experimental/BufferConfig.h b/src/libYARP_telemetry/src/yarp/telemetry/experimental/BufferConfig.h
index ae8bb0e..af19eea 100644
--- a/src/libYARP_telemetry/src/yarp/telemetry/experimental/BufferConfig.h
+++ b/src/libYARP_telemetry/src/yarp/telemetry/experimental/BufferConfig.h
@@ -23,7 +23,7 @@ using elements_names_t = std::vector<std::string>;
  * @brief Struct representing a channel(variable) in terms of
  * name and dimensions and names of the each element of a variable.
  */
-struct ChannelInfo {
+struct YARP_telemetry_API  ChannelInfo {
     std::string name; /**< Name of the channel */
     dimensions_t dimensions; /**< Dimension of the channel */
     elements_names_t elements_names; /**< Vector containing the names of each element of the channel */

fyi @GiulioRomualdi

@Nicogene
Copy link
Member

I've also noticed that the project version is 0.2.0, even if the current release is 0.4.0. Do you think it is possible to bump the CMake project version to 0.4.100 as soon as this PR is merged so the consumer application can ask for yarp_telementry 0.4.100 to use the new features implemented?

Ah good catch! Yes I forgot to bump it, if you can change it otherwise I can do it later thanks!

@GiulioRomualdi
Copy link
Member Author

I've also noticed that the project version is 0.2.0, even if the current release is 0.4.0. Do you think it is possible to bump the CMake project version to 0.4.100 as soon as this PR is merged so the consumer application can ask for yarp_telementry 0.4.100 to use the new features implemented?

Ah good catch! Yes I forgot to bump it, if you can change it otherwise I can do it later thanks!

I can change it 😄

- BufferInfo is now a struct that contains the name, the dimension and the elements_names.
- To access the BufferInfo name we should use BufferInfo.name (before was BufferInfo.first)
- To access the BufferInfo name we should use BufferInfo.dimensions (before was BufferInfo.second)
@GiulioRomualdi
Copy link
Member Author

I rebased the PR on master and I updated the CMakeLists.txt file.

@GiulioRomualdi
Copy link
Member Author

The conda ci is failing on unix systems. However I don't think it is related to this PR

Copy link
Member

@Nicogene Nicogene left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

4 participants