Enable BATTx_options for AP_Periph builds if useful - Battery summing on a node #28630
+3
−1
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Enable the options parameter for the battery monitor on AP_Periph devices if they could be useful - ie, battery summing is enabled and in use on a node with more than one source for battery inputs.
IamPete1
reviewed
Nov 15, 2024
Single definition for non periph builds or if AP_BATTERY_SUM_ENABLED
peterbarker
requested changes
Nov 15, 2024
There was a problem hiding this comment.
Please squash all commits together and prefix with AP_Battery:
So perhaps
AP_Battery: include OPTIONS parameter on boards with SUM backend
It might be we actually just want to set it to (!defined(HAL_BUILD_AP_PERIPH)` and let the person creating the Perioph hwdef set the option in that hwdef.
|
|
||
| #if !defined(HAL_BUILD_AP_PERIPH) || AP_BATTERY_SUM_ENABLED |
There was a problem hiding this comment.
Please create a fresh define name in AP_Battery_config.h.
| // @Param: OPTIONS | ||
| // @DisplayName: Battery monitor options | ||
| // @Description: This sets options to change the behaviour of the battery monitor | ||
| // @Bitmask: 0:Ignore DroneCAN SoC, 1:MPPT reports input voltage and current, 2:MPPT Powered off when disarmed, 3:MPPT Powered on when armed, 4:MPPT Powered off at boot, 5:MPPT Powered on at boot, 6:Send resistance compensated voltage to GCS, 7:Allow DroneCAN InfoAux to be from a different CAN node, 9:Sum monitor measures minimum voltage instead of average | ||
| // @User: Advanced | ||
| AP_GROUPINFO("OPTIONS", 21, AP_BattMonitor_Params, _options, 0), | ||
| #endif // HAL_BUILD_AP_PERIPH | ||
| #endif |
There was a problem hiding this comment.
include the new define name on this line too
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds a condition for the options parameter to be included in the build for AP_Periph firmware if AP_BATTERY_SUM_ENABLED is true.
This is a niche use case where a user is using a battery monitor sum of other monitor(s) and may want to be able to set the voltage source as minimum rather than average. Current builds do not include this options parameter as it is kept out of AP_Periph builds.
This has been tested on a MATEK L431 Periph CAN Node with the hwdef.dat file modified. I have verified that the options are included when
define AP_BATTERY_SUM_ENABLED 1is included and are not present when removed from the hwdef.dat.Current hardware definitions for the L431 don't have these in by default however it may be beneficial to include them if they don't hinder other functionality. The inclusion of 4 adcs allows for voltage and current to be measured from two battery sources and combining them may be helpful for more complex or non-standard vehicles.
I have included the
defined(HAL_BUILD_AP_PERIPH)as I was unsure of any consequences if the parameter is defined twice.