ardupilot icon indicating copy to clipboard operation
ardupilot copied to clipboard

AC_PrecLand: Mavlink V2 messages support.

Open kapacheuski opened this issue 2 years ago • 1 comments

Summary:

  • Backend modified to support LOCAL_FRD, LOCAL_NED, LOCAL_OFFSET_NED. It simplify custom drivers code.
  • Companion modified to accept Mavlink V2 LANDING_TARGET messages.
  • PrecLanding modified to process target position in FRD and NED LOCAL frames.

Supported LANDING_TARGET message frames:

  • MAV_FRAME_BODY_FRD
  • MAV_FRAME_LOCAL_FRD
  • MAV_FRAME_LOCAL_OFFSET_NED
  • MAV_FRAME_LOCAL_NED

Current pullrequest are contains changes requested in #21329 Some code are taken from this PR ( #21329 ). Thanks to @chobitsfan.

Testing: Performed in real flights on PrecLoiter mode for MAV_FRAME_LOCAL_FRD,MAV_FRAME_LOCAL_OFFSET_NED and MAV_FRAME_LOCAL_NED frames.

kapacheuski avatar Aug 27 '22 09:08 kapacheuski

@rishabsingh3003 Hi. Thanks for the review! Sorry for the mess - it my first PR in Ardupilot.

There are some extensive changes to the library's front end (AC_PrecLand.cpp). In my opinion, the front end does not need to know all the types of frames that we support with the mavlink messages. All it needs to know is if it should rotate the frame or not. The rest of the stuff should be handled by: AC_PrecLand_Companion::handle_msg. The way you have done it adds a lot of excess and duplicate code which just isn't needed. This PR needs to follow https://github.com/ArduPilot/ardupilot/pull/21329 very closely in terms of how it is implemented.

I agree with you in terms of duplicate code completely. But from my point of view, AC_Precland needs to be updated with code that works with different frames OR it can be added in the backend functionality to accept different frames. The main goal is to avoid to duplicating 'rotation' code in all backend successors (to make it simple). For new drivers such as Companion IRLock and others in the future, it needs to be a simple way to update position with different frames. So I've thought about it and for now, we have 2 options: 1-st add a function to the backend to convert all frames to 1 that is accepted by AC_Precland or 2-nd accept different frames in AC_Precland.

So if go with 1-st option 4(converting frames in the backend) - From my point of view, it is not a good idea to convert NED frames to angular information (BODY) that is accepted by AC_PrecLand now because it will be 2 conversion total with attitude information required - and it will be a mess :) In another way, if we converted angular information from BODY to NED in the backend we need to implement inertial history and maybe a Kalman filter.

So after all I decided that AC_PrecLand needs to accept at least NED data and BODY data. And to simplify code (yes I'm not happy with duplicate code too ) I make the decision to add support functions to the backend to be able to transfer information to AC_PrecLand from the driver. And to avoid mixing functionality FRD is added to the backend the same way. I think it is better to discuss this on the call :) .

kapacheuski avatar Sep 13 '22 13:09 kapacheuski

Hi @kapacheuski. I think maybe we could add one frame at a time and only change AC_PrecLand_Companion.cpp to reduce complexity. Would you mind taking a look at https://github.com/chobitsfan/ardupilot/commit/b85b6b5afe53326f15c026679d77b004d76ff55d ? Thank you.

chobitsfan avatar Oct 05 '22 14:10 chobitsfan

Hi @chobitsfan It looks good! By the way, we are talking with @rishabsingh3003 according to the structure of PrecLand. Maybe it needs to change the Backend completely to support different frames. The main idea is to create the function in the backend to accept vector and frame type and convert it in the backend (like bool updateTargetPos(Vector3d pos, int farmeType, bool distanceValid = false)). And all filters and rotations make in AC_Backend.cpp. It will simplify all drivers and it will be easy to add support for new frames. What do you think about it?

kapacheuski avatar Oct 06 '22 07:10 kapacheuski