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.

Sign up for GitHub

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

Jump to bottom

Refactor the teraranger driver: uniform initialization, format whitespace, deprecate usage of the ringbuffer, etc. #11892

Merged
merged 1 commit into from
Sep 22, 2019
Merged

Refactor the teraranger driver: uniform initialization, format whitespace, deprecate usage of the ringbuffer, etc. #11892

merged 1 commit into from
Sep 22, 2019

Conversation

mcsauder
Copy link
Contributor

@mcsauder mcsauder commented Apr 23, 2019 •
edited
Loading

Describe problem solved by the proposed pull request
This PR refactors the teraranger distance sensor driver. Usage of the ringbuffer is deprecated. Uniform initialization is employed along with formatting/alphabetizing/organizing, and standardizing the driver against other current distance sensor driver implementations.

Describe your preferred solution
Standardizing all of the distance sensor driver variable initialization, method ordering and general style will allow for future inheritance structure work to be performed on the distance sensor driver classes.

Describe possible alternatives
All of the distance sensor driver work could be accomplished in one massive PR, but breaking up work in each driver should minimize risk and reduce review effort.

Additional context
See #9279, #11853, #11857, #1858, #11859, #11891

Please let me know if you have any questions on this PR. Thanks!

-Mark

This was referenced Apr 23, 2019
Merged
Merged
Merged
@mcsauder mcsauder force-pushed the teraranger_driver_work branch 2 times, most recently from ec40787 to c9d44cd Compare April 30, 2019 16:57
@mcsauder
Copy link
Contributor Author

@msadowski , would you be willing to review this PR for me? I am working to align the distance sensor drivers so that an inheritance hierarchy can be created. Thanks for any feedback you might have!

@mcsauder mcsauder force-pushed the teraranger_driver_work branch from c9d44cd to f2c1fd1 Compare April 30, 2019 19:36
@msadowski
Copy link
Contributor

@mcsauder great effort on reducing code duplication! I had a read through and the changes make sense to me. Unfortunately I don't have access to TeraRanger sensors anymore but I'll forward this issue to some people from Terabee so that they can verify the changes work as expected with the sensors.

@mcsauder
Copy link
Contributor Author

Thank you @msadowski !

@weekly-digest weekly-digest bot mentioned this pull request May 5, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch 2 times, most recently from 4eacf9b to 9f41bbd Compare May 10, 2019 04:01
@weekly-digest weekly-digest bot mentioned this pull request May 12, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch from 9f41bbd to 97ad131 Compare May 14, 2019 15:55
@weekly-digest weekly-digest bot mentioned this pull request May 19, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch from 97ad131 to eda6e1a Compare May 20, 2019 00:02
@weekly-digest weekly-digest bot mentioned this pull request May 26, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch from eda6e1a to ea52109 Compare June 1, 2019 15:42
@weekly-digest weekly-digest bot mentioned this pull request Jun 2, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch from ea52109 to 2a22b5e Compare June 5, 2019 16:44
@weekly-digest weekly-digest bot mentioned this pull request Jun 9, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch 2 times, most recently from bc6b050 to 47405a2 Compare June 19, 2019 15:42
@weekly-digest weekly-digest bot mentioned this pull request Jun 23, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch 2 times, most recently from f70e65d to 89f45cf Compare June 29, 2019 16:46
@weekly-digest weekly-digest bot mentioned this pull request Jun 30, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch 2 times, most recently from 859e379 to aa3a609 Compare July 6, 2019 20:26
@weekly-digest weekly-digest bot mentioned this pull request Jul 7, 2019
Closed
@weekly-digest weekly-digest bot mentioned this pull request Aug 11, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch from 0f00b62 to 212d6da Compare August 18, 2019 15:52
@mcsauder mcsauder force-pushed the teraranger_driver_work branch 2 times, most recently from 41e0445 to 30587af Compare August 21, 2019 18:15
@mcsauder
Copy link
Contributor Author

Hi @julianoes , I've rebased this PR against current master and reverted the changes to the driver usage() method as we discussed in the dev call. Please let me know if you would like to see any other changes made to this PR! Thanks!

Console output was tested:

teraranger usage
Usage: teraranger <command> [arguments...]
 Commands:

   start         Start driver
     [-a]        Attempt to start driver on all I2C buses (first one found)
     [-b <val>]  Start driver on specific I2C bus
                 default: 1
     [-R <val>]  Sensor rotation - downward facing by default
                 default: 25

   stop          Stop driver

   status        Print driver information

julianoes
julianoes previously approved these changes Aug 22, 2019
View reviewed changes
src/drivers/distance_sensor/teraranger/teraranger.cpp
PRINT_MODULE_USAGE_COMMAND_DESCR("info","Print driver information");


PRINT_MODULE_USAGE_COMMAND_DESCR("status","Print driver information");
Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

@weekly-digest weekly-digest bot mentioned this pull request Aug 25, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch from 30587af to c211d9b Compare August 28, 2019 15:21
@mcsauder
Copy link
Contributor Author

Rebased against current master.

@weekly-digest weekly-digest bot mentioned this pull request Sep 1, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch from b47a07d to c550ca4 Compare September 2, 2019 06:16
@mcsauder
Copy link
Contributor Author

mcsauder commented Sep 2, 2019

Upstream changes today completely borked the entire master branch history to which I have no explanation.... Merging master became a mess in every branch I have even though I updated them all this morning. Every last one of my open PR's all became complete disasters.

Here is a new bench test from this PR, it appears to still function:
image

@weekly-digest weekly-digest bot mentioned this pull request Sep 8, 2019
Closed
@mcsauder mcsauder force-pushed the teraranger_driver_work branch 2 times, most recently from e2bb2ca to 75abd3a Compare September 11, 2019 16:24
@mcsauder
Copy link
Contributor Author

Rebased against current master and bench tested again with pixhawk 4, driver starts automatically when configured to do so and readings all look good:
image

@mcsauder mcsauder force-pushed the teraranger_driver_work branch from 75abd3a to e0c347c Compare September 12, 2019 02:35
@mcsauder
Copy link
Contributor Author

Rebased on current master to kick the stalled CI tools again.

@mcsauder mcsauder force-pushed the teraranger_driver_work branch from e0c347c to 5e0ee58 Compare September 14, 2019 02:27
@mcsauder
Copy link
Contributor Author

Thanks @msadowski ! Fixed, rebased, and pushed up!

@weekly-digest weekly-digest bot mentioned this pull request Sep 15, 2019
Closed
@dagar dagar merged commit cdbe4a3 into PX4:master Sep 22, 2019
@mcsauder mcsauder deleted the teraranger_driver_work branch September 23, 2019 02:01
@weekly-digest weekly-digest bot mentioned this pull request Sep 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dagar dagar dagar approved these changes

@julianoes julianoes julianoes left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mcsauder @msadowski @dagar @julianoes