Log display time taken waypoints #3384

Ryanf55 · 2023-01-25T16:36:29Z

Basic Info

Info	Please fill out this column
Display elapsed time in RVIZ panel and add time logging	#3347
Primary OS tested on	Ubuntu 22.04

Description of contribution in a few bullet points

I added two new fields in the waypoint follower action for time logging
I fixed a typo for a random field that was defined wrong
I added a field to the RVIZ panel to display total elapsed time
-->

Description of documentation updates required from your changes

Added description of additional time reporting fields in the nav2_waypoint_follower

Future work that may be required in bullet points

Display current waypoint number and current loop count in RVIZ
Display of the result.wp_elapsed_time as a table in RVIZ

TODO list before merge

Propose message contents have comments explaining the usage of the fields
Add some unit tests in thenav2_waypoint_follower package
Properly handle end of waypoint mission setting time to zero
Decide if the total elapsed time field should not appear if not in waypoint follow mode
Convert to QTableWidget and make the UI look nearly the same as before
Handle hiding/showing UI items dynamically like before, but with QTableWidget
Investigate if calling new every render is a bad idea for the table elements

For Maintainers:

Check that any new parameters added are updated in navigation.ros.org
Check that any significant change is added to the migration guide
Check that any new features OR changes to existing behaviors are reflected in the tuning guide
Check that any new functions have Doxygen added
Check that any new features have test coverage
Check that any new plugins is added to the plugins page
If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

padhupradheep · 2023-01-25T17:35:55Z

You have to rebase from main.. so that you have an updated copy of mergify.yaml in your branch..

Ryanf55 · 2023-01-25T18:39:14Z

Ok, so mostly it's working well, however there are times it seems the waypoint follower never terminates despite meeting the goal.

[component_container_isolated-6] [INFO] [1674671838.601573493] [controller_server]: Passing new path to controller.
[component_container_isolated-6] [INFO] [1674671839.055998472] [controller_server]: Reached the goal!
[component_container_isolated-6] [INFO] [1674671839.090739459] [bt_navigator]: Goal succeeded

That is what's printed, however the feedback for follow waypoints is still incrementing elapsed time, and the robot is not moving. Any suggestions?

Ryanf55 · 2023-01-25T19:26:46Z

I've just implemented the final part of setting total time taken back to 0 on completion using the follow waypoints goal status message.

The one bug in the UI right now is there is a spurious Feedback indicator, but only when the waypoints are executing. No idea what's causing it yet.

Ryanf55 · 2023-01-25T20:27:16Z

You have to rebase from main.. so that you have an updated copy of mergify.yaml in your branch..

Just rebased, still failing.

SteveMacenski · 2023-01-25T20:45:26Z

Yeah, the extra feedback status needs to be removed. Why is there a larger space between the time taken and the recovery counter? I'd reduce the displayed time elapsed for waypoint following to 1 decimal point. I don't think anyone needs the millisecond level accuracy in this tool

nav2_waypoint_follower/include/nav2_waypoint_follower/waypoint_follower.hpp

nav2_waypoint_follower/src/waypoint_follower.cpp

nav2_bringup/waypoints/tb3_simulation_waypoint_example.yaml

nav2_rviz_plugins/src/nav2_panel.cpp

SteveMacenski · 2023-02-02T21:48:16Z

Any updates? 😄

Ryanf55 · 2023-02-02T21:55:10Z

Hey there,

I got my head wrapped around the axle on the state machine and couldn't figure out exactly how to transition the state machine to the new "running" state; I'm brand new to QT. Just need some dedicated time to draw it out. Some open source debugging tools I tried adding to show the state machine and transitions did not compile with our version of QT (of course).
I got very sidetracked with the table generation and realized that it would be a whole ton cleaner probably to generate the tables dynamically with the QTableWidget class; that would be for a different PR. For this one, there just might be some more code duplication because the strings are hard coded XML and that's how it's already done
I am struggling to test my changes repeatably and don't know how to trigger certain failure conditions (these can be seen by the lack of code coverage for failure modes in waypoint navigation looking at the codecov report). This excellent video shows how one might automate testing of a QT application to give more confidence in making changes to something like a state machine. This also would be a separate issue. As much as it would be great to use TDD for the UI in some sense, I'd rather spend some time contributing to some camera control waypoint following algorithm type stuff in NAV2 than a UI that I can't use on my robots.

I'll be joining the NAV2 working group meeting to formally introduce myself. See ya there.

Ryanf55 · 2023-02-03T03:43:56Z

Ok for problem 2, I just manually copied the state machine through inspection to a state-machine capable renderer for markdown called mermaid. See below. Now that I've actually mapped out the state machine, it's clearer how the new state will fit in.

If you like, this diagram could be tracked in source code of the nav2_rviz_plugins package in the spirit of a design doc? It ideally would need to maintained in parallel with source.

stateDiagram-v2
    state "Pre Initial" as pre_initial
    state "Initial" as initial
    state "Idle" as idle
    state "Reset" as reset
    state "Paused" as paused
    state "Resumed" as resumed
    state "Paused WP" as paused_wp
    state "Resumed WP" as resumed_wp
    state "Running Nav through Poses" as running_nav_through_poses
    state "Running Follow Waypoints" as running_follow_waypoints
    state "Canceled" as canceled
    state "Accumulating" as accumulating
    state "Accumulated WP" as accumulated_wp
    state "Accumulated Nav through Poses" as accumulated_nav_through_poses

    [*] --> pre_initial

    %% Start/Reset button click transitions
    pre_initial --> idle: InitialThread navigationActive()
    pre_initial --> initial: InitialThread navigationInactive()
    initial --> idle: start reset button clicked()
    idle --> reset: start reset button clicked()
    running_nav_through_poses --> canceled: start reset button clicked()
    paused --> reset: start reset button clicked()
    idle --> accumulating: navigation mode button clicked()
    accumulating --> accumulated_wp: navigation mode button clicked()
    accumulating --> accumulated_nav_through_poses: pause resume button clicked()
    accumulating --> idle: start reset button clicked()
    accumulated_wp --> canceled: start reset button clicked()
    accumulated_nav_through_poses --> canceled: start reset button clicked()

    %% Internal state transitions
    canceled --> idle: Internal Transition
    reset --> initial: Internal Transition
    resumed --> idle: Internal Transition

    %% Pause/Resume button click transitions
    idle --> paused: pause resume button clicked()
    paused --> resumed: pause resume button clicked()

    %% Pause/Resume button waypoint transition
    accumulated_wp --> resumed_wp: pause waypoint button clicked()
    resumed_wp --> accumulated_wp: pause waypoint button clicked()
    resumed_wp --> canceled: start_reset_button clicked()
    

    %% ROSAction Transitions
    idle --> running_nav_through_poses: ROSActionQTransition IdleTransition
    running_nav_through_poses --> idle: ROSActionQTransition runningNavThroughPosesTransition
    running_follow_waypoints --> idle: ROSActionQTransition runningFollowWaypointsTransition
    idle --> accumulated_wp_: ROSActionQTransition idleAccumulatedWpTransition
    accumulated_wp --> accumulating: ROSActionQTransition accumulatedWpTransition
    idle --> accumulated_nav_through_poses: ROSActionQTransition idleAccumulatedNTPTransition
    accumulated_nav_through_poses --> idle: ROSActionQTransition accumulatedNTPTransition

SteveMacenski · 2023-02-03T19:21:10Z

I'm not sure to the extent that you should have to play with the state machine. You should (?) be able to make a function for populating that new table when the state for WPF is called (e.g. your getFollowWaypointsFeedbackLabel).

For example, if you don't even make a new feedback indicator object, just use the navigation_feedback_indicator_, you could still set the text on those feedback messages and add in the new Total Time Taken but also call the other FeedbackLabel() function(s) to populate the table within the getFollowWaypointsFeedbackLabel function so its all 1 table assembed together. I haven't done the work to know how to store/sync the navigation information with the WPF results, but that's perhaps a direction to go rather than adding a new field that you have to integrate with a new state (?). We already use the same navigation_feedback_indicator_ to display different information via getNavToPoseFeedbackLabel and getNavThroughPosesFeedbackLabel. I don't see how the WPF would be manifestly different.

Pradheep shows an example of that for the WPF already with the WP and Loop numbers: https://github.com/ros-planning/navigation2/blob/ef2c9fc725ccd8fe53cd46165cbf6c70c266087e/nav2_rviz_plugins/src/nav2_panel.cpp#L804-L813. I'm sure you could just add in your additional fields here for only a couple of lines of changes, no?

I'll say that the RVIZ plugin is by far the least well designed thing in the stack. Unlike the rest of it, this is just for demos and for folks to play with when getting started or testing. So I'm less strict to have it be super clean and well tested (plus, when I inherited it, it was already kind of difficult to read, but new features have only made it worse) as its not a "runtime" tool for any serious use. This is the one place hacking is OK.

If you like, this diagram could be tracked in source code of the nav2_rviz_plugins package in the spirit of a design doc? It ideally would need to maintained in parallel with source.

Sure!

padhupradheep · 2023-02-03T21:39:07Z

Hey @Ryanf55, Steve is giving you all the necessary help..

just in case if you need an extra hand, let me know(ping me on Slack) 😃 I’ll also try to give you an hand.. 🤙🏼

Ryanf55 · 2023-02-13T19:32:28Z

getFollowWaypointsFeedbackLabel

After trying this approach, I've run into the snag that there is mixed information between nav through poses and follow waypoints. The toLabel function used in both getNavThroughPosesFeedbackLabel and getNavToPoseFeedbackLabel templated out the message, assuming it has estimated_time_remaining, distance_remaining, navigation_time and number_of_recoveries, which is not present in the feedback for NavigateWaypoints.

Unless the panel starts to cache feedback data between the three actions, getFollowWaypointsFeedbackLabel does not have access to all the data needed. The table needs to be populated async from two different feedback subs. Thoughts on this: Use a QTableWidget to be populated by the three different feedback indicators. This table would be stored as a member variable of the class. Instead of getNavThroughPosesFeedbackLabel returning a Qstring to populate navigation_feedback_indicator_, QLabel, change navigation_feedback_indicator_ to a QTableWidget. The Get*Feedback signature is modified to populate a passed in table.

  static inline void PopulateNavThroughPosesFeedback(
    const nav2_msgs::action::NavigateThroughPoses::Feedback =
    nav2_msgs::action::NavigateThroughPoses::Feedback(),
    QTableWidget* table);

An enum is built up for all the table widget data like so:

enum class NavigationFeedbackRows {
    waypoint, 
   loop:
   poses_remaining,
   total_time_taken,
   eta,
   time_taken_for_current_wp,
   distance_remaining,
   recoveries
   ...
}

This enum is used to populate the rows in each table. Depending on the mode, at each transition, the rows that need to be displayed are either hidden or shown with hideRow during the transitions between states.

SteveMacenski · 2023-02-13T21:42:42Z

the panel starts to cache feedback data

Sure, why not? There's a bunch of stuff already stored:

https://github.com/ros-planning/navigation2/blob/ef2c9fc725ccd8fe53cd46165cbf6c70c266087e/nav2_rviz_plugins/include/nav2_rviz_plugins/nav2_panel.hpp#L144-L149

We store goals, why not feedback?

A table could work too!

SteveMacenski · 2023-02-14T01:06:50Z

LMK when you want me to take a look again

Ryanf55 · 2023-02-14T01:08:20Z

LMK when you want me to take a look again

Will do. QTableWidget is the best (no more hard coded XML strings), but I'm spending time mucking with the appearance to get it to be fixed size. I'll ping you here when it's ready for some eyes. No blocks.

Ryanf55 · 2023-02-16T20:18:49Z

At this point, I have done an attempt at replacing the get*Label() functions with the following:

RenderNavThroughWPFeedback(
    QTableWidget* table,
    const nav2_msgs::action::FollowWaypoints::Feedback msg =
    nav2_msgs::action::FollowWaypoints::Feedback()
);

There is a single QTableWidget that stores all the previous data. It has allocated enough rows to view all the previous data. In the above commit, I implemented the waypoint feedback and result rendering, and it works. The table is not formatted at all, so that will need to be done (coloring, sizing, etc). I think it will work good.

Other than styling of the table, and implementing the other feedback/result channels for the table, I'll need to implement state change behavior. At each state change, the table rows may need to be hidden/shown.

That should cover the rest of the work.

Ryanf55 · 2023-02-16T23:12:28Z

I could use some clarification on the "cleanup" behavior after actions complete. There are some spots where data is left as-is, and others where it's immediately reset to zero at the state change. Is this inconsistency intentional depending on which data member it is, or is there a general process I should follow? This applies to rows in the feedback table that stick around.

For example, you do a simple Nav2 goal. While the robot is going to goal, data updates. Then, the action completes. Should Distance Remaining, and Time taken for Current WP be reset to 0, or do users want to see the data for a bit?

To me, it makes sense to preserve it until you go to the Reset state, or obviously, start another action.

SteveMacenski · 2023-02-16T23:42:57Z

There are some spots where data is left as-is

As far as I can tell from a quick test, its always reset back to 0 after being cancelled or completed (the only difference is the Feedback: field being "canceled" in yellow or "reached" in green). What's not cleaned up?

Keeping time taken and recoveries would actually be nice to keep up until the next run. The ETA/distance remaining should both be exactly 0 if we have achieved the goal, so those should be reset in case there's an small error.

Ryanf55 · 2023-02-17T00:13:06Z

Yea, overall that agrees with my expectations. Pushing what I have now in 68cfb2f, which is everything is always visible. I'll go through and figure out how to use assignProperty on the row visibility, and also setting the things to zero as appropriate, when I have more time. Thanks!

Ryanf55 · 2023-02-18T06:30:42Z

Good news - UI is looking [less look a data table and more like the original. Rows are dynamically added fine now too.

I think found some bugs in behavior, not sure if they are in scope to fix. I'll need to confirm they are present on rolling.

In Follow WP mode, Waypoint cound does not change UNLESS there is more than 0 loops. To me, this does not seem correct, and the user would like to know what waypoint number they are on, even if there are no loops.
In Nav to Pose mode, the result condition is never met because goal_index_ == static_cast<int>(store_poses_.size()) - 1 is never true. This is preventing from using that code block to reset the data members to 0 in that goal handle. Furthermore, your statement about The ETA/distance remaining should both be exactly 0 if we have achieved the goal is only true, sometimes. I'm pretty regularly seeing ~0.23m distance remaining, so it does need to be reset to zero. The other actions there are also not meeting the equality condition when they complete successfully, but it's not consistently off by 1. Some internal state tracking could be off...

Finally, I cannot figure out how meet the following requirement:

Preserve total time taken from a WP run until next run in the situation the next run is just a navigate to pose. At this point, there is no way I can tell from any of the QState's or transitions that only a NavToPose is requested that I can find. My idea is to use navigation_goal_status_sub_ where store_poses_.size() -1 == 0 && msg->status_list.back().status == action_msgs::msg::GoalStatus::STATUS_EXECUTING. Problem is, the intended condition there is never met as far as I can tell in the current code; store_poses_ is empty for a Nav2 Goal sent from Rviz . It appears because a NavToPose action never set store_poses_; and that only comes from onAccumulatedWp, thus those UI elements won't ever reset. More debugging needed...

EDIT: Testing on main hash 6d12726, the behavior is the same. 1 is not seen because waypoint number is hidden in follow WP mode with loop count == 0, and for 2, same thing. The data is not reset. Since this is already current behavior, I'd prefer to keep it out of scope as a separate issue from this one.

Ryanf55 · 2023-02-19T19:04:45Z

PR is ready for review. I've met all the acceptance criteria. Of note, I couldn't ever figure out how to get the state machine into running_follow_waypoints. So either I can remove everything referencing that state, or we can figure out how to enter it. The logic for hiding/showing navigation feedback isn't the clearest because I couldn't link it to the states with assignProperty (it's not a public attribute of the table).

SteveMacenski · 2023-02-21T19:45:40Z

Awesome! What about the big gap at the end of the table your last image shows?

In Follow WP mode, Waypoint cound does not change UNLESS there is more than 0 loops. To me, this does not seem correct, and the user would like to know what waypoint number they are on, even if there are no loops.

@padhupradheep ?

For your second issue - I'm just reporting what I saw when I tested out the resetting behavior myself on a test environment. It seemed to work fine for me - which is odd if that condition as you say is never met! That doesn't feel like something that would be flaky as in working sometimes but not others.

If you can't preserve things - just reset everything to 0 then at the end of a run as current behavior does.

Please rebase onto main since alot has changed since this was opened. @padhupradheep can you review?

padhupradheep · 2023-02-22T16:00:31Z

In Follow WP mode, Waypoint cound does not change UNLESS there is more than 0 loops. To me, this does not seem correct, and the user would like to know what waypoint number they are on, even if there are no loops.

I understand! Rhe reason I left it because, the waypoint numbers can be seen on the waypoint marker. But thinking of it, I guess it'll be a good idea to add it for "no loop" case as well. I'd suggest a separate PR for that.

@padhupradheep can you review?

Sure! I'll be doing it in few hours..

padhupradheep

Good job..

First of satisfy linters... run ament_cpplint and ament_uncrustify to see what all things that need to be changed for satisfying for it.

I have requested some minor changes after doing a quick glance. I'll go through it once again and give you another review, I also have to test it in my side.

nav2_rviz_plugins/include/nav2_rviz_plugins/nav2_panel.hpp

nav2_rviz_plugins/src/nav2_panel.cpp

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

* Fix typo in elasped vs elapsed * Add the calculation for elapsed time in the waypoint follower Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Ryanf55 · 2023-02-23T17:41:32Z

Awesome! What about the big gap at the end of the table your last image shows?

In Follow WP mode, Waypoint cound does not change UNLESS there is more than 0 loops. To me, this does not seem correct, and the user would like to know what waypoint number they are on, even if there are no loops.

@padhupradheep ?

For your second issue - I'm just reporting what I saw when I tested out the resetting behavior myself on a test environment. It seemed to work fine for me - which is odd if that condition as you say is never met! That doesn't feel like something that would be flaky as in working sometimes but not others.

If you can't preserve things - just reset everything to 0 then at the end of a run as current behavior does.

Please rebase onto main since alot has changed since this was opened. @padhupradheep can you review?

Will do.

Rebased on main to now branch from 6d12726

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

SteveMacenski

I didn't have the opportunity today to review the rviz parts, but I did the WPF parts

SteveMacenski · 2023-02-23T20:05:35Z

nav2_waypoint_follower/src/waypoint_follower.cpp

@@ -175,6 +175,9 @@ WaypointFollower::followWaypoints()

  rclcpp::WallRate r(loop_rate_);

+  auto start_time = this->now();


start_time and last_wp_time isn't reset when the action is preempted with a new task

Same with result = std::make_shared<ActionT::Result>(); to reset the wp states if we have a new task. This isn't a bug you introduced, but just finding it now

Do you want me to fix it in this PR? Or can I split that into its own new issue?

It should be fixed here, its just 2 lines - both of which are required for your changes to work

nav2_waypoint_follower/src/waypoint_follower.cpp

padhupradheep · 2023-02-24T14:50:48Z

Of note, I couldn't ever figure out how to get the state machine into running_follow_waypoints

@Ryanf55 could you please explain why do you want to go into the running_follow_waypoints state? exactly at which scenario?

padhupradheep

I think there seems to be a confusion in the understanding of running_ , accumulated_nav_through_poses_ and accumulated_wp_.

As explained in the comments, running_ state is for just a single goal. There is no waypoint following happening there.. So you do not have to split them.

I'm happy to be corrected if there was any conversation between you and Steve regarding this and there was a plan to change in the design ?

nav2_rviz_plugins/src/nav2_panel.cpp

padhupradheep · 2023-02-24T15:31:33Z

nav2_rviz_plugins/include/nav2_rviz_plugins/nav2_panel.hpp

  // and the button state to change automatically.
-  QState * running_{nullptr};


on first place, why is this being split into two different states? running_ is kind of the state that the state machine transition to, when you give a single goal. This is no ways related to the waypoint navigation.

The states accumulated_wp_ and accumulated_nav_through_poses_ are the equivalent to the running_ state for the navigation in the waypoint mode.

padhupradheep · 2023-02-24T15:49:31Z

nav2_rviz_plugins/src/nav2_panel.cpp

+  running_nav_through_poses_->assignProperty(pause_resume_button_, "enabled", false);
+
+  running_nav_through_poses_->assignProperty(navigation_mode_button_, "text", "Waypoint mode");
+  running_nav_through_poses_->assignProperty(navigation_mode_button_, "enabled", false);


This needs to go back to the original state.. running_ state should be untouched..

padhupradheep · 2023-02-24T15:50:57Z

nav2_rviz_plugins/src/nav2_panel.cpp

+
+  ROSActionQTransition * runningNavThroughPosesTransition = new ROSActionQTransition(QActionState::ACTIVE);
+  runningNavThroughPosesTransition->setTargetState(idle_);
+  running_nav_through_poses_->addTransition(runningNavThroughPosesTransition);


No the idle accumulated_state_transition is taken care somewhere in the below, you do not have to do it here..

Ryanf55 · 2023-02-24T16:36:55Z

Of note, I couldn't ever figure out how to get the state machine into running_follow_waypoints

@Ryanf55 could you please explain why do you want to go into the running_follow_waypoints state? exactly at which scenario?

I wanted a different state than running_ which I incorrectly thought was used by all the nav through poses and follow WP. Based on your other comments about this, I will revert those specific changes of adding a new state since it's not needed.

Would you be open to a rename of the three states though? IMHO, accumulated_wp_ does not mean robot is actually doing anything, more that it has waypoints available to run, but isn't actively going through them.

My proposal:

Original State Name	New State Name
running_	running_nav_to_pose_
accumulated_wp_	running_wp_
accumulated_nav_through_poses_	running_nav_through_poses

padhupradheep · 2023-02-24T16:51:18Z

Yeah, I have to agree with you, the naming sounds bit confusing. Sure, I would guess Steve will also agree with your proposal.

As Steve pointed out sometime ago, the RViz plugin is getting bit crowded and messy, there must be at least a partial refactor sometime soon in the future.

Before the next request for review, make sure that you remove all the debuggers. It would be easy for us to review, if you can show a working GIF. Use ffmpeg for creating the GiF and simplescreenrecorder for creating the video.

Once you think you are done with the feature, try to satisfy the linters and then let us know once again.

I've to really appreciate the time and effort you are putting @Ryanf55 .. There will be many people benefiting out on what you are working on. So keep it up.

Ryanf55 · 2023-02-24T17:26:47Z

Yeah, I have to agree with you, the naming sounds bit confusing. Sure, I would guess Steve will also agree with your proposal.

As Steve pointed out sometime ago, the RViz plugin is getting bit crowded and messy, there must be at least a partial refactor sometime soon in the future.

Before the next request for review, make sure that you remove all the debuggers. It would be easy for us to review, if you can show a working GIF. Use ffmpeg for creating the GiF and simplescreenrecorder for creating the video.

Once you think you are done with the feature, try to satisfy the linters and then let us know once again.

I've to really appreciate the time and effort you are putting @Ryanf55 .. There will be many people benefiting out on what you are working on. So keep it up.

Will do on touching up those last things. I'll add a video once it's done.

Thanks, sounds good. Hopefully some of the changes I've done here using QTableWidget, using more pure functions rather than global state, and building more of the UI at compile time rather than through manual HTML will make that easier.

* Steve is a ninja for seeing that Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

* See ros-navigation#3384 (comment) Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Ryanf55 · 2023-02-26T04:34:47Z

Of note, I couldn't ever figure out how to get the state machine into running_follow_waypoints

@Ryanf55 could you please explain why do you want to go into the running_follow_waypoints state? exactly at which scenario?

I wanted a different state than running_ which I incorrectly thought was used by all the nav through poses and follow WP. Based on your other comments about this, I will revert those specific changes of adding a new state since it's not needed.

Would you be open to a rename of the three states though? IMHO, accumulated_wp_ does not mean robot is actually doing anything, more that it has waypoints available to run, but isn't actively going through them.

My proposal:

Original State Name New State Name
running_ running_nav_to_pose_
accumulated_wp_ running_wp_
accumulated_nav_through_poses_ running_nav_through_poses

This is now changed on 4fb895d

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Ryanf55 · 2023-02-26T04:59:24Z

ament_cpplint

Done in 4381b27

No actions left other than dealing with transitive includes or not. Making the video now.

Edit: No luck with the video, I tried a bunch of different conversions, and all I get is a black video.
https://engineering.giphy.com/how-to-make-gifs-with-ffmpeg/
ffmpeg -i ~/Videos/nav2_1-2023-02-25_22.09.46.mkv -f gif nav2_wp_time.gif

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

padhupradheep · 2023-02-27T10:02:32Z

Edit: No luck with the video, I tried a bunch of different conversions, and all I get is a black video.

Probably try this.. https://superuser.com/questions/556029/how-do-i-convert-a-video-to-gif-using-ffmpeg-with-reasonable-quality

SteveMacenski · 2023-02-27T19:53:43Z

The renaming sounds good to me - i think that's the only thing from the discussion above I needed to respond to?

padhupradheep · 2023-02-27T19:54:35Z

yes!

And the transitive includes..

I personally looked into the transitive includes.. I guess what @Ryanf55 done is fine..

SteveMacenski · 2023-03-01T20:05:10Z

I went through the code and overall looks good to be beyond:

I didn't do any functional testing at this point, so I can't say if its works or whatnot
There were alot of inlined TODOs that need to be addressed
I didn't pick up during the scan if things like "loop number" are always present on the table even when in non-looping operating modes. I did see some hiding stuff going on, but it wasn't at least immediately clear to me that was what was going on there
It sounds like from Pradheep's comments that there are missing transitions

Ryanf55 · 2023-03-01T21:42:13Z

I went through the code and overall looks good to be beyond:

I didn't do any functional testing at this point, so I can't say if its works or whatnot

There were alot of inlined TODOs that need to be addressed

I didn't pick up during the scan if things like "loop number" are always present on the table even when in non-looping operating modes. I did see some hiding stuff going on, but it wasn't at least immediately clear to me that was what was going on there

It sounds like from Pradheep's comments that there are missing transitions

Thanks. Yes, the showing and hiding is annoyingly hard to reason about because I couldn't tie it to the state object directly, rather the transition logic. Overall, the logic does not make any assumptions about what was shown or hidden before, it always goes through the whole table. It's only a few more cycles.

The missing transitions are no longer there since that state was removed. It was unnecessary. The state machine has renamed states, but no logic has been changed regarding transitions in this PR.

padhupradheep · 2023-03-10T16:35:26Z

I still see a lot of space... 🤔

Ryanf55 · 2023-03-10T20:32:15Z

It looks like before the elements were equally spaced out.

I had to allocate the size for the table (it's there the whole time at max size, you just don't see it). If the size allocated was too small, QT added a scroll bar, which we don't want. To solve this, I could implement dynamic sizing, however I can't figure out how. Any tips?

Ryanf55 · 2023-03-11T16:42:58Z

This is the closest reference I found. Pretty wild this isn't built into the widget.
https://stackoverflow.com/questions/8766633/how-to-determine-the-correct-size-of-a-qtablewidget

SteveMacenski · 2023-03-13T23:24:03Z

I'm not entirely sure - the QT Table is a different tool than I've used before to be able to give you concrete advise. I think using only the minimum amount of space could help. Obviously that last screen shot was more space than the table would use even if fully populated.

From your own screen shot from earlier in this PR #3384 (comment) that's more in line with what we're thinking

Ryanf55 · 2023-03-13T23:51:48Z

Sadly, that screenshot was pre-QTable. I think QTable is the right thing, so I'll just need some more time to play with it. If you happen to know any competent QT devs in the ROS2 community, please let them know I could use a hand.

SteveMacenski · 2023-06-05T19:44:32Z

Hi @Ryanf55 any updates? 😄

padhupradheep · 2023-06-08T07:15:27Z

I'll probably take it over if @Ryanf55 is busy with something else.

Ryanf55 · 2023-06-08T16:30:53Z

I'll probably take it over if @Ryanf55 is busy with something else.

That would be wonderful. I expect to be super tied up till the end of June. Many thanks. From what I can tell, there's just a few tidbits to fix.

mergify · 2023-07-27T16:49:31Z

This pull request is in conflict. Could you fix it @Ryanf55?

SteveMacenski · 2023-11-02T20:43:44Z

What's the status of this? Something you're planning on finishing up?

padhupradheep · 2023-11-02T20:58:46Z

First of all really sorry for the long silence.

My schedule is clearing up and I was planning to come back to this by mid of November.

It’s definitely in my plans.

Ryanf55 force-pushed the log_display_time_taken_waypoints branch from 1fb8852 to 5248bdc Compare January 25, 2023 20:11

SteveMacenski requested changes Jan 25, 2023

View reviewed changes

ros-navigation deleted a comment from mergify bot Jan 27, 2023

Ryanf55 marked this pull request as ready for review February 19, 2023 18:59

padhupradheep requested changes Feb 23, 2023

View reviewed changes

Ryanf55 added 3 commits February 23, 2023 10:40

Add total_elapsed_time to FollowWaypoints.action

1eea368

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Implement elapsed time in the waypoint follow

6bc81e5

* Fix typo in elasped vs elapsed * Add the calculation for elapsed time in the waypoint follower Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Add an example waypoint file

60a555c

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Add trailing underscores to all member vars

3facdbb

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

SteveMacenski reviewed Feb 23, 2023

View reviewed changes

padhupradheep requested changes Feb 24, 2023

View reviewed changes

Ryanf55 added 2 commits February 25, 2023 21:31

Fix double semicolon

08419cc

* Steve is a ninja for seeing that Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Remove extra state, rename running states to be clear

4fb895d

* See ros-navigation#3384 (comment) Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Format all changed files such that colcon test passes

4381b27

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

Remove debug statement for failure to clear data on goal

75ac12b

Signed-off-by: Ryan Friedman <ryanfriedman5410+github@gmail.com>

		@@ -175,6 +175,9 @@ WaypointFollower::followWaypoints()

		rclcpp::WallRate r(loop_rate_);

		auto start_time = this->now();

		// and the button state to change automatically.
		QState * running_{nullptr};

Log display time taken waypoints #3384

Are you sure you want to change the base?

Log display time taken waypoints #3384

Conversation

Ryanf55 commented Jan 25, 2023 • edited

Basic Info

Description of contribution in a few bullet points

Description of documentation updates required from your changes

Future work that may be required in bullet points

TODO list before merge

For Maintainers:

padhupradheep commented Jan 25, 2023

Ryanf55 commented Jan 25, 2023

Ryanf55 commented Jan 25, 2023

Ryanf55 commented Jan 25, 2023

SteveMacenski commented Jan 25, 2023

SteveMacenski commented Feb 2, 2023

Ryanf55 commented Feb 2, 2023 • edited

Ryanf55 commented Feb 3, 2023

SteveMacenski commented Feb 3, 2023 • edited

padhupradheep commented Feb 3, 2023

Ryanf55 commented Feb 13, 2023

SteveMacenski commented Feb 13, 2023 • edited

SteveMacenski commented Feb 14, 2023

Ryanf55 commented Feb 14, 2023

Ryanf55 commented Feb 16, 2023 • edited

Ryanf55 commented Feb 16, 2023

SteveMacenski commented Feb 16, 2023 • edited

Ryanf55 commented Feb 17, 2023

Ryanf55 commented Feb 18, 2023 • edited

Ryanf55 commented Feb 19, 2023

SteveMacenski commented Feb 21, 2023 • edited

padhupradheep commented Feb 22, 2023 • edited

padhupradheep left a comment

Choose a reason for hiding this comment

Ryanf55 commented Feb 23, 2023

SteveMacenski left a comment

Choose a reason for hiding this comment

SteveMacenski Feb 23, 2023

Choose a reason for hiding this comment

SteveMacenski Feb 23, 2023

Choose a reason for hiding this comment

Ryanf55 Feb 26, 2023 • edited

Choose a reason for hiding this comment

SteveMacenski Feb 27, 2023 • edited

Choose a reason for hiding this comment

padhupradheep commented Feb 24, 2023

padhupradheep left a comment • edited

Choose a reason for hiding this comment

padhupradheep Feb 24, 2023

Choose a reason for hiding this comment

padhupradheep Feb 24, 2023

Choose a reason for hiding this comment

padhupradheep Feb 24, 2023

Choose a reason for hiding this comment

Ryanf55 commented Feb 24, 2023

padhupradheep commented Feb 24, 2023 • edited

Ryanf55 commented Feb 24, 2023

Ryanf55 commented Feb 26, 2023

Ryanf55 commented Feb 26, 2023 • edited

padhupradheep commented Feb 27, 2023 • edited

SteveMacenski commented Feb 27, 2023

padhupradheep commented Feb 27, 2023 • edited

SteveMacenski commented Mar 1, 2023 • edited

Ryanf55 commented Mar 1, 2023

padhupradheep commented Mar 10, 2023

Ryanf55 commented Mar 10, 2023

Ryanf55 commented Mar 11, 2023

SteveMacenski commented Mar 13, 2023 • edited

Ryanf55 commented Mar 13, 2023

SteveMacenski commented Jun 5, 2023

padhupradheep commented Jun 8, 2023

Ryanf55 commented Jun 8, 2023

mergify bot commented Jul 27, 2023

SteveMacenski commented Nov 2, 2023

padhupradheep commented Nov 2, 2023

Ryanf55 commented Jan 25, 2023 •

edited

Ryanf55 commented Feb 2, 2023 •

edited

SteveMacenski commented Feb 3, 2023 •

edited

SteveMacenski commented Feb 13, 2023 •

edited

Ryanf55 commented Feb 16, 2023 •

edited

SteveMacenski commented Feb 16, 2023 •

edited

Ryanf55 commented Feb 18, 2023 •

edited

SteveMacenski commented Feb 21, 2023 •

edited

padhupradheep commented Feb 22, 2023 •

edited

Ryanf55 Feb 26, 2023 •

edited

SteveMacenski Feb 27, 2023 •

edited

padhupradheep left a comment •

edited

padhupradheep commented Feb 24, 2023 •

edited

Ryanf55 commented Feb 26, 2023 •

edited

padhupradheep commented Feb 27, 2023 •

edited

padhupradheep commented Feb 27, 2023 •

edited

SteveMacenski commented Mar 1, 2023 •

edited

SteveMacenski commented Mar 13, 2023 •

edited