Action goal checker callback (wip)#2118
Conversation
| if (result): | ||
| result = test_RobotGetsBadBehaviorTree(robot_tester) | ||
|
|
||
| time.sleep(5) |
There was a problem hiding this comment.
I had very strange errors here.. I think there exist already a PR/issue somewhere that's about complaining that we cannot "stress test" the bt_navigator with new goals or something.. Might also be a DDS specific error.. Actually seeing quite a few here with the new Callback (but very sporadic.. Once existing, 3 times not. This in general was reported also by others that local tests might fail sometimes and pass on CI..)
| typename nodeT::SharedPtr node, | ||
| const std::string & action_name, | ||
| ExecuteCallback execute_callback, | ||
| GoalCheckerCallback goal_checker_callback = nullptr, |
There was a problem hiding this comment.
as an optional argument, all the existing action servers (Planner, ComposePath, BT_Navigator, ...) do not need to bother about it much. But if interested it can be added. I hope this is performant enough?
Nullptr seems appropriate for an empty std::function<>..?
Codecov Report
@@ Coverage Diff @@
## main #2118 +/- ##
==========================================
- Coverage 83.02% 82.89% -0.14%
==========================================
Files 245 245
Lines 11944 11950 +6
==========================================
- Hits 9917 9906 -11
- Misses 2027 2044 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
Please offer a clear explanation of what this feature is and why it's necessary, I don't see that and I can't easily decipher it from reading the PR code. Also |
There was a problem hiding this comment.
It seems like you want to expose more of the raw action server from the simple action server - and for what you would like to do, there is already an option to handle preemption requests and accept / deny them. I think maybe this didn't come to mind since all of our uses of the simple action server simply accepts blindly all preemption requests, but in the is_preempt_requested blocks, we can chose to accept or not each request.
I think what you're trying to implement can just go inside of the if is_preempt_requested blocks to check if we want to accept it without any action server changes. The action server changes you propose are valid, but also exposes so much of the normal action server that its probably good to just use the normal action server in that situation.
There's definitely nothing wrong with what you propose, but I wonder thematically if that is appropriate for the "simple" action server to do, especially when there's a way to accomplish the same task using the current server structure we have in the BT/control/planner/recovery action server implementations.
| } | ||
|
|
||
| // Only ckeck the goal if a callback was bind | ||
| if (goal_checker_callback_ != nullptr) { |
There was a problem hiding this comment.
https://github.com/ros-planning/navigation2/blob/main/nav2_bt_navigator/src/bt_navigator.cpp#L283 our servers have the ability already to check if a new goal is been requested to handle. This seems like an analog that breaks the "simple" of the simple action server. One of the goals of the simple action server is to remove all of these callback functions and let us talk to an action server object.
This re-exposes one of the action server callbacks, at that point, I'd think it makes sense to just use the normal action server in this situation. I recognize that the existing implementation relies on polling for updates, so there would have to be a good & specific reason to also expose this (or remove the existing polling feature)
There was a problem hiding this comment.
But with the catch that the simple server can stay simple..?
This callback function is not a hard requirement.
But yes. It re-exposes this one feature. I find it valuable to reexpose this and place "checkers" like that in the designed position.
This would keep your robot to stop for a goal that does not meet very simple requirements. Because when you accept it you have to deal with preemtion and try to stop it at a far later point. Everything is doable, but I just like to have such checkers at the most valuable position.
Stuff about preemtion should probably live in a extra preemtion_checker space. But I believe there exists a valid space for such action_goal_checkers
There was a problem hiding this comment.
I think we're covering all the comment topics in the main-level discussion, let's chat there about these "big" questions and then readdress the specific comments once we have some agreement in general design
| } | ||
|
|
||
| bool | ||
| BtNavigator::goalCheckerCallback(std::shared_ptr<const typename Action::Goal> goal) |
There was a problem hiding this comment.
It seems like this same logic could just be placed in the preemption checks https://github.com/ros-planning/navigation2/blob/7bc04a6c5f0f081292d04339ec1fcbd6d61bf669/nav2_bt_navigator/src/bt_navigator.cpp#L294 that we just blindly accept right now. We could just add a check there and modulate the return code appropriately.
There was a problem hiding this comment.
yes. But this implementation is a little bit higher in the callstack. Preemption checks come right after the action-goal is accepted
There was a problem hiding this comment.
But wouldn't adding
if is_preempt_requested:
// get and check goal's validity per specific server needs
if OK:
accept
else:
reject
Accomplish the same thing without modifying simple action server and enable the behavior you were looking to add to the BT navigator?
There was a problem hiding this comment.
nope.
I want to check if the xml is loadable before accepting the action_goal.
is_preempt_requested is called by the current active / running "goal", actually by the BT. So far more down the "callstack":
So if the robot is just in IDLE and waiting for a new goal -> I would not be able to "pre" check if the xml is loadable with the is_preempt_requested. And this started questioning me if it is necessary to check this on multiple occasions:
- in the
load_bt()-> load the new bt - in
is_preempt_requested(otherwise we would stop a current goal for a bad one) - Potentially some other places
This (PR) however is the point of minimal overlap and should be the exact place of this new (re-introduced) callback.
Also: You cannot REJECT it any more at this place as you accepted it per Action-server-design. So you would cancel it. But I think that would not be the worst, as I just got my head around that there could be multiple accepted goals (current + pending or potential more if wanted). So Canceling in this case would not cancel all goals (including the current, which I was mainly afraid of)
Still: I think it would be better if goals are REJECTED and not Cancelled. This would be a better implementation.
(Probably OT) Also:
https://github.com/ros-planning/navigation2/blob/199ef58f04bce9da4fcf8800630d171665349015/nav2_util/include/nav2_util/simple_action_server.hpp#L230
This is the reason why with PREEMTION no new xmls can be loaded.. As it just updates the goal variable and the BT keeps running.
(No new work() gets triggered/launched)
|
Ah snap. Yes.. So my proposition: Currently, all So I thought it might be interesting to potential increase performance in some instances where the action-goal can be rejected based on very simple pre-checks. Look at the example Action Server implementation: Here you can see that it is intended by Therefore, adding those small tests is A not nicely doable directly in our Or just ignore it, as it is optional. Maybe also see my 2 comments on your comments for further details on your questions |
|
As I was writing this answer, you seemed to wrote your comment. I somehow saw your comment on those files but not the overlaying comment. Sorry for ignoring that. I believe there are 2 problems which deserve 2 solutions. But I still believe that it makes sense in some cases like this if very very basic conditions apply to your specific goal. Can the file be opened from the server, is a goal_field out of some very basic limit. Stuff like that. It is really cleverly solved in the bt_navigator. Thanks for pointing it out! The worker constantly pulls if it has to be terminated. Pulling in general is not the best, but the position where it lives is totally fine. But I still would like to give this a light from a different perspective. Re-exposing this does not justify throwing the rest of our SimpleActionServer out.. And creating a whole new Child-class just for overwriting that I'm not totally hanging on this thing to live. I just saw that there was some lack in the stack to cover some basic things while writing tests for it. |
Keep in mind that its "accepted" in a weird sense. Its accepted by the server, but gets shoved off into the preempting goal variables so its not directly doing anything until the
I think this is a discussion we should have as part of a design discussion for the preemption BT handling ticket you linked to. But I think we're in rough agreement that this solution containing simple action server changes is probably not the right one to fix the BT issue because we have to poll. It might be applicable to other servers, but as I mentioned, there's other options that work to keep it "simple". |
|
I agree on all your points. But, I still think that the application of a Simple Server is not able to add simple application specific checks that would prevent doing anything when it is already clear that the input is bad. Also, there is currently no way to get the goal from the protected variable So it seems we would need to alter the SimpleActionServer for this anyhow, if we decide to go for it. Side note on the preemt handeling and not starting a new Here you could add logic what to do if the BT_XML-field is != the current one.. --> reloading the BT. Reloading a BT while the old one is not halted yet. Should be doable somehow. But we should shift that discussion in the original topic. Just found it mention-able what the current restrictions of the |
I think regardless of the intent, breaking the design encapsulation of the simple action server to expose the callbacks fundamentally breaks the "simple action server" that this is. If you need those callbacks for any reason at all, then I think you should be using the actual action server which is always available and free for use. There's no requirement any action server is a simple action server in nav2, and I'm sure we'll end up adding some raw-action-servers in the future for very niche needs. If this was a question of "I need this feature exposed" and this was the action server, then I'd 100% be on board with this proposal (and even probably make you expose even more). But since this is specifically a "simplified" wrapper, the design very specifically made it so that those callbacks were hidden from the user. The simple action server is not meant to be a 1:1 full replacement (hence the name) so you're right, there are going to be situations where this doesn't do everything you need. But if you're in that situation, then using the full action server makes alot of sense. Those getters would be a good addition so you can inspect the new goal to know if you want to accept or reject it, that would be a good addition! I assumed those were already there. I don't 100% understand the sidebar below, I think this is a case of not enough prior context before proposing a fix (I'm not sure what you're working on solving here right now, it seems like it covers a few things but we should do 1 thing at a time). The current problem I think we're addressing is in the simple action server design to be able to inspect preemption requested goals. Once we have some alignment there, we can go into more specific things. |
|
okay okay.. I think we are getting somewhere. Just to recap:
Things that we agreed on (pls confirm):
(personally, I would outweigh keeping more in line with the ActionServer Design than keeping our SimpleActionServer simple. Because I think that it just fails to comply the Design of an ActionServer, in this very instance. And implementing a whole new ActionServer-Impl from "scratch" is also not my preferred way, as there is some good stuff (preemption-handling) happening in our SimpleActionServer) --> But for now we shift the complexity into the What this decision leaves open / next step:
Currently I see those open threads:
|
Yes and I don't understand what " but we cancel bad ones on multiple checks instead of one (in this instance: NavigateToPose, loadBT, preemtCheck + getter)" means. I don't see how any of this relates to #2010, that seems really off topic |
This here would ensure that the rest of anything run after the initial goal was transmitted includes a valid XML.
And 1 also needs to run trough 2. This does not really add up as file-operations are usually not that heavy. As #2010 rightly wants to split NavigateToPose from the BTActionServer implementation, it would be good if we consider stronger checks by design there. Probably even making it interesting to directly implement an ActionServer to better fit certain conditions. I might have an idea or to for further features for a BTActionServer, that might justify switching from SimpleActionServer to an own implementation. |
|
Maybe we should close out this PR and move the conversations into their appropriate tickets. I'm having a very difficult time figuring out what you're trying to do from the context alone. This PR is very distracting from what I think you want to have be a more general conversation about how to address the preemption with a new or invalid BT within BT navigator. We haven't even had the discussion if allowing for it is even the appropriate action. In general, I think having 2 different places to check the XML is fine. If anything, that's an argument to abstract out the XML validation to its own function so it can be used in both places. I don't see calling |
|
I think #2261 supersedes this, so closing out the PR. We're welcome to reopen if there's progress or if we don't feel that it fills the niche - but I think it does (can check the goal for validity before processing) |
2 participants
Adding a new callback function into SimpleActionServer.
This callback is optional, so no ABI breaking changes here.
BT_Navigator for instance uses this to check if a BT.XML is accessible and therefor can REJECT the goal even before it further processes it.
This callback can be used by any other Action server and is template based (Action independent).
Could also come in handy for #1971 -> check if there is currently another goal happening that is using another current_bt_ than the one from the new goal. Therefor REJECTING the request and not PREEMTING the current BT (or some other behavior that has yet to be decided from/in #1971 )
This callback is called during the initial
goal request(also see the action design docs):So instead of the ACCEPT here, the goal checker could initiate a REJECT inside this Service-Request. This could save some time. But bigger checks like (Can I get a path to this goal) probably would exceed the acceptable lifespan of a service request?
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Future work that may be required in bullet points