Conversation
KonradRudin
left a comment
There was a problem hiding this comment.
Thanks for sending the PR. This part seems to be very buggy when you connect with a second instance trying to read the mission. However, i think there are more things that need to be changed:
- https://github.com/Dani3L9H/PX4-Autopilot/blob/a76095030ec51dcd797c9eae701670774113beee/src/modules/mavlink/mavlink_mission.cpp#L390-L391 it makes no sense to use the _transfer_partner_sys_id here as the function inputs are the sysid and compid you want it to send to.
- This one is tricky: https://github.com/Dani3L9H/PX4-Autopilot/blob/a76095030ec51dcd797c9eae701670774113beee/src/modules/mavlink/mavlink_mission.cpp#L539 its in the send function which gets periodically called. But where to we send the update for a jump item if it changed? Currently it is send to the last identitiy who up/downloaded the mssion. At least we would need to make sure it is send to the GCS if your isntance is also needing this information then we would need to distribute it to all. I think the send to GCS is an acceptable first step.
- Here we should first check that the msg is really coming from the transfer_partner_sys_id https://github.com/Dani3L9H/PX4-Autopilot/blob/a76095030ec51dcd797c9eae701670774113beee/src/modules/mavlink/mavlink_mission.cpp#L1066-L1067
- That ones tricky: https://github.com/Dani3L9H/PX4-Autopilot/blob/a76095030ec51dcd797c9eae701670774113beee/src/modules/mavlink/mavlink_mission.cpp#L1863-L1864 here you want it to send to all partners who work with the mission.
… right sysid/compid
…tected since a receiver can now check this by changing mission ids in the MISSION_CURRENT stream
…nsfer, when it is the same partner
|
I mae a proposal now for the fixes. @Dani3L9H Can you please retest? Just have most of my requests addressed. Except: This
However, this is not so easy to be solved, and the system should preferably just broadcast a change in the jump counter instead of sending it to one specific partner only. So we do not need to keep track of it Also with 4753aec i removed the mission count when the mission has changed. AS now we could detect if a mssion has changed over the MISSION_CURRENT id. I do not know if we should already remove this as i don't think it is already implemented in QGC. However i would be in favor to remove it |
|
I have created an issue in QGC to use the mission ID in the MISSION_CURRENT message to detect plan changes: |
|
I tested it and it looks good. |
Reviews addressed by myself. Need someone else to check
Solved Problem
While connected over a groundstation and e.g. using a separate instance like MAVSDK to also download a mission, there will be timeout warnings in the groundstation when trying to make operations in the mission like clearing it.
This is due to the fact that PX4 currently stores the system and component ID of the last connection partner who uploaded or downloaded a mission. When a mission clear command is received by PX4 it sends the Ack to the last learnt connection partner and not to the partner who send the clear request.
Solution
For release notes:
Alternatives
We could keep the mission count being sent but then PX4 needs to keep track of the connection partner who are interested in the mission and sent it to each of them. This was not deemed worthy to implement as each partner has the ability to detect a mission change over the mission ID, which is anyways more robust.
What is still missing is that the jump mission item the jump counter is still only sent for the last connected partner. But instead of sending a mission item the jump counter should have a separate mavlink message which can be broadcasted. Would need to be implemented in a separate PR.
Test coverage
Tested in SIH connecting a groundstation (with AMC and QGC) and an APP using MAVsdk to download the mission. Different possible mission interactions (download, upload, save, clear) were tested.