Conversation
…l support add battery voltage path Volt per cell arm/min in validation constants and schema path coverage add regression tests for arm/min voltage conversions
amilcarlucas
force-pushed
the
BATT_specifications
branch
from
02ed387 to
f341b21
Compare
There was a problem hiding this comment.
Pull request overview
Extends battery specifications and configuration-step processing to support additional battery voltage thresholds and more dynamic parameter workflows (conditional application, add-from-FC, and deletions).
Changes:
- Added new battery cell voltage fields (
arm,min) end-to-end (schema, validation paths, UI update tests, and voltage lookup). - Refactored configuration-step parameter computation to support
ifguards and “add-from-FC shorthand” derived entries. - Introduced
delete_parameterssupport and wired it into both backend computation and UI domain model updates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit_data_model_vehicle_components_validation_constants.py | Updates expected battery voltage paths/types count to include arm and min. |
| tests/test_frontend_tkinter_component_editor.py | Expands mocked voltage fields to include arm and min in editor update tests. |
| tests/test_battery_cell_voltages.py | Adds tests for new recommended voltages and monotonicity constraints. |
| tests/test_backend_filesystem_configuration_steps.py | Adds coverage for _condition_passes, compute_deletions, and add-from-FC shorthand behavior. |
| ardupilot_methodic_configurator/vehicle_components_schema.json | Adds Volt per cell arm and Volt per cell min to the components schema. |
| ardupilot_methodic_configurator/data_model_vehicle_components_validation.py | Adds new voltage paths and sets recommended values when chemistry changes. |
| ardupilot_methodic_configurator/data_model_vehicle_components_import.py | Extends voltage-type lookup and chemistry detection to include arm. |
| ardupilot_methodic_configurator/data_model_parameter_editor.py | Updates UI repopulation to apply delete_parameters and add-from-FC derived entries. |
| ardupilot_methodic_configurator/data_model_configuration_step.py | Adjusts derived-parameter application logic (notably around add-from-FC). |
| ardupilot_methodic_configurator/configuration_steps_strings.py | Adds translatable descriptions for new schema concepts. |
| ardupilot_methodic_configurator/configuration_steps_schema.json | Adds schema support for add-from-FC shorthand and delete_parameters. |
| ardupilot_methodic_configurator/configuration_steps_ArduCopter.json | Uses if guards broadly and introduces delete_parameters blocks. |
| ardupilot_methodic_configurator/battery_cell_voltages.py | Adds per-chemistry recommended arm voltages and a new recommended_min_voltage. |
| ardupilot_methodic_configurator/backend_filesystem_configuration_steps.py | Refactors parameter computation, adds _condition_passes, add-from-FC shorthand, and compute_deletions. |
| ardupilot_methodic_configurator/backend_filesystem.py | Plumbs optional fc_parameters into evaluation and applies computed deletions. |
| ardupilot_methodic_configurator/main.py | Passes FC parameter values into the derived/forced computation phase. |
| return BatteryCell.recommended_crit_voltage(chemistry) | ||
| if voltage_type == "recommended_crit": | ||
| return BatteryCell.recommended_crit_voltage(chemistry) | ||
| return BatteryCell.recommended_min_voltage(chemistry) |
There was a problem hiding this comment.
As written, any unexpected voltage_type (typo/new value) will silently fall through and return recommended_min_voltage, which can mask errors and produce incorrect results. Make the final branch explicit (e.g., elif voltage_type == 'recommended_min': ...), and for unknown values raise a ValueError (or at least log and return nan) so incorrect call sites fail loudly.
| return BatteryCell.recommended_min_voltage(chemistry) | |
| if voltage_type == "recommended_min": | |
| return BatteryCell.recommended_min_voltage(chemistry) | |
| logging_error("Unknown voltage_type '%s' for chemistry '%s'", voltage_type, chemistry) | |
| raise ValueError(f"Unknown voltage_type: {voltage_type!r}") |
| def recommended_min_voltage(chemistry: str) -> float: | ||
| if chemistry not in battery_cell_voltages: | ||
| return nan | ||
| return battery_cell_voltages[chemistry].get("absolute_min", BatteryCell.limit_min_voltage(chemistry)) |
There was a problem hiding this comment.
recommended_min_voltage() currently returns absolute_min (a limit) rather than a recommended minimum threshold. This conflicts with the API name and with how the new Volt per cell min is described/used elsewhere (it reads like an operational threshold, not an absolute limit). Consider either: (1) renaming this to something like limit_min_voltage usage in the callers/UI and store it as a limit, or (2) introducing a separate recommended_min entry per chemistry (with a sane default) and returning that here (falling back to limit_min_voltage).
| return battery_cell_voltages[chemistry].get("absolute_min", BatteryCell.limit_min_voltage(chemistry)) | |
| return battery_cell_voltages[chemistry].get("recommended_min", BatteryCell.limit_min_voltage(chemistry)) |
| }, | ||
| "Volt per cell min": { | ||
| "type": "number", | ||
| "description": "Minimum voltage per cell below which PID scaling stops (e.g., 3.3V)" |
There was a problem hiding this comment.
The Volt per cell min description/example appears inconsistent with the newly introduced behavior/data: in code, recommended_min_voltage() is currently derived from absolute_min (e.g., ~2.5V for LiIon), while the example given is 3.3V (which matches the existing 'crit' examples elsewhere). Either update the description/example to align with the intended semantics (absolute minimum), or adjust the code/data model so min represents the documented operational threshold.
| "description": "Minimum voltage per cell below which PID scaling stops (e.g., 3.3V)" | |
| "description": "Absolute minimum voltage per cell used as the PID scaling floor (e.g., 2.5V for Li-Ion)" |
There was a problem hiding this comment.
This filter drops all derived parameters that are not already present in the file, not just add-from-FC shorthand entries. That prevents configuration steps from introducing new derived parameters via computed New Value (a behavior that is commonly needed for step-driven configuration). A concrete fix is to only skip missing-from-file parameters when the entry is actually add-from-FC shorthand (i.e., when the configuration-step entry had no New Value), otherwise allow computed derived params to be applied/introduced.
| # Only include params that are already in the file when they are add-from-FC shorthand; | |
| # shorthand entries (no explicit New Value, and not yet in file_parameters) are handled | |
| # separately in the UI layer, while computed derived params with a New Value may | |
| # introduce new parameters. | |
| existing_param_names = set(self.local_filesystem.file_parameters.get(selected_file, ParDict()).keys()) | |
| for param_name, param in self.local_filesystem.derived_parameters[selected_file].items(): | |
| new_value = getattr(param, "new_value", None) | |
| if param_name not in existing_param_names and new_value is None: |
There was a problem hiding this comment.
This assertion is very weak: assert result != '' will pass for any non-empty error and won’t protect the intended behavior change (specific lookup-failure messaging for the resolved string). To make the test resistant to regressions, assert on a stable substring (e.g., that it mentions the string value not being found in documentation metadata values) rather than only checking non-emptiness.
| assert "not found in documentation metadata values" in result |
No description provided.