Add acceptance_radius back to MissionItem #1443

bazfp · 2021-05-24T10:41:08Z

Appears this added in a previous PR but disappeared during a refactor. It's a useful option to have as sometimes the defaults are causing some items to never be achieved in our SITL testing (We've now swapped to MissionRaw)

Happy to update the factory funcs + proto to include this, unless there's a reason it was removed :)

JonasVautherin

I am fine with adding it, but you should do it by adding it to the mission.proto definition and generate the code.

E.g. mission.h is not a file you change manually, right?

julianoes

Oh, did this get lost? I wonder how? Thanks for the contribution @bazfp. And as @JonasVautherin wrote, please change it in the proto repo:
https://github.com/mavlink/MAVSDK-Proto/blob/main/protos/mission/mission.proto#L175

bazfp · 2021-05-24T15:49:59Z

Thanks guys

mavlink/MAVSDK-Proto#229

JonasVautherin

LGTM, thanks a lot!

JonasVautherin · 2021-05-24T21:00:21Z

src/plugins/mission/mission_impl.cpp

-    if (item.is_fly_through) {
+    if (std::isfinite(item.acceptance_radius_m)) {
+        acceptance_radius_m = item.acceptance_radius_m;
+    } else if (item.is_fly_through) {
        // _acceptance_radius_m is 0, determine the radius using fly_through


Not related to this PR, but I don't get this comment. @julianoes would you have some insights about it?

Me neither 🤔

JonasVautherin · 2021-05-24T21:50:54Z

@bazfp: some integration tests are still failing, actually. Please pull my latest changes, I updated your branch and the proto submodule 👍

bazfp · 2021-05-24T22:13:49Z

@bazfp: some integration tests are still failing, actually. Please pull my latest changes, I updated your branch and the proto submodule 👍

Seems to be failing due to the NAN acceptance_radius logic. It sets the radius to 1 or 3 depending on flythrough if we don't explicitly set the acceptance_radius, which means the downloaded mission back doesn't match the one we sent if it was NAN.

I could try and work backwards and set the acceptance_radius to NAN if it's 1 or 3 depending on flythrough, but then you could break it if you set it explicitly to 1!

JonasVautherin · 2021-05-25T09:04:40Z

It sets the radius to 1 or 3 depending on flythrough if we don't explicitly set the acceptance_radius, which means the downloaded mission back doesn't match the one we sent if it was NAN.

@julianoes are 1 and 3 some "standard" defaults? I guess MAVSDK does that because sending NAN to PX4 results in an invalid waypoints?

@bazfp would you mind trying to remove the logic that sets it to 1 or 3 and leave it to NAN instead?

I would be tempted to say that if there is a default, it should be set on the PX4 side, not in MAVSDK... 🤔

bazfp · 2021-05-25T13:57:47Z

I've removed the offending section that was setting the acceptance radius.

I could set the acceptance radius default to be 1.0m in the proto definition. It wouldn't account for flythrough waypoints anymore but that logic could go anywhere else.

JonasVautherin · 2021-05-25T14:43:09Z

I could set the acceptance radius default to be 1.0m in the proto definition.

I understand the idea, but I still believe that if the user does not set it, then it should be the responsibility of the autopilot to have a sane default (which may differ from a frame to the other). So I would keep it set to NAN and send that to the autopilot.

julianoes · 2021-05-26T05:17:37Z

For some reason SitlTest.MissionChangeSpeed is now broken and running indefinitely. It seems like that's actually related to this PR.

julianoes · 2021-05-26T17:50:29Z

@bazfp looks like it's still timing out. I'm gonna have a look tomorrow to see what's going on.

julianoes · 2021-05-27T05:58:46Z

Ok, I can reproduce it with PX4 v1.10. It looks like it's something that has been fixed on master. It never accepts the first mission item as "reached" although it is basically hovering there.

This reverts commit 9ed0a20. It turns out these defaults are actually required because PX4 v1.9, v1.10, v1.11 do not handle NAN properly and will be stuck forever at a waypoint. Also, it probably makes sense to keep the current implementation not to break anyone using it that way.

julianoes · 2021-05-27T08:25:03Z

@bazfp I've added the defaults back in. It turns out they are required for PX4 v1.9, v1.10, v1.11 because PX4 gets stuck indefinitely with NAN. In PX4 master/v1.12 the acceptance radius is completely ignored which is wrong in my opinion and I'm going to fix.

JonasVautherin · 2021-05-27T08:26:18Z

Oh, my bad then 😅. Do you agree that PX4 should have a sane default if NAN is sent?

julianoes · 2021-05-27T08:37:08Z

@JonasVautherin yes, I agree. It should but it will take some time until we can rely on that. Or we need to have a version check to check for it.

JonasVautherin · 2021-05-27T08:55:15Z

No need for a version check, but it's good to see that it is fixed upstream. Will come some day 😊

julianoes · 2021-05-27T09:01:40Z

Aha, now CI fails because the check back for acceptance radius is not correct. Because we don't know if 1 or 3 meters are set on purpose or because of NAN 🤦. This is harder than expected.

bazfp · 2021-05-27T10:32:44Z

Could set the acceptance radius default to be 1.0m in the proto definition. It wouldn't account for flythrough waypoints anymore but that logic could go anywhere else.

One solution?

julianoes · 2021-05-27T11:33:39Z

@JonasVautherin the suggestion by @bazfp would work, right?

JonasVautherin · 2021-05-27T12:33:04Z

Yes, we could define to 1 in the proto, and then should we return an error code if the user sets it (explicitly) to NAN, since we know that it crashes on the PX4 side?

julianoes · 2021-05-27T12:35:18Z

@JonasVautherin hm, I would accept NAN but change it to ~~NAN~~ 1.0 unless we know we are connected to a PX4 new enough?

JonasVautherin · 2021-05-27T12:42:02Z

But in that case, should it be a default in the proto definition, or should we just hardcode that behavior? Because it seems like anyway we will hardcode parts of it...

julianoes · 2021-05-27T13:37:41Z

Hard-coding like we do right now is fine but the problem is that the test fails which checks if the downloaded items are equal to the ones uploaded.

JonasVautherin · 2021-05-27T13:54:27Z

But the proto file won't fix that, right?

julianoes · 2021-05-28T07:36:46Z

We would change the test not to use NAN, I suppose, but we could also do that already.

JonasVautherin · 2021-05-28T10:37:49Z

I would vote for that: the default in the proto file is "NAN", because it means "let PX4 choose", and that's what we want. And for versions earlier than 1.12, we have a fix that says "if it is NAN, set to 1". And in the test we don't use NAN, I guess we don't have a choice. So:

Keep the proto as it is
In mission_impl.cpp, change NAN for 1 (bonus: check the PX4 version and do it only if < 1.12)
In the tests, don't use NAN

How does that sound?

julianoes · 2021-05-28T11:18:22Z

Yes, that's great. I'll make the change.

We need to set the acceptance radius to something other than NAN. Otherwise, NAN gets converted to a default and then fails the validation if the mission items are still the same after an upload and download cycle.

JonasVautherin

Great! Thanks 🚀

JonasVautherin requested changes May 24, 2021

View reviewed changes

julianoes requested changes May 24, 2021

View reviewed changes

Seb Horsewell and others added 5 commits May 24, 2021 22:45

Add acceptance_radius back to Missionitem

133be1f

Add acceptance radius to rpc service

24aecff

Convert ItemInt Acceptance Radius

b98ab4b

Fix style and include acceptance_radius comparison

88dda47

Update branch and generate from proto

8d14d2f

JonasVautherin force-pushed the acceptance_radius branch from a099080 to 8d14d2f Compare May 24, 2021 20:55

JonasVautherin previously approved these changes May 24, 2021

View reviewed changes

julianoes previously approved these changes May 25, 2021

View reviewed changes

Remove acceptance radius flythrough defaults

9ed0a20

bazfp dismissed stale reviews from julianoes and JonasVautherin via 9ed0a20 May 25, 2021 13:48

Set acc_radius in change_spd + lossy tests

de1fc8f

julianoes mentioned this pull request May 27, 2021

navigator: fix acceptance radius for multicopter PX4/PX4-Autopilot#17661

Merged

integration_tests: fix mission comparison

781eded

We need to set the acceptance radius to something other than NAN. Otherwise, NAN gets converted to a default and then fails the validation if the mission items are still the same after an upload and download cycle.

JonasVautherin approved these changes May 28, 2021

View reviewed changes

JonasVautherin merged commit e93f108 into mavlink:main May 28, 2021

julianoes added the bug label Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add acceptance_radius back to MissionItem #1443

Add acceptance_radius back to MissionItem #1443

bazfp commented May 24, 2021

JonasVautherin left a comment

julianoes left a comment

bazfp commented May 24, 2021

JonasVautherin left a comment

JonasVautherin May 24, 2021

julianoes May 25, 2021

JonasVautherin commented May 24, 2021

bazfp commented May 24, 2021

JonasVautherin commented May 25, 2021

bazfp commented May 25, 2021

JonasVautherin commented May 25, 2021 •

edited

Loading

julianoes commented May 26, 2021

julianoes commented May 26, 2021

julianoes commented May 27, 2021

julianoes commented May 27, 2021

JonasVautherin commented May 27, 2021

julianoes commented May 27, 2021

JonasVautherin commented May 27, 2021

julianoes commented May 27, 2021

bazfp commented May 27, 2021

julianoes commented May 27, 2021

JonasVautherin commented May 27, 2021

julianoes commented May 27, 2021 •

edited

Loading

JonasVautherin commented May 27, 2021

julianoes commented May 27, 2021

JonasVautherin commented May 27, 2021

julianoes commented May 28, 2021

JonasVautherin commented May 28, 2021

julianoes commented May 28, 2021

JonasVautherin left a comment

Add acceptance_radius back to MissionItem #1443

Add acceptance_radius back to MissionItem #1443

Conversation

bazfp commented May 24, 2021

JonasVautherin left a comment

Choose a reason for hiding this comment

julianoes left a comment

Choose a reason for hiding this comment

bazfp commented May 24, 2021

JonasVautherin left a comment

Choose a reason for hiding this comment

JonasVautherin May 24, 2021

Choose a reason for hiding this comment

julianoes May 25, 2021

Choose a reason for hiding this comment

JonasVautherin commented May 24, 2021

bazfp commented May 24, 2021

JonasVautherin commented May 25, 2021

bazfp commented May 25, 2021

JonasVautherin commented May 25, 2021 • edited Loading

julianoes commented May 26, 2021

julianoes commented May 26, 2021

julianoes commented May 27, 2021

julianoes commented May 27, 2021

JonasVautherin commented May 27, 2021

julianoes commented May 27, 2021

JonasVautherin commented May 27, 2021

julianoes commented May 27, 2021

bazfp commented May 27, 2021

julianoes commented May 27, 2021

JonasVautherin commented May 27, 2021

julianoes commented May 27, 2021 • edited Loading

JonasVautherin commented May 27, 2021

julianoes commented May 27, 2021

JonasVautherin commented May 27, 2021

julianoes commented May 28, 2021

JonasVautherin commented May 28, 2021

julianoes commented May 28, 2021

JonasVautherin left a comment

Choose a reason for hiding this comment

JonasVautherin commented May 25, 2021 •

edited

Loading

julianoes commented May 27, 2021 •

edited

Loading