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

Migrate tfmini driver class member variable initialization to declarations, format whitespace and alphabetize/group/order var/method declarations #11893

Merged
merged 3 commits into from
Jun 16, 2019
Merged

Migrate tfmini driver class member variable initialization to declarations, format whitespace and alphabetize/group/order var/method declarations #11893

merged 3 commits into from
Jun 16, 2019

Conversation

mcsauder
Copy link
Contributor

@mcsauder mcsauder commented Apr 23, 2019 •
edited
Loading

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

This PR is primarily copy/paste of the variable initialization values and method declarations along with whitespace formatting but also includes some minor standardization of the logic in tfmini_main() and cycle_trampoline variable names.

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, #11892

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

-Mark

@mcsauder mcsauder mentioned this pull request Apr 23, 2019
Merged
@mcsauder mcsauder force-pushed the tfmini_driver_work branch from 63b9893 to 86485b5 Compare April 23, 2019 22:40
@mcsauder mcsauder mentioned this pull request Apr 23, 2019
Merged
@mcsauder mcsauder force-pushed the tfmini_driver_work branch 4 times, most recently from 2cd457e to f15edbe Compare April 30, 2019 16:57
@mcsauder
Copy link
Contributor Author

@ChristophTobler , would you be willing to review this PR for me? I am working to align the distance sensor drivers so that an inheritance structure can be created and code duplication can be reduced. Thanks in advance for any feedback you might have!

@mcsauder mcsauder force-pushed the tfmini_driver_work branch 4 times, most recently from ae9b17a to 583308b Compare April 30, 2019 20:50
ChristophTobler
ChristophTobler reviewed May 2, 2019
View reviewed changes
Copy link
Contributor

@ChristophTobler ChristophTobler left a comment

Choose a reason for hiding this comment

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

Generally looks good. But I have no way of testing this

src/drivers/distance_sensor/tfmini/tfmini.cpp Outdated
@@ -875,52 +862,44 @@ tfmini_main(int argc, char *argv[])

default:
PX4_WARN("Unknown option!");
return -1;
return -PX4_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove -

Copy link
Contributor Author

@mcsauder mcsauder May 2, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for catching that! Fixed.

src/drivers/distance_sensor/tfmini/tfmini.cpp Outdated
goto out_error;
PX4_ERR("unrecognized command");
tfmini::usage();
return -PX4_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

src/drivers/distance_sensor/tfmini/tfmini.cpp Outdated
if (!strcmp(argv[myoptind], "start")) {
if (strcmp(device_path, "") != 0) {
return tfmini::start(device_path, rotation);

} else {
PX4_WARN("Please specify device path!");
tfmini::usage();
return -1;
return -PX4_ERROR;
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy/paste. :} Fixed. Thanks!

@mcsauder mcsauder force-pushed the tfmini_driver_work branch from 583308b to df24a04 Compare May 2, 2019 21:13
@mcsauder
Copy link
Contributor Author

mcsauder commented May 2, 2019

Thanks @thomasgubler for looking it over! I found you in the commit history, which is why I reached out. I'll ask the group on the dev call next week to try to find someone who can hardware test it.

Thanks again for your time!

@bys1123
Copy link
Contributor

bys1123 commented May 3, 2019

I have one, gonna test tomorrow.

@weekly-digest weekly-digest bot mentioned this pull request May 5, 2019
Closed
@mcsauder mcsauder force-pushed the tfmini_driver_work branch 2 times, most recently from 9aee03e to efcaac4 Compare May 10, 2019 04:01
@weekly-digest weekly-digest bot mentioned this pull request May 12, 2019
Closed
@mcsauder mcsauder force-pushed the tfmini_driver_work branch from efcaac4 to 26a2384 Compare May 14, 2019 15:55
@weekly-digest weekly-digest bot mentioned this pull request May 19, 2019
Closed
@mcsauder mcsauder force-pushed the tfmini_driver_work branch from 26a2384 to 1114f3d Compare May 20, 2019 00:03
@weekly-digest weekly-digest bot mentioned this pull request May 26, 2019
Closed
@mcsauder mcsauder force-pushed the tfmini_driver_work branch 5 times, most recently from 7c3f1ad to bd1e11e Compare June 8, 2019 02:44
@weekly-digest weekly-digest bot mentioned this pull request Jun 9, 2019
Closed
@mcsauder mcsauder force-pushed the tfmini_driver_work branch 2 times, most recently from 11e5d5c to 1af3325 Compare June 11, 2019 16:02
@mcsauder
Migrate variable initialization from constructor list to declaration …
cd8e982 
…uniform initialization, format whitespace, alphabetize/group/order variables and methods in tfmini.cpp.
@mcsauder mcsauder force-pushed the tfmini_driver_work branch from 1af3325 to cd8e982 Compare June 13, 2019 20:22
@mcsauder mcsauder force-pushed the tfmini_driver_work branch from 50e796e to df26ee0 Compare June 13, 2019 23:43
@mcsauder
Copy link
Contributor Author

mcsauder commented Jun 13, 2019 •
edited
Loading

Bench tested on pixhawk 4 by rotating the sensor 90 degrees to measure distance between a floor and a wall, results show behavior indistinguishable from current master:
This PR:
image

master:
image

@mhkabir , would you be willing to review this PR? Thank you!

@mcsauder
Copy link
Contributor Author

@ChristophTobler , would you be willing to review and perhaps accept/reject this PR as well? Thank you!

@dagar dagar merged commit eaa3d4a into PX4:master Jun 16, 2019
@mcsauder
Copy link
Contributor Author

Thanks @dagar!

@mcsauder mcsauder deleted the tfmini_driver_work branch June 16, 2019 01:44
@weekly-digest weekly-digest bot mentioned this pull request Jun 23, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristophTobler ChristophTobler ChristophTobler left review comments

@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.
4 participants
@mcsauder @bys1123 @dagar @ChristophTobler