Robustify mission upload/download #230
Conversation
julianoes
force-pushed
the
fix-mission-download
branch
from
01cbaf2
to
32c9e9b
Compare
julianoes
changed the title
integration_tests/mission.cpp
Outdated
| @@ -23,6 +23,9 @@ static std::shared_ptr<MissionItem> add_mission_item(double latitude_deg, | |||
| static void compare_mission_items(const std::shared_ptr<MissionItem> original, | |||
| const std::shared_ptr<MissionItem> downloaded); | |||
|
|
|||
| // Set to 1 to test with 1200 mission items. | |||
| #define MANY_ITEMS_TEST 0 | |||
There was a problem hiding this comment.
Why not having a variable (boolean) instead of an ifdef? Ifdefs tend to end up with code that does not compile...
There was a problem hiding this comment.
You're right, fixed in b99c779.
plugins/mission/mission_impl.cpp
Outdated
| @@ -741,15 +762,16 @@ void MissionImpl::set_current_mission_item_async(int current, Mission::result_ca | |||
| _result_callback = callback; | |||
| } | |||
|
|
|||
| void MissionImpl::upload_mission_item(uint16_t seq) | |||
| void MissionImpl::upload_next_mission_item() | |||
There was a problem hiding this comment.
I'd rather pass the item id as a parameter (as it was done before) and have this function const. So that we know it doesn't have side effects.
plugins/mission/mission_impl.cpp
Outdated
| @@ -96,7 +95,9 @@ void MissionImpl::process_mission_request_int(const mavlink_message_t &message) | |||
| return; | |||
| } | |||
|
|
|||
| upload_mission_item(mission_request_int.seq); | |||
| _next_mission_item_to_upload = mission_request_int.seq; | |||
There was a problem hiding this comment.
Does it really need to be a member? Seems to be part of the fix somehow, but I'm not sure I understand =/.
There was a problem hiding this comment.
For retransmission you need to keep track of what the next mission item to upload is. This is done using this member.
julianoes
force-pushed
the
fix-mission-download
branch
from
b99c779
to
a03b57c
Compare
This allows to test with many mission items but instead of a define we use a variable, so that we can catch compile errors.
In case any of the MISSION_ITEM or MISSION_REQUEST messages get lost on the way, we need to retry a couple of times. We do this with two different timeouts, one for the passive case where the autopilot pulls (requests) mission items from us and an active case with a lower timeout where we pull mission items.
julianoes
force-pushed
the
fix-mission-download
branch
from
a03b57c
to
1cb4be2
Compare
|
@JonasVautherin thanks for reviewing. I realized that the retransmission logic when uploading was incorrect. Instead of re-pushing items we just need to wait for the autopilot to pull them. I've reworked the commits and it is now one commit implementing the retransmission. Your concerns with the member variable is resolved as well. |
Remove obsolete ListRunningPlugins and add SetMavlinkTimeout to core
This fixes the case where a mission item gets lost from the autopilot to
DroneCore and we need to request the same one again.
Previously, we just requested the next one, essentially skipping the one
we missed. This then lead the autopilot to respond with a failure
because the wrong sequence was requested.
These fixes go together with:
mavlink/qgroundcontrol#6059
PX4/PX4-Autopilot#8750