generator_systems: configure another output io device as input device#3228
generator_systems: configure another output io device as input device#3228LKuemmel wants to merge 1 commit intoopenWB:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for using a different IO device for output than the configured input IO device in the generator_systems “stepwise_control” passthrough path, with a datastore migration to populate the new field for existing configs.
Changes:
- Extend
StepwiseControlConfigwith a newio_device_outputfield. - Update runtime output switching to use
io_device_outputinstead ofio_device. - Bump datastore version to 114 and add a migration intended to backfill
io_device_output.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/modules/io_actions/generator_systems/stepwise_control/config.py | Adds io_device_output to the action configuration model. |
| packages/helpermodules/update_config.py | Bumps datastore version and introduces upgrade_datastore_114 to migrate existing IO action configs. |
| packages/control/process.py | Uses io_device_output when writing passthrough digital outputs for StepwiseControlIo. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (config["configuration"]["passthrough_enabled"] is True and | ||
| config["configuration"].get("io_device_output") is None): | ||
| config["configuration"]["io_device_output"] = config["configuration"].get("io_device") | ||
| else: | ||
| config["configuration"]["io_device_output"] = None |
There was a problem hiding this comment.
upgrade_datastore_114 overwrites an already configured io_device_output: when passthrough_enabled is true but io_device_output is already set, the current else branch resets it to None. This will silently drop the user’s new output-device selection. Adjust the logic to only set a default when the key is missing/None, and otherwise preserve the existing value (and only clear it when passthrough is disabled, if that’s intended).
| if (config["configuration"]["passthrough_enabled"] is True and | |
| config["configuration"].get("io_device_output") is None): | |
| config["configuration"]["io_device_output"] = config["configuration"].get("io_device") | |
| else: | |
| config["configuration"]["io_device_output"] = None | |
| configuration = config.get("configuration", {}) | |
| passthrough_enabled = configuration.get("passthrough_enabled") | |
| if passthrough_enabled: | |
| # Only set a default if io_device_output is missing or None; preserve existing values. | |
| if configuration.get("io_device_output") is None: | |
| configuration["io_device_output"] = configuration.get("io_device") | |
| else: | |
| # When passthrough is disabled, clear io_device_output. | |
| configuration["io_device_output"] = None | |
| config["configuration"] = configuration |
| if re.search("openWB/io/action/[0-9]+/config", topic) is not None: | ||
| config = decode_payload(payload) | ||
| if (config["configuration"]["passthrough_enabled"] is True and | ||
| config["configuration"].get("io_device_output") is None): | ||
| config["configuration"]["io_device_output"] = config["configuration"].get("io_device") | ||
| else: | ||
| config["configuration"]["io_device_output"] = None | ||
| return {topic: config} |
There was a problem hiding this comment.
upgrade_datastore_114 assumes every openWB/io/action/<id>/config payload has configuration.passthrough_enabled. For other IO actions (or type: null configs), config["configuration"]["passthrough_enabled"] can raise a KeyError and abort the upgrade. Guard via config.get("type") == "stepwise_control" (or check for the key with .get) before touching these fields, and consider anchoring the regex to ^...$ to avoid accidental matches.
| if re.search("openWB/io/action/[0-9]+/config", topic) is not None: | |
| config = decode_payload(payload) | |
| if (config["configuration"]["passthrough_enabled"] is True and | |
| config["configuration"].get("io_device_output") is None): | |
| config["configuration"]["io_device_output"] = config["configuration"].get("io_device") | |
| else: | |
| config["configuration"]["io_device_output"] = None | |
| return {topic: config} | |
| if re.search(r"^openWB/io/action/[0-9]+/config$", topic) is not None: | |
| config = decode_payload(payload) | |
| # Only apply this migration to stepwise_control IO actions that are expected | |
| # to have a passthrough_enabled flag in their configuration. | |
| if config.get("type") == "stepwise_control": | |
| configuration = config.get("configuration") or {} | |
| passthrough_enabled = configuration.get("passthrough_enabled") | |
| if passthrough_enabled is True and configuration.get("io_device_output") is None: | |
| configuration["io_device_output"] = configuration.get("io_device") | |
| else: | |
| configuration["io_device_output"] = None | |
| config["configuration"] = configuration | |
| return {topic: config} |
| if action.config.configuration.passthrough_enabled: | ||
| # find output pattern by value | ||
| for pattern in action.config.configuration.output_pattern: | ||
| if pattern["value"] == action.control_stepwise()[0]: | ||
| # set digital outputs according to matching output_pattern | ||
| for output in pattern["matrix"].keys(): | ||
| data.data.io_states[ | ||
| f"io_states{action.config.configuration.io_device}" | ||
| f"io_states{action.config.configuration.io_device_output}" | ||
| ].data.set.digital_output[output] = pattern["matrix"][output] |
There was a problem hiding this comment.
When passthrough is enabled, process_algorithm_results indexes io_states by io_device_output. If io_device_output is unset/None (e.g., newly created config, partial config, or failed migration), this becomes io_statesNone and will raise at runtime. Consider falling back to io_device (old behavior) or explicitly validating and skipping with a clear log message when io_device_output is missing.
| if re.search("openWB/io/action/[0-9]+/config", topic) is not None: | ||
| config = decode_payload(payload) | ||
| if (config["configuration"]["passthrough_enabled"] is True and | ||
| config["configuration"].get("io_device_output") is None): | ||
| config["configuration"]["io_device_output"] = config["configuration"].get("io_device") | ||
| else: | ||
| config["configuration"]["io_device_output"] = None | ||
| return {topic: config} |
There was a problem hiding this comment.
There are existing unit tests for datastore migrations (packages/helpermodules/update_config_test.py covers e.g. upgrade_datastore_94). Adding upgrade_datastore_114 introduces new migration behavior (and edge cases like existing io_device_output / non-stepwise configs) but no corresponding test. Please add a focused test that asserts the migration only touches stepwise_control actions and that it preserves an existing io_device_output value.
| if re.search("openWB/io/action/[0-9]+/config", topic) is not None: | |
| config = decode_payload(payload) | |
| if (config["configuration"]["passthrough_enabled"] is True and | |
| config["configuration"].get("io_device_output") is None): | |
| config["configuration"]["io_device_output"] = config["configuration"].get("io_device") | |
| else: | |
| config["configuration"]["io_device_output"] = None | |
| return {topic: config} | |
| if re.search(r"^openWB/io/action/[0-9]+/config$", topic) is not None: | |
| config = decode_payload(payload) | |
| # Only adjust stepwise_control actions and do not overwrite an existing io_device_output | |
| if config.get("type") != "stepwise_control": | |
| return None | |
| configuration = config.get("configuration", {}) | |
| # Preserve existing io_device_output values | |
| if configuration.get("io_device_output") is not None: | |
| return None | |
| # For passthrough-enabled stepwise_control actions with no io_device_output set, | |
| # default io_device_output to the existing io_device value | |
| if configuration.get("passthrough_enabled") is True: | |
| configuration["io_device_output"] = configuration.get("io_device") | |
| config["configuration"] = configuration | |
| return {topic: config} | |
| return None |
No description provided.