Adding parameter array support#443
Conversation
|
Thanks @ayrton04, as mentionned offline this should be the only set of changes we need for now. We could think of updating Thanks for the contribution and the tests! We'll try to review this soon |
f70c7ef to
e074e6d
Compare
mikaelarguedas
left a comment
There was a problem hiding this comment.
Sorry for taking so long to review this @ayrton04.
This looks good overall, I have a few comments inline, mostly style related.
I made come changes on this branch to fix the build on windows.
Some tests are still failing on windows though: https://ci.ros2.org/job/ci_windows/4229/testReport. Do you happen to have a windows environment to look into it?
rclcpp/include/rclcpp/parameter.hpp
Outdated
| RCLCPP_PUBLIC | ||
| std::string | ||
| value_to_string() const; | ||
| std::string value_to_string() const; |
There was a problem hiding this comment.
Can you explain the motivation behind this change ? (removing the macro declaring that function as public)
There was a problem hiding this comment.
Actually this change breaks the build on windows and should be reverted (https://ci.ros2.org/job/ci_windows/4228/)
There was a problem hiding this comment.
That appears to be the result of some poor merging on my part. Will fix.
rclcpp/src/rclcpp/parameter.cpp
Outdated
|
|
||
| std::string | ||
| ParameterVariant::value_to_string() const | ||
| std::string ParameterVariant::value_to_string() const |
There was a problem hiding this comment.
Nitpick: please keep the existing style of return value on its own line
rclcpp/test/test_parameter.cpp
Outdated
|
|
||
| EXPECT_EQ(double_variant.value_to_string(), "-42.100000"); | ||
|
|
||
| const float TEST_VALUE_F {static_cast<float>(-TEST_VALUE)}; |
There was a problem hiding this comment.
is the - on purpose here ? Reusing the value used for the double but by performing operations on it appears to be a bit confusing to me. I also had to double check that the float_variant tests were actually independent of the double_variant as it's tested in the middle of the double_variant tests.
We could either test both the float_variant and the double_variant from message. Or just move the float test either a the very beginning of this test or into it's own test to avoid confusion
There was a problem hiding this comment.
This is in here because float and double arrays get mapped to the same parameter type. I meant to follow the same pattern as above, with ints and long ints. Will review.
I was also trying to kill two birds with one stone: check that floats work, but also check for negative values.
Would you prefer that all variants, even when mapped to the same array type, have their own tests?
rclcpp/CMakeLists.txt
Outdated
| target_link_libraries(test_node ${PROJECT_NAME}) | ||
| endif() | ||
| ament_add_gtest(test_parameter_events_filter test/test_parameter_events_filter.cpp) | ||
| ament_add_gtest(test_parameter test/test_parameter.cpp) |
There was a problem hiding this comment.
Nit: this bloc should be added after the test_parameter_events_filter target check
ayrton04
left a comment
There was a problem hiding this comment.
Thanks for the feedback. I do have a Windows partition that occasionally sees the light of day. I can try building there.
rclcpp/CMakeLists.txt
Outdated
| target_link_libraries(test_node ${PROJECT_NAME}) | ||
| endif() | ||
| ament_add_gtest(test_parameter_events_filter test/test_parameter_events_filter.cpp) | ||
| ament_add_gtest(test_parameter test/test_parameter.cpp) |
rclcpp/include/rclcpp/parameter.hpp
Outdated
| RCLCPP_PUBLIC | ||
| std::string | ||
| value_to_string() const; | ||
| std::string value_to_string() const; |
There was a problem hiding this comment.
That appears to be the result of some poor merging on my part. Will fix.
rclcpp/src/rclcpp/parameter.cpp
Outdated
|
|
||
| std::string | ||
| ParameterVariant::value_to_string() const | ||
| std::string ParameterVariant::value_to_string() const |
rclcpp/test/test_parameter.cpp
Outdated
|
|
||
| EXPECT_EQ(double_variant.value_to_string(), "-42.100000"); | ||
|
|
||
| const float TEST_VALUE_F {static_cast<float>(-TEST_VALUE)}; |
There was a problem hiding this comment.
This is in here because float and double arrays get mapped to the same parameter type. I meant to follow the same pattern as above, with ints and long ints. Will review.
I was also trying to kill two birds with one stone: check that floats work, but also check for negative values.
Would you prefer that all variants, even when mapped to the same array type, have their own tests?
|
OK, I didn't try in Windows yet, but I'm hoping I figured out the issue anyway. I was doing this previously: But Anyway, I changed that. I also explicitly separated out tests for float/double and integer/long integer types. |
|
Thanks @ayrton04 for the quick follow-up, it looks good. I pushed another style fix and am running CI now to see if all platforms are happy with the change. |
|
Great, thanks for that. Sorry about the flipped gtest params. |
No worries, This change seems to have broken some communication tests on all platforms (list below). DetailsEdit:
Stack trace with break point each time the default history attributes are used: Details |
|
It looks like when it's only Fast-RTPS talking to itself it works. |
|
I can't say that I do, no. My workspace contains really only what I required to get this to build and pass its tests. I'm not familiar with the RMW, but am happy to do what I can to investigate. |
|
I just ran the |
|
Just checking in. Is there anything I could be doing on my end to aid in fixing the issue? |
f2a2904 to
0fdccfe
Compare
Sorry for the delay, now that the underlying issue has been resolved (ros2/rmw_connext#288), we can move forward on this. I just pushed a rebased version of this branch that I'll use for testing and CI |
|
Nice! Thanks for the info. Let me know if I can assist in any way. |
|
The code still looks good to me 👍 Debug: With demos using parameter arrays from ros2/demos#235: |
|
Any time! |
|
Thanks @ayrton04 for the feature addition and the patience 👍 |
Signed-off-by: Abby Xu <abbyxu@amazon.com>
To go with ros2/rcl_interfaces#32.
This PR includes the following:
true/false, and bytes are printed as hexadecimal with a0xprefix.Connects to ros2/rcl_interfaces#32