iocchi / PetriNetPlans

Petri Net Plans library and applications
31 stars 15 forks source link

Adding the option of addressing failing action during plan generation #12

Closed cdondrup closed 8 years ago

cdondrup commented 8 years ago

Adding the option of dealing with failing actions during PNP generation via execution rules.

Given the plan goto_forward|5; say_hello|2 and the execution rules:

*if* action_failed *during* goto *do* restart_action
*if* robot_dist_far *during* say *do* restart_action

It now creates this plan:

pnp5

Therefore, when the action reports failure upon completion the recovery is triggered. This reserves the term action_failed for conditions to mark which recovery should be executed when the action fails.

iocchi commented 8 years ago

Hi, I still do not see why all this is needed. You can use an explicit condition and the standard way of generating interrupts in this way.

1) define an ER with a condition ("action_failed" or better "_failed") with the standard way of generating interrupts.

2) generate a condition "action_failed" when the action fails and send it to PNP engine that will execute the transition. This can be done in two ways: 2.1) the action itself sends the condition 2.2) the PNPActionServer sends the condition when it receives notification from the action that has failed. Notice that sending a condition to PNP engine just requires to send a string to the topic PNPConditionEvent

Anyway I am not against your update, so I am ready to merge this, if you confirm that it is actually what do you need and that it is fully working for you.

Thank you.

iocchi commented 8 years ago

P.S. I think that this will not work anyway because the semantics of the failure in PNP engine is still linked to a condition. I am afraid that in your example above goto_forward.fail is an always enabled transition and thus it will fire immediately when the token reaches the exec place. Thus it will not give you the expected behaviors: since the transition fail is not linked to the actual failure of the action according to the ROS action lib.

This feature has been designed to be used in this way (if I am not wrong): You use "action.fail [condition]" in the PNP and when the condition becomes true then the procedure fail of the action is activated and the transition fires.

To implement the other behavior, modifications in the PNP engine are necessary and I am afraid they are quite complex and it would take more time.

cdondrup commented 8 years ago

Hi I just tried it again and it works for me. As far as I understand action.fail is a reserved keyword like action.end and this bit of code: https://github.com/iocchi/PetriNetPlans/blob/master/PNP/src/basic_plan/xml_plan_instantiator.cpp#L158-L165 generates a guard condition so the transition is only executed if action.failed is true. Same behaviour as with the action.end transition only being triggered when action.finished is true. So it already continuously checks for action.finished() and action.failed() here: https://github.com/iocchi/PetriNetPlans/blob/master/PNP/src/pnp_executable.cpp#L7-L13 anyway. You only have to make sure that action.failed() returns true by overriding this function: https://github.com/iocchi/PetriNetPlans/blob/master/PNP/include/pnp/pnp_executable.h#L75-L81

So all the functionality is already there it is just not used because the plan generation currently doesn't allow for it. I agree that I could also publish on the topic but just overriding the failed() function seemed to be more straight forward and saves an additional communication step.

cdondrup commented 8 years ago

P.S.: That's also why there are no conditions because action.failed is already the implicit guard condition like action.finished for the action.end transition.

cdondrup commented 8 years ago

Correction: You have to override this function: https://github.com/iocchi/PetriNetPlans/blob/master/PNP/include/pnp/pnp_executable.h#L112-L118

Sorry, got the wrong one there ;)

iocchi commented 8 years ago

Ok good. I'll check and merge everything later. Il giorno 05/ago/2016 16:30, "Christian Dondrup" notifications@github.com ha scritto:

Correction: You have to override this function: https://github.com/iocchi/ PetriNetPlans/blob/master/PNP/include/pnp/pnp_executable.h#L112-L118

Sorry, got the wrong one there ;)

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/iocchi/PetriNetPlans/pull/12#issuecomment-237865796, or mute the thread https://github.com/notifications/unsubscribe-auth/AC7HF-OpVW7uBrmW0SKzYyX5SPDIsXWYks5qc0j5gaJpZM4Jdrro .

cdondrup commented 8 years ago

Thank you.

cdondrup commented 8 years ago

For some reason, it only publishes this:

data: init;
---
data: wait_5.exec;goto_forward.exec;

where the goto action fails so there should be fail afterwards because I can see it being the active place in the Jarp gui. It also publishes a lot of data: '' but it does not publish all the transitional places like X42 or the fail place. I'll investigate.

Edit: This comment was meant for #9 Got the wrong browser window. Sorry.

iocchi commented 8 years ago

it was another item in the todo... now it is fixed.

Thank you