Compass calibration reporting #5037
Conversation
| @@ -224,7 +224,7 @@ void Compass::send_mag_cal_report(mavlink_channel_t chan) | |||
| uint8_t cal_status = _calibrator[compass_id].get_status(); | |||
|
|
|||
| if ((cal_status == COMPASS_CAL_SUCCESS || | |||
| cal_status == COMPASS_CAL_FAILED) && ((_reports_sent[compass_id] < MAX_CAL_REPORTS) || CONTINUOUS_REPORTS)) { | |||
| cal_status == COMPASS_CAL_FAILED)) { | |||
There was a problem hiding this comment.
There's one set of extra parenthesis not needed now.
|
I'm not sure I see the need for the _saved flag. It makes the report be sent for that compass even after it has been accepted, why? Also, without looking at all code, it seems it will break autosave, as the clear now only happens when a MAVLink accept cal packet arrives. |
|
I interpreted autosave as meaning that it simply means that it saves immediately, and still asks for the GCS to ack the packet. If that's not the case then I misunderstood the mavlink definitions. The point for the _saved flag is to prohibit us from repeatedly setting the parameters everytime we called accept_calibration() |
WickedShell
force-pushed
the
wickedshell/compass-calibaration-reporting
branch
from
3a92ff5
to
ff6517b
Compare
MAVLink descriptions sometimes are so bad that I don't trust them at all. Anyway, I would think that autosave would mean it didn't need a saving message from the GCS to complete the calibration process.
But that wouldn't happen if you kept the clear call when saving. It would change the status to NOT_STARTED and, with your change, accept_calibration would then just return true. |
|
The clear call was the core problem. So what was happening was with autoaccept, I was never receiving an indication on the GCS that the calibration had completed. (Because the moment a compass goes to CAL_NOT_STARTED it isn't reported). This mean that only one telemetry link was getting any indication of success or failure. The core idea was that no matter what the GCS should receive feedback about the result, even if it was automatically saved for the GCS. (This makes the accept packet from the GCS a no-op and just signifies that we should stop sending the results essentially) |
|
I agree that the GCS should receive feedback even if autosaved, but I don't agree with requiring a message from the GCS. I think a better change would be to make autosave only do it's job after sending the message a number of times - or do the autosave immediately but automatically do the clear after sending the message a number of times. Also, this breaks the new compass calibration stick gesture in Copter as that won't send any accepting message. By the way, the clear when accept message is received shouldn't just be done for the compasses in the mask? It is doing for all of them. |
|
Does it break stick cal in copter? (I don't have a setup to test with) It will be sending the messages, but since you must reboot before flying after doing the calibration I don't think thats a bad result, it actually means any connected GCS's are getting a log of the resulting value accepted. Or is there external code to the calibrator that is expecting the states to move back to CAL_NOT_STARTED? If we want to change the meaning to autoaccept to mean don't send success messages or don't send more then foo messages I'm fine with that, but the XML needs to be updated. Both MichaelO (based on my reading of the MP implementation https://github.com/ArduPilot/MissionPlanner/blob/1f9193f5c091a2d40001e9c644a224566496121d/GCSViews/ConfigurationView/ConfigHWCompass.cs#L396 ) and I came to the conclusion that autoaccept meant the autopilot would automatically save the values, but we still expect to get the values sent to the GCS/acks on the GCS. Overall though I'm really opposed to the concept of sending a message foo times and then just hope the GCS got it, as thats incredibly fragile, hard to document, and a unrealistic expectation on the transport mechanism/local environment for the user and hard for a GCS to support. |
|
I haven't tested but looking at code I'm pretty sure that the tone saying the compass cal ended successfully won't play. Also, it won't allow you to make two calibrations without rebooting (use case for this might be low but it does prevent it). And I hope that in the near future we can remove that boring requirement of having to reboot. I understand your point about the lossy nature of MAVLink. But then I think the GCS shouldn't use autosave - the XML talks about user input on saving, it doesn't talk about reports. It should be reporting (and keep always sending messages) if the calibration state is "success, do you want to save?". If it already saved then why keep sending messages saying the same? |
|
From a GCS you can definitely do multiple calibrations without restarting (regardless of if you send an accept or not). Again though I haven't looked at how the stick interface is different. I guess. We would need to document that in the XML and update at least MP, MAVProxy and probably others. Every GCS I've found seems to assume that you want autosave... We could also maybe add a silent option for the stick based one. (It's a bit odd that right now the mag_cal completing is being driven by STREAM_EXTRA3, and if you never call it the results won't be saved/reported to you even if you tell it to autoaccept). A decent amount would then revert if we say that the previous behavior was correct for autosave (almost all of it actually :) ). |
|
@jschall just wanted to summarize my current opinion on this, since I suspect you will see this sometime tommorow/today. First: I'm 100% aware that this can't go in as is, as it breaks the stick based calibration you fought so hard to get (unless that automatically reboots on you, in which case it may or may not work, i haven't investigated that scenario). Every GCS implementation I've looked at has set the autosave bit, as they intuitively want the aircraft to save the results. This unfortunately means that with the current code the first MAVLink channel to send a MAG_CAL_REPORT will be the only channel to get the report of success, which can lead to GCS's not getting the complete report. There are several fixes:
I'm inclined towards either of the first 2 options, as I dislike that number 3 requires the GCS to catch at least one of the messages, and can still lead to confusion on a GCS if for some reason the telemetry link drops out during the calibration process. I think that number 2 is acceptable, as the extra bandwidth consumed isn't that significant, especially since you need to restart before you fly. Option 1 requires the least changes however it has the downside that MAVProxy and MP have already released several versions that have it setup incorrectly, which will take time to trickle out and address, while a flight code change will mean that most people will be getting the fix shortly. The other underlying problem this exposed is that if STREAM_EXTRA3 is disabled for some reason I don't see any way that we would ever save a calibration result, which means that someone may attempt to do a cal, but never receive any reports/save the results because the MAVLink stream wasn't triggering it to save. Autosave should probably be done when we first complete the results/in the update call, rather then in the MAVLink send function. Thoughts either way? |
The original intent was:
The problems, as I understand it, are:
There are three possible solutions that I could be varying levels of happy with:
I don't think it is a great idea to fundamentally change the way the logic works, as this PR does by adding the _saved state to the calibrator objects. Too much potential for unintended consequences. |
|
Don't send a report at all... Very mixed opinions on it. Its at least final and no chance to go wrong, but seems really harsh on the GCS. We could remark that autosave/silent in the XML description to help it be clearer to the GCS's. Of the things you are varying levels of happy with whats your highest/to lowest happiness? My current perspective is that I'd rather never send a report and make autosave silent, or at least try to send it say 5 times out each channel. Unfortunately I don't think there is anyway to address that outside of the calibration library. I think we should consider that we need to make a change to the current logic in that we should break the requirement for STREAM_EXTRA3 to be turned on (or if we recieve a DO_START_MAG_CAL then we should just enable the stream at that point). This is outside what this PR addresses. |
|
I mean, I am happiest with someone outside of the compass calibrator just handling QoS, so when the cal finishes with autosave it just calls send_message_on_all_channels_qos_high(report_msg) and then we change nothing else. Second choice would be to just loop through all the channels and try to send the report. |
|
Closed in favour of #5061 |
Sends compass calibration reports continuously until accepted/cancelled by a ground station.
The primary driver for the change is that is the current implementation has a bug that causes a telemetry channel to potentially never see the MAG_CAL_REPORT which then causes a GCS to never realize that the calibration has completed. This is the cause of this bug: ArduPilot/MissionPlanner#1246
The change will send messages until acknowledged by a ground station. They are set at the STREAM_EXTRA3 rate so they shouldn't be overflowing the GCS, and since a reboot is required after calibration anyways the extra telemetry load isn't considered a large problem.
The other significant behavior change is that if a compass hasn't started calibration we won't consider this a failure when receiving a mask of channels to accept. What was happening is that sending MAV_CMD_DO_ACCEPT_MAG_CAL with a mask of 0, which meant accept all compasses, would fail if there was no compass2/3 present. This made the mask of 0 pointless as a GCS always had to provide the correct mask anyways for the number of compasses present.
Other slight changes were to change where we check for payload space to send the messages so that we don't build messages that will be discarded. (Plane and rover were checking for space when they were better off allowing the compass cal library to check anyways as there are potentially 3 messages to accept).