-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Odometery calibration #2934
Odometery calibration #2934
Conversation
@jwallace42, please properly fill in PR template in the future. @SteveMacenski, use this instead.
|
You have some linting and test failures |
it looksl ike your BT is somehow invalid? That test simply tries to load all the BT XMLs in the directory and report if it fails to properly load. I don't immediately see anything wrong myself |
So I fixed the loading issue but I ran into another error that is quite confusing. That test is failing due to a timeout. On further inspection I found the line that was causing the issue. The test only times out when the DriveOnHeading BT is in the new xml file. This is quite odd for the following reasons.
I would expect the issue to time out every time the BT was trying to load DriveOnHeading. I am not quite sure what is going on. It seems to be deeper with the behavior tree library. |
Is it because its not in the library's list? https://github.com/jwallace42/navigation2/blob/main/nav2_system_tests/src/behavior_tree/test_behavior_tree_node.cpp#L54-L92 I think that's it, if it doesn't have access to the library, then it can't load. But I'm floored that BT.CPP doesn't throw an error and just hangs instead :eye roll: |
I wish that was the issue but I have it here https://github.com/ros-planning/navigation2/pull/2934/files. |
Huh, that's super strange |
I dug into this a bit more. The new Cancel nodes have the same issue. I tried digging into test with gdb but came across another unexpected result.
|
Neat... I wonder if something about the messages wasn't exported right for the drive on heading work. It could be that we missed some export that is causing this since its unique to that (and wasn't even introduced in this PR). With that said, I looked and wasn't able to find it |
Yeah, checked though the cmake and didn't find any discrepancies between DriveOnHeading and other plugins. I also see this issue in
|
Is it just the cancel plugins? @padhupradheep it sounds like we may have missed something on the cancel behavior tree plugins you worked on That's odd, but also gives us a datapoint. It's probably nothing to do with messages then. If Spin but not Spin Cancel works, then its also probably not on the behavior server side, its probably on the BT nodes themselves. And only impacting relatively new ones. |
I was actually planning to jump in the conversation. I dug deep into this during that PR. #2787 (comment) For some reason the node didn’t register at all.. |
I was just looking at the backtrace and it went back to
I didn't really find any useful information out of this. |
Also DriveOnHeading. |
I see no reason why it should matter, but none of those have |
hey @jwallace42, if you need some help here.. I can give you an hand here.. provided I know, some more details from your last update.. |
@padhupradheep not really sure how to go about solving the issue as a result I hadn't done anything this past week. If you want to reproduce just put one of the troublesome BT nodes in the behavior tree xml and run the nav2_system_test. It should fail due to a timeout. |
let me give it a dig and come back.. |
Cool! Cool! Let me know if you have any questions.... I tried a lot of different things :) |
@padhupradheep find anything? |
not yet.. need few more days.. |
I think, there is not much that I can find here. Unfortunately! |
What did you try? Just so we can use that as a basis of what doesn't work |
BehaviorTree/BehaviorTree.CPP#390 I also filed a ticket |
@jwallace42 please rebase, I doubt it'll fix it, but we updated docker images to 22.04 |
Oh wait, I see the problematic nodes are being registered after all.. I was doing some tests now.. I made our regular navigate_to_pose BT node into a non-sense BT (just for this test), by adding the following changes:
So, what I inferred is that both the I verified the tick by just adding a print to the tick() function found in the bt_cancel_node.hpp @jwallace42 am I missing something here to reproduce your issue? |
Weirder yet, its been reported as the result of a problem in the other system tests for keepout/speed zones which just use the normal BT navigator version (not even that same test script): #2962 (comment) Something seems wrong with BT.CPP or our use of it in CI... |
My comment in #2962 (comment) explains why we might be seeing it in that particular PR. If that could be analogous here, it might be worth adding some logging inside of the BT action server to see where and what node is hanging, and specifically where. Like... err https://github.com/ros-planning/navigation2/blob/e6639fca2f56ec700810a4eec55de31293d3b890/nav2_system_tests/src/behavior_tree/server_handler.cpp#L35-L44 we haven't added dummy servers for these actions for the BT trees to be able to load 🤦 It would have actually taken me a much, much longer time to have found that if it weren't for #2962 (comment) (thank you @wep21!! your comment helped in another timely place) |
Rebase or pull in main, there are some failing tests which concern me but I believe they've been handled elsewhere |
CI failed in an unusual way, retriggering |
@jwallace42, your PR has failed to build. Please check CI outputs and resolve issues. |
* added a BT for driving in a square * removed extra printouts * decreased timeout * do not increment recovery * code review * code review * test and linting fix * updated plugin list * fixed BT loading issue * bump up speed
Basic Info
Description of contribution in a few bullet points
Added a new behavior tree for odometry calibration.
Description of documentation updates required from your changes
Added description of the odometry behavior tree.
Future work that may be required in bullet points
For Maintainers: