Migrate vl53lxx driver class member variable initialization to declarations, formatting, and deprecate IOCTL usage. #11896

mcsauder · 2019-04-23T22:45:00Z

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

This PR is primarily copy/paste of the variable initialization values and method declarations along with whitespace formatting but also renames/refactors task_main_trampoline() and task_main() to match convention in the other distance sensor drivers.

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, #11893, #11894

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

-Mark

bys1123 · 2019-04-29T09:46:07Z

Do you have time test this on Pixhawk4 I2C A port, I think it's not working.
#11842

mcsauder · 2019-04-29T15:42:41Z

Hy @bys1123 , I do have time, but unfortunately I do not have hardware to test with. I have a Sensordots Mappydot+ and functional driver, (https://sensordots.org/mappydot_plus), so I've spent some time combing through the vl53lxx driver and pushed up another commit to this PR with a bit more code cleanup.

If you might be willing, try out this PR and make sure to start your driver with vl53lxx start -a . You can do this from the console after boot. I'll post the same in your issue and we can try to make some progress there from the current master branch as well.

bys1123 · 2019-04-29T16:02:44Z

@mcsauder Thanks very much, but I realized this vl53lxx driver is only for vl53l0x, vl53l1x's registers addresses is very different with this one. Hope maybe you could make a vl53l1x driver some time.

bys1123 · 2019-04-30T02:52:05Z

Thanks!

mcsauder · 2019-04-30T02:56:14Z

@bys1123 , I've made an attempt to create functionality for the vl531x sensor in the latest commit to this PR. I appreciate any feedback from testing you can offer.

You will need to alter lines 71/72 to build for either the vl530x or vl531x. If there is a more elegant solution to this please clue me in! :)

bys1123 · 2019-04-30T05:05:35Z

@mcsauder
Feedback:

Looks like the prefix in VL53L1x.h, lost an 'x'.
Yes, we need a better way choose the driver. I think separate them is the best.
I'm checking these registers with '????'

mcsauder · 2019-04-30T05:22:07Z

@bys1123 , Thanks for your feedback! I obtained/modified the VL53l1X register map from ST's vl53l1_register_map.h, which omits the 'x'... and I must admit, I never even noticed that the 'x' was missing! :}

Likewise, I obtained/modified the register map for the VL530X from ST's VL530X API, vl530x_device.h, which includes the 'x'...

We have a choice... match the ST api, or be self-consistent within the PX4 code base. I am interested in advice!

bys1123

I think there is no need to match the ST api. Look like the whole project only consider self-consistent (personal opinion).

bys1123 · 2019-04-30T05:42:58Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp


 	writeRegister(0x81, 0x00);
 	writeRegister(0xFF, 0x06);

 	readRegister(0x83, val);
-	writeRegister(0x83, val  & ~0x04);
+	writeRegister(0x83, val & ~0x04);

 	writeRegister(0xFF, 0x01);


There is lot of hard coded registers in the driver, so I think it will not work for VL53L1x.

bys1123 · 2019-04-30T06:05:43Z

@DanielePettenuzzo Do you have any idea how to add the vl53l1x's driver is the best?

mcsauder · 2019-04-30T15:37:24Z

Hi @bys1123 , Sorry that it wasn't an easy hack to get this driver to play nicely. The hard coded values in the current driver do not have aliases in the ST API for that sensor and searching the web it appears that ST is not inclined to distribute a full register map for that device. So, I've reverted this PR back to its' original intent and we can work on opening up a new PR to introduce the vl53l1x driver. Thanks for making a quick attempt with me on the vl53l1x!

@DanielePettenuzzo , if you are willing/able to test this PR on hardware at your leisure, I am happy to start fleshing out work on a driver for the vl53l1 using the work in this PR to start from.

bys1123 · 2019-05-03T16:18:50Z

@mcsauder Do you think it's better call this driver "vl53l0x" instead of "vl53lxx" ?

mcsauder · 2019-05-03T16:25:00Z

@bys1123 , thanks. Yes, I do. I'd like to propose that change when we submit a vl53l1x driver PR. (I'm making progress on that implementation but am not at a point I can open a PR yet. :} )

bys1123 · 2019-05-08T17:04:38Z

I've tested this driver before you optimize it.
If this driver didn't start successfully. It will break down the progress that calling it. Such as if I didn't connect my vl53l0x, add "vl53lxx start" to rc.board_sensors, it will make autopilot start fail.
If use console to start it, that will make console stop work.

I'm not sure are your changes fix that?

mcsauder · 2019-05-08T17:31:59Z

@bys1123 , you are correct, my changes do not fix that yet... I will do some digging and see if I can correct that behavior in this driver. Updates to follow! Thanks!

mcsauder · 2019-05-29T18:27:42Z

The most recent commit I've rebased/pushed deprecates use of IOCTL.

All bench testing looks good including the move to the general work queue and the additional refactoring I've accomplished recently, distances varied from approximately 15cm-30cm and results are accurate:

LorenzMeier · 2019-05-30T09:09:35Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp

@@ -891,16 +797,15 @@ VL53LXX::sensorTuning()
 	writeRegister(0xFF, 0x00);
 	writeRegister(0x80, 0x00);

-	return OK;
+	return PX4_OK;


PX4_OK is not the correct return value when returning bool

Return type modified to int. Let me know if you'd prefer to see anything different.
Thanks so much for your review!

LorenzMeier · 2019-05-30T09:10:13Z

src/drivers/distance_sensor/vl53lxx/vl53lxx.cpp


+	return PX4_OK;


PX4_OK is not the correct return value when returning bool

Thanks @LorenzMeier , you are absolutely correct. Nice catch! The method return remains the same value, but it was certainly wrong previously.

For the convenience of other readers: from px4_defines.h:

#define OK 0 #define ERROR -1

To preserve the return value I have changed bool method return types to int, this matches convention in this and other driver method return types. If you would prefer otherwise just let me know!

Thanks again for catching this!!

Rebased on current master and bench tested.

mcsauder · 2019-06-01T03:14:43Z

@bys and @dagar , is there anyone else in the world that can hardware validate this PR?

dagar · 2019-06-01T14:05:51Z

@mcsauder could you recommend something for the @PX4/testflights to pickup?

mcsauder · 2019-06-01T17:11:04Z

@mcsauder could you recommend something for the @PX4/testflights to pickup?

Hi @dagar , thanks. Yes. This is the unit I am using for bench testing: https://www.robotshop.com/en/tof-range-finder-sensor-breakout-board-voltage-regulator-vl53l0x.html

If this PR is able to be accepted/rejected I am happy to proceed with the VL53L1X driver work creation I have in queue. The VL53L1X sensor is a better sensor on all accounts, there are two easy choices for that sensor:

I have the sparkfun VL53L1X sensor.

I almost have a flight test rig finished with this unit. I will post flight results here when I complete the build.

mcsauder · 2019-06-11T00:04:32Z

@DanielePettenuzzo and @dagar, here is a flight log running this PR: https://review.px4.io/plot_app?log=0937ae19-5987-4970-a273-992e59c13d67

Here are the distance sensor readings from the same log:

I would like to finish off this PR before working on the vl53l1x implementation. Would you like to see anything else from this PR?

Thanks for your feedback!

…uniform initialization in vl53lxx.cpp. Format whitespace. Alphabetize/group/order variables and methods. Deprecate IOCTL usage and simplify a few methods. Modify return types to correct errata in sensorTuning(), singleRefCalibration(), and spadCalculations() in vl53lxx.cpp.

dagar · 2019-06-16T01:07:46Z

@DanielePettenuzzo and @dagar, here is a flight log running this PR: https://review.px4.io/plot_app?log=0937ae19-5987-4970-a273-992e59c13d67

Here are the distance sensor readings from the same log:

I would like to finish off this PR before working on the vl53l1x implementation. Would you like to see anything else from this PR?

Thanks for your feedback!

The distance sensor data in that log looks terrible. Sensor basically became useless after 0.7m?

mcsauder · 2019-06-16T01:27:45Z

The distance sensor data in that log looks terrible. Sensor basically became useless after 0.7m?

I do agree, however, this flight test was probably worst case; the flight test was over grass in direct overhead (bright summer) sunlight. However, the VL53L1 is a much, much better sensor than the VL53L0X.

dagar · 2019-06-16T03:17:56Z

The distance sensor data in that log looks terrible. Sensor basically became useless after 0.7m?

I do agree, however, this flight test was probably worst case; the flight test was over grass in direct overhead (bright summer) sunlight. However, the VL53L1 is a much, much better sensor than the VL53L0X.

Just double checking that it didn't get any worse.

mcsauder · 2019-06-16T03:23:53Z

Thanks @dagar!

mcsauder force-pushed the vl53lxx_driver_work branch 2 times, most recently from 7075524 to 5124e22 Compare April 24, 2019 16:26

bys1123 mentioned this pull request Apr 29, 2019

Feature Request: VL53L1x Driver. vl53lxx driver is currently only for vl53l0x #11932

Open

mcsauder force-pushed the vl53lxx_driver_work branch from 5124e22 to 2bfcccc Compare April 29, 2019 15:37

mcsauder force-pushed the vl53lxx_driver_work branch 2 times, most recently from 168892a to ea09827 Compare April 29, 2019 15:57

bys1123 mentioned this pull request Apr 29, 2019

Fix vl53l0x driver name #11935

Closed

mcsauder changed the title ~~Migrate vl53lxx driver class member variable initialization to declarations, format whitespace and alphabetize/group/order var/method declarations~~ [WIP] Add register maps to the VL53XX driver for both VL530X and VL531X sensors. Apr 30, 2019

mcsauder force-pushed the vl53lxx_driver_work branch from 213314a to 54bda63 Compare April 30, 2019 02:53

mcsauder force-pushed the vl53lxx_driver_work branch from 54bda63 to 1d2f3ce Compare April 30, 2019 04:41

bys1123 reviewed Apr 30, 2019

View reviewed changes

mcsauder force-pushed the vl53lxx_driver_work branch from 1d2f3ce to ea09827 Compare April 30, 2019 15:32

mcsauder changed the title ~~[WIP] Add register maps to the VL53XX driver for both VL530X and VL531X sensors.~~ Migrate vl53lxx driver class member variable initialization to declarations, format whitespace and alphabetize/group/order var/method declarations Apr 30, 2019

mcsauder force-pushed the vl53lxx_driver_work branch from ea09827 to a6e550d Compare April 30, 2019 17:11

weekly-digest bot mentioned this pull request May 5, 2019

Weekly Digest (28 April, 2019 - 5 May, 2019) #11965

Closed

mcsauder force-pushed the vl53lxx_driver_work branch from a6e550d to 004d32e Compare May 8, 2019 16:46

weekly-digest bot mentioned this pull request May 26, 2019

Weekly Digest (19 May, 2019 - 26 May, 2019) #12076

Closed

mcsauder force-pushed the vl53lxx_driver_work branch from ea52d7d to fabfff5 Compare May 29, 2019 18:24

mcsauder force-pushed the vl53lxx_driver_work branch from fabfff5 to a0daf5e Compare May 29, 2019 18:37

LorenzMeier reviewed May 30, 2019

View reviewed changes

mcsauder force-pushed the vl53lxx_driver_work branch 4 times, most recently from a51af15 to c34fc55 Compare May 30, 2019 21:26

weekly-digest bot mentioned this pull request Jun 2, 2019

Weekly Digest (26 May, 2019 - 2 June, 2019) #12143

Closed

mcsauder force-pushed the vl53lxx_driver_work branch from c34fc55 to d27e7bf Compare June 5, 2019 17:33

weekly-digest bot mentioned this pull request Jun 9, 2019

Weekly Digest (2 June, 2019 - 9 June, 2019) #12219

Closed

mcsauder force-pushed the vl53lxx_driver_work branch 2 times, most recently from 2254503 to 2cf4aad Compare June 10, 2019 23:19

mcsauder force-pushed the vl53lxx_driver_work branch from 2cf4aad to d9f4a7a Compare June 12, 2019 14:35

dagar approved these changes Jun 16, 2019

View reviewed changes

dagar merged commit f1c6674 into PX4:master Jun 16, 2019

mcsauder deleted the vl53lxx_driver_work branch June 16, 2019 03:24

weekly-digest bot mentioned this pull request Jun 23, 2019

Weekly Digest (16 June, 2019 - 23 June, 2019) #12318

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate vl53lxx driver class member variable initialization to declarations, formatting, and deprecate IOCTL usage. #11896

Migrate vl53lxx driver class member variable initialization to declarations, formatting, and deprecate IOCTL usage. #11896

mcsauder commented Apr 23, 2019 •

edited

Loading

bys1123 commented Apr 29, 2019

mcsauder commented Apr 29, 2019

bys1123 commented Apr 29, 2019 •

edited

Loading

bys1123 commented Apr 30, 2019

mcsauder commented Apr 30, 2019 •

edited

Loading

bys1123 commented Apr 30, 2019 •

edited

Loading

mcsauder commented Apr 30, 2019

bys1123 left a comment •

edited

Loading

bys1123 Apr 30, 2019 •

edited

Loading

bys1123 commented Apr 30, 2019

mcsauder commented Apr 30, 2019 •

edited

Loading

bys1123 commented May 3, 2019

mcsauder commented May 3, 2019

bys1123 commented May 8, 2019

mcsauder commented May 8, 2019

mcsauder commented May 29, 2019 •

edited

Loading

LorenzMeier May 30, 2019

mcsauder May 30, 2019

LorenzMeier May 30, 2019

mcsauder May 30, 2019

mcsauder commented Jun 1, 2019

dagar commented Jun 1, 2019

mcsauder commented Jun 1, 2019

mcsauder commented Jun 11, 2019

dagar commented Jun 16, 2019

mcsauder commented Jun 16, 2019 •

edited

Loading

dagar commented Jun 16, 2019

mcsauder commented Jun 16, 2019

Migrate vl53lxx driver class member variable initialization to declarations, formatting, and deprecate IOCTL usage. #11896

Migrate vl53lxx driver class member variable initialization to declarations, formatting, and deprecate IOCTL usage. #11896

Conversation

mcsauder commented Apr 23, 2019 • edited Loading

bys1123 commented Apr 29, 2019

mcsauder commented Apr 29, 2019

bys1123 commented Apr 29, 2019 • edited Loading

bys1123 commented Apr 30, 2019

mcsauder commented Apr 30, 2019 • edited Loading

bys1123 commented Apr 30, 2019 • edited Loading

mcsauder commented Apr 30, 2019

bys1123 left a comment • edited Loading

Choose a reason for hiding this comment

bys1123 Apr 30, 2019 • edited Loading

Choose a reason for hiding this comment

bys1123 commented Apr 30, 2019

mcsauder commented Apr 30, 2019 • edited Loading

bys1123 commented May 3, 2019

mcsauder commented May 3, 2019

bys1123 commented May 8, 2019

mcsauder commented May 8, 2019

mcsauder commented May 29, 2019 • edited Loading

LorenzMeier May 30, 2019

Choose a reason for hiding this comment

mcsauder May 30, 2019

Choose a reason for hiding this comment

LorenzMeier May 30, 2019

Choose a reason for hiding this comment

mcsauder May 30, 2019

Choose a reason for hiding this comment

mcsauder commented Jun 1, 2019

dagar commented Jun 1, 2019

mcsauder commented Jun 1, 2019

mcsauder commented Jun 11, 2019

dagar commented Jun 16, 2019

mcsauder commented Jun 16, 2019 • edited Loading

dagar commented Jun 16, 2019

mcsauder commented Jun 16, 2019

mcsauder commented Apr 23, 2019 •

edited

Loading

bys1123 commented Apr 29, 2019 •

edited

Loading

mcsauder commented Apr 30, 2019 •

edited

Loading

bys1123 commented Apr 30, 2019 •

edited

Loading

bys1123 left a comment •

edited

Loading

bys1123 Apr 30, 2019 •

edited

Loading

mcsauder commented Apr 30, 2019 •

edited

Loading

mcsauder commented May 29, 2019 •

edited

Loading

mcsauder commented Jun 16, 2019 •

edited

Loading