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 mb12xx driver class to allow for multiple sensors. Add ability to set sensor addresses. Deprecate rignbuffer and IOCTL. #11859

Merged
merged 6 commits into from
Jun 27, 2019
Merged

Refactor mb12xx driver class to allow for multiple sensors. Add ability to set sensor addresses. Deprecate rignbuffer and IOCTL. #11859

merged 6 commits into from
Jun 27, 2019

Conversation

mcsauder
Copy link
Contributor

@mcsauder mcsauder commented Apr 16, 2019 •
edited
Loading

Describe problem solved by the proposed pull request
This PR refactors the MB12XX class to allow multiple sensors to be run simultaneously, set sensor device addresses, and deprecates use of the hardware ringbuffer and IOCTL.

This PR is a major re-write/refactor with flight test validation demonstrated in the comments below.

Describe your preferred solution
This PR standardizes the MB12XX class with the MappyDot distance sensor driver. Standardizing the distance sensor drivers will allow for future inheritance structure work to be performed on the distance sensor driver classes.

This PR is additional work to promote #9279 in conjunction with #11853, #11857, #11858, et al., and remaining distance sensor driver work.

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

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

-Mark

@mcsauder mcsauder force-pushed the mb12xx_driver_work branch 3 times, most recently from 6a3448a to a0b4c8d Compare April 16, 2019 00:54
@mcsauder mcsauder mentioned this pull request Apr 16, 2019
Merged
@mcsauder mcsauder force-pushed the mb12xx_driver_work branch 3 times, most recently from a95f741 to 3679732 Compare April 23, 2019 15:58
This was referenced Apr 23, 2019
Merged
Merged
Merged
Merged
Merged
@mcsauder mcsauder force-pushed the mb12xx_driver_work branch 2 times, most recently from 9952109 to 78d532d Compare April 30, 2019 15:51
@mcsauder
Copy link
Contributor Author

@DanielePettenuzzo, @jverbeke, or @ghulands would one of you be able/willing to review this PR for me? I am trying to wrangle the distance sensor drivers into a standard structure and this PR helps align them. Thank you in advance!

@mcsauder mcsauder force-pushed the mb12xx_driver_work branch 2 times, most recently from a0cc1fd to 80c92bd Compare April 30, 2019 20:47
@weekly-digest weekly-digest bot mentioned this pull request May 5, 2019
Closed
@mcsauder mcsauder force-pushed the mb12xx_driver_work branch 2 times, most recently from f2d4333 to b405b73 Compare May 10, 2019 03:58
@weekly-digest weekly-digest bot mentioned this pull request May 12, 2019
Closed
@mcsauder mcsauder force-pushed the mb12xx_driver_work branch from b405b73 to ca0f422 Compare May 14, 2019 15:53
@weekly-digest weekly-digest bot mentioned this pull request May 19, 2019
Closed
@mcsauder mcsauder force-pushed the mb12xx_driver_work branch 2 times, most recently from 5e1db3c to 0fe5f92 Compare May 24, 2019 16:57
@weekly-digest weekly-digest bot mentioned this pull request May 26, 2019
Closed
@mcsauder mcsauder force-pushed the mb12xx_driver_work branch 2 times, most recently from a419724 to de69661 Compare May 31, 2019 23:04
@weekly-digest weekly-digest bot mentioned this pull request Jun 2, 2019
Closed
@mcsauder mcsauder force-pushed the mb12xx_driver_work branch from de69661 to 403dbcf Compare June 4, 2019 03:48
@mcsauder
Copy link
Contributor Author

@dagar, I've pushed up a commit that makes this driver mirror the mappydot driver that I helped to author. The current PX4:master implementation cannot run multiple sensors. This PR runs multiple sensors, adds the ability to set sensor addresses, and runs on interval rather than delay.

image

These screenshots from the bench show both sensors operating together, (obviously obtaining different values).
image

I will post flight logs tomorrow.

@mcsauder mcsauder force-pushed the mb12xx_driver_work branch from 69e3e0d to 7288e72 Compare June 18, 2019 04:39
@mcsauder
Copy link
Contributor Author

Hi @dagar, here are flight logs and plots from PX4:master and this PR during flights ranging from ~20cm to ~4m. This PR outperforms master with a single sensor as indicated by datapoints that actually get higher than 50cm on the right side of the plot.

PX4:master https://review.px4.io/plot_app?log=799c35ad-f9dc-4143-95d1-6a067cff581e
image

This PR: https://review.px4.io/plot_app?log=8b89aa50-2228-4629-922c-843d4ba2fc29
image

@mcsauder
Copy link
Contributor Author

Here is a log and plot with one downward and one horizontally oriented sensor from this PR, (not possible with current master):
image

The red zig-zag line at the top is the sensor ID, the green distance is shown in the plot above.
image

@mcsauder mcsauder changed the title Migrate mb12xx driver class member variable initialization to declarations. Format whitespace and alphabetize/group/order var declarations Refactor mb12xx driver class to allow for multiple sensors. Add ability to set sensor addresses. Deprecate rignbuffer and IOCTL. Jun 18, 2019
@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 mb12xx_driver_work branch from e72b12f to d032a1f Compare June 26, 2019 05:11
@mcsauder
Copy link
Contributor Author

@dagar and @davids5 ,

Both current master and this PR fail at the same line when performing a stackcheck as discussed. (At least the failure is identical.) This isn't a great state of affairs for this driver, but is is sufficient for this PR? Thanks for your feedback!

Master:
image

This PR:
image

mcsauder added 5 commits June 26, 2019 19:35
@mcsauder
Migrate mb12xx driver class member variable initialization to declara…
e589982 
…tions.

Format whitespace and alphabetize/group/order var declarations.
Refactor driver reset() and mb12xx_main() methods for readability.
Deprecate usage of IOCTL.
@mcsauder
Deprecate the _reports ringbuffer, _class_instance, and read() method…
67521db 
… and minor formatting in the mb12xx driver.
@mcsauder
Add time_literal usage and #define MB12XX_BUS_SPEED.
19fff33
@mcsauder
Refactor the mb12xx driver to mirror the mappy_dot driver implementat…
940949d 
…ion and create functionality for multiple sensors.
@mcsauder
Revert PX4_I2C_BUS_EXPANSION2 to PX4_I2C_BUS_EXPANSION to satisfy CI …
088ea0c 
…tools.
@mcsauder mcsauder force-pushed the mb12xx_driver_work branch from d032a1f to 088ea0c Compare June 27, 2019 01:35
@mcsauder
Copy link
Contributor Author

mcsauder commented Jun 27, 2019 •
edited
Loading

@dagar and @davids5 , increasing the mb12xx stack allows the driver to start successfully... overall there are still issues running stackcheck, but it gets past the failure with this driver when stack allocation is increased (latest commit_:
image

@mcsauder
Copy link
Contributor Author

This can't possibly be any worse than the status quo, right? I've presented a dearth of data, and no one else has weighed in, that's got to count for something, right? :)

@mcsauder
Copy link
Contributor Author

@okalachev and @philipoe , you are the only other authors in this file. I am hoping to have this PR signed off in the next few days so that I can carry on with the generalized work this is focused on. Please weigh in! Thanks!

@dagar
Copy link
Member

dagar commented Jun 27, 2019

I think we should pursue a fundamentally different structure for multiple instances, but let's go with this for now.

Each sensor needs metadata including rotation (already handled), but also offsets.

@dagar dagar merged commit 94c1f21 into PX4:master Jun 27, 2019
@mcsauder mcsauder deleted the mb12xx_driver_work branch June 27, 2019 21:15
@mcsauder
Copy link
Contributor Author

I think we should pursue a fundamentally different structure for multiple instances, but let's go with this for now.

Each sensor needs metadata including rotation (already handled), but also offsets.

Agreed. I'll put some work into getting a better (re-usable) framework. I have some ideas that I will put forward once the remaining distance sensor drivers are aligned.

Thank you for reviewing and accepting this PR @dagar !

@weekly-digest weekly-digest bot mentioned this pull request Jun 30, 2019
Closed
@mcsauder mcsauder mentioned this pull request Jul 10, 2019
Closed
14 tasks
@JosepBC JosepBC mentioned this pull request Mar 7, 2021
Open
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
Admin: Cleanup (housekeeping) 🧹 Admin: Enhancement (improvement) 💡
Projects
None yet
Milestone
No milestone
Development

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