Refactor bus wire API to use driver handles#64
Open
xross wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Pyxsim.bus resolved-wire model to use object-oriented “driver handles” bound to wires via a Bus, enabling call sites like controller.sda.release() instead of passing driver identifiers into every operation.
Changes:
- Introduces
Bus,BusDriver, andBusWireDriverto bind drivers to per-wire handles exposed as real attributes (setattr()). - Removes the old string/enum-based
BusWirepublic driver-operation API in favor of internal_drive/_release/_set_driver. - Updates and expands unit tests to cover the new OO binding, uniqueness validation, and attribute exposure behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/python/Pyxsim/bus.py |
Adds OO driver-handle API (Bus, BusDriver, BusWireDriver) and refactors BusWire driver operations into internal methods. |
tests/test_bus_line_model.py |
Migrates tests to OO call sites and adds new tests for binding/validation and per-driver behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+264
to
+266
| # Validate wire name is a valid Python identifier | ||
| if not wire.name.isidentifier(): | ||
| raise ValueError(f"Wire name must be a valid Python identifier: '{wire.name}'") |
Comment on lines
+268
to
+274
| # Check for conflicts with existing BusDriver attributes | ||
| if hasattr(self, wire.name): | ||
| raise ValueError(f"Wire name '{wire.name}' conflicts with BusDriver attribute") | ||
|
|
||
| # Check for duplicate binding | ||
| if wire.name in self._wires: | ||
| raise ValueError(f"Driver '{self.name}' is already bound to wire '{wire.name}'") |
Comment on lines
+286
to
+303
| # Validate wire name uniqueness first | ||
| wire_names = [w.name for w in wires] | ||
| if len(wire_names) != len(set(wire_names)): | ||
| duplicates = [name for name in wire_names if wire_names.count(name) > 1] | ||
| raise ValueError(f"Duplicate wire names: {set(duplicates)}") | ||
|
|
||
| # Validate driver name uniqueness | ||
| driver_names = [d.name for d in drivers] | ||
| if len(driver_names) != len(set(driver_names)): | ||
| duplicates = [name for name in driver_names if driver_names.count(name) > 1] | ||
| raise ValueError(f"Duplicate driver names: {set(duplicates)}") | ||
|
|
||
| # Validate global uniqueness across all wire and driver names | ||
| all_names = wire_names + driver_names | ||
| if len(all_names) != len(set(all_names)): | ||
| duplicates = [name for name in all_names if all_names.count(name) > 1] | ||
| raise ValueError(f"Duplicate names found (wires and drivers must have globally unique names): {set(duplicates)}") | ||
|
|
This was referenced Jun 18, 2026
Merged
humphrey-xmos
approved these changes
Jun 19, 2026
humphrey-xmos
left a comment
humphrey-xmos
left a comment
There was a problem hiding this comment.
Looks good, yes it does create more objects and more setup code, but hopefully its easier to extend.
| controller.sda.drive(value) | ||
|
|
||
|
|
||
| # New OO API-specific tests |
There was a problem hiding this comment.
Does this also support multiple targets?
This file contains hidden or 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
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.
Refactor BusWire driver API to use OO driver handles
There was a desire to make the resolved-wire API more object-oriented and easier to read at call sites. Instead of passing a driver identifier into every wire operation, drivers are now registered against wires through a
Bus, which binds each driver to per-wire handles.This adds some setup and indirection internally: the bus topology is declared up front, and each
BusDrivergets real per-wire attributes viasetattr()so call sites can usecontroller.sdadirectly. The underlying wire model still tracks driver state internally, but callers no longer pass string/enum driver names for each operation.Old:
New:
The refactor removes the old string-based driver API, validates wire/driver names during Bus construction, and keeps the existing resolution/clash-detection behavior unchanged.
There are tradeoffs. The new API makes the operation site nicer and more self-describing, but it adds more objects, upfront bus binding, and some internal indirection. To support the controller.sda style without getattr, the implementation uses setattr() to expose bound wire handles as real attributes on each BusDriver. The underlying wire model still tracks per-driver state internally.
One alternative would be index-based access, for example controller[SDA].release() or controller.wire(SDA).release(). That would avoid dynamic attributes and the Python-identifier restriction on wire names, but it is less readable for protocol wires like scl/sda.
So this PR is partly a design tradeoff: if the extra setup and indirection are not worth the cleaner call sites, we may decide to keep the older string/enum-based API instead as it is flatter and easier to understand internally.