Skip to content
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

driver: use default rates for JTC goal monitor rates. #221

Merged

Conversation

gavanderhoorn
Copy link
Contributor

As per subject.

See the commit comment for some rationale.

Note: the state_publish_rate has been left to be equal to the loop_hz, as this controls how often the internal (position/velocity/effort) controller of the JTC publishes its state (ie: tracking error, target position, etc). That is something which should be published at the same rate the controller is run to actually be useful.

@gavanderhoorn
Copy link
Contributor Author

This is for #151.

And for wrid20.

@fmauch
Copy link
Contributor

fmauch commented Jul 7, 2020

I am a bit confused. I though you were talking about state_publish_rate all the time. The action_monitor_rate never was at loop_hz but at 10 Hz as far as I can see.

If I read the diff correct, this PR basically increases the action_monitor_rate from 10 to 20 Hz. Do I get something wrong here?

@gavanderhoorn
Copy link
Contributor Author

No. That's what I did.

Please see the commit comment.

It should clarify things.

It doesn't make sense to lower state_publish_rate actually.

It does make sense to increase action_monitor_rate.

So that's what I did.

@fmauch
Copy link
Contributor

fmauch commented Jul 7, 2020

No. That's what I did.

Please see the commit comment.

I did, that's part of my confusion

These were set to run at the 'loop rate' of the controller, which would be 125 Hz and 500 Hz for the CB3 and e-Series respectively.

As these rates are mostly concerned with publishing the state of the goal, not the internal state of the controller, and the goal state is mostly concerned with whether a trajectory has completed execution, was cancelled or preempted, running this at 500 hz was deemed a bit too much.

That's the part I don't understand. Why were they running at loop_hz before making those changes?

It doesn't make sense to lower state_publish_rate actually.

I agree :-)

It does make sense to increase action_monitor_rate.

I agree :-)

So that's what I did.

I'm fine with merging the result.

@gavanderhoorn
Copy link
Contributor Author

Why were they running at loop_hz before making those changes?

Yes, that is confusing. I wrote that comment when I still had a change in there for state_publish_rate.

I'll update the commit comment.

The old values overrode the default of 20 Hz, which is low and leads to a worst-case delay of approx 100 ms between a goal state change and action clients being notified of that change.

This restores the rate to the default of 20 Hz.

If a higher update-rate would be desirable for a particular application, users should change it in their own configuration of the controllers.
@gavanderhoorn gavanderhoorn force-pushed the lower_action_server_pub_rate branch from d9be441 to 06469a4 Compare July 7, 2020 13:32
@fmauch
Copy link
Contributor

fmauch commented Jul 7, 2020

@gavanderhoorn Thank you, this is more understandable :-)

@gavanderhoorn
Copy link
Contributor Author

"Fixed"

branch name is still a bit weird, but I'll leave it like this.

Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Thanks for cleaning that up @gavanderhoorn

@fmauch fmauch merged commit 3ce3621 into UniversalRobots:master Jul 7, 2020
@gavanderhoorn gavanderhoorn deleted the lower_action_server_pub_rate branch July 7, 2020 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants