Skip to content

Tf2 republisher service #144

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

Merged
merged 13 commits into from
Dec 11, 2014
Merged

Conversation

T045T
Copy link
Contributor

@T045T T045T commented Dec 8, 2014

Change TFClient to use the new version of tf2_web_republisher (see RobotWebTools/tf2_web_republisher#15 and RobotWebTools/tf2_web_republisher#16 )

Travis job will fail until the new tf2_web_republisher is propagated to the apt sources. Sorry about that.

@rctoris
Copy link
Contributor

rctoris commented Dec 8, 2014

What about backwards compatibility? My concern is with Groovy clients (e.g., many PR2 users). Groovy build farms are no longer running so the new tf2 cannot be passed to Groovy. This would effectively make RWT Hydro+ from here on.

@T045T
Copy link
Contributor Author

T045T commented Dec 8, 2014

Oh, I hadn't thought of that. Would it be better to make this a separate TFClient2 or something then?

Or, more conveniently, check the "/rosversion" parameter to decide which TFClient implementation to use?

@rctoris
Copy link
Contributor

rctoris commented Dec 10, 2014

Is it possible to keep both methods in TF2 and have roslib default to the old one and have a flag for the service based method?

@T045T
Copy link
Contributor Author

T045T commented Dec 10, 2014

👍
I'm thinking of giving the ros object a new constructor option like groovy_compatibility that defaults to true. TFClient could then query its rosfor that and decide based on it.

By "keep both methods in TF2", do you mean keeping the action interface in tf2_web_republisher too?

@rctoris
Copy link
Contributor

rctoris commented Dec 10, 2014

Yeah

@T045T
Copy link
Contributor Author

T045T commented Dec 10, 2014

Will do that tonight. Is it okay to keep the old action implementation as tf2_web_republisher and call the new one tf2_web_republisher_service?

@rctoris
Copy link
Contributor

rctoris commented Dec 10, 2014

I think they should be in the same node, just different interfaces into it. Seems like a lot of reusable code between the two interfaces.

@T045T
Copy link
Contributor Author

T045T commented Dec 11, 2014

Alright, the current version passes the travis tests, but also works with the new Service interface if ros.groovyCompatibility is false (the option defaults to true to preserve backward compatibility).

goalMessage : goalMessage
});

this.currentGoal.on('feedback', this.processTFArray.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to bind here, EventEmitter will call in the context of this

@rctoris
Copy link
Contributor

rctoris commented Dec 11, 2014

Merge conflict?

@rctoris
Copy link
Contributor

rctoris commented Dec 11, 2014

Also, #144 should now let the tests pass once the merge conflict is fixed, it installs tf2 republisher locally from source until it is released.

@T045T T045T merged commit 77cccd9 into RobotWebTools:develop Dec 11, 2014
T045T added a commit that referenced this pull request Dec 11, 2014
this.updateDelay = options.updateDelay || 50;
var seconds = options.topicTimeout || 2.0;
var secs = Math.floor(seconds);
var nsecs = Math.floor((seconds - secs) * 1000000000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should have a helper for doing this, I've been mixing it in quite frequently

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.

3 participants