Skip to content
This repository was archived by the owner on Jan 14, 2023. It is now read-only.

Conversation

@tulku
Copy link

@tulku tulku commented Jan 21, 2014

Previously only a fixed angle was asked from the laser. We now use the
maximum angle as reported by the laser.

Previously only a fixed angle was asked from the laser. We now use the
maximum angle as reported by the laser.
@stonier
Copy link
Contributor

stonier commented Feb 9, 2014

I don't have a laser to test with. @damonkohler does this look ok to you?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change necessary? Is numberOfConfiguredRanges computed incorrectly?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will avoid painful debugging and unnecessary crashes in favor of a clear warning message if, for any unanticipated reason, numberOfConfigured ranges doesn't match scan.getRanges().length - as it happened to us when trying to use this code.

The reason numberOfConfiguredRanges could be different than scan.getRanges().length is because the former is calculated at configuration time (FF command) but the number of ranges on each scan can technically be different for each distance acquisition request (MD).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, just set numberOfConfiguredRanges = scan.getRanges().length.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ... are you sure about that? In most cases both should match. A mismatch between them might indicate a higher-level logic error. I think this will end up being a useful check, it does provide additional information about what's going on to the programmer. And in most circumstances, both numbers should match anyway, particularly with the change below

@adamantivm
Copy link
Contributor

@damonkohler thanks for your review, we'll address your comments. Meanwhile, any reason why you simply closed the PR? Was that accidental? Otherwise could you clarify your intent? Are you requesting a new PR or are you simply stating you reject the contributions?

@damonkohler
Copy link
Member

Whoops. Totally an accident.

@damonkohler damonkohler reopened this Feb 17, 2014
@adamantivm adamantivm changed the base branch from master to obsolete/master December 16, 2016 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants