refactor: modularize pyspacemouse core, introduce new API, and update… #44
Conversation
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
… examples and documentation.
… links to new documentation.
Reviewer's GuideRefactors pyspacemouse into a modular, typed core with a new public API and TOML‑based device definitions, overhauls the CLI and examples to use the new API, and modernizes documentation and build/deploy workflows (README/docs, CONTRIBUTING, troubleshooting, Makefile, GitHub Actions, dependencies). Class diagram for the new modular pyspacemouse core APIclassDiagram
direction LR
class SpaceMouseDevice {
- DeviceInfo _info
- HIDDevice _device
- SpaceMouseState _state
- dict~Axis, float~ _last_axis_time
- StateCallback _callback
- StateCallback _dof_callback
- Sequence~DofCallback~ _dof_callbacks
- ButtonChangeCallback _button_callback
- Sequence~ButtonCallback~ _button_callbacks
- bool _nonblocking
- str _product_name
- str _vendor_name
- str _version_number
- str _serial_number
+ SpaceMouseDevice(info DeviceInfo, device HIDDevice)
+ __enter() SpaceMouseDevice
+ __exit(exc_type, exc_val, exc_tb) void
+ open() void
+ close() void
+ read() SpaceMouseState
+ describe_connection() str
+ set_led(state bool) void
+ set_config(config Config) void
+ configure(callback StateCallback, dof_callback StateCallback, dof_callbacks Sequence~DofCallback~, button_callback ButtonChangeCallback, button_callbacks Sequence~ButtonCallback~) void
+ clear_callbacks() void
+ get_button_name(index int) str
+ info DeviceInfo
+ name str
+ connected bool
+ state SpaceMouseState
+ product_name str
+ vendor_name str
+ version_number str
+ serial_number str
}
class DeviceInfo {
+ str name
+ int vendor_id
+ int product_id
+ tuple~int, int~ led_id
+ float axis_scale
+ dict~Axis, AxisSpec~ mappings
+ tuple~ButtonSpec~ button_specs
+ tuple~str~ button_names
+ hid_id tuple~int, int~
+ bytes_to_read int
+ get_button_name(index int) str
}
class AxisSpec {
+ int channel
+ int byte1
+ int byte2
+ int scale
}
class ButtonSpec {
+ int channel
+ int byte
+ int bit
}
class ButtonState {
+ __int() int
}
ButtonState ..|> list
class SpaceMouseState {
+ float t
+ float x
+ float y
+ float z
+ float roll
+ float pitch
+ float yaw
+ ButtonState buttons
+ __getitem(key str) float
}
class Config {
+ StateCallback callback
+ StateCallback dof_callback
+ Sequence~DofCallback~ dof_callbacks
+ ButtonChangeCallback button_callback
+ Sequence~ButtonCallback~ button_callbacks
+ dof_callback_arr Sequence~DofCallback~
+ button_callback_arr Sequence~ButtonCallback~
}
class ButtonCallback {
+ Union~int, List~int~~ buttons
+ ButtonPressCallback callback
}
class DofCallback {
+ str axis
+ DofValueCallback callback
+ float sleep
+ DofValueCallback callback_minus
+ float filter
}
class ApiModule {
+ get_connected_devices() List~str~
+ get_supported_devices() List~tuple~str, int, int~~
+ get_all_hid_devices() List~tuple~str, str, int, int~~
+ open(callback StateCallback, dof_callback StateCallback, dof_callbacks Sequence~DofCallback~, button_callback ButtonChangeCallback, button_callbacks Sequence~ButtonCallback~, nonblocking bool, device str, device_index int, device_spec DeviceInfo) SpaceMouseDevice
+ open_by_path(path Path, callback StateCallback, dof_callback StateCallback, dof_callbacks Sequence~DofCallback~, button_callback ButtonChangeCallback, button_callbacks Sequence~ButtonCallback~, nonblocking bool, device_spec DeviceInfo) SpaceMouseDevice
+ open_with_config(config Config, nonblocking bool, device str, device_index int) SpaceMouseDevice
}
class LoaderModule {
+ load_device_specs(toml_path Path) dict~str, DeviceInfo~
+ get_device_specs() dict~str, DeviceInfo~
}
class ConfigHelpersModule {
+ create_device_info(name str, vendor_id int, product_id int, mappings dict~str, tuple~int, int, int, int~~, buttons dict~str, tuple~int, int, int~~, led_id tuple~int, int~, axis_scale float) DeviceInfo
+ modify_device_info(base DeviceInfo, name str, invert_axes list~str~, axis_scale float) DeviceInfo
}
class UtilsModule {
+ print_state(state SpaceMouseState) void
+ print_buttons(state SpaceMouseState, buttons List~int~) void
+ silent_callback(state SpaceMouseState) void
}
SpaceMouseDevice --> DeviceInfo : uses
SpaceMouseDevice --> SpaceMouseState : updates
SpaceMouseDevice --> ButtonCallback : uses
SpaceMouseDevice --> DofCallback : uses
SpaceMouseDevice --> Config : uses
DeviceInfo --> AxisSpec : has
DeviceInfo --> ButtonSpec : has
Config --> ButtonCallback : aggregates
Config --> DofCallback : aggregates
ApiModule --> SpaceMouseDevice : creates
ApiModule --> DeviceInfo : uses
ApiModule --> LoaderModule : uses
LoaderModule --> DeviceInfo : constructs
LoaderModule --> AxisSpec : uses
LoaderModule --> ButtonSpec : uses
ConfigHelpersModule --> DeviceInfo : constructs
ConfigHelpersModule --> AxisSpec : constructs
ConfigHelpersModule --> ButtonSpec : constructs
UtilsModule --> SpaceMouseState : reads
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The Linux permissions snippet in README/docs now uses
sudo echo '...' > /etc/udev/rules.d/..., which will usually fail because the redirection is executed without sudo; consider reverting to thetee-based approach you had before or otherwise using a pattern where the redirection also runs with elevated privileges. - The new
open()andopen_by_path()APIs print messages like"{device} found"and"{spec.name} found at {path}"directly to stdout, which can be noisy for library consumers; consider switching these to a logger or removing the prints to keep the API side-effect free.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Linux permissions snippet in README/docs now uses `sudo echo '...' > /etc/udev/rules.d/...`, which will usually fail because the redirection is executed without sudo; consider reverting to the `tee`-based approach you had before or otherwise using a pattern where the redirection also runs with elevated privileges.
- The new `open()` and `open_by_path()` APIs print messages like `"{device} found"` and `"{spec.name} found at {path}"` directly to stdout, which can be noisy for library consumers; consider switching these to a logger or removing the prints to keep the API side-effect free.
## Individual Comments
### Comment 1
<location> `CONTRIBUTING.md:9` </location>
<code_context>
3. Create a pull request to `master` and assign a reviewer.
4. The reviewer will review your code and merge it into `master`.
+## Adding new device
+
+This section describes how to add support for a new SpaceMouse or similar 6-DOF device.
</code_context>
<issue_to_address>
**nitpick (typo):** Consider rephrasing the section title to "Adding a new device" for grammatical correctness.
The current heading reads a bit abrupt; "Adding a new device" is more natural while preserving the meaning.
```suggestion
## Adding a new device
```
</issue_to_address>
### Comment 2
<location> `docs/CONTRIBUTING.md:9` </location>
<code_context>
3. Create a pull request to `master` and assign a reviewer.
4. The reviewer will review your code and merge it into `master`.
+## Adding new device
+
+This section describes how to add support for a new SpaceMouse or similar 6-DOF device.
</code_context>
<issue_to_address>
**nitpick (typo):** Mirror the wording change here to "Adding a new device" for consistency and grammar.
As this file mirrors the root CONTRIBUTING, please update this heading to "Adding a new device" to match and fix the grammar.
```suggestion
## Adding a new device
```
</issue_to_address>
### Comment 3
<location> `README.md:89` </location>
<code_context>
-# Build package
-hatch build
+# Open second device (when multiple same devices)
+with pyspacemouse.open(device_index=1) as device:
+ state = device.read()
</code_context>
<issue_to_address>
**suggestion (typo):** Clarify the wording "when multiple same devices" to be more grammatically clear.
Consider rephrasing to something like "when multiple devices of the same type are connected" or "when multiple identical devices are connected" so the meaning is clearer.
```suggestion
# Open second device when multiple identical devices are connected
```
</issue_to_address>
### Comment 4
<location> `README.md:167` </location>
<code_context>
+ state = device.read()
+```
+
+See [Custom Device Configuration](./mouseApi/index.md#custom-device-configuration) for full API.
+
+## CLI
</code_context>
<issue_to_address>
**issue (bug_risk):** The relative link to `mouseApi/index.md` from the repo root is likely incorrect.
From the repo root, `./mouseApi/index.md` doesn’t exist; the file is at `docs/mouseApi/index.md`. To keep the link working on GitHub and in generated docs, update it to something like `./docs/mouseApi/index.md#custom-device-configuration` or another path that resolves correctly in both places.
</issue_to_address>
### Comment 5
<location> `docs/mouseApi/index.md:52` </location>
<code_context>
* t: timestamp in seconds since the script started.
* x,y,z: translations in the range [-1.0, 1.0]
* roll, pitch, yaw: rotations in the range [-1.0, 1.0].
-* button: list of button states (0 or 1), in order specified in the device specifier
\ No newline at end of file
+* button: list of button states (0 or 1), in order specified in the device specifier
</code_context>
<issue_to_address>
**nitpick (typo):** Use "buttons" instead of "button" to match the plural list being described.
This keeps the description grammatically aligned with the fact it’s a list of button states and consistent with the other plural parameter entries.
```suggestion
* buttons: list of button states (0 or 1), in order specified in the device specifier
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…paths, and minor wording.
JakubAndrysek
force-pushed
the
pyspacemouse-ng
branch
from
4a811ff to
a0093fd
Compare
Collaborator
|
It's cool to see the problems getting addressed, but maybe a bit too much too fast! I can promise you there are dozens of companies/projects using this repository, and pushing such a major change straight to pypi without any review or extensive testing, especially with the widespread use of AI coding tools (I presume) will likely make some people upset. I get that this is just your personal project, so you can do whatever you want! Just sharing some constructive criticism from another perspective. I will hopefully have time to test this update this week and give some feedback 🤞 |
Owner
Author
|
Hi, thanks for the feedback! I understand your concerns about the rapid changes. I have started this project as a personal test during my free time to control my robotic project. I have been planning to make bigger changes for a while, and since there were some active issues and PRs, I decided to make time for them now. I know I can not find any more time for this project, so here is my offer to you, @peter-mitrano-ar, or @v4hn, or anyone else who would like to take over the maintenance of this repository. I can give you access to PyPI and this repository as well. It could move the project forward better than I can. If you also want to revert some of the changes I made this weekend, I will understand. Just let me know what you think. |
Collaborator
|
That's totally understandable, and I appreciate the offer and we appreciate you volunteering your time and effort so far! I would be happy if you add me as a maintainer to this repo, and to pypi. For transparency, while I have done some open source and released one or two packages on pypi for personal use, I don't have a ton of experience. But I do use this project often at work, and also did at my last job, so it seems reasonable. As for the current changes, I still haven't had a chance to test, but still hoping I find time this week 🤞 |
Owner
Author
|
Hi @peter-mitrano-ar, thank you for your interest! I will be happy to add you as a maintainer to this repo and PyPI. Can you please send me an email to email@kubaandrysek.cz with your PyPI username? |
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 pull request introduces several improvements to documentation, build/deployment workflows, and the overall developer experience for the project. The most significant changes are the addition of detailed instructions for adding new SpaceMouse devices, a major update to the
README.mdto provide clearer usage and API documentation, and modernization of the build and deployment process using Hatch and updated GitHub Actions. These changes aim to make the project easier to contribute to, maintain, and use.Documentation Improvements:
Added a comprehensive "Adding new device" section to both
CONTRIBUTING.mdanddocs/CONTRIBUTING.md, detailing how to identify device IDs, analyze HID data, configure axis/button mappings, test, and submit new device support. Also describes custom configuration via Python API.Completely rewrote the
README.mdfor clarity and usability, including quick start examples, API reference, device discovery, usage patterns, CLI commands, dependencies, troubleshooting, and links to further documentation and examples.Build & Deployment Workflow Modernization:
Refactored the
Makefileto use Hatch for building, cleaning, publishing, and version management, replacing legacy commands and adding new targets for modern Python packaging workflows.Updated
deploy-web.yamlGitHub Actions workflow to use newer action versions, addworkflow_dispatchfor manual deployments, improve caching, and switch tomake docs-deployfor documentation deployment.Simplified the CI workflow trigger in
ci.yamlto only run on pull requests tomaster, removing redundant branch triggers.Close #25
Close #43
Close #38
Close #37
Close #42
Thank you very much for the help. I have summarised all the issues and PRs into the refactoring branch.