-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
LidarLite driver fix #12756
LidarLite driver fix #12756
Conversation
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.
The range change does not seem to be fully thought through.
@@ -46,8 +46,8 @@ | |||
|
|||
/* Device limits */ | |||
#define LL40LS_MIN_DISTANCE (0.05f) | |||
#define LL40LS_MAX_DISTANCE_V3 (35.00f) | |||
#define LL40LS_MAX_DISTANCE_V1 (25.00f) | |||
#define LL40LS_MAX_DISTANCE (35.00f) |
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.
I'm not sure about this change. The v1 sensors really don't work on grass at 35 meters and so you end up claiming that you have a good signal when you really will have a quite erratic reading. The argument provided in the PR itself is non-convincing, as the specs of LIDAR sensors tend to be measured against a material with 70% reflectance (which is a white sheet of paper). But real world materials are way, way below.
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.
The main issue in this driver is that there is not a clear way to identify different hardware versions.
In the actual upstream implementation (see here ) v1 versions are distinguished based on the device address (0x42 for v1, 0x62 otherwise), which I think is not properly correct.
As a matter of fact the v1 datasheet reports that "The sensor module has a 7bit slave address with a default value of 0x62 in hexadecimal notation", didn't find any reference to 0x42 address.
If we stick to this statement then the actual logic for distinguishing the v1 among the others is broken. I don't currently have any v1 hardware to be able to properly verify if this is the case.
The proposed implementation looks at hw_version, sw_version and device id which seemed to be more reliable.
I did some tests with v2, v3, v3hp and the logic holds well, unfortunately I don't have data to catch the v1 case.
This brought me to remove the V1 max distance as I think that this was never applied.
Summing up I would go for one of the following solutions:
- If we verify that actually all the v1 sensors are on 0x42 address then I would re-introduce the address-based logic for setting the v1 limit.
- If the v1 have the usual 0x62 address then I would need to define a new case based on hw/sw version
Results of the probing:
v3:
INFO [ll40ls] hw: 23, sw:1
INFO [ll40ls] id: 0
INFO [ll40ls] probe success - hw: 23, sw:1
v3HP:
INFO [ll40ls] hw: 0, sw:200
INFO [ll40ls] id: 13113
INFO [ll40ls] probe success - id: 13113
v2:
INFO [ll40ls] hw: 21, sw:2
INFO [ll40ls] id: 23612
INFO [ll40ls] probe success - hw: 21, sw:2
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.
Stupid question, and I appreciate this is a complex problem to solve: If we can identify v3, can't we just say those are 35 meters and the rest is 20 meters? Do we have proof that the v3 is indeed delivering 35 meters, or are we just hoping it improved?
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.
I actually did some further testing to verify if there is actually a common baseline between all the versions:
Flight was performed on grass in a cloudy day:
For the v3 I have actually checked older logs and 25 meters are the best case; usually they drop at about 15-20meters.
Seems that a good baseline would be 25 meters and 35 meters for the v2. What do you think?
I can try and find some old logs but we never hit 35m with v1/2. It was actually the source of some hugely botched landings in the early days. 25m is even a stretch over grass. Iirc I think the average in real world was 12-15m reliably. Anything else was spotty at best. This is not on v3/hp though. |
@mcsauder, I can still only get the lidar to work by running the ll40ls start command in the mavlink console. Looks like you guys are making good progress though! Let me know if there is anything else I can do to help! |
@mcsauder , kind of. Originally there was no way for me to connect to the Lidar. Now I can read the lidar data but only if I run the "ll40ls start" command in the mavlink console. Previously, this did not work. I was hoping for a solution on boot up. But I did test the lidar and it is getting data, but not on boot, only when running that command in the console. |
@ar1040 there is any chance you can provide a record of the boot messages? |
@ar1040 , can you ensure that your @cmic0 , it's working for me:
|
Ok, based on the experimental results I just have lowered the maximum distance to 25 meters for all the sensors except for the v2 which will be initialized with 35 meters (here 1260b2b). |
1260b2b
to
39bd1e7
Compare
@dagar we hit a flash overflow on v2 again |
39bd1e7
to
ec6377e
Compare
While I can dig up some really old logs if absolutely necessary. I can definitively say on more than 20+ occasions, the v2 doesn’t work anywhere close to 35m. All over grass flights and in the 20 m/s range. Of course had its moments of pinging data back but real world flying in differing areas, it was half of that. 25m is pushing it but I’d keep it there if wanting to keep all things equal. We had these in production planes for 1-2 years and it was a huge source of ensuring people knew the limitations during auto-landing. |
@ryanjAA I just did another test with the v2 and can confirm that its performances in hovering are good (>30m). Maybe for fixed wing applications performance degradation is due to the horizontal velocity. |
@cmic0 can you rebase? |
Signed-off-by: Claudio Micheli <claudio@auterion.com>
Signed-off-by: Claudio Micheli <claudio@auterion.com>
Signed-off-by: Claudio Micheli <claudio@auterion.com>
Signed-off-by: Claudio Micheli <claudio@auterion.com>
Signed-off-by: Claudio Micheli <claudio@auterion.com>
Signed-off-by: Claudio Micheli <claudio@auterion.com>
Signed-off-by: Claudio Micheli <claudio@auterion.com>
ec6377e
to
f731864
Compare
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.
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.
All good to me! I think we are good for a merge now.
This PR addresses some issues that have been founded on Lidar Lite sensors:
Unified the maximum distance of the sensor back to 35 meters: for v1/v2 sensors the previous value was 25 meters but was obtained empirically. According to the datasheets such sensors should be capable to read ~40m on 70% reflective surfaces, thus setting a general value at 35m could make sense to unify all the versions.
For altitude estimation the user has still the possibility to set
EKF2_RNG_A_HMAX
value to a more specific one.Improved probe() logic for distinguish v1/v2/v3 from v3hp.
Fixed reset_sensor() logic: Thanks to Wingtra for the fix. Actually the v3 was not starting on current master. The proposed reset logic properly resets the sensors.
Fixed v3HP signal quality reporting: The
LL40LS_PEAK_STRENGTH_REG
register is not available in the v3HP sensors thus signal quality was reported to be always 0. For the v3HP the signal quality is now computed by looking at the signal strength (LL40LS_SIGNAL_STRENGTH_REG
).Refactored the start() method
Test data / coverage
Tested on v3 and v3hp. Will add logs tomorrow.
FYI @DanielePettenuzzo