-
Notifications
You must be signed in to change notification settings - Fork 20.2k
GCS_MAVLink: Add FLIGHT_INFORMATION message #25489
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
GCS_MAVLink: Add FLIGHT_INFORMATION message #25489
Conversation
|
I wonder if the new metrics would be useful as part of |
|
@nexton-winjeel I think this PR is now no-longer-required as we're going to use |
|
@peterbarker: It's not required as-is, but I'll convert it to use |
Long week? |
0becf38 to
c987d57
Compare
Very. Still not my oldest open PR though... |
c987d57 to
faff94b
Compare
faff94b to
d0af9b8
Compare
peterbarker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to get the message updates down from mavlink/mavlink.
I don't think we need to tie this with AP_Stats now. It's not a huge amount of shared state.
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
| // We send time since boot (in micros) rather than a UTC timestamp, as this | ||
| // works better with SITL when SIM_SPEEDUP > 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is out of date once the patches to the mavlink message upstream are brought back.
Instead it should read something like the fields are misnamed...
Also, the flag value is a little problematic with the problematic ARMING_REQUIRE flag on Rover and Plane. There's always an argument that they're not really armed in that state, but just pretend to be.... but still, how does this message work for an ARMING_REQUIRE 0 vehicle?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[...] There's always an argument that they're not really armed in that state, but just pretend to be...
They will set the MAV_MODE_FLAG_SAFETY_ARMED flag of the base_mode field of the HEARTBEAT message, so as far as a GCS knows, they are armed.
[...] how does this message work for an
ARMING_REQUIRE0 vehicle?
It will report arming time as 0 if AP_Arming::arm() is never called. This field shouldn't be treated as a proxy for "is armed", it needs to be read in conjunction with the arming state reported in the HEARTBEAT.
|
Closes #25284 |
d0af9b8 to
f4312c8
Compare
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
| float current, consumed_mah, consumed_wh; | ||
| const int8_t percentage = battery_remaining_pct(instance); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the whitespace changes.
Fix the whitespace problems as you change the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
f4312c8 to
3447e4f
Compare
peterbarker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to fix that type, but LGTM otherwise.
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
| #if AP_MAVLINK_MSG_FLIGHT_INFORMATION_ENABLED | ||
| void GCS_MAVLINK::send_flight_information() | ||
| { | ||
| const uint32_t time_boot_us = AP_HAL::millis64(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const uint32_t time_boot_us = AP_HAL::millis64(); | |
| const uint64_t time_boot_us = AP_HAL::millis64(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
| switch (current_landed_state) { | ||
| case MAV_LANDED_STATE_IN_AIR: | ||
| case MAV_LANDED_STATE_TAKEOFF: | ||
| case MAV_LANDED_STATE_LANDING: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't really need to open braces if you're not declaring variables.
Doesn't need to be fixed, just maybe leave them out in future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defensive programming habit. Fixed while I was in there...
| #endif | ||
|
|
||
| #ifndef AP_MAVLINK_MSG_FLIGHT_INFORMATION_ENABLED | ||
| #define AP_MAVLINK_MSG_FLIGHT_INFORMATION_ENABLED 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the flash size cost of this new feature?
Will it cause us to start overflowing on smaller boards?
Should we add it into minimize_common.inc? (@andyp1per and @Hwurzburg might have ideas on that)
We don't need to change this as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the flash size cost of this new feature?
~728 bytes (tested on CubeOrange).
Target Text (B) Data (B) BSS (B) Total Flash Used (B) Free Flash (B) External Flash Used (B)
----------------------------------------------------------------------------------------------------------
Disabled: bin/arducopter 1840268 3920 258372 1844188 121876 Not Applicable
Enabled: bin/arducopter 1840996 3920 258372 1844916 121148 Not Applicable
Hwurzburg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be disabled by def and a build option...not going to be used by 95% of users....or at least disabled in minimize include...also needs file for build server and features list added
This is going to be a good/useful message for a lot of people. I plan on patching MAVProxy for it to fix the resetting-flight-time problem it has. @nexton-winjeel please add an entry into |
3447e4f to
ea6e560
Compare
Done. |
peterbarker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need a mapping in GCS_MAVLINK::mavlink_id_to_ap_message_id for this message
Also a fix for ms vs us
Consider grabbing my patch from my pr/Winjeel/upstream/add-FLIGHT_DURATION-message branch
libraries/GCS_MAVLink/GCS_Common.cpp
Outdated
|
|
||
| mavlink_msg_flight_information_send( | ||
| chan, | ||
| time_boot_us, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a time_boot_ms field...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
In-progress MAVProxy support is here: ArduPilot/MAVProxy#1438 Works |
|
Ping @nexton-winjeel - might still be viable for 4.6 - but even than had better be soon :-) |
ea6e560 to
8ba89e2
Compare
8ba89e2 to
bd9b7cd
Compare
|
@peterbarker: Can I get you to review this again? |
|
Can we have this merged ? |
| const uint64_t time_boot_micros = AP_HAL::micros64(); | ||
| const uint32_t time_boot_ms = static_cast<uint32_t>(time_boot_micros / 1000); | ||
|
|
||
| // This field is misnamed as `arming_time_utc` in MAVLink. However, it is | ||
| // not a UTC time, it is the microseconds since boot. | ||
| const uint64_t arm_time_us = AP::arming().arm_time_us(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to declare/define variables as late as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, they're declared in the order they are used in the mavlink_msg_flight_information_send function.
|
I can't force-push this branch. I've rebased this branch over here: #29541 - just resolved the conflicts. I intend to merge that other PR once it passes CI. And then the MAVProxy one. |
Apologies - missed this request. |
bd9b7cd to
b365113
Compare
I've rebased this one if you want to merge this instead of the other branch. |
|
Merged, thanks! |
Adds
a newtheFLIGHT_DURATIONFLIGHT_INFORMATIONmessage to report the time that the vehicle armed and took off (if known).This message is needed if connecting a GCS to a vehicle that is already armed/flying, as otherwise these times can't be accurately determined.
Also adds an accessor to
AP_Armingto record the arming time.Requires: ArduPilot/mavlink#336