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: fix override in constructor #17660

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

khancyr
Copy link
Contributor

@khancyr khancyr commented Jun 4, 2021

this was catch by coverage.
The issue is that we are calling virtual function in the constructor from the base class, therefore it got no idea of the child class and always use the base class function.
Impact :

  • rx_bufsize() on ulanding,
  • initial_baudrate() in several backend
    This happen in SITL, I haven't test on STM hardware.

Downside is that it costs a little of flash : around 100bytes as some functions are now used and the not removed on compilation anymore.
I got another patch on rangefinder that will make us save the same amount of flash so it should be good
This is totally tested by the autotest

@peterbarker
Copy link
Contributor

Good find! But how about a uint8_t _serial_instance on AP_Rangefinder_Backend_Serial` instead?

@tridge
Copy link
Contributor

tridge commented Jun 30, 2021

@peterbarker please merge when you are OK with it

@tridge tridge removed the DevCallEU label Jun 30, 2021
@peterbarker peterbarker merged commit bcfb07b into ArduPilot:master Aug 26, 2021
@peterbarker
Copy link
Contributor

Merged,thanks!

@rmackay9
Copy link
Contributor

Let's think about whether these fixes need back porting to 4.1 or not.. I don't really understand how significant this is.. if it meant we used the wrong baud rate for the lightware lw20 lidars then that would certainly mean it is worth backporting.

@khancyr khancyr deleted the ap_rangefinder_override branch August 26, 2021 12:07
@peterbarker
Copy link
Contributor

peterbarker commented Aug 26, 2021 via email

@rmackay9
Copy link
Contributor

rmackay9 commented Sep 6, 2021

We should backport this fix to 4.1 because it came up in this user report that says we've broken the uLanding driver in 4.1 but this PR fixes it. https://discuss.ardupilot.org/t/ainteins-us-d1-radar-not-working-with-arducopter-4-1-beta/75414/4

@rmackay9 rmackay9 added this to pending in Copter 4.1 Sep 9, 2021
@rmackay9 rmackay9 moved this from pending to 4.1.0-rc1 in Copter 4.1 Sep 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Copter 4.1
4.1.0-rc1
Plane 4.1
Awaiting triage
Rover 4.1
Awaiting triage
Development

Successfully merging this pull request may close these issues.

None yet

4 participants