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 srf02 driver class. Move member variable initialization to declarations, standardize against other drivers and format. #11891

Merged
merged 2 commits into from
Jul 11, 2019
Merged

Refactor srf02 driver class. Move member variable initialization to declarations, standardize against other drivers and format. #11891

merged 2 commits into from
Jul 11, 2019

Conversation

mcsauder
Copy link
Contributor

Describe problem solved by the proposed pull request
This PR migrates srf02 class member variable initialization to respective declarations, formats whitespace, and alphabetizes/organizes variable and method declarations in srf02.cpp.

This PR is copy/paste of the variable initialization values and method declarastion along with whitespace formatting.

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.

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

-Mark

This was referenced Apr 23, 2019
Merged
Merged
Merged
Merged
@mcsauder mcsauder force-pushed the srf02_driver_work branch 2 times, most recently from 479261e to 4f58ed8 Compare April 30, 2019 16:56
@mcsauder
Copy link
Contributor Author

@philipoe , would you be willing to review this PR for me? I am trying to standardize the distance sensor drivers so that an inheritance hierarchy can be created. Thank you!

@weekly-digest weekly-digest bot mentioned this pull request May 5, 2019
Closed
@mcsauder mcsauder force-pushed the srf02_driver_work branch 2 times, most recently from 27146d1 to 17f1694 Compare May 10, 2019 04:00
@weekly-digest weekly-digest bot mentioned this pull request May 12, 2019
Closed
@mcsauder mcsauder force-pushed the srf02_driver_work branch from 17f1694 to bd238fd Compare May 14, 2019 15:54
@weekly-digest weekly-digest bot mentioned this pull request May 19, 2019
Closed
@mcsauder mcsauder force-pushed the srf02_driver_work branch from bd238fd to df27807 Compare May 20, 2019 00:01
@mcsauder
Copy link
Contributor Author

Bench testing of this PR measuring a fixed distance object of approximately 25cm gives similar readings to PX4 master branch.

This PR:
image

PX4 master:
image

@mcsauder mcsauder force-pushed the srf02_driver_work branch from 9d10e1b to 5a41af0 Compare May 20, 2019 03:10
dagar
dagar reviewed May 20, 2019
View reviewed changes
src/drivers/distance_sensor/srf02/srf02.cpp Outdated

#include <drivers/device/i2c.h>

#include <sys/types.h>
Copy link
Member

Choose a reason for hiding this comment

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

Many of these includes probably aren't even needed.

Copy link
Contributor Author

@mcsauder mcsauder May 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Taken care of! Thanks!

@mcsauder mcsauder force-pushed the srf02_driver_work branch 8 times, most recently from 50069df to 11d5201 Compare May 24, 2019 18:51
@weekly-digest weekly-digest bot mentioned this pull request May 26, 2019
Closed
@mcsauder mcsauder force-pushed the srf02_driver_work branch from 11d5201 to 8a7b895 Compare June 1, 2019 00:00
@weekly-digest weekly-digest bot mentioned this pull request Jun 2, 2019
Closed
@mcsauder mcsauder force-pushed the srf02_driver_work branch from 8a7b895 to 9d8a2d6 Compare June 5, 2019 16:44
@weekly-digest weekly-digest bot mentioned this pull request Jun 9, 2019
Closed
@mcsauder
Copy link
Contributor Author

mcsauder commented Jun 16, 2019 •
edited
Loading

Hi @dagar, here are flight logs and plots of this PR and current master. Performance is indistinguishable to me.

Flight log and plots running the SRF02 on current master in position mode varying altitude between approximately 20cm and 2.5m: https://review.px4.io/plot_app?log=246e025c-b744-4176-9bf0-cbf545d1a303
image

Flight log and plots running the SRF02 from this PR in position mode varying altitude between approximately 20cm and 3m: https://review.px4.io/plot_app?log=99cb01c3-62b6-4999-bdba-f7bde750b010

image

Please let me know if you'd like to see anything else in this PR. Thanks!

@mcsauder mcsauder force-pushed the srf02_driver_work branch from 9d8a2d6 to 003a1c9 Compare June 18, 2019 17:40
@weekly-digest weekly-digest bot mentioned this pull request Jun 23, 2019
Closed
@mcsauder
Copy link
Contributor Author

@dagar , ping.

@mcsauder mcsauder mentioned this pull request Jun 24, 2019
Closed
@mcsauder mcsauder force-pushed the srf02_driver_work branch from 003a1c9 to 83511c9 Compare June 26, 2019 05:11
@mcsauder
Copy link
Contributor Author

mcsauder commented Jun 27, 2019 •
edited
Loading

If anyone in the world can prove that this PR has issues, I would like to speak with them about my experiences :) Is there anyone out there with hardware to test this beside me?

@mcsauder
Copy link
Contributor Author

mcsauder commented Jun 27, 2019 •
edited
Loading

@philipoe , @okalachev @julianoes @bkueng , please weigh in on this PR. Thank you!

@mcsauder mcsauder force-pushed the srf02_driver_work branch from 83511c9 to 37a233f Compare June 28, 2019 05:52
@mcsauder
Copy link
Contributor Author

@davids5 and @dagar , The PX4:master stack size is insufficient for this driver. Here is output from current PX4:master stackcheck when starting the driver:
image

I have added a commit to increase the stack size, here it output from loading a stackcheck build with this PR:
image

@mcsauder mcsauder changed the title Migrate srf02 driver class member variable initialization to declarations, format whitespace and alphabetize/group/order var/method declarations Refactor srf02 driver class. Move member variable initialization to declarations, standardize against other drivers and format. Jun 28, 2019
@mcsauder mcsauder force-pushed the srf02_driver_work branch from dbf6a34 to 03423ae Compare June 29, 2019 16:46
@weekly-digest weekly-digest bot mentioned this pull request Jun 30, 2019
Closed
@mcsauder mcsauder force-pushed the srf02_driver_work branch from 03423ae to c3667c1 Compare June 30, 2019 18:28
mcsauder added 2 commits July 1, 2019 11:25
@mcsauder
Migrate variable initialization from constructor list to declarations…
4a7e606 
…, standardize whitespace formatting, and alphabetize/organize order of methods and variables in srf02.cpp.
@mcsauder
Increase stack allocation size for the srf02 driver.
546f69e
@mcsauder mcsauder force-pushed the srf02_driver_work branch from c3667c1 to 546f69e Compare July 1, 2019 17:25
@weekly-digest weekly-digest bot mentioned this pull request Jul 7, 2019
Closed
@mcsauder mcsauder mentioned this pull request Jul 10, 2019
Closed
14 tasks
@mcsauder
Copy link
Contributor Author

@davids5 and @dagar, there wasn't enough time to get to this in the dev call today. Would you mind letting me know what else you'd like done? Thanks!

@dagar dagar merged commit 4e5974f into PX4:master Jul 11, 2019
@mcsauder mcsauder deleted the srf02_driver_work branch July 11, 2019 14:35
@weekly-digest weekly-digest bot mentioned this pull request Jul 14, 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

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

Successfully merging this pull request may close these issues.
2 participants
@mcsauder @dagar