Leddar one driver work to fix Issue #12508, simplify logic, improve update rate, and inherit from PX4RangeFinder #12847
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR was flight tested on a 250 quadrotor with pixhawk 4 mini fmu-v5. The system performs as it should. Flight log from the last commit: https://review.px4.io/plot_app?log=65d6eb7e-e676-49cd-8105-c23862aa2fc3 FlightPlot data of the distance sensor measured values during the flight: |
|
I have added a commit to this PR that adds functionality to the PX4RangeFinder class and modifies the LeddarOne driver to inherit from it. Functional bench tests behave as they should: |
mcsauder
changed the title
mcsauder
force-pushed
the
leddar_one
branch
2 times, most recently
from
0e7d1ba to
a624608
Compare
…r distance sensor driver classes and fix perf counter errata.
…espace and console output. Reduce measure/collect timing logic complexity to use ScheduleOnInterval() rather than SchuleDelayed().
…_topic after migrating to PX4RangeFinder.
|
Rebased against current master. |
julianoes
reviewed
Sep 5, 2019
| int | ||
| LeddarOne::measure() | ||
| { | ||
| // Flush the recieve buffer. |
There was a problem hiding this comment.
Suggested change
| // Flush the recieve buffer. | |
| // Flush the receive buffer. |
julianoes
approved these changes
Sep 5, 2019
|
Thanks @mcsauder! |
|
Thanks for your review @julianoes ! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe problem solved by the proposed pull request
This PR fixes issue #12508. This PR also improves the update rate of the sensor from ~8.6Hz to ~9.1Hz and simplifies the timing logic to use
ScheduleOnInterval()rather thanScheduleDelayedand timeout logic that exists in v1.19.2 and previous. Lastly, this PR fixes the broken test() method for the driver.Test data / coverage
My recent PR #11858 on the LeddarOne driver broke automatic starting of the driver, (I apologize for causing that breakage and not catching it in bench testing.) I have tested driver start on boot with telem 1,2, and 4 in this PR. The driver functions as expected and as it does in v1.9.2:
Describe possible alternatives
One alternative to this PR would be to revert the driver to its' state in v1.9.2. I am in favor of this PR because it simplifies the driver quite a bit while getting better update rates over v1.9.2.
Additional context
See also #12508. This work also benefits #11977 and migrates the Terraranger driver closer to common base class method structure and functionality.
It is probably best to incorporate these changes or revert the driver to v1.9.2 code prior to v1.10.
@tomcattigerkkk, if you get a chance, could you test this PR? Thank you!
Please let me know if you have any questions or feedback on this PR. Thanks!
-Mark