Update get_connected_devices API and examples to more clearly handle multiple devices of different types#47
Open
peter-mitrano-ar wants to merge 5 commits intomasterfrom
Open
Update get_connected_devices API and examples to more clearly handle multiple devices of different types#47peter-mitrano-ar wants to merge 5 commits intomasterfrom
get_connected_devices API and examples to more clearly handle multiple devices of different types#47peter-mitrano-ar wants to merge 5 commits intomasterfrom
Conversation
Reviewer's GuideThis PR changes get_connected_devices to return (device_path, device_name) tuples instead of plain names and updates the API consumers (examples and CLI) to use paths for device selection and display, improving support for multiple identical devices. Sequence diagram for updated device listing via CLIsequenceDiagram
actor User
participant PySpaceMouseCLI as pyspacemouse_cli
participant API as api
participant HID as hid
User->>PySpaceMouseCLI: list_spacemouse_cli()
PySpaceMouseCLI->>API: get_connected_devices()
API->>API: device_specs = get_device_specs()
API->>HID: hid.find()
HID-->>API: hid_devices
loop match_supported_devices
API->>API: compare vendor_id and product_id with device_specs
API->>API: devices_by_path[hid_device.path] = device_name
end
API-->>PySpaceMouseCLI: list(devices_by_path.items())
alt devices_found
loop for_each_device
PySpaceMouseCLI->>PySpaceMouseCLI: print device_name and device_path
end
else no_devices_found
PySpaceMouseCLI->>PySpaceMouseCLI: print no devices message
end
PySpaceMouseCLI-->>User: console output with names and paths
Flow diagram for updated get_connected_devices return structureflowchart TD
A["Start get_connected_devices"] --> B["Call get_device_specs"]
B --> C["Initialize devices_by_path as empty dict"]
C --> D["Call hid.find to get hid_devices"]
D --> E{Any hid_device remaining?}
E -->|Yes| F["Take next hid_device"]
F --> G["Compare vendor_id and product_id with each spec in device_specs"]
G --> H{Match found?}
H -->|Yes| I["Set devices_by_path[hid_device.path] = device_name"]
H -->|No| E
I --> E
E -->|No| J["Convert devices_by_path.items to list of (device_path, device_name)"]
J --> K["Return list of (device_path, device_name)"]
K --> L["End get_connected_devices"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
examples/09_custom_config.py,base_spec = specs[device_path]looks incorrect sinceget_device_specs()appears to be keyed by device name (as used elsewhere in the file); this should likely index bydevice_nameinstead to avoid a runtime KeyError. - Changing
get_connected_devices()from returning a list of names to a list of(path, name)tuples is a breaking API change; consider providing a backward-compatible helper or alias (e.g., a wrapper that returns just names) to avoid surprising existing users.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `examples/09_custom_config.py`, `base_spec = specs[device_path]` looks incorrect since `get_device_specs()` appears to be keyed by device name (as used elsewhere in the file); this should likely index by `device_name` instead to avoid a runtime KeyError.
- Changing `get_connected_devices()` from returning a list of names to a list of `(path, name)` tuples is a breaking API change; consider providing a backward-compatible helper or alias (e.g., a wrapper that returns just names) to avoid surprising existing users.
## Individual Comments
### Comment 1
<location> `examples/09_custom_config.py:39` </location>
<code_context>
# Get base spec and create modified version
- base_spec = specs[device_name]
+ base_spec = specs[device_path]
print(f"Original mappings: y scale = {base_spec.mappings['y'].scale}")
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `device_path` as the key into `specs` is likely incorrect and will raise at runtime.
In `example_modify_existing`, `specs` still comes from `pyspacemouse.get_device_specs()`, which is keyed by device name, not device path. In `example_invert_rotations` you already use `specs[device_name]`. Using `device_path` here will likely raise a `KeyError`; this should be `base_spec = specs[device_name]` to match the expected `specs` structure.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Previously, the function only returned device type names, e.g.
SpaceMouseCompactorSpaceMousePro. But these names do not uniquely identify the device. If you have two of the same device, you cannot know which one is which.Now, it returns both names and file paths (e.g.
[('SpaceMouseCompact', '/dev/hidraw4')])For example, you can now get print out of the device names (types) and paths: