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.

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

ll40ls cleanup and create PX4Rangerfinder helper class #12567

Merged
merged 1 commit into from
Jul 31, 2019

Conversation

dagar
Copy link
Member

@dagar dagar commented Jul 27, 2019

@mcsauder would you be able to give this a quick review and test (if you have any version of the lidar lite)?

Saves more than 2 kilobytes of flash on fmu-v2.

@@ -39,5 +39,6 @@ add_subdirectory(gyroscope)
add_subdirectory(led)
add_subdirectory(linux_gpio)
add_subdirectory(magnetometer)
add_subdirectory(rangefinder)
Copy link
Contributor

Choose a reason for hiding this comment

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

@dagar , this PR is looking good. Would you be opposed to renaming this directory distance_sensor to match the src/drivers/distance_sensor directory name/structure, (and the class name as well)?

Copy link
Contributor

Choose a reason for hiding this comment

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

One more thought, the ToneAlarmInterface might be a good naming/design pattern to follow. perhaps a distance_sensor directory with a DistanceSensorInterface class would be appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Having sensor in the name started to feel inconsistent with the others (accel, gyro, differential_pressure).

@mcsauder
Copy link
Contributor

@dagar ,

Data on the bench looks like PX4:master, basic functionality all looks good:
image

I have a few little thoughts, like keeping the naming convention adhered to some kind of standard, but I think getting this PR in and following with minor adjustments later is the right approach.

This PR looks good to me. Depending on how you feel about the directory naming convention, I'd suggest merging this PR as-is and iterating any remaining improvements from there.

Nice work!

@@ -40,4 +40,6 @@ px4_add_module(
LidarLite.cpp
LidarLiteI2C.cpp
LidarLitePWM.cpp
DEPENDS
drivers_rangefinder
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @dagar, can you add STACK_MAIN 1200 to the CMakeLists.txt? (Or allocate more if you prefer.)

@mcsauder
Copy link
Contributor

Hi @dagar , it would be good to get PR #12453 in before this as well.

@dagar
Copy link
Member Author

dagar commented Jul 31, 2019

Rebased on master (with #12453 merged), stack main set to 1500, and copyright year updated.

@dagar
Copy link
Member Author

dagar commented Jul 31, 2019

Saves 2.1 kB of flash on fmu-v2.

image

@dagar dagar force-pushed the pr-ll40ls_cleanup branch from 8bc1bfd to ff288f0 Compare July 31, 2019 17:11
@mcsauder mcsauder self-requested a review July 31, 2019 17:50
@dagar dagar merged commit 29c50da into PX4:master Jul 31, 2019
@dagar dagar deleted the pr-ll40ls_cleanup branch July 31, 2019 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants