Plane: On Critical Battery, Land #5000
Conversation
|
This is using a separate set of FS_BATT_CRT_* parameters rather then adding a RTL_AUTO_LAND 3 option. I think this is the better way to go as I would prefer a vehicle RTL well before the critical battery level and allow me time to handle it as a user rather then only executing the RTL on a critical level of battery. (Basically this approach to implementation provides a second stage of battery failsafe). |
|
related: #3561 Feature request: Multiple low battery thresholds |
| { | ||
| // exit immediately if no monitors setup | ||
| if (_num_instances == 0 || instance >= _num_instances) { | ||
| return false; | ||
| return BattMonitor_STATUS_NORMAL; |
There was a problem hiding this comment.
there should probably be a BattMonitor_STATUS_UNKNOWN to handle this case but then you can't use != BattMonitor_STATUS_NORMAL in the code
| /// returns BattMonitor_STATUS_NORMAL otherwise. | ||
| enum BattMonitor_Status status(uint8_t instance, float low_voltage, float critical_voltage, float min_capacity_mah, float critical_capacity_mah); | ||
| enum BattMonitor_Status status(float low_voltage, float critical_voltage, float min_capacity_mah, float critical_capacity_mah) { | ||
| return status(AP_BATT_PRIMARY_INSTANCE, low_voltage, critical_voltage, min_capacity_mah, critical_capacity_mah); } |
There was a problem hiding this comment.
when doing multi-line functions in header, move the last } below it.
| } | ||
| } else { | ||
| // acceptable voltage so reset timer | ||
| state[instance].low_voltage_start_ms = 0; | ||
| state[instance].critical_voltage_start_ms = 0; |
There was a problem hiding this comment.
This also needs to be reset on it's own for proper debounce
|
I really like this PR. I'm thinking the param should go into battery library. Low battery should go there too. That would make it common for copter too. We don't want a FS_BATT* but that might be better done on another PR. What I'd really like to see is an auto calculation of mAh per distance and then a failsafe being called for low/critical battery that depends on distance away from home/Land point. |
| (double)battery.voltage(), (double)battery.current_total_mah()); | ||
| if (flight_stage != AP_SpdHgtControl::FLIGHT_LAND_FINAL && | ||
| flight_stage != AP_SpdHgtControl::FLIGHT_LAND_PREFLARE && | ||
| flight_stage != AP_SpdHgtControl::FLIGHT_LAND_APPROACH) { |
There was a problem hiding this comment.
also needs to check for ::FLIGHT_LAND_ABORT. If you're ever in that state then you're already at home trying to land
There was a problem hiding this comment.
@magicrub Should that thought be applied to the low_battery_event() as well? (Line 146 of this same file) I can see the arguments either way on that one...
| aparm.throttle_cruise.load(); | ||
|
|
||
| // Go directly to the landing sequence | ||
| jump_to_landing_sequence(); |
There was a problem hiding this comment.
remove jump_to_landing_seq and auto_state.checked_for_auto. They are handled when RTL is set and RTL_AUTOLAND >0.
There was a problem hiding this comment.
@magicrub this is meant to be independent of RTL_AUTOLAND
There was a problem hiding this comment.
we don't want it to autoland if RTL_AUTOLAND == 0, it should come back home and loiter like a simple RTL.
There was a problem hiding this comment.
RTL_AUTOLAND 3 being this new behaviour would be fine. The objection is that none of the current RTL_AUTOLAND options allow RTL without also doing a landing. I want RTL on low battery, but I want to have time before just going into the landing sequence, and on normal RTL there should be a option that says loiter indefinitely at a rallypoint/home until battery is low, then land. That is primarily what this is meant to address. If you think RTL_AUTOLAND 3 meaning perform a normal RTL until critical battery and only then perform a autoland I'm fine with that.
|
@tridge and @WickedShell how do you feel about moving the low/crit battery level into AP_BATTERY? |
|
@magicrub Would you move the FS_BATT_ params there as well? An advantage I could see in consolidating them into the BattMonitor is that you could have a low/critical event for each battery monitor which would actually be better for VTOL stuff/anything where you really need the second battery. EDIT: Yeah just saw the question about moving both. I'm for it, I would really like to see that in a per BattMonitor instance as well as I think it gives us more options. |
|
yeah, independent low/crit levels for each battery make sense. |
|
It would mean losing the name of FS_BATT_VOLTAGE/FS_BATT_MAH though, even with the translation table, which is the biggest concern I had with that. |
|
If we move FS low/crit voltage to battery library then the GCSs will need to be aware of this change. |
|
This was discussed during the weekly dev call. Your PR is touching on a subject that we've been wanting to do for a while and this PR addresses
So, this PR does a little bit of that but we need to see what we can pull in and what we'd like seen changed to better suit our long-term goal of this library |
|
btw, I did not mean to sound like I/we wanted those things changed in this PR, only that they should be in mind. We need to figure out how to tweak in this PR to not inhibit that future |
|
This needs a rebase. But my gues is that most of this changes have wandered to other PRs. @WickedShell can this be closed ? |
|
This is a case of someone throwing code at us and walking away. |
|
#7213 will be how this is moving forwards. |
Created a critical voltage and critical mAh parameter. When these are crossed, the plane enters landing sequence if present, otherwise RTL. See #3950.