-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
Conversation
@@ -39,5 +39,6 @@ add_subdirectory(gyroscope) | |||
add_subdirectory(led) | |||
add_subdirectory(linux_gpio) | |||
add_subdirectory(magnetometer) | |||
add_subdirectory(rangefinder) |
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
@dagar , Data on the bench looks like PX4:master, basic functionality all looks good: 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 | |||
) |
There was a problem hiding this comment.
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.)
Rebased on master (with #12453 merged), stack main set to 1500, and copyright year updated. |
@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.