-
Notifications
You must be signed in to change notification settings - Fork 418
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
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the motivation behind this change ? (removing the macro declaring that function as public)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That appears to be the result of some poor merging on my part. Will fix.
rclcpp/src/rclcpp/parameter.cpp
Outdated
@@ -189,10 +277,11 @@ ParameterVariant::to_parameter() | |||
return parameter; | |||
} | |||
|
|||
std::string | |||
ParameterVariant::value_to_string() const | |||
std::string ParameterVariant::value_to_string() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: please keep the existing style of return value on its own line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -191,6 +191,16 @@ if(BUILD_TESTING) | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: this bloc should be added after the test_parameter_events_filter
target check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -191,6 +191,16 @@ if(BUILD_TESTING) | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That appears to be the result of some poor merging on my part. Will fix.
rclcpp/src/rclcpp/parameter.cpp
Outdated
@@ -189,10 +277,11 @@ ParameterVariant::to_parameter() | |||
return parameter; | |||
} | |||
|
|||
std::string | |||
ParameterVariant::value_to_string() const | |||
std::string ParameterVariant::value_to_string() const |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
Edit:
Stack trace with break point each time the default history attributes are used:
|
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @ayrton04 !
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 a0x
prefix.Connects to ros2/rcl_interfaces#32