-
Notifications
You must be signed in to change notification settings - Fork 88
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
safety_limiter: status broadcasting from safety_limiter node #383
Conversation
[#1065] PASSED on kineticAll tests passed
[#1065] PASSED on melodicAll tests passed
|
Codecov Report
@@ Coverage Diff @@
## master #383 +/- ##
==========================================
+ Coverage 73.87% 73.97% +0.09%
==========================================
Files 56 56
Lines 4146 4157 +11
==========================================
+ Hits 3063 3075 +12
+ Misses 1083 1082 -1
Continue to review full report at Codecov.
|
@at-wat PTAL |
[#1066] PASSED on kineticAll tests passed
[#1066] PASSED on melodicAll tests passed
|
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.
@DaikiMaekawa could you add ASSERTs to the test like https://github.com/at-wat/neonavigation/pull/374/files?
safety_limiter/package.xml
Outdated
@@ -20,6 +20,7 @@ | |||
<depend>pcl_conversions</depend> | |||
<depend>sensor_msgs</depend> | |||
<depend>std_msgs</depend> | |||
<depend>safety_limiter_msgs</depend> |
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.
please move this to the block at L27
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.
addressed.
{ | ||
neonavigation_common::compat::checkCompatMode(); | ||
pub_twist_ = neonavigation_common::compat::advertise<geometry_msgs::Twist>( | ||
nh_, "cmd_vel", | ||
pnh_, "cmd_vel_out", 1, true); | ||
pub_cloud_ = nh_.advertise<sensor_msgs::PointCloud>("collision", 1, true); | ||
pub_debug_ = nh_.advertise<sensor_msgs::PointCloud>("debug", 1, true); | ||
pub_status_ = nh_.advertise<safety_limiter_msgs::SafetyLimiterStatus>("status", 1, true); |
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.
/status
is too general and may cause conflict.
trajectory_tracker is publishing status to the node private namespace.
pub_status_ = pnh_.advertise<trajectory_tracker_msgs::TrajectoryTrackerStatus>("status", 10); |
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.
switched to use pnh_ instead.
@at-wat Addressed your comments. |
[#1068] PASSED on kineticAll tests passed
[#1068] PASSED on melodicAll tests passed
|
.travis/Dockerfile
Outdated
@@ -9,7 +9,7 @@ WORKDIR /repos | |||
|
|||
COPY . /repos/src/neonavigation | |||
RUN cd /repos/src \ | |||
&& git clone --depth=1 https://github.com/at-wat/neonavigation_msgs.git \ | |||
&& git clone --branch=safety-limiter-msgs --depth=1 https://github.com/at-wat/neonavigation_msgs.git \ |
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.
Needs to be removed before merging the PR.
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.
The PR on neonavigation_msgs has been merged.
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.
I added some comments to the test.
Implementation looks good to me.
I'll merge PR in neonavigation_msgs
once the comments in that PR is addressed.
if (with_watchdog_reset > 0 && with_cloud > 1) | ||
{ | ||
ASSERT_TRUE(hasDiag()) << test_condition; | ||
EXPECT_EQ(diagnostic_msgs::DiagnosticStatus::OK, diag_->status[0].level) | ||
<< test_condition << ", " | ||
<< "message: " << diag_->status[0].message; | ||
|
||
ASSERT_FALSE(status_->has_watchdog_timed_out) << test_condition; |
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.
ASSERT_TRUE
in below else block
@@ -225,6 +232,11 @@ TEST_F(SafetyLimiterTest, SafetyLimitLinear) | |||
EXPECT_EQ(diagnostic_msgs::DiagnosticStatus::OK, diag_->status[0].level) | |||
<< "message: " << diag_->status[0].message; | |||
|
|||
ASSERT_TRUE(hasStatus()); | |||
ASSERT_TRUE(status_->is_cloud_available); |
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.
It would be better to make it EXPECT
to check all fields of status_
even if it is partially wrong.
(other checks of status_
fields would be same.)
@at-wat Updated the code where you pointed out. |
[#1069] FAILED on melodic
[#1069] PASSED on kineticAll tests passed
|
[#1070] PASSED on kineticAll tests passed
[#1070] PASSED on melodicAll tests passed
|
ASSERT_TRUE(hasStatus()); | ||
EXPECT_TRUE(status_->is_cloud_available); | ||
EXPECT_FALSE(status_->has_watchdog_timed_out); | ||
ASSERT_NE(status_->stuck_started_since, ros::Time(0)); |
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.
could you unify ASSERT/EXPECT among test cases?
.travis/Dockerfile
Outdated
@@ -9,7 +9,7 @@ WORKDIR /repos | |||
|
|||
COPY . /repos/src/neonavigation | |||
RUN cd /repos/src \ | |||
&& git clone --depth=1 https://github.com/at-wat/neonavigation_msgs.git \ | |||
&& git clone --branch=safety-limiter-msgs --depth=1 https://github.com/at-wat/neonavigation_msgs.git \ |
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.
The PR on neonavigation_msgs has been merged.
[#1071] PASSED on kineticAll tests passed
[#1071] PASSED on melodicAll tests passed
|
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.
LGTM
Let me update the PR title to "safety_limiter: status broadcasting from safety_limiter node" and merge.
closes #373