Skip to content

AP_RangeFinder: ensure that too low and too far states are propagated in DroneCAN#31155

Merged
andyp1per merged 1 commit intoArduPilot:masterfrom
andyp1per:pr-rng-dronecan-low
Feb 11, 2026
Merged

AP_RangeFinder: ensure that too low and too far states are propagated in DroneCAN#31155
andyp1per merged 1 commit intoArduPilot:masterfrom
andyp1per:pr-rng-dronecan-low

Conversation

@andyp1per
Copy link
Copy Markdown
Contributor

The current code means that you will always get "No Data" (which is fatal) rather than too low (which happens when you are sitting on the ground).

Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Could you refactor this for early-return, please?

I think it may also be amenable to a switch but only 30% sure

Comment thread libraries/AP_RangeFinder/AP_RangeFinder_DroneCAN.cpp
tridge
tridge previously requested changes Oct 1, 2025
Comment thread libraries/AP_RangeFinder/AP_RangeFinder_DroneCAN.cpp Outdated
@rmackay9
Copy link
Copy Markdown
Contributor

rmackay9 commented Oct 1, 2025

It's an existing issue so not a blocker but I think it would be good to put comments in the AP_RangeFinder_DroneCAN.h file beside the internal variables (e.g. _status) to clarify that these are the values received from the sensor. maybe it's just me but I find it quite confusing to have set_status(_status).

@rmackay9
Copy link
Copy Markdown
Contributor

rmackay9 commented Oct 1, 2025

I think that @peterbarker also suggested that we change the logic in the update() method to have early returns and/or a switch statement.

I think the logic could be:

  • always update the distance with whatever is received from the sensor

Re the status:

  1. if we haven't received an update in 500ms (or whatever) set_status to NoData
  2. if the status from the sensor is NoData then we set status to NoData (e.g. the sensor timed out)
  3. if the status from the sensor is TooLow or TooHigh we set status to the sensor's status
  4. if the status from the sensors is Good then we compare the distance to the min/max parameters and if it's out-of-range then we set status to TooLow or TooHigh appropriately. otherwise we set status to Good.

@andyp1per andyp1per force-pushed the pr-rng-dronecan-low branch 2 times, most recently from 2cc4afc to 1383b1c Compare October 1, 2025 19:41
@andyp1per
Copy link
Copy Markdown
Contributor Author

I think that @peterbarker also suggested that we change the logic in the update() method to have early returns and/or a switch statement.

Fixed

I think the logic could be:

* always update the distance with whatever is received from the sensor

Yes, but in this case the distance we have received is "TooLow" the only thing we can then usefully set it to is 0

Re the status:

1. if we haven't received an update in 500ms (or whatever) set_status to NoData

2. if the status from the sensor is NoData then we set status to NoData (e.g. the sensor timed out)

3. if the status from the sensor is TooLow or TooHigh we set status to the sensor's status

Fixed these

4. if the status from the sensors is Good then we compare the distance to the min/max parameters and if it's out-of-range then we set status to TooLow or TooHigh appropriately.  otherwise we set status to Good.

This was already happening

Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

This is a really good fix.

Let's make sure we get this in for 4.7....

Comment thread libraries/AP_RangeFinder/AP_RangeFinder_DroneCAN.h Outdated
Comment thread libraries/AP_RangeFinder/AP_RangeFinder_Backend.h Outdated
Comment thread libraries/AP_RangeFinder/AP_RangeFinder_DroneCAN.cpp Outdated
Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

One this change is made my only question will be, "has it been tested in its current form?"

Comment thread libraries/AP_RangeFinder/AP_RangeFinder_DroneCAN.cpp Outdated
@tridge
Copy link
Copy Markdown
Contributor

tridge commented Feb 10, 2026

@andyp1per should set new_data on all types of new data from device

Comment thread libraries/AP_RangeFinder/AP_RangeFinder_DroneCAN.cpp
Copy link
Copy Markdown
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

LGTM. Can be merged after testing

@tridge
Copy link
Copy Markdown
Contributor

tridge commented Feb 11, 2026

@andyp1per looks good to me, thanks for the changes! just needs testing

@peterbarker
Copy link
Copy Markdown
Contributor

7:19 PM]Andy Piper: ok tested
[7:19 PM]Andy Piper: interestingly this device just returns 0 when too low
[7:20 PM]Peter Barker: So long as the flag is set 🙂

@andyp1per andyp1per merged commit df3d35f into ArduPilot:master Feb 11, 2026
112 of 113 checks passed
@andyp1per andyp1per deleted the pr-rng-dronecan-low branch February 11, 2026 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants