Skip to content

Conversation

@jacobperron
Copy link
Contributor

@jacobperron jacobperron commented Aug 7, 2019

It's a heavy refactor of the original code following the proposal in #42.

This is a work-in-progress, but opening for visibility and early feedback.

Some things still to do:

  • Add feedback publisher + API to interactive marker client.
  • Add liveliness check between server and client.
  • Port Python code
  • Revisit some TODOs I left in the code
  • More tests

Before polishing this, I plan to get it working with the RVIz display and write a standalone node showing how we can bridge with ROS 1.

Resolves #42

Depends on ros2/common_interfaces#70

sloretz and others added 28 commits July 23, 2019 18:24
Now with .hpp suffix

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
This satisfies the ament_copyright linter.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Using this abstract interface enables use of the interactive marker client with other buffer implementations.

Also, switched to using shared pointers to avoid potential dangling reference.

Signed-off-by: Jacob Perron <[email protected]>
It seems more intuitive for clients to call a service to get the latest state for interactive
markers, versus subscribing to a latched topic and waiting for the server to publish.

Signed-off-by: Jacob Perron <[email protected]>
Failing tests are commented out for now.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Useful for introspection, especially during testing.

Signed-off-by: Jacob Perron <[email protected]>
Failing tests are commented out.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Merge SingleClient logic into InteractiveMarkerClient.
Now it is assumed that there is maximum one server per client.
This helps simplify the implementation.

Signed-off-by: Jacob Perron <[email protected]>
* Update doc-block comment style
* Fix tests
* Renaming of variables for clarity

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
The functionality it provided is not needed.

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Contributor Author

jacobperron commented Aug 22, 2019

Attempt to resolve various compiler warnings on macOS and Windows:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron jacobperron requested a review from wjwwood August 22, 2019 22:47
@jacobperron
Copy link
Contributor Author

I think this is pretty much ready for review, while I debug the Windows warnings and try to figure out why Clang and GCC disagree on what type something is.

Signed-off-by: Jacob Perron <[email protected]>
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

All lgtm!

Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Signed-off-by: Jacob Perron <[email protected]>
Adding UNUSED macro arguments fixes "too many arguments" and "too few
arguments" warnings.

Signed-off-by: Jacob Perron <[email protected]>
@jacobperron
Copy link
Contributor Author

I think I've worked out all the issues with the Windows build 🤞

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@jacobperron
Copy link
Contributor Author

I was so close... but an expected test error message is being picked up as a MSBuild warning 😞 Build Status

@jacobperron
Copy link
Contributor Author

The latest MSBuild warning is a false positive; an instance of ros2/ci#316.

@jacobperron
Copy link
Contributor Author

jacobperron commented Aug 30, 2019

Windows: Build Status

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.

3 participants