team-vigir / flexbe_behavior_engine

Contains the behavior engine FlexBE.
https://flexbe.github.io
BSD 3-Clause "New" or "Revised" License
131 stars 73 forks source link

Wishlist #93

Open LoyVanBeek opened 4 years ago

LoyVanBeek commented 4 years ago

Recently I started collecting a list of things I'd really like to have in FlexBE. This was a private list but why not make it public? Most are GUI-related and I don't have much experience there to make patches with.

BTW: FlexBE is really nice as it is and I recommend it to whoever will listen :-)

pschillinger commented 4 years ago

Thank you for recommending and thanks for sharing your wishlist. With Christmas approaching, this is the perfect time to do so. ;)

In general, I share many of these items with you. Still, for some of them, I'm not quite sure how to best realize them conceptually. So let me share my thoughts on each of them:

Happy to discuss all of these points further.

LoyVanBeek commented 4 years ago

Thanks for the response & tips, I'll try rosrun flexbe_app run_app -o. I've edited my original post for clarity on the progress thing.

As for breakpoints: something dynamic would be desirable, so they can be changed/added/removed at runtime. Just checking if the state's name is in a list on the paramserver would enough indeed.

pschillinger commented 4 years ago

Regarding subclassing EventState, I drafted an alternative state parser for the Flexbe App: It uses a Python subprocess to import and analyse the files to check whether it contains a state definition. This way, I can rely on Python to tell whether a class is a state class. In fact, I can extract all required information this way and would be able to fully replace the previous state parser.

However, the new parser is significantly slower because JavaScript takes some time for its subprocess stuff. For me, this means now around 15s of parsing time instead of <1s, required on each start. Also, using Python means now that all files of state packages are loaded (i.e., executed) in Python and states are half-way instantiated to determine their outcomes and userdata keys. One consequence is that states with import errors are not listed anymore. Not sure if all of the above is desirable... What do you think?

LoyVanBeek commented 4 years ago

Thanks for doing this!

I have to admit, the 15 second penalty is tough! I'm highly doubting if other users think the subclassing worth that. Neither is not seeing them in case of failed imports.

But: parsing in Python would maybe allow for another feature that has popped up. Namely: allowing CI to do the 'Check behavior' thing. I'm not deep enough into the code ATM to verify that that would help though. A colleague of mine is looking into that as well.

How come the python parsing is so slow?

pschillinger commented 4 years ago

It is not the parsing that is slow, it appears to solely be the overhead introduced by starting the subprocesses. If I let Python return immediately, I get about the same execution time as when actually parsing.

If I have time this weekend, I will try a slightly more advanced implementation of using a single, central Python process instead of separate calls. This is a bit more involved because it requires to manage the parallel calls of the different packages, but should remove most of the previous overhead. In case this is successful, I will integrate it and add an option to the settings for switching between the two parsers so that we can try it in practice.

pschillinger commented 4 years ago

I added the new Python parser as alternative, you can switch it in the settings. It now appears to have roughly the same execution time and warnings are printed whenever a state cannot be loaded. Let me know in case you face any problems.

pschillinger commented 4 years ago

FYI:

Run 'Check behavior' from external process to aid CI

-> See FlexBE/flexbe_app#55:

xvfb-run rosrun flexbe_app run_app --offline --check-behaviors
catkin run_tests flexbe_app
LoyVanBeek commented 4 years ago

Thanks! I've also gotten around to trying the breakpoints and behavior tests and finally trying out subclassing and cleaning up some code is on my TODO list as well.

AravindaDP commented 4 years ago

I would like to chime in and add couple of generic states assuming this is correct place to do so.

If the proposal for these are too generic let me know. Just thinking out loud

Generic Publisher/Subscriber State (At least for common set of message types including std_msgs primitive types, Pose variants, Twist etc) -- topic string topic name -- msg_type string message type

> data {msg_type} input value (In case publisher or output value in case of subscriber. Assumption userdata.data is of type given by msg_type)

<= done

this will load type by reflection of input parameter msg_type. (or from a prepopulated type dictionary)

Generic Service Client State (At least for common set of service calls (std_srv bool/empty/string etc)) Similar to above with the exception of request as input key and response as output key

Generic Simple Action Client State (At least for common set of action calls move base actions/moveit actions etc)

AravindaDP commented 4 years ago

I also believe ability to play/capture topics (Where state would publish or subscribe to) during state testing is benifitial. This might be facilitated by either allowing playing bag file in parallel to test (in case of state subscribe to topics) http://wiki.ros.org/flexbe/Tutorials/Writing%20State%20Tests%20Using%20flexbe_testing#Using_Bag_Files

For the case of state publishing to a topic we need additional parameter for specifying output topics in test configuration yaml file and then comparing against an array of message values/dat bag file content messages (Note that in case of publishing there could be multiple messages in contrast to input/output of a user data where only a single value need to be compared)

Here also we might need to restrict support to predefined set of data types. We might extend this to external service call/action call mocking as well but I guess that would be a lot of stretch work.