AP_Proximity: add support for RP Lidar A2 (360° Lidar with upto 16m range) #6552

amilcarlucas · 2017-07-07T16:16:48Z

No description provided.

OXINARF · 2017-07-09T13:47:03Z

@amilcarlucas Is this ready for review? It's marked as WIP...

amilcarlucas · 2017-07-10T07:49:05Z

We are still going to do small changes to it, so please wait a while longer before reviewing.

khancyr · 2017-08-17T12:05:55Z

@amilcarlucas ping !

amilcarlucas · 2017-08-17T12:31:49Z

This is ready for review since 11 July.
Is there anything I can do ?

khancyr · 2017-08-17T13:04:09Z

it doesn't pass travis, can you look at it ?

amilcarlucas · 2017-08-17T13:09:09Z

@SteveJos Can you please take a look at it ?

amilcarlucas · 2017-08-21T09:17:18Z

This fixed the tests on our local CI server. Let's see how Travis-CI likes it :)

OXINARF · 2017-08-21T11:26:39Z

@amilcarlucas

This is ready for review since 11 July.

You didn't write anything, we can't guess 🙂

This fixed the tests on our local CI server. Let's see how Travis-CI likes it :)

Well, it doesn't even build... You need to rebase on master and fix the issues.

amilcarlucas · 2017-08-21T14:06:42Z

Rebased on master and fixed the build

OXINARF

I've made some inline comments. I have to say the driver looks way more complicated than it needed to be, but if it works, that's fine.

All the checks for RP_LIDAR_DEBUG are terrible, you should do something similar to a lot of other drivers:

#if RPI_LIDAR_DEBUG
  #include <GCS_MAVLINK/GCS.h>
  #define Debug(level, fmt, args ...)  do { if (level <= RPI_LIDAR_DEBUG) { GCS_MAVLINK::send_text(MAV_SEVERITY_INFO, fmt, ## args); } } while (0)
#else
  #define Debug(level, fmt, args ...)
#endif

And then just call Debug(1, "LIDAR IS OK - state: %s", state_string).

OXINARF · 2017-08-24T15:30:38Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.cpp

+                                               AP_SerialManager &serial_manager) :
+                                               AP_Proximity_Backend(_frontend, _state)
+{
+    _uart = serial_manager.find_serial(AP_SerialManager::SerialProtocol_Lidar360, 0);                // selected in MP at SerialConfig


Comment at end of line should be removed

OXINARF · 2017-08-24T15:31:26Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.cpp

+extern const AP_HAL::HAL& hal;
+
+/*
+   The constructor also initialises the proximity sensor. Note that this


Doesn't look like this is true, probably a left-over from earlier code?

The SF40 codes does the same, the comment must be fixed in there as well

OXINARF · 2017-08-24T15:38:04Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.cpp

+
+    // initialise sensor if necessary
+    if(!_initialised)
+    {


Wrong formatting

OXINARF · 2017-08-24T15:40:07Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.h

+#define RPLIDAR_CMD_GET_DEVICE_HEALTH  0x52
+
+// Commands with payload and have response
+#define RPLIDAR_CMD_EXPRESS_SCAN       0x82


All macros above should be moved to cpp file.

OXINARF · 2017-08-24T15:59:06Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.h

+#include "AP_Proximity.h"
+#include "AP_Proximity_Backend.h"
+#include <GCS_MAVLink/GCS.h>
+#include <AP_Common/AP_Common.h>      ///< using overloaded uart->write() function for transmitting HEX bytes


Last two includes seem not needed, AP_HAL.h is though (for UARTDriver).

OXINARF · 2017-08-24T17:05:37Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.cpp

+}
+
+// initialise sector angles using user defined ignore areas, left same as SF40C
+// TODO: adapt to RPLIDAR A2 measurements


What is this TODO?

This function is a 1:1 copy of AP_Proximity_LightWareSF40C::init_sectors().

I think we can remove this To-Do. The To-Do itself isn't in the SF40C version.
We should probably move this to the backend to remove the duplication..

OXINARF · 2017-08-24T17:12:56Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.cpp

+    uint8_t ignore_area_count = get_ignore_area_count();
+    if (ignore_area_count == 0) {
+        _sector_initialised = true;
+        return;


This looks wrong. What about _num_sectors and init_boundary()? In this case, the for loop below wouldn't do anything, so why is a special check needed for it?

Again that is the same code as AP_Proximity_LightWareSF40C.cpp
That again needs to be fixed in there as well.

we need the special check so that if the "ignore_area_count" is zero, we leave the _num_sectors and _boundary array at their defaults which is 8 sectors of 45degrees each. Without the special check the _num_sectors would be set to zero.

OXINARF · 2017-08-24T17:27:22Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.cpp

+    GCS_MAVLINK::send_text(MAV_SEVERITY_INFO, "             CURRENT STATE: %d ", _rp_state);
+#endif
+    uint32_t nbytes = _uart->available();
+    static uint16_t byte_count = 0;


Put this in the class, no need for static.

OXINARF · 2017-08-24T18:22:28Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.cpp

+                if (convert_angle_to_sector(angle_deg, sector)) {
+                    _angle[sector] = angle_deg;
+                    _distance[sector] = distance_m;
+                    _distance_valid[sector] = true;


There's no check for distance being 0 and so invalid.

OXINARF · 2017-08-24T18:23:13Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.h

+
+    // reply related variables
+    AP_HAL::UARTDriver *_uart;
+  //  char element_buf[2][10];


Don't leave dead code.

amilcarlucas · 2017-09-01T10:50:58Z

We improved it a bit more and will push more comits soon.

…ange)

…ted if necessary

…istance for that sector

amilcarlucas · 2017-09-03T15:28:49Z

@OXINARF I think we have addressed all of your points. The missing ones need to be addressed by @rmackay9 because he is the author of the original SF40 code and the issues you raise are SF40 issues as well.

amilcarlucas · 2017-09-13T20:35:56Z

libraries/AP_Proximity/AP_Proximity_RPLidarA2.cpp

+    _uart->write(tx_buffer, 2);
+    _resetted = true;                   ///< be aware of extra 63 bytes coming after reset containing FW information
+    Debug(1, "LIDAR reset");
+    hal.scheduler->delay(RESET_RPA2_WAIT_MS);                      ///< wait at least 8ms after sending new request


Any Ideas on How to improve and or avoid this hal.scheduler->delay(RESET_RPA2_WAIT_MS); call ?

rmackay9 · 2017-09-17T23:30:50Z

I've made some formatting changes, squashed the commits and removed the killer delay (and added a To-Do in it's place) and pushed into master. Thanks!

amilcarlucas added Driver WIP labels Jul 7, 2017

amilcarlucas requested review from rmackay9 and OXINARF July 7, 2017 16:16

amilcarlucas force-pushed the pr-rp-lidar-a2-support branch from 0af5589 to b509678 Compare July 10, 2017 11:14

amilcarlucas removed the WIP label Jul 10, 2017

This was referenced Jul 10, 2017

Add support for the RPLIDAR #2717

Closed

Copter: object avoidance should support cheaper lidar #5605

Closed

amilcarlucas force-pushed the pr-rp-lidar-a2-support branch from b509678 to 93d5b02 Compare August 21, 2017 09:14

amilcarlucas force-pushed the pr-rp-lidar-a2-support branch from 93d5b02 to e850728 Compare August 21, 2017 12:14

amilcarlucas force-pushed the pr-rp-lidar-a2-support branch from e850728 to 6e1aaed Compare August 24, 2017 16:12

OXINARF requested changes Aug 24, 2017

View reviewed changes

amilcarlucas added the WIP label Sep 1, 2017

Steven Josefs and others added 5 commits September 3, 2017 17:25

AP_Proximity: add support for RP Lidar A2 (360° Lidar with upto 16m r…

3887f30

…ange)

AP_SerialMananger: add RP-Lidar A2

2cced70

AP_Proximity: address code review issues

9ef4232

AP_Proximity: remove FTDI cable debugging code, can be later be rever…

219dd2f

…ted if necessary

AP_Proximity: Use the minimum distance found inside a sector as the d…

f0a07bf

…istance for that sector

amilcarlucas force-pushed the pr-rp-lidar-a2-support branch from 26dac40 to 002aa96 Compare September 3, 2017 15:25

AP_Proximity: fix the update of the minimum distance in RPLIDAR

0808c51

amilcarlucas force-pushed the pr-rp-lidar-a2-support branch from 002aa96 to 0808c51 Compare September 4, 2017 08:43

rmackay9 removed the WIP label Sep 4, 2017

rmackay9 assigned rmackay9 and unassigned rmackay9 Sep 4, 2017

amilcarlucas commented Sep 13, 2017

View reviewed changes

rmackay9 closed this Sep 17, 2017

amilcarlucas deleted the pr-rp-lidar-a2-support branch September 18, 2017 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP_Proximity: add support for RP Lidar A2 (360° Lidar with upto 16m range) #6552

AP_Proximity: add support for RP Lidar A2 (360° Lidar with upto 16m range) #6552

amilcarlucas commented Jul 7, 2017

OXINARF commented Jul 9, 2017

amilcarlucas commented Jul 10, 2017

khancyr commented Aug 17, 2017

amilcarlucas commented Aug 17, 2017

khancyr commented Aug 17, 2017

amilcarlucas commented Aug 17, 2017 •

edited

Loading

amilcarlucas commented Aug 21, 2017

OXINARF commented Aug 21, 2017

amilcarlucas commented Aug 21, 2017

OXINARF left a comment

OXINARF Aug 24, 2017

OXINARF Aug 24, 2017

amilcarlucas Aug 25, 2017

OXINARF Aug 24, 2017

OXINARF Aug 24, 2017

OXINARF Aug 24, 2017

OXINARF Aug 24, 2017

amilcarlucas Aug 25, 2017

rmackay9 Sep 16, 2017 •

edited

Loading

OXINARF Aug 24, 2017

amilcarlucas Aug 25, 2017

rmackay9 Sep 16, 2017

OXINARF Aug 24, 2017

OXINARF Aug 24, 2017

OXINARF Aug 24, 2017

amilcarlucas commented Sep 1, 2017

amilcarlucas commented Sep 3, 2017

amilcarlucas Sep 13, 2017

rmackay9 commented Sep 17, 2017

AP_Proximity: add support for RP Lidar A2 (360° Lidar with upto 16m range) #6552

AP_Proximity: add support for RP Lidar A2 (360° Lidar with upto 16m range) #6552

Conversation

amilcarlucas commented Jul 7, 2017

OXINARF commented Jul 9, 2017

amilcarlucas commented Jul 10, 2017

khancyr commented Aug 17, 2017

amilcarlucas commented Aug 17, 2017

khancyr commented Aug 17, 2017

amilcarlucas commented Aug 17, 2017 • edited Loading

amilcarlucas commented Aug 21, 2017

OXINARF commented Aug 21, 2017

amilcarlucas commented Aug 21, 2017

OXINARF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmackay9 Sep 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amilcarlucas commented Sep 1, 2017

amilcarlucas commented Sep 3, 2017

Choose a reason for hiding this comment

rmackay9 commented Sep 17, 2017

amilcarlucas commented Aug 17, 2017 •

edited

Loading

rmackay9 Sep 16, 2017 •

edited

Loading