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

Takeoff altitude fixed to 2 #742

Open
kripper opened this issue Dec 8, 2024 · 14 comments
Open

Takeoff altitude fixed to 2 #742

kripper opened this issue Dec 8, 2024 · 14 comments

Comments

@kripper
Copy link

kripper commented Dec 8, 2024

We do:

await drone.action.set_takeoff_altitude(1)
await self.vehicle.system.action.takeoff()

But the drone is receiving a Mavlink message with param alt = 2 [m]

This is the received mavlink msg:

17337519058475224451664005374289

@julianoes
Copy link
Collaborator

PX4 or ArduPilot?

@kripper
Copy link
Author

kripper commented Dec 9, 2024

RosettaDrone (I wrote the Mavkink 2 implementation).

@kripper kripper mentioned this issue Dec 9, 2024
@JonasVautherin
Copy link
Collaborator

RosettaDrone (I wrote the Mavkink 2 implementation).

Oh interesting! First time I hear of MAVSDK being used for something other than Ardupilot or PX4!

This is the implementation of the set_takeoff_altitude function:

Action::Result ActionImpl::set_takeoff_altitude(float relative_altitude_m)
{
    if (_system_impl->autopilot() == Autopilot::Px4) {
        return set_takeoff_altitude_px4(relative_altitude_m);
    } else {
        return set_takeoff_altitude_apm(relative_altitude_m);
    }
}

I see here that sometimes the implementation defaults to PX4 if the autopilot is not detected as Ardupilot.

So now it brings us to this new interesting situation: RosettaDrone! How does MAVSDK detect it / how does RosettaDrone advertise itself?

I would think that ideally, RosettaDrone should implement the MAVLink from Ardupilot or the MAVLink from PX4, but not yet another incompatible flavour of MAVLink 🙈.

What do you think?

@kripper
Copy link
Author

kripper commented Dec 9, 2024

Well, on the RD side, I prefer to be agnostic and use the altitude parameter sent via Mavlink.

On the MAVSDK side. I would prefer it to use the value set with set_takeoff_altitude(1), and otherwise send a 0 or null instead of some arbitrary hardcoded parameter.

@kripper
Copy link
Author

kripper commented Dec 9, 2024

Oh interesting! First time I hear of MAVSDK being used for something other than Ardupilot or PX4!

I decided to use MAVSDK because of the QGC mission implementation.

And yesterday I finished implementing and testing a computer vision based precision landing algorithm using MAVSDK which was previously based on Dronekit.

I'm now playing with ORB SLAM.

@kripper
Copy link
Author

kripper commented Dec 9, 2024

This is the implementation of the set_takeoff_altitude

The strange thing is that the take off command (int 22) is received with an altitude parameter = 2, which is different than the one I set with set_takeoff_altitude.

@JonasVautherin
Copy link
Collaborator

Well, on the RD side, I prefer to be agnostic and use the altitude parameter sent via Mavlink.

Hmm I don't think there is such a thing as being "agnostic". Either you use MAVLink-PX4, or you use MAVLink-Ardupilot, or you use MAVLink-your-own. But there is no such thing as "the one, standard MAVLink". That's why MAVSDK has if-else in those places where PX4 and Ardupilot do not agree 😇.

The strange thing is that the take off command (int 22) is received with an altitude parameter = 2, which is different than the one I set with set_takeoff_altitude.

Sure, apparently it does not work properly. But to understand why, we need to understand how your autopilot is being detected by MAVSDK. Looking at the action implementation, it seems like if it is detected neither as Ardupilot nor as PX4, then you will get a mix of them and it most likely won't work.

So what does your autopilot advertise itself as? I guess not as PX4 and not as Ardupilot, right? If that's correct, could you try having it advertise itself as one or the other and see if that helps?

@kripper
Copy link
Author

kripper commented Dec 9, 2024

In the heartbeats, we send:

msg.autopilot = MAV_AUTOPILOT.MAV_AUTOPILOT_ARDUPILOTMEGA;

What could be the reason that MAVSDK sends altitude = 2 in the takeoff msg, even when we did set_takeoff_altitude(1) before?
Why doesn't system.action.takeoff() accept an altitude argument when the mavlink takeoff msg supports this parameter?

@JonasVautherin
Copy link
Collaborator

What could be the reason that MAVSDK sends altitude = 2

Looking in the code, the logic won't be consistent if the autopilot is neither detected as PX4 nor Ardupilot. Not sure why it would be "2", maybe it's just undefined behavior?

Why doesn't system.action.takeoff() accept an altitude argument when the mavlink takeoff msg supports this parameter?

I believe it's because PX4 does not read this parameter, so it has to be done in two steps (set the param with the param protocol first, then takeoff).

@julianoes
Copy link
Collaborator

I believe it's because PX4 does not read this parameter, so it has to be done in two steps (set the param with the param protocol first, then takeoff).

Yes, PX4 is in the wrong here. I just wonder why it doesn't work though.

The param is:
https://docs.px4.io/v1.14/en/advanced_config/parameter_reference.html#MIS_TAKEOFF_ALT

@kripper
Copy link
Author

kripper commented Dec 10, 2024

Not sure why it would be "2", maybe it's just undefined behavior?

Defined in MAVSDK server?

Can we add an optional altitude parameter to system.action.takeoff() and send it in the Mavlink message instead of this mysterious constant?

@JonasVautherin
Copy link
Collaborator

Defined in MAVSDK server?

In the code I showed here, yes.

If I was to debug this, I would start there: see how MAVSDK detects your autopilot. As I said earlier, the MAVSDK implementation sometimes does if (PX4) ... else ... and sometimes if (Ardupilot) ... else ..., which means that if you are neither, you once get the logic for PX4 and once for Ardupilot. And that cannot be good 🙈.

@kripper
Copy link
Author

kripper commented Dec 10, 2024

Can we add an optional altitude parameter to system.action.takeoff() and send it in the Mavlink message instead of this mysterious constant?

I don't understand why two messages are required by MAVSDK, especially when the takeoff Mavlink message supports an altitude parameter.

Imagine the first message doesn't reach the aircraft (UDP) before the takeoff message is received and executed.

I believe other Mavlink implantations send the altitude parameter with the takeoff message.

@JonasVautherin
Copy link
Collaborator

I don't understand why two messages are required by MAVSDK, especially when the takeoff Mavlink message supports an altitude parameter.

Again, PX4 and Ardupilot don't use MAVLink the same way. Of course it would be great if they did, but that is not the case in practice and I don't think it will ever change. So we have to deal with it. QGC also implements different logic between Ardupilot and PX4 (and maybe even others).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants