AP_BattMonitor: add null check to mppt power state #30067
Conversation
There was a problem hiding this comment.
LGTM.
@magicrub does this need to be backported to 4.6? Do Bad Things happen on real vehicles?
| @@ -1270,7 +1270,7 @@ void AP_BattMonitor::MPPT_set_powered_state_to_all(const bool power_on) | |||
| // it will supply energy if available. | |||
| void AP_BattMonitor::MPPT_set_powered_state(const uint8_t instance, const bool power_on) | |||
| { | |||
| if (instance < _num_instances) { | |||
| if (instance < _num_instances && drivers[instance] != nullptr) { | |||
There was a problem hiding this comment.
| if (instance < _num_instances && drivers[instance] != nullptr) { | |
| if (instance < ARRAY_SIZE(drivers) && drivers[instance] != nullptr) { |
... makes it clear that the following deref is valid
There was a problem hiding this comment.
I'm not sure we should replace _num_instances with ARRAY_SIZE(drivers) because this will make the single call different from the 10+ other similar checks we do in AP_BattMonitor. Either way may be fine but I think we should be consistent to avoid the possibility of odd bugs creeping in.
BTW the "voltage(uint8_t instance) const" getter is misisng the nullptr check as well it seems
|
@peterbarker @rmackay9 Yes, bad things happen (aka it reboots) so worthwhile to backport to 4.6. I'll make a PR for that. I never noticed this problem until recently because I just added a battery instance AFTER the mppts which are occasionally disabled on test aircraft. Example: Like what Randy says, we use _num_instance a dozen times so lets keep that scheme. ARRAY_SIZE is not accurate. Technically we would want to use MIN(_num_instances, ARRAY_SIZE(drivers)) to limit loop count AND stay within bounds but the null check is still always required |
|
Hi @magicrub, great, thanks for the feedback and offer of a backport PR. We probably don't actually need that because it'll be an easy cherry-pick into 4.6. Either way is fine though |
AP_BattMonitor: add null check to mppt power state