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

AP_RangeFinder: benewake reports at least 12m or 23m when out-of-range #9778

Merged
merged 3 commits into from
Nov 15, 2018

Conversation

rmackay9
Copy link
Contributor

@rmackay9 rmackay9 commented Nov 14, 2018

This PR has two minor improvements:

  • the Benewake TFmini and TF02 driver reports at least 12m (for the TFmini) or 23m (for the TF02) when the Lidar cannot return a reading (i.e. when it goes out of range or passes over something shiny). This has no impact on how the vehicle code uses the lidar (the vehicle code does not use the lidar range when it's status is anything but "4"/"Good") but avoids an edge case where the lidar could return a shorter value when it goes out of range. This is the scenario:
    • user sets up the RNGFND_MAX_CM = 400 (i.e. 4m) even though the lidar can read up to 1000 (i.e. 10m)
    • the vehicle is flying at 6m and the lidar reports 6m even though the vehicle code doesn't use the lidar because it is out-of-range.
    • the vehicle momentarily passes over something shiny, the lidar cannot read the distance so it returns 400+100 = 5m. I.e. the range drops as it passes over something shiny which the user would find odd. With this change the lidar would instead return 12m.
  • the dataflash RFND message records the range finders status so that we can more easily see whether the vehicle code will be using the lidar or not.

Here is a screen shot of the RFND message during a bench test with the Benewake TFmini lidar
benewake-fix3-test

value logged and shown to the user will be at least 12m for TFmini or 22m for TF02 which ensure the user won't see the range drop even if they have configured a very short useable distance
@rmackay9
Copy link
Contributor Author

I've found a much more critical bug in the driver that I've fixed in the last commit. The checksum byte from the sensor was being promoted to a uint16_t (I think) meaning that it could be much larger than the calculated checksum. I've retested and we are getting far fewer lost messages now (practically none). Below is a screen shot showing distance (in red) and status (in green). The status is always 3 (out-of-range-high) or 4 (good) where as previously we were seeing 1 (No data).
benewake-fix3-test2

Copy link
Member

@OXINARF OXINARF left a comment

Choose a reason for hiding this comment

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

LGTM, although I don't really understand how the last commit is a fix, it seems a NFC change to me. The old code would keep adding and then discard upper bits, the new one will overflow (and so immediately discard the upper bits instead of doing it later).

@rmackay9
Copy link
Contributor Author

@OXINARF, I think the key change is actually the "(uint8_t)linebuf[BENEWAKE_FRAME_LENGTH-1]" part. So it was the conversion of the single byte from the buffer that was being converted into an 16bit integer that was not equal to the left. While testing I would see the left side would see things like the left would be 0xF0 and the right would maybe be 0xFFF0 (or something like that).

Anyway, thanks very much for the review, this will go out for beta testing today!

@rmackay9 rmackay9 merged commit e9c1abf into ArduPilot:master Nov 15, 2018
@rmackay9 rmackay9 deleted the benewake-fix3 branch November 15, 2018 00:18
@OXINARF
Copy link
Member

OXINARF commented Nov 15, 2018

@rmackay9 You are correct, issue is the buffer being of chars instead of uint8_t as it should, I missed that. Reading from the UART to a char is actually wrong as it returns an uint16_t and we should check it isn't -1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Copter 3.6 Backports
  
3.6.2-rc1
Development

Successfully merging this pull request may close these issues.

None yet

2 participants