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 ll40ls namespace driver methods to more closely match other distance sensor driver implementations #12695

Merged
merged 3 commits into from
Sep 27, 2019
Merged

Refactor the ll40ls namespace driver methods to more closely match other distance sensor driver implementations #12695

merged 3 commits into from
Sep 27, 2019

Conversation

mcsauder
Copy link
Contributor

@mcsauder mcsauder commented Aug 14, 2019 •
edited
Loading

Describe problem solved by the proposed pull request
The LidarLite driver is somewhat non-standard at the moment. This PR aligns the driver with other distance sensor drive implementations.

Additional context
Please see #12278 and #12690

@mcsauder mcsauder mentioned this pull request Aug 14, 2019
Closed
@weekly-digest weekly-digest bot mentioned this pull request Aug 18, 2019
Closed
@mcsauder mcsauder force-pushed the lidar_lite branch 5 times, most recently from 3c610b7 to cc14586 Compare August 22, 2019 04:47
@mcsauder
Copy link
Contributor Author

mcsauder commented Aug 22, 2019 •
edited
Loading

@cmic0 , I put in place in this PR your LidarLiteI2C::probe() simplifications in PR #12756, (nice work on that).

I've simplified and standardized this driver's namespace methods against the other i2c distance sensor drivers' methods and although the diff isn't beautiful, once all of the distance sensor drivers are aligned it will be worth it. (Back in March I mentioned in a dev call that I thought this would be a 6 month process... it's getting close now.)

@ar1040, would you mind giving this one a go? (Ensure your have SENS_EN_LL40LS set appropriately for your hardware.) Thanks!

@mcsauder mcsauder changed the title [WIP] Refactor the ll40ls namespace driver methods to more closely match other distance sensor driver implementations Refactor the ll40ls namespace driver methods to more closely match other distance sensor driver implementations Aug 22, 2019
@TSC21
Copy link
Member

TSC21 commented Aug 24, 2019

@dagar Could we try to bring this PR first than #12756?

TSC21
TSC21 previously approved these changes Aug 24, 2019
View reviewed changes
@weekly-digest weekly-digest bot mentioned this pull request Aug 25, 2019
Closed
@mcsauder mcsauder force-pushed the lidar_lite branch 2 times, most recently from f2911eb to a9c4eed Compare August 27, 2019 01:04
@mcsauder
Copy link
Contributor Author

mcsauder commented Aug 27, 2019 •
edited
Loading

CI, your were right, I was wrong. It wasn't documented but now it is. I hope this pleases you. :)
image

@ar1040
Copy link

ar1040 commented Aug 27, 2019

@mcsauder , sorry for the late reply, have been caught up at work. So i put the latest master on my flight controller and tried again. Still no distance sensor on boot, until I run the ll40ls start command in the mavlink console. This is the output I get:
image

@mcsauder
Copy link
Contributor Author

Hi @ar1040, the current master branch does not contain these changes yet. You will need to check out this branch and build/flash to see if these changes fix the issue for you. (Also, just double checking to make sure you set the parameter to start the driver on boot.) Feel free to find me on px4 slack if you could use a hand testing this, I am glad to give you a hand to help test this!

@ar1040
Copy link

ar1040 commented Aug 27, 2019

@mcsauder, is there any documentation on checking out and building this branch? I'm mainly a hardware engineer but i'm always willing to learn!

@TSC21
Copy link
Member

TSC21 commented Aug 27, 2019

@ar1040 On a separate dir: git clone --recursive https://github.com/mcsauder/Firmware.git -b lidar_lite. Then build and test.

@mcsauder
Copy link
Contributor Author

Hi @HenryHuYu , are you able to test this PR to see if it fixes the issue for you? You will need to checkout this PR's branch, (see comments above), and build then flash to your hardware.

If you run into any issues testing this just let me know on PX4 slack. Thanks!

@weekly-digest weekly-digest bot mentioned this pull request Sep 1, 2019
Closed
@mcsauder mcsauder force-pushed the lidar_lite branch 2 times, most recently from 2aaca76 to f31e842 Compare September 2, 2019 05:45
@mcsauder mcsauder mentioned this pull request Sep 2, 2019
Merged
@weekly-digest weekly-digest bot mentioned this pull request Sep 8, 2019
Closed
@mcsauder mcsauder force-pushed the lidar_lite branch 3 times, most recently from 0b06d47 to 156f45b Compare September 11, 2019 15:35
@mcsauder
Copy link
Contributor Author

Rebased with current master and changes from #12934 pulled into this PR. Bench tests look as they should:
image

@mcsauder mcsauder force-pushed the lidar_lite branch 3 times, most recently from 7d8c107 to 86332c4 Compare September 14, 2019 23:50
@mcsauder
Copy link
Contributor Author

Fixed two copy/paste typos, squashed, rebased, and retested. There is more work I'd like to accomplish in this driver, but I am happy with this PR right now. Let me know if you have any questions on this PR.

image

@weekly-digest weekly-digest bot mentioned this pull request Sep 15, 2019
Closed
@mcsauder mcsauder mentioned this pull request Sep 18, 2019
Closed
@weekly-digest weekly-digest bot mentioned this pull request Sep 22, 2019
Closed
TSC21
TSC21 previously approved these changes Sep 24, 2019
View reviewed changes
@mcsauder
Copy link
Contributor Author

Merged current master and rebased, no code changes.

julianoes
julianoes reviewed Sep 26, 2019
View reviewed changes
Copy link
Contributor

@julianoes julianoes left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

@mcsauder
Refactor the ll40ls namespace driver methods to more closely match th…
5299ab7 
…e cm8jl65, mappydot, leddar_one, and other distance sensor driver implementations.
@mcsauder
LidarLite driver minor comment updates/formatting and change instance…
3675958 
…s of OK to PX4_OK.
@mcsauder mcsauder force-pushed the lidar_lite branch 3 times, most recently from f321883 to 2d1ba62 Compare September 27, 2019 04:01
@mcsauder
Change #defines to static constexpr types in LidateLite.h, updated th…
2e40501 
…e docs link in ll40ls.cpp, and shortened comments to allow uniform indentation in LidarLiteI2C.h.
TSC21
TSC21 approved these changes Sep 27, 2019
View reviewed changes
Copy link
Member

@TSC21 TSC21 left a comment

Choose a reason for hiding this comment

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

LGTM!

@TSC21 TSC21 merged commit 4f71c4e into PX4:master Sep 27, 2019
@mcsauder
Copy link
Contributor Author

Thanks @TSC21 ! I appreciate your review!

@mcsauder mcsauder deleted the lidar_lite branch September 28, 2019 02:07
@mcsauder mcsauder mentioned this pull request Sep 28, 2019
Closed
@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

@julianoes julianoes julianoes left review comments

@TSC21 TSC21 TSC21 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 @TSC21 @ar1040 @julianoes