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

Improvements and cleanup of signals and ports #33

Merged

Conversation

diegoferigo
Copy link
Member

@diegoferigo diegoferigo commented Jan 29, 2019

Since the time of wb-toolbox there was some confusion between properties and information assigned to signals and ports. Sometimes things related to one were used on the other.

This PR:

  • Introduces a new core::Port class that collects most of the information about ports, and removes it from core::Signal and core::BlockInformation.
  • Removes the size optional argument when asking a Signal object from BlockInformation implementations. This should never be the case since this information should be stored inside the BlockInformation object in the configuration phase and the blocks should not have the power of asking a signal with a size that might mismatch with the right one.
  • Cleans buffer initialization of the core::Signal class. Now the size of the buffer is specified when the buffer pointer is stored. This should provide more robust APIs since there won't be anymore the need to remember to call Signal::setWidth in order to have a valid object.

For the time being it only stores port information
The signal size must be stored in the BlockInformation implementation, and callers of get{In,Out}putSignal should not need to specify the size of the signal they are accessing.
- Use the new core::Port class
- Updated to the new core::BlockInformation method names
- Use the new core::Port class
- Updated to the new core::BlockInformation method names
Now the size must be specified while passing the buffer, being also a more intuitive usage for the users since it does not require remembering to call setWidth in order to have a valid Signal object. This approach is more consistent to a possible switch to std::span.
Adapted the class to the new core::Port class and the updated core::BlockInformation methods
Set a higher priority to rtwtypes.h header. This is necessary because the order or Simulink / Simulink Coder headers matters. Previously the headers were sorted alphabetically, and the build was failing.
@diegoferigo diegoferigo force-pushed the feature/PortAndSignalImprovements branch from b2eb639 to 899ec2d Compare January 29, 2019 15:53
@diegoferigo diegoferigo changed the title [WIP] Improvements and cleanup of signals and ports Improvements and cleanup of signals and ports Jan 29, 2019
Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

Ack.

One thing to note (even if not directly related to this PR) is that there is no documentation on where to install the blockfactory plugins in the install prefix. As a superbuild mantainer, I would like that all the projects in the superbuild just install their blockfactory plugins in the same install directory, so I just need to define BLOCKFACTORY_PLUGIN_PATH once.

Additionally, w.r.t. to debian packaging, it would be good if the default installation prefix was in the search path even without the need to define BLOCKFACTORY_PLUGIN_PATH, so that if in the future we have deb package of BlockFactory + WB-Toolbox, they will just work out of the box without the need to define any env variable.

@diegoferigo
Copy link
Member Author

One thing to note (even if not directly related to this PR) is that there is no documentation on where to install the blockfactory plugins in the install prefix.

I tried to handle it with the install_blockfactory_plugin CMake function:

install(
TARGETS ${plugin_name}
EXPORT ${plugin_name}Export
RUNTIME DESTINATION "${CMAKE_INSTALL_BINDIR}/blockfactory" # Location of the .dll
ARCHIVE DESTINATION "${CMAKE_INSTALL_LIBDIR}/blockfactory" # Location of the .lib
LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}/blockfactory" # Location of the .so / .dylib
PUBLIC_HEADER DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/${plugin_name}/Block")
endfunction()

And there is a message displayed when configuring the project (in addition to the website):

if(WIN32)
message(STATUS "Remember to add \"${CMAKE_INSTALL_PREFIX}\${CMAKE_INSTALL_BINDIR}\blockfactory\""
" to BLOCKFACTORY_PLUGIN_PATH environment variable")
else()
message(STATUS "Remember to add \"${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/blockfactory\""
" to BLOCKFACTORY_PLUGIN_PATH environment variable")
endif()

Additionally, w.r.t. to debian packaging, it would be good if the default installation prefix was in the search path even without the need to define BLOCKFACTORY_PLUGIN_PATH, so that if in the future we have deb package of BlockFactory + WB-Toolbox, they will just work out of the box without the need to define any env variable.

This is more complicated. If we install plugins in one of the default linker folder, we get the same problems due to LD_LIBRARY_PATH we recently discussed in robotology/ycm-cmake-modules#199 and robotology/robotology-superbuild#97.

@diegoferigo
Copy link
Member Author

diegoferigo commented Jan 29, 2019

@traversaro I would keep this discussion going in a new issue, feel free to open it. In the meantime, I proceed merging this PR.

@traversaro
Copy link
Member

I tried to handle it with the install_blockfactory_plugin CMake function:

You are right, sorry for missing it!

@traversaro
Copy link
Member

I would keep this discussion going in a new issue, feel free to open it. In the meantime, I proceed merging this PR.

Ok, new issue open in #34 .

@diegoferigo diegoferigo merged commit 8c3a0ea into robotology:master Jan 29, 2019
@diegoferigo
Copy link
Member Author

Merged, thanks!

@diegoferigo diegoferigo deleted the feature/PortAndSignalImprovements branch January 29, 2019 20:19
diegoferigo added a commit to diegoferigo/WB-Toolbox that referenced this pull request Jan 30, 2019
diegoferigo added a commit to diegoferigo/WB-Toolbox that referenced this pull request Jan 30, 2019
diegoferigo added a commit to diegoferigo/WB-Toolbox that referenced this pull request Jan 31, 2019
diegoferigo added a commit to diegoferigo/WB-Toolbox that referenced this pull request Jan 31, 2019
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.

2 participants