Fix rejected actions when goal requests arrive inbetween watchdog reset und arrival of next status package #113

simonschmeisser · 2015-06-03T12:15:14Z

this fixes #112 , a regression due to #86 / #85 . Sometimes execution of valid trajectories is rejected with this message:
Joint trajectory action rejected: waiting for (initial) feedback from controller

I poll my robot with 20 Hz (arbitrary), the watchdog is set to 1Hz. So I think the following happens:

0s: JointTrajectoryAction::controllerStateCB: trajectory_state_recvd_ = true;
0.01s: JointTrajectoryAction::watchdog: trajectory_state_recvd_ = false;
0.02s: JointTrajectoryAction::goalCB -> rejected
(0.03s: JointTrajectoryAction::goalCB -> rejected)
(0.04s: JointTrajectoryAction::goalCB -> rejected)
0.05s: JointTrajectoryAction::controllerStateCB: trajectory_state_recvd_ = true; but too late!

(the events in brackets are due to me retrying the execution if it doesn't work)

So I think the watchdog should manage some other variable like "controller_alive_" which is then checked by goalCB. trajectory_state_recvd_ is meant as an internal variable to the watchdog.

gavanderhoorn · 2015-06-11T14:10:56Z

industrial_robot_client/src/joint_trajectory_action.cpp

+  controller_alive_ = true;
+
+  watchdog_timer_.stop();
+  watchdog_timer_.start();


afaict, it should be possible to 'restart' a timer using setPeriod(..) (eventually in TimerManager). That would remove the need for the stop(), start() pair.

Not sure about one-shot timers though.

that feels like using a side-effect, stop start is a bit clumsy but quite legible, but I have no problem with being overvoted

You're right, it is a side-effect. Explicit start/stop is better.

…et und arrival of next status package Fix by reseting the watchdog_timer on package arrival make Timer oneShot, increase notification severeness from debug to warn, rename WATCHD0G_PERIOD_ to WATCHDOG_PERIOD_ initializing status variable in initialization list of the constructor

shaun-edwards · 2015-06-25T01:52:00Z

+1.... @simonschmeisser, thanks for the contribution.

Fix rejected actions when goal requests arrive inbetween watchdog reset und arrival of next status package

gavanderhoorn · 2015-06-25T07:57:18Z

@shaun-edwards: you merged this PR into the indigo release branch, not into indigo-devel (the PR was setup to do that, so not your fault).

simonschmeisser · 2015-06-25T08:07:01Z

I was just using the branch with the most recent commits, maybe I misunderstood the branching concept used. Shouldn't there in theory be a jade-devel branch since indigo was released already? Is this documented somewhere?

gavanderhoorn · 2015-06-25T08:22:19Z

Shouldn't there in theory be a jade-devel branch since indigo was released already?

Are you implying that we don't develop for a ROS version anymore after it's been released :)?

The -devel suffix is used in many ROS related repositories, and is used to indicate that all development is targeted at (or compatible with) a specific ROS release. It does not mean "developments to get an X release ready".

'Best practice' suggests that a repository should only branch for a specific ROS release if there is a need for it. Introducing changes incompatible with the previous release would be such a need. There is nothing in industrial_core right now that isn't Jade compatible, so it would follow that there is no real need for a branch. Branching would only increase the maintenance burden, as we'd need to keep all branches in sync.

Releases are then created by tagging the -devel branch. In ROS-I specifically, we adopted so called release branches, which track the development branches but do not mirror them (they include all changes up-to a tagged release).

As to documentation: it is a bit hidden, but the Install page does refer to it:

The ROS-Industrial git repository is organized similar to the ROS repository conventions, with one exception. ROS-Industrial releases (stable) are made from release name branches. release name-devel branches are reserved for continued development (potentially unstable). :
repository
  ...tags                     (all released versions)
  ...branches
    ...<release_name>         (latest release - matches documentation)
    ...<release_name>-devel   (latest development - may not match documentation)

from wiki/Industrial/Install - ROS-Industrial Stacks - Source Installation (Recommended for Developers) - Source Repository Layout.

gavanderhoorn · 2015-06-25T08:43:32Z

O and just to make sure: this is also not 'your fault'. We should've just checked more carefully how the PR was created.

simonschmeisser · 2015-06-25T09:05:00Z

ok, thanks, now I get the concept. Don't worry, I didn't feel offended. Do I need to change anything or will you be able to fix things?

Just some more questions while we're at it: Binary releases (dpkg) happen from <release_name> branch? Do fixes get backported typically?

shaun-edwards · 2015-06-25T12:01:54Z

This is definitely my fault. I should have checked (I thought I did but apparently not).

gavanderhoorn · 2015-06-25T12:31:35Z

@shaun-edwards: perhaps @simonschmeisser's question is a good reason to create some Branching Policy documentation and / or make a separate page out of that section on the Install page about source repository layout.

gavanderhoorn · 2015-06-25T12:31:59Z

Also: @simonschmeisser: this is easily fixed, and you don't need to do anything.

shaun-edwards · 2015-06-25T12:35:10Z

I'll fix this when I get into the office. Does everyone think the wiki is the right place. The "GitHub" way is to have a CONTRIBUTING readme.

gavanderhoorn · 2015-06-25T12:38:42Z

Hm, using the contribution hook is a nice idea. I've only seen it used to draw attention to any licensing requirements, but I guess you could put this kind of info into it as well. I created an issue for that some time ago over at ros-industrial/ros_industrial_issues#26.

I'd still make a page on the wiki though: that way it is a stand-alone document we can refer ppl to, not a file in a vcs somewhere. It would be similar to the PR Review Guidelines.

This reverts commit c51aa83, reversing changes made to d6207e0.

This reverts commit fb28e26.

shaun-edwards · 2015-06-25T22:47:26Z

OK...that was ugly (and possibly way more complicated than it needed to be). I have successfully reverted the merge into indigo. In order to avoid merge problems down the road, I had to sync indigo and indigo-devel. I then reverted the revert in indigo-devel. The correct code is in indigo-devel and I have verified a PR from indigo-devel into indigo catches these changes. In case you are curious, I was trying to avoid this.

…isser/indigo"" This reverts commit fb28e26.

shaun-edwards mentioned this pull request Jun 9, 2015

Industrial_robot_client: Greedy Watchdog leads to rejection of valid goal #112

Closed

gavanderhoorn reviewed Jun 11, 2015
View reviewed changes

shaun-edwards added a commit that referenced this pull request Jun 25, 2015

Merge pull request #113 from simonschmeisser/indigo

c51aa83

Fix rejected actions when goal requests arrive inbetween watchdog reset und arrival of next status package

shaun-edwards merged commit c51aa83 into ros-industrial:indigo Jun 25, 2015

shaun-edwards added a commit that referenced this pull request Jun 25, 2015

Revert "Merge pull request #113 from simonschmeisser/indigo"

fb28e26

This reverts commit c51aa83, reversing changes made to d6207e0.

shaun-edwards added a commit that referenced this pull request Jun 25, 2015

Revert "Revert "Merge pull request #113 from simonschmeisser/indigo""

bcad8cf

This reverts commit fb28e26.

shaun-edwards added a commit to shaun-edwards/industrial_core that referenced this pull request Sep 28, 2015

Revert "Revert "Merge pull request ros-industrial#113 from simonschme…

a0d885d

…isser/indigo"" This reverts commit fb28e26.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix rejected actions when goal requests arrive inbetween watchdog reset und arrival of next status package #113

Fix rejected actions when goal requests arrive inbetween watchdog reset und arrival of next status package #113

simonschmeisser commented Jun 3, 2015

gavanderhoorn Jun 11, 2015

gavanderhoorn Jun 11, 2015

simonschmeisser Jun 11, 2015

gavanderhoorn Jun 11, 2015

shaun-edwards commented Jun 25, 2015

gavanderhoorn commented Jun 25, 2015

simonschmeisser commented Jun 25, 2015

gavanderhoorn commented Jun 25, 2015

gavanderhoorn commented Jun 25, 2015

simonschmeisser commented Jun 25, 2015

shaun-edwards commented Jun 25, 2015 •

edited by gavanderhoorn

Loading

gavanderhoorn commented Jun 25, 2015

gavanderhoorn commented Jun 25, 2015

shaun-edwards commented Jun 25, 2015 •

edited by gavanderhoorn

Loading

gavanderhoorn commented Jun 25, 2015

shaun-edwards commented Jun 25, 2015

Fix rejected actions when goal requests arrive inbetween watchdog reset und arrival of next status package #113

Fix rejected actions when goal requests arrive inbetween watchdog reset und arrival of next status package #113

Conversation

simonschmeisser commented Jun 3, 2015

gavanderhoorn Jun 11, 2015

Choose a reason for hiding this comment

gavanderhoorn Jun 11, 2015

Choose a reason for hiding this comment

simonschmeisser Jun 11, 2015

Choose a reason for hiding this comment

gavanderhoorn Jun 11, 2015

Choose a reason for hiding this comment

shaun-edwards commented Jun 25, 2015

gavanderhoorn commented Jun 25, 2015

simonschmeisser commented Jun 25, 2015

gavanderhoorn commented Jun 25, 2015

gavanderhoorn commented Jun 25, 2015

simonschmeisser commented Jun 25, 2015

shaun-edwards commented Jun 25, 2015 • edited by gavanderhoorn Loading

gavanderhoorn commented Jun 25, 2015

gavanderhoorn commented Jun 25, 2015

shaun-edwards commented Jun 25, 2015 • edited by gavanderhoorn Loading

gavanderhoorn commented Jun 25, 2015

shaun-edwards commented Jun 25, 2015

shaun-edwards commented Jun 25, 2015 •

edited by gavanderhoorn

Loading

shaun-edwards commented Jun 25, 2015 •

edited by gavanderhoorn

Loading