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

Make sure the transformer publisher is starte in regular scripts. #5

Closed
wants to merge 1 commit into from

Conversation

goldhoorn
Copy link
Contributor

We had a few times that the transformer does not work correctly when
bundles are not used. The problem was that the broadcaster did not
get started. This PR automatically starts the broadcaster if this
was not done before.

@goldhoorn
Copy link
Contributor Author

As usual i'm open for better solutions.

@doudou
Copy link
Member

doudou commented Aug 20, 2015

To me, this says: we should all use bundles. Once and for all. This forest of "if we have a bundle" or "if we don't" is beyond annoying.

@goldhoorn
Copy link
Contributor Author

Seriously?
I am not willing and strongly against dropping the lowest layer

@doudou
Copy link
Member

doudou commented Aug 20, 2015

Misread the diff ... the functionality does make sense ... sorry for the noise.

The implementation, on the other hand ... See my comments.

Orocos::Process.run(options) do
@broadcaster = Orocos.name_service.get(name)
yield
end
else
start_time = Time.new
Thread.new do
Copy link
Member

Choose a reason for hiding this comment

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

This is a dangling, never-ending thread. Horrible.

@goldhoorn
Copy link
Contributor Author

I updated the PR, but there is still a problem left, the process which was launced by Orocos.run (the transformer) did not get stopped automatically. If i run a script and ctrl+c the transformer is still running.

Even if i call stop_broadcaster the broadcaster (task) get stopped but the process is still alive.
How can i do a good cleanup here?

@doudou
Copy link
Member

doudou commented Aug 21, 2015

If you want to stop things automatically, use the block version. That's the whole point of having a block construct used that way in Ruby: that whatever-cleanup-is-required is done.

Orocos.run when called without a block returns the set of processes that have been started. Pass them to Orocos.guard the same way than Orocos.run-with-a-block does (see https://github.com/rock-core/tools-orocosrb/blob/master/lib/orocos/process.rb#L829), or check Orocos.guard if you really really want to do it manually.

@goldhoorn
Copy link
Contributor Author

Do i need a thread in this case?
As i see the guard needs a block to be given and the guards itself blocks?!

@doudou
Copy link
Member

doudou commented Aug 21, 2015

Let's walk one step back

The only way you can guarantee cleanup is by having a form

begin
   < do stuff >
ensure
   < do cleanup >
end

which is essentially what Orocos.run with block / Orocos.guard do. Why isn't that good enough for you ?

@goldhoorn
Copy link
Contributor Author

updated, hope this is what you have expected.

You first mentioned that i should use Orocos.run without a block. So i don't need a thread anymore. Then you suggested Orocos.guard for which i need a thread. This is what confuses me.

We had a few times that the transformer does not work correctly when
bundles are not used. The problem was that the broadcaster did not
get started. This PR automatically starts the broadcaster if this
was not done before.
@doudou
Copy link
Member

doudou commented Aug 24, 2015

Why do you NEED a block-less version ?

@doudou
Copy link
Member

doudou commented Aug 24, 2015

(and, you know how much I like threads, no ?)

@doudou
Copy link
Member

doudou commented Aug 24, 2015

OK. This is getting too far.

You want automated "under the scene" start/stop of the transformer broadcaster ? Use Bundles.run.

You want to start the broadcaster manually and get automated stop ? Use the block form of start_broadcaster.

Otherwise, you have to do your own cleanup.

@doudou
Copy link
Member

doudou commented Aug 24, 2015

And all of these are process objects. They are asynchronous. You can kill them by, you know, looking into the Process API and finding out that kill is what Orocos.run and Orocos.guard do !

@goldhoorn
Copy link
Contributor Author

I only want to recover behaviour how it is documented. And still dont know what your solution would be. And yes i still will keepborocos.run support for the Transformers....

@doudou
Copy link
Member

doudou commented Aug 25, 2015

I only want to recover behaviour how it is documented.

That is probably the root of the misunderstanding, then. What "documented behaviour" are you referring to ?

@goldhoorn
Copy link
Contributor Author

Ok so let's restart here

My thougts on this PR

  1. i want to keep the Orocos.run layer fully functional, because i don't want to 'force' people to use bundles.
  2. This page mentions howto setup the transformer with Orocos.run syntax, Vizkit is not mentioned and the broadcaster is fully internal (unknown to me in the first place) This is why i assuming "follow this tutorial, everything is working"
  3. I tried to "hide" the start/setup from the user because i don't want to add more user calls/ i want to make the broadcaster working based on the doc (because i assumed it should work)

regarding Orocos.run <-> Bundles.run:

This is a larger discussion, it's not a orocos.rb thing in the end, it's the way how far plugins should use automate things.

We have two ways:

  1. add a way to start the broadcaster nicely and mention this in the doc
  2. do automation like/similar i proposed

I don't care so much when plugin do "magic" the scope is really limited to the plugin itself, thats why i would say it's not nice but okay here. It we would have a ruby-based deployer this would make it easier and not require such threads.

summary

When you @doudou still want to go for 1. i would like to ask other dev's on the ML for a base-decision if you are fine with that. It's more a design principle at this point. If we go for 1) i don't reject and i will rework this PR and the DOC. But i would prefer 2.

@2maz
Copy link
Member

2maz commented Oct 21, 2015

I would go with 1) in order to have a clear (in the sense of understandable for users) startup of the transformer in orocos scripts.

@goldhoorn
Copy link
Contributor Author

Then i have no idea howto start the broadcaster nicely (without having a large overhead like the functions i propose) but i'm open for solutions...

@doudou
Copy link
Member

doudou commented Oct 21, 2015

What I am again wondering is why the current API bad for you

Orocos.transformer.start_broadcaster do
   Orocos.run or other similar things
end

@doudou doudou closed this Oct 21, 2015
@doudou doudou reopened this Oct 21, 2015
@doudou
Copy link
Member

doudou commented Oct 21, 2015

Wrong button .. sorry

@goldhoorn
Copy link
Contributor Author

Don't know this before...

On 21.10.2015 12:05, Sylvain Joyeux wrote:

|Orocos.transformer.start_broadcaster do
Orocos.run or other similar things
end|

Dipl.-Inf. Matthias Goldhoorn
Space and Underwater Robotic

Universität Bremen
FB 3 - Mathematik und Informatik
AG Robotik
Robert-Hooke-Straße 1
28359 Bremen, Germany

Zentrale: +49 421 178 45-6611

Besuchsadresse der Nebengeschäftstelle:
Robert-Hooke-Straße 5
28359 Bremen, Germany

Tel.: +49 421 178 45-4193
Empfang: +49 421 178 45-6600
Fax: +49 421 178 45-4150
E-Mail: matthias.goldhoorn@informatik.uni-bremen.de

Weitere Informationen: http://www.informatik.uni-bremen.de/robotik

@goldhoorn
Copy link
Contributor Author

I created a Issue on the doc (for now) and close this

@goldhoorn goldhoorn closed this Oct 21, 2015
@doudou doudou deleted the start_broadcaster branch October 21, 2015 15:38
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.

None yet

3 participants