Skip to content
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

AMCL: Assertion 'cidx >= 0' failed #634

Closed
mfe7 opened this issue Nov 14, 2017 · 24 comments
Closed

AMCL: Assertion 'cidx >= 0' failed #634

mfe7 opened this issue Nov 14, 2017 · 24 comments

Comments

@mfe7
Copy link

mfe7 commented Nov 14, 2017

I'm using AMCL on Ubuntu 16.04 with Kinetic. The error that often comes up is:
amcl: .../amcl/src/amcl/pf/pf.c:525: pf_cluster_stats: Assertion cidx >= 0' failed.`

This sometimes occurs after I send a new initialpose estimate after driving for a few seconds, but I haven't yet found a consistent procedure that causes it to happen.

I have tried building from source and downloading the package with apt, and see the same behavior both ways. I didn't see this error on my robot with Ubuntu 14.04 with Indigo, but I also have changed the type of laserscanner.

Looking at the source code, this error seems to mean there are no particles - any suggestions of where to look for the cause?

@mfe7 mfe7 changed the title Asserion 'cidx >= 0' failed Assertion 'cidx >= 0' failed Nov 14, 2017
@mfe7 mfe7 changed the title Assertion 'cidx >= 0' failed AMCL: Assertion 'cidx >= 0' failed Nov 14, 2017
@mfe7
Copy link
Author

mfe7 commented Nov 21, 2017

I built from source with the indigo-devel branch and haven't seen the error (still on 16.04)

@mikeferguson
Copy link
Contributor

Given that new info, there are only two possibly relevant commits in Kinetic that were not in indigo:

If I had to guess, I'd think it was the latter? Perhaps try reverting that commit on the kinetic-devel branch and see if your problem goes away? If so, we can look deeper into why / how to fix it.

@tutorgaming
Copy link

I also have this issue. (ros-kinetic) when trying to set the initial pose from rviz, amcl always crashed.

@SteveMacenski
Copy link
Member

I have also experienced this issue. I see it happen when using robot_localization frequently but for some reason doesn't rear its head using robot_pose_ekf or not that I'm able to notice in terms of a crash.

@DLu
Copy link
Member

DLu commented Dec 11, 2017

@tutorgaming @SteveMacenski If you can provide replicable steps, it will be easier to debug.

@SteveMacenski
Copy link
Member

I'm generally unable to trigger it on demand. We don't recall seeing this issue in Indigo either.

I agree the second commit is probably the issue. I would test to see if reverting that resolves the issue but the behavior is sporadic for me. I'll have to wait until next time it comes up to test. If @tutorgaming could test if it happens more consistently in your application, that would be awesome.

@JinglinZhang
Copy link

This issue happens more frequently when set a new pose in the kinetic version. Maybe there's no need to assert cidx >= 0 in line 525 in pf.c.

@SteveMacenski
Copy link
Member

I'm not sure that's the most robust answer. From diff 232cfd2#diff-f9c8b2ff23a212af56d0b3de46fcf48b I don't think that situation needs pf_cluster_stats. Can someone explain why that was needed or if we can revert that?

I have been running a fork of AMCL with that commit commented out for about 2 weeks now and I haven't seen the issue in that time. I think it would be good @DLu @mikeferguson to revert this if you agree.

@chungying
Copy link

Hi @SteveMacenski, pf_kdtree_cluster clusters all samples into several groups to represent multiple hypotheses of robot poses, where a cluster of samples is a hypothesis of a robot pose. pf_cluster_stats is needed to calculate sum of weights, mean and covariance of each hypothesis. The cluster with the highest sum of sample weights is the most likely robot pose, and the mean and covariance is published as the robot pose in AmclNode::laserReceived(). This is the reason why pf_cluster_stats is needed.

Speaking to the assertion error cidx>=0, the error means a sample in a set is not found in the set's kdtree. However, the sample pose should be found in the kdtree. I think this error occurs after the Gaussian-distributed samples of the initial pose couldn't be inserted correctly into the set's kdtree or after pf_kdtree_cluster doesn't work well.

@SteveMacenski
Copy link
Member

SteveMacenski commented Dec 25, 2017

I'm not questioning the need for it in general, it is called elsewhere in the program, my mistake for being non-specific. This was added in the condition when force_publication = true and resampled = false. If we haven't resampled, why would we need to cluster again?

I've been working fine for 2 weeks removing that recent commit. Before, I hardly went a couple days without seeing it.

@chungying
Copy link

Sorry for misunderstanding. However, I think we still need to cluster again because there is a possibility that two moving clusters of hypotheses overlap after odom_->UpdateAction.

@SteveMacenski
Copy link
Member

Hope everyone had a good holiday-
I've run with this for now 3 weeks. I don't disagree that it could be good but I believe that I have isolated it to this commit.

Can we revert it?

@DLu
Copy link
Member

DLu commented Feb 15, 2018

I think so. Please open a PR.

@SteveMacenski
Copy link
Member

PR #666 resolves (I hear that number is good luck in some... particular circles)

@boilerbots
Copy link

Great to see this got resolved, when will we see it built and pushed out to kinetic, I updated 2/22/2018 and it is still bad.

@SteveMacenski
Copy link
Member

SteveMacenski commented Feb 26, 2018

That's a question for @DLu and @mikeferguson

For now, you can use my branch for a solution

@mikeferguson
Copy link
Contributor

I'll have to check with @DLu on if we need to get anything else merged (I do know we need to update a few maintainer emails before release), but I can do a release in the next few days.

@SteveMacenski
Copy link
Member

Awesome.

@SteveMacenski
Copy link
Member

Any movement here?

@mikeferguson
Copy link
Contributor

Apologies on that -- when I commented on this issue, I thought it was already reviewed & merged. That was not the case.

I have a thumbs-up to do a release. I had spent a few minutes to see if there was a more elegant fix than just removing those lines (since the original PR fixes one edge condition error). I had not found a better fix and ended up running out of time that day.

I'm currently in the middle of moving, if there's no other comments on that PR by Monday, I'll merge and release then.

mikeferguson added a commit that referenced this issue Mar 13, 2018
removing recomputation of cluster stats causing assertion error (#634)
@mikeferguson
Copy link
Contributor

merged into kinetic, ported to lunar (still to be merged). releases shortly.

@SteveMacenski
Copy link
Member

SteveMacenski commented Mar 13, 2018

Thanks Fergs

Apparently I can't close this issue, do you mind?

mikeferguson added a commit that referenced this issue Mar 13, 2018
removing recomputation of cluster stats causing assertion error (#634)
@mikeferguson
Copy link
Contributor

Now released to Kinetic: ros/rosdistro#17159

@SteveMacenski
Copy link
Member

Thanks everyone!

johaq pushed a commit to CentralLabFacilities/navigation that referenced this issue Mar 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants