https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #16 from sander <sander [dot] vandenbroucke [..] ...> 2009-06-18 15:26:17 ---
(In reply to comment #15)
> (In reply to comment #13)
> > (In reply to comment #11)
> > > That's the whole point: Activity instances are used by one and only one
> > > ExecutionEngine, which is in turn used by one and only one TaskContext.
> > > So there is *no* sharing whatsoever.
> > Two times wrong from my use case:
> > - An Activity _is_ used by more tha one executionEngine.
> > - An ExecutionEngine _is_ used by more than one TaskContext.
> > If we can't do this in RTT 2.0 then we will not use it _ever_. Sharing
> > resources is something we must do.
> Could you tell more about this sharing of yours ? All I know of the current
> Activity/ExecutionEngine design is that it handles only one taskcontext. So I
> would be glad to see what you are doing with those objects ...
> Sylvain
To share a EE:
ee->addChild(tc);

You are right on the Activity, it runs only one EE. Is not realy the Activity
that is shared, it is the underlying thread.

Sander.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #15 from Sylvain Joyeux <sylvain [dot] joyeux [..] ...> 2009-06-18 15:17:17 ---
(In reply to comment #13)
> (In reply to comment #11)
> > That's the whole point: Activity instances are used by one and only one
> > ExecutionEngine, which is in turn used by one and only one TaskContext.
> > So there is *no* sharing whatsoever.
> Two times wrong from my use case:
> - An Activity _is_ used by more tha one executionEngine.
> - An ExecutionEngine _is_ used by more than one TaskContext.
> If we can't do this in RTT 2.0 then we will not use it _ever_. Sharing
> resources is something we must do.
Could you tell more about this sharing of yours ? All I know of the current
Activity/ExecutionEngine design is that it handles only one taskcontext. So I
would be glad to see what you are doing with those objects ...

Sylvain

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #14 from sander <sander [dot] vandenbroucke [..] ...> 2009-06-18 15:04:29 ---
(In reply to comment #12)
> So to summarise:
> RTT 1.x : earlier proposed quick-fix in getActivity(), we all acknowledge that
> I screwed up, but at least we keep/fix backwards compatibility.
> RTT 2.x : TaskContext::set/getActvivity just forwards to EE and EE owns that
> pointer. In no case should the user pass a statically allocated object to
> setActivity.
Use a shared_ptr for that?

> The remaining 'danger' is that the user stores the ActivityInterface somewhere,
> and that the Activity is deleted by a reconfiguration of the EE. Maybe we
> should hide the Activity completely and expose the relevant functions in the
> TaskContext API which forward to EE and making TC a friend class of EE. In that
> case, getActivity() would no longer exist and people would have getPeriod()
> etc.
Fine by me. You could also remove setActivity() and simply attach a TaskContext
to an ExecutionEngine.

Sander.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #13 from sander <sander [dot] vandenbroucke [..] ...> 2009-06-18 14:59:54 ---
(In reply to comment #11)
> That's the whole point: Activity instances are used by one and only one
> ExecutionEngine, which is in turn used by one and only one TaskContext.
> So there is *no* sharing whatsoever.
Two times wrong from my use case:
- An Activity _is_ used by more tha one executionEngine.
- An ExecutionEngine _is_ used by more than one TaskContext.
If we can't do this in RTT 2.0 then we will not use it _ever_. Sharing
resources is something we must do.

Anyway, how are you sure a component never ever gives its activity pointer to
an other object?

> Now that I think of it, one other (also temporary!) solution would be to just
> add to ExecutionEngine::setActivity a flag telling if the ownership is passed
> to ExecutionEngine. If true, ExecutionEngine would delete the pointer in its
> destructor.
That is what shared_ptr's are there for. Why reinventing the wheel?

Sander.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #12 from Peter Soetens <peter [..] ...> 2009-06-18 14:46:22 ---
So to summarise:

RTT 1.x : earlier proposed quick-fix in getActivity(), we all acknowledge that
I screwed up, but at least we keep/fix backwards compatibility.

RTT 2.x : TaskContext::set/getActvivity just forwards to EE and EE owns that
pointer. In no case should the user pass a statically allocated object to
setActivity.

The remaining 'danger' is that the user stores the ActivityInterface somewhere,
and that the Activity is deleted by a reconfiguration of the EE. Maybe we
should hide the Activity completely and expose the relevant functions in the
TaskContext API which forward to EE and making TC a friend class of EE. In that
case, getActivity() would no longer exist and people would have getPeriod()
etc.

I'm lost.

Peter

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #11 from Sylvain Joyeux <sylvain [dot] joyeux [..] ...> 2009-06-18 14:20:47 ---
That's the whole point: Activity instances are used by one and only one
ExecutionEngine, which is in turn used by one and only one TaskContext.

So there is *no* sharing whatsoever.

The issue is that until now, the Activity instance was owned by the user of the
taskcontext/executionengine. Because Peter wanted to add to the 1.x branch
(i.e. without breaking backward compatibility) the ability to have a default
Activity instance -- which has to be owned by TaskContext or ExecutionEngine --
he had to rely on this scheme where
- TaskContext uses a shared_ptr and owns the activity set by
TaskContext::setActivity
- ExecutionEngine still does not own its activity

Given that (I repeat ...)
Activity instances are used by one and only one
ExecutionEngine, which is in turn used by one and only one TaskContext.

The only good design IMO is to make ExecutionEngine own its activity. But that
would be for 2.0

In the meantime, I'd be happy with peter's proposal from two days ago, but only
as a temporary solution.

Now that I think of it, one other (also temporary!) solution would be to just
add to ExecutionEngine::setActivity a flag telling if the ownership is passed
to ExecutionEngine. If true, ExecutionEngine would delete the pointer in its
destructor.

I, for my side, am really worried about having unnecessary use of shared_ptr
(i.e. holding pointers that are actually not shared with shared_ptr). It
usually hides bad design decisions.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

sander <sander [dot] vandenbroucke [..] ...> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |sander.vandenbroucke@vandew
| |iele.be

--- Comment #10 from sander <sander [dot] vandenbroucke [..] ...> 2009-06-18 13:43:25 ---
(In reply to comment #6)
> (In reply to comment #4)
> >
> > No, you don't. You *set* the activity on engine() when TC::setActivity is
> > called. What I meant was simply
> >
> > TaskContext::setActivity(new_activity)
> > {
> > if(new_activity == NULL)
> > new_activity = Sequential;
> > new_act->run(this->engine())
> > }
> > TaskCOntext::getActivity()
> > { return this->engine()->getActivity(); }
> >
> > our_act should just be a duplicate of engine()->getActivity(). The fact that
> > this duplicate exists leads to the behaviour I mentionned (i.e. having the
> > getActivity() return an activity that is actually *not* the one which is
> > currently supporting the TC). And I don't see the purpose of that duplicate.
> I'm taking a look at this again. I screwed up the API big time. Since
> TaskContext::getActivity() returns a shared_ptr and not a plain pointer, it's
> not possible to put your code in place unless we change back to plain pointers.
> So what I propose is to change API back to (with our_act still being the
> shared_ptr):
>

>     void TaskContext::setActivity(RTT::ActivityInterface* new_act)
>     {
>         if (this->isActive())
>             return;
>         if ( new_act == 0) {
>             new_act = new SequentialActivity();
>         }
>         new_act->run( this->engine() );
>         our_act = ActivityInterface::shared_ptr( new_act );
>     }
>     ActivityInterface* TaskContext::getActivity()
>     {
>         if (this->engine()->getActivity() != our_act.get() )
>             return this->engine()->getActivity();
>         return our_act.get();
>     }
> 

Why don't you use a shared_ptr in the setActivity()/getActivity()? What if, for
some reason, new_act get deleted by the one that created it? getActivity() will
return a pointer to a deleted peice of memory.

Personaly I don't feel all that confortable with sharing plain pointers...

Sander.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #9 from Sylvain Joyeux <sylvain [dot] joyeux [..] ...> 2009-06-18 12:34:26 ---
> > Note that, anyway, by adding a shared_ptr you introduce an API change:
> > Activity cannot be held on the stack anymore, because shared_ptr will
> > call delete() on it.
>
> Exactly, that's why only activities added with TaskContext::setActivity
> were subject to this.
... and why you have a mixup between two ways of achieving the same thing.

Now:
- if you want to keep it on the 1.x branch, then just do what you suggested.
This is ugly, but can be a bandaid for now.
- please, *please* fix if for 2.x. I don't see the point of breaking BC if it
is not to fix those weird designs.

> For RTT 2.0 we could (try to) enforce
>

> mytc->setActivity( new Activity(args) );
> 

mytc->setActivity( activity );

and

execution_engine->setActivity( activity );

to have *exactly the same effect*. Why ? Because if some code wants to
manipulate execution engines, without caring about tasks *it should be able
to*.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #8 from Peter Soetens <peter [..] ...> 2009-06-18 12:24:38 ---
(In reply to comment #7)
> You actually never did explain the profound reason why you so desesperatly want
> to have a shared_ptr on an activity inside TaskContext, instead of leaving the
> whole thing to the ExecutionEngine.

It's easy :-) Backwards compatibility. The EE can't own the activity, because
it never did and many/some people used statically allocated activity objects or
delete them theirselves. The only way out was then to add a method
'setActivity' to the TC interface which had transfer-of-ownership semantics.

>
> As I gather, the only use case for this is the ability to let the user manage
> its Activity (i.e. destroy them) while being able to create a default activity.
> The problem I see is that it makes the whole design overly complex (two
> activity pointers, use one or maybe the other, it depends, who knows).
>
> There is my proposal:
> - dropping our_act. Having two activity pointers makes the whole thing way too
> complex.
> - keeping the getActivity()/setActivity() methods in TaskContext, they
> make a nicer interface for people "not in the know"
> - either making ExecutionEngine's holding of an Acitvity a shared_ptr
> - or changing the API, stating that ExecutionEngine destroys the
> Activity it points to.

I can't do this in the 1.x branch.

>
> Note that, anyway, by adding a shared_ptr you introduce an API change: Activity
> cannot be held on the stack anymore, because shared_ptr will call delete() on
> it.

Exactly, that's why only activities added with TaskContext::setActivity were
subject to this.

>
> As far as I understood, there is a one-to-one mapping between activities, tasks
> and execution engines (i.e. Activity objects are not shared). If this is the
> case, I would personally prefer the last solution. Otherwise, there is a need
> to have a shared_ptr, so the first solution is better.

You are correct about the 1:1 mapping, but for BC reasons, I can not change the
ownership semantics that quickly.

For RTT 2.0 we could (try to) enforce

mytc->setActivity( new Activity(args) );

Peter

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #7 from Sylvain Joyeux <sylvain [dot] joyeux [..] ...> 2009-06-17 17:05:39 ---
You actually never did explain the profound reason why you so desesperatly want
to have a shared_ptr on an activity inside TaskContext, instead of leaving the
whole thing to the ExecutionEngine.

As I gather, the only use case for this is the ability to let the user manage
its Activity (i.e. destroy them) while being able to create a default activity.
The problem I see is that it makes the whole design overly complex (two
activity pointers, use one or maybe the other, it depends, who knows).

There is my proposal:
- dropping our_act. Having two activity pointers makes the whole thing way too
complex.
- keeping the getActivity()/setActivity() methods in TaskContext, they
make a nicer interface for people "not in the know"
- either making ExecutionEngine's holding of an Acitvity a shared_ptr
- or changing the API, stating that ExecutionEngine destroys the
Activity it points to.

Note that, anyway, by adding a shared_ptr you introduce an API change: Activity
cannot be held on the stack anymore, because shared_ptr will call delete() on
it.

As far as I understood, there is a one-to-one mapping between activities, tasks
and execution engines (i.e. Activity objects are not shared). If this is the
case, I would personally prefer the last solution. Otherwise, there is a need
to have a shared_ptr, so the first solution is better.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

Peter Soetens <peter [..] ...> changed:

What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED

--- Comment #6 from Peter Soetens <peter [..] ...> 2009-06-17 16:27:05 ---
(In reply to comment #4)
>
> No, you don't. You *set* the activity on engine() when TC::setActivity is
> called. What I meant was simply
>
> TaskContext::setActivity(new_activity)
> {
> if(new_activity == NULL)
> new_activity = Sequential;
> new_act->run(this->engine())
> }
> TaskCOntext::getActivity()
> { return this->engine()->getActivity(); }
>
> our_act should just be a duplicate of engine()->getActivity(). The fact that
> this duplicate exists leads to the behaviour I mentionned (i.e. having the
> getActivity() return an activity that is actually *not* the one which is
> currently supporting the TC). And I don't see the purpose of that duplicate.

I'm taking a look at this again. I screwed up the API big time. Since
TaskContext::getActivity() returns a shared_ptr and not a plain pointer, it's
not possible to put your code in place unless we change back to plain pointers.

So what I propose is to change API back to (with our_act still being the
shared_ptr):

    void TaskContext::setActivity(RTT::ActivityInterface* new_act)
    {
        if (this->isActive())
            return;
        if ( new_act == 0) {
            new_act = new SequentialActivity();
        }
        new_act->run( this->engine() );
        our_act = ActivityInterface::shared_ptr( new_act );
    }
 
    ActivityInterface* TaskContext::getActivity()
    {
        if (this->engine()->getActivity() != our_act.get() )
            return this->engine()->getActivity();
        return our_act.get();
    }

But still strongly advise users to use tc->setActivity( act ) instead of
tc->engine()->setActivity( act ) (unless they want to own the activity or it is
allocated on the stack)

Thoughts ?

Peter

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #5 from Peter Soetens <peter [..] ...> 2009-05-07 23:14:35 ---
(In reply to comment #4)
> > > > You should be able to use something similar to:
> > > >
> > > > tc->setActivity( new PeriodicActivity(0, 0.5) );
> > > >
> > > > Additionally, old-style is still supported:
> > > >
> > > > TaskContext tc("tc");
> > > > PeriodicActivity act(0,0.5, tc.engine() );
> > > >
> > > > In the first case, the tc owns the activity object, in the second
> > > > case, the activity object is not owned by the tc.
> > > That does not make sense. You can do
> > > tc->setActivity( new PeriodicActivity(0, 0.5))
> > >
> > > and *then* change the activity on the engine object, in which
> > > case the TC "thinks" it is run by one activity while actually
> > > it is run by another. Having the same information in two places
> > > is evil.
> >
> > It is, but in that case it's a bug in user code. The user assigned an
> > activity in two places, to two different things. But even here I wonder
> > why that case is not covered, and at least one of both is picked consistently.
>
> What ?
>
> Both the TC and its engine are supported by one single activity. It is not sane
> that it is possible to have the TC report that one activity runs it while the
> "true" underlying activity is actually another. Given that just by having the
> engine() storing the activity and the TC returning engine()->getActivity() you
> actually fix that with no added complexity, I really don't see the point.

OK. Letting it return engine()->getActivity() is necessary to support your
usage model. So that must be fixed for sure.

>
> Why do you want to have one activity stored in TC ? What is the added value of
> that scheme ?

That you can ask the TC to own the activity object, just like it owns its own
engine. Since it's a one-to-one relation, this makes sense. You 'give' an
activity implementation to a TC and then don't care about owning that object.

> > But we do that, be saying new_act->run( this->engine() ):
>
> No, you don't. You *set* the activity on engine() when TC::setActivity is
> called. What I meant was simply
>
> TaskContext::setActivity(new_activity)
> {
> if(new_activity == NULL)
> new_activity = Sequential;
> new_act->run(this->engine())
> }
> TaskCOntext::getActivity()
> { return this->engine()->getActivity(); }
>
> our_act should just be a duplicate of engine()->getActivity(). The fact that
> this duplicate exists leads to the behaviour I mentionned (i.e. having the
> getActivity() return an activity that is actually *not* the one which is
> currently supporting the TC). And I don't see the purpose of that duplicate.

I agree with the getActivity() change, but conceptually, I don't want the user
to do my_act->start(). I want him to call my_tc->start(). Just like the
engine(), the activity object should be largely hidden to the user, once
plugged into the component, and only used indirectly.

Peter

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #5 from Peter Soetens <peter [..] ...> 2009-05-07 23:14:35 ---
(In reply to comment #4)
> > > > You should be able to use something similar to:
> > > >
> > > > tc->setActivity( new PeriodicActivity(0, 0.5) );
> > > >
> > > > Additionally, old-style is still supported:
> > > >
> > > > TaskContext tc("tc");
> > > > PeriodicActivity act(0,0.5, tc.engine() );
> > > >
> > > > In the first case, the tc owns the activity object, in the second
> > > > case, the activity object is not owned by the tc.
> > > That does not make sense. You can do
> > > tc->setActivity( new PeriodicActivity(0, 0.5))
> > >
> > > and *then* change the activity on the engine object, in which
> > > case the TC "thinks" it is run by one activity while actually
> > > it is run by another. Having the same information in two places
> > > is evil.
> >
> > It is, but in that case it's a bug in user code. The user assigned an
> > activity in two places, to two different things. But even here I wonder
> > why that case is not covered, and at least one of both is picked consistently.
>
> What ?
>
> Both the TC and its engine are supported by one single activity. It is not sane
> that it is possible to have the TC report that one activity runs it while the
> "true" underlying activity is actually another. Given that just by having the
> engine() storing the activity and the TC returning engine()->getActivity() you
> actually fix that with no added complexity, I really don't see the point.

OK. Letting it return engine()->getActivity() is necessary to support your
usage model. So that must be fixed for sure.

>
> Why do you want to have one activity stored in TC ? What is the added value of
> that scheme ?

That you can ask the TC to own the activity object, just like it owns its own
engine. Since it's a one-to-one relation, this makes sense. You 'give' an
activity implementation to a TC and then don't care about owning that object.

> > But we do that, be saying new_act->run( this->engine() ):
>
> No, you don't. You *set* the activity on engine() when TC::setActivity is
> called. What I meant was simply
>
> TaskContext::setActivity(new_activity)
> {
> if(new_activity == NULL)
> new_activity = Sequential;
> new_act->run(this->engine())
> }
> TaskCOntext::getActivity()
> { return this->engine()->getActivity(); }
>
> our_act should just be a duplicate of engine()->getActivity(). The fact that
> this duplicate exists leads to the behaviour I mentionned (i.e. having the
> getActivity() return an activity that is actually *not* the one which is
> currently supporting the TC). And I don't see the purpose of that duplicate.

I agree with the getActivity() change, but conceptually, I don't want the user
to do my_act->start(). I want him to call my_tc->start(). Just like the
engine(), the activity object should be largely hidden to the user, once
plugged into the component, and only used indirectly.

Peter

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #4 from Sylvain Joyeux <sylvain [dot] joyeux [..] ...> 2009-05-07 22:44:56 ---
> > > You should be able to use something similar to:
> > >
> > > tc->setActivity( new PeriodicActivity(0, 0.5) );
> > >
> > > Additionally, old-style is still supported:
> > >
> > > TaskContext tc("tc");
> > > PeriodicActivity act(0,0.5, tc.engine() );
> > >
> > > In the first case, the tc owns the activity object, in the second
> > > case, the activity object is not owned by the tc.
> > That does not make sense. You can do
> > tc->setActivity( new PeriodicActivity(0, 0.5))
> >
> > and *then* change the activity on the engine object, in which
> > case the TC "thinks" it is run by one activity while actually
> > it is run by another. Having the same information in two places
> > is evil.
>
> It is, but in that case it's a bug in user code. The user assigned an
> activity in two places, to two different things. But even here I wonder
> why that case is not covered, and at least one of both is picked consistently.

What ?

Both the TC and its engine are supported by one single activity. It is not sane
that it is possible to have the TC report that one activity runs it while the
"true" underlying activity is actually another. Given that just by having the
engine() storing the activity and the TC returning engine()->getActivity() you
actually fix that with no added complexity, I really don't see the point.

Why do you want to have one activity stored in TC ? What is the added value of
that scheme ?

> > Same issue. If you use the "old-style", then tc->getActivity() returns
> > NULL, which is not the reality. Reality is, it *is* run by a periodic
> > activity.
>
> What you claim is false (or I don't understand it). This is the code in TC:

What I claimed *is* false ;-). What happened in my code was that I was
expecting the activity to be of a certain type (in a unit test), was
dynamic_cast<> it and got NULL in return.

> > > > I actually don't see why there is a need to store an activity in the
> > > > TaskContext. Should not it simply use engine()->setActivity() and
> > > > engine()->getActivity() instead ? That would (probably) fix the issue.
> > >
> > > 1. I want to hide the engine from the 'user' API.
> > You can implement ::setActivity and ::getActivity in TC, simply forwarding
> > the call to engine(). Then, user's don't see the engine (which is a good
> > point), but there is no consistency issues.
>
> But we do that, be saying new_act->run( this->engine() ):

No, you don't. You *set* the activity on engine() when TC::setActivity is
called. What I meant was simply

TaskContext::setActivity(new_activity)
{
if(new_activity == NULL)
new_activity = Sequential;
new_act->run(this->engine())
}
TaskCOntext::getActivity()
{ return this->engine()->getActivity(); }

our_act should just be a duplicate of engine()->getActivity(). The fact that
this duplicate exists leads to the behaviour I mentionned (i.e. having the
getActivity() return an activity that is actually *not* the one which is
currently supporting the TC). And I don't see the purpose of that duplicate.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #3 from Peter Soetens <peter [..] ...> 2009-05-07 22:17:24 ---
(In reply to comment #2)
> > That's not true. Are you sure that you ported your own activity
> > classes to the new activity style ? Some functions were implemented
> > in the base classes (like run() ) maybe you overlooked that ?
> What is old and what is new ? In my activities, I simply implement loop() and
> breakLoop(), so I don't think that's my problem.

Ok then.

>
> > You should be able to use something similar to:
> >
> > tc->setActivity( new PeriodicActivity(0, 0.5) );
> >
> > Additionally, old-style is still supported:
> >
> > TaskContext tc("tc");
> > PeriodicActivity act(0,0.5, tc.engine() );
> >
> > In the first case, the tc owns the activity object, in the second
> > case, the activity object is not owned by the tc.
> That does not make sense. You can do
> tc->setActivity( new PeriodicActivity(0, 0.5))
>
> and *then* change the activity on the engine object, in which
> case the TC "thinks" it is run by one activity while actually
> it is run by another. Having the same information in two places
> is evil.

It is, but in that case it's a bug in user code. The user assigned an
activity in two places, to two different things. But even here I wonder
why that case is not covered, and at least one of both is picked consistently.

>
> Same issue. If you use the "old-style", then tc->getActivity() returns
> NULL, which is not the reality. Reality is, it *is* run by a periodic
> activity.

What you claim is false (or I don't understand it). This is the code in TC:

   void TaskContext::setActivity(RTT::ActivityInterface* new_act)
    {
        if (this->isActive())
            return;
        if ( new_act == 0) {
            new_act = new SequentialActivity();
        }
        new_act->run( this->engine() );
        our_act = ActivityInterface::shared_ptr( new_act );
    }
 
    ActivityInterface::shared_ptr TaskContext::getActivity()
    {
        return our_act;
    }

How can our_act ever be NULL ?

>
> > > I actually don't see why there is a need to store an activity in the
> > > TaskContext. Should not it simply use engine()->setActivity() and
> > > engine()->getActivity() instead ? That would (probably) fix the issue.
> >
> > 1. I want to hide the engine from the 'user' API.
> You can implement ::setActivity and ::getActivity in TC, simply forwarding
> the call to engine(). Then, user's don't see the engine (which is a good
> point), but there is no consistency issues.

But we do that, be saying new_act->run( this->engine() ):

>
> > 2. Having the tc manage the activity (and not user code)
> > makes a lot of sense, especially in the mantra of ' A TC always has
> > an activity '. tc->getActivity() should never return null. This means
> > a TC must have an activity when constructed, so it must manage it as
> > well.
> I actually dislike the way you do this. I like the fact that there is one
> default activity assigned at construction time, but the setActivity(NULL)
> that sets the default activity is ugly.

I had two choices in TC::setActivity: or return false, or install a 'sane'(*)
default. I chose the latter to interprete the null as a 'reset to default'.

Peter

(*) We need another thread on orocos-dev about SequentialActivity being a
'sane' default. Davide tags it as 'very advanced' and 'dangerous'. He might be
right.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #3 from Peter Soetens <peter [..] ...> 2009-05-07 22:17:24 ---
(In reply to comment #2)
> > That's not true. Are you sure that you ported your own activity
> > classes to the new activity style ? Some functions were implemented
> > in the base classes (like run() ) maybe you overlooked that ?
> What is old and what is new ? In my activities, I simply implement loop() and
> breakLoop(), so I don't think that's my problem.

Ok then.

>
> > You should be able to use something similar to:
> >
> > tc->setActivity( new PeriodicActivity(0, 0.5) );
> >
> > Additionally, old-style is still supported:
> >
> > TaskContext tc("tc");
> > PeriodicActivity act(0,0.5, tc.engine() );
> >
> > In the first case, the tc owns the activity object, in the second
> > case, the activity object is not owned by the tc.
> That does not make sense. You can do
> tc->setActivity( new PeriodicActivity(0, 0.5))
>
> and *then* change the activity on the engine object, in which
> case the TC "thinks" it is run by one activity while actually
> it is run by another. Having the same information in two places
> is evil.

It is, but in that case it's a bug in user code. The user assigned an
activity in two places, to two different things. But even here I wonder
why that case is not covered, and at least one of both is picked consistently.

>
> Same issue. If you use the "old-style", then tc->getActivity() returns
> NULL, which is not the reality. Reality is, it *is* run by a periodic
> activity.

What you claim is false (or I don't understand it). This is the code in TC:

   void TaskContext::setActivity(RTT::ActivityInterface* new_act)
    {
        if (this->isActive())
            return;
        if ( new_act == 0) {
            new_act = new SequentialActivity();
        }
        new_act->run( this->engine() );
        our_act = ActivityInterface::shared_ptr( new_act );
    }
 
    ActivityInterface::shared_ptr TaskContext::getActivity()
    {
        return our_act;
    }

How can our_act ever be NULL ?

>
> > > I actually don't see why there is a need to store an activity in the
> > > TaskContext. Should not it simply use engine()->setActivity() and
> > > engine()->getActivity() instead ? That would (probably) fix the issue.
> >
> > 1. I want to hide the engine from the 'user' API.
> You can implement ::setActivity and ::getActivity in TC, simply forwarding
> the call to engine(). Then, user's don't see the engine (which is a good
> point), but there is no consistency issues.

But we do that, be saying new_act->run( this->engine() ):

>
> > 2. Having the tc manage the activity (and not user code)
> > makes a lot of sense, especially in the mantra of ' A TC always has
> > an activity '. tc->getActivity() should never return null. This means
> > a TC must have an activity when constructed, so it must manage it as
> > well.
> I actually dislike the way you do this. I like the fact that there is one
> default activity assigned at construction time, but the setActivity(NULL)
> that sets the default activity is ugly.

I had two choices in TC::setActivity: or return false, or install a 'sane'(*)
default. I chose the latter to interprete the null as a 'reset to default'.

Peter

(*) We need another thread on orocos-dev about SequentialActivity being a
'sane' default. Davide tags it as 'very advanced' and 'dangerous'. He might be
right.

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

--- Comment #2 from Sylvain Joyeux <sylvain [dot] joyeux [..] ...> 2009-05-07 13:06:19 ---
> That's not true. Are you sure that you ported your own activity
> classes to the new activity style ? Some functions were implemented
> in the base classes (like run() ) maybe you overlooked that ?
What is old and what is new ? In my activities, I simply implement loop() and
breakLoop(), so I don't think that's my problem.

> You should be able to use something similar to:
>
> tc->setActivity( new PeriodicActivity(0, 0.5) );
>
> Additionally, old-style is still supported:
>
> TaskContext tc("tc");
> PeriodicActivity act(0,0.5, tc.engine() );
>
> In the first case, the tc owns the activity object, in the second
> case, the activity object is not owned by the tc.
That does not make sense. You can do
tc->setActivity( new PeriodicActivity(0, 0.5))

and *then* change the activity on the engine object, in which
case the TC "thinks" it is run by one activity while actually
it is run by another. Having the same information in two places
is evil.

Same issue. If you use the "old-style", then tc->getActivity() returns
NULL, which is not the reality. Reality is, it *is* run by a periodic
activity.

> > I actually don't see why there is a need to store an activity in the
> > TaskContext. Should not it simply use engine()->setActivity() and
> > engine()->getActivity() instead ? That would (probably) fix the issue.
>
> 1. I want to hide the engine from the 'user' API.
You can implement ::setActivity and ::getActivity in TC, simply forwarding
the call to engine(). Then, user's don't see the engine (which is a good
point), but there is no consistency issues.

> 2. Having the tc manage the activity (and not user code)
> makes a lot of sense, especially in the mantra of ' A TC always has
> an activity '. tc->getActivity() should never return null. This means
> a TC must have an activity when constructed, so it must manage it as
> well.
I actually dislike the way you do this. I like the fact that there is one
default activity assigned at construction time, but the setActivity(NULL)
that sets the default activity is ugly.

> 3. It's less writing and it looked more logical
See my remark on 1.

Sylvain

https://www.fmtc.be/bugzilla/orocos/show_bug.cgi?id=650

Peter Soetens <peter [..] ...> changed:

What |Removed |Added
----------------------------------------------------------------------------
CC| |peter [..] ...

--- Comment #1 from Peter Soetens <peter [..] ...> 2009-05-07 11:11:46 ---
(In reply to comment #0)
> The three methods listed in the subject are used to change the
> activity<->runnable interaction. The problem with TaskContext::setActivity as
> it is right now is that it stores an activity object in the task context.
>
> One can therefore have the taskcontext think it is bound to one activity
> while its engine is bound to another. Moreover, I'm experiencing that, to
> successfully bind a task to an activity one needs to
> - give the task engine to the activity's constructor
> - AND call TaskContext::setActivity()
>
> Doing only one or the other will fail.

That's not true. Are you sure that you ported your own activity
classes to the new activity style ? Some functions were implemented
in the base classes (like run() ) maybe you overlooked that ?

You should be able to use something similar to:

tc->setActivity( new PeriodicActivity(0, 0.5) );

Additionally, old-style is still supported:

TaskContext tc("tc");
PeriodicActivity act(0,0.5, tc.engine() );

In the first case, the tc owns the activity object, in the second
case, the activity object is not owned by the tc.

>
> I actually don't see why there is a need to store an activity in the
> TaskContext. Should not it simply use engine()->setActivity() and
> engine()->getActivity() instead ? That would (probably) fix the issue.

1. I want to hide the engine from the 'user' API.
2. Having the tc manage the activity (and not user code)
makes a lot of sense, especially in the mantra of ' A TC always has
an activity '. tc->getActivity() should never return null. This means
a TC must have an activity when constructed, so it must manage it as
well.
3. It's less writing and it looked more logical

Peter