Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up battery, alert and hidpp related code #2428

Merged
merged 5 commits into from
May 23, 2024

Conversation

MattHag
Copy link
Collaborator

@MattHag MattHag commented Apr 13, 2024

Simplifies hierarchy, adds type hints and enhances readability. Fixes capitalization of constants, which is violated by NamedInts.

Related #2273

@MattHag MattHag force-pushed the code_improvements branch from 3b2cb39 to 4719827 Compare April 13, 2024 14:47
@MattHag
Copy link
Collaborator Author

MattHag commented Apr 13, 2024

Don't know what's the issue with the more or less unrelated test failure

Copy link
Collaborator

@pfps pfps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first two commits look fine.

@pfps
Copy link
Collaborator

pfps commented Apr 13, 2024

The third commit looks fine.

@pfps
Copy link
Collaborator

pfps commented Apr 13, 2024

I do not understand the rationale for creating fake classes for a small part of Device.

@MattHag
Copy link
Collaborator Author

MattHag commented Apr 13, 2024

Which fake classes?

@pfps
Copy link
Collaborator

pfps commented Apr 13, 2024

Can IntEnum be used instead of NamedInt everywhere in Solaar? If so, then it is a good idea to do the replacement. If not, then Solaar should stick with NamedInt throughout.

The NamedInts that need to be checked are the ones that have default values, I think.

@pfps
Copy link
Collaborator

pfps commented Apr 13, 2024

DeviceProtocol

@MattHag
Copy link
Collaborator Author

MattHag commented Apr 13, 2024

Can IntEnum be used instead of NamedInt everywhere in Solaar?
Sure, here it was useless to have special implementation, loose proper type hints and linter checks.

The protocol makes clear which functions are used in that module. And that's the foundation for further decoupling to simplify the code complexity and enable boring, but effective tests and implementations.

@MattHag MattHag force-pushed the code_improvements branch 4 times, most recently from 33e1847 to 9df4aab Compare April 16, 2024 22:13
@MattHag
Copy link
Collaborator Author

MattHag commented Apr 17, 2024

@pfps ready

@MattHag MattHag force-pushed the code_improvements branch from 9df4aab to 36e5bb4 Compare April 20, 2024 21:06
@MattHag MattHag force-pushed the code_improvements branch from dc3587d to e5484d3 Compare April 28, 2024 09:44
@pfps
Copy link
Collaborator

pfps commented May 5, 2024

I am not convinced of the need to add the protocols. If there is a PR without these commits, I'll merge it in.

@MattHag
Copy link
Collaborator Author

MattHag commented May 5, 2024

The protocols are important for further clean up of the data read/write related code, as they indicate, which actions are needed in a module. The hidpp, device and base modules have interesting data paths and thus dependencies, which can be tackled on top of that.

@pfps
Copy link
Collaborator

pfps commented May 5, 2024

If the prototols are important for future work split them off for now. When the future work is being performed they can be re-evaluated.

@MattHag MattHag force-pushed the code_improvements branch 4 times, most recently from cd08733 to 6ec8122 Compare May 10, 2024 14:37
@MattHag
Copy link
Collaborator Author

MattHag commented May 10, 2024

Fixed and simplified

@pfps
Copy link
Collaborator

pfps commented May 11, 2024

I'm seeing extra messages when running bin/solaar -dd and a device not showing up.

2024-05-11 11:50:34,378,378     INFO [MainThread] hidapi.udev: Report Descriptor not processed for DEVICE /dev/hidraw0 BID 0003 VID 0000046D PID 0000C52B: open.<locals>.matchfn() takes 3 positional arguments but 5 were given

@pfps
Copy link
Collaborator

pfps commented May 12, 2024

@MattHag Is there a better way to set up the Linux vs MacOS hid code? Would it be a good idea to move the hidapi directory to be a subdirectory of logitech_receiver?

@MattHag
Copy link
Collaborator Author

MattHag commented May 12, 2024

Yes, there's certainly a different one, but it's not the most relevant.

No, all those low level communication interface should be self-contained and have no dependency or tight connection to the main Solaar code /solaar.
The HID package should probably be used like a pip dependency and all Solaar specific code from there should be moved to the solaar package, as described in #2480. The addition of #2481 will make it easier to talk about the purpose of the packages and reason about improvements.

@pfps
Copy link
Collaborator

pfps commented May 13, 2024

The problem is that currently Solaar installs hidapi as a top-level module in .../site-packages and there is a separate hidapi package in PyPI. So either Solaar should just use that package or the Solaar HID code should be moved inside the low-level part of Solaar, i.e., inside logitech_receiver. The question is which way to go and if the HID code is retained how best to interact with the two back-ends.

At the same time, it is probably a good idea to rename logitech_receiver as it isn't just about receivers.

@MattHag MattHag force-pushed the code_improvements branch 4 times, most recently from 3362457 to e73c1ab Compare May 15, 2024 21:26
@MattHag
Copy link
Collaborator Author

MattHag commented May 15, 2024

I'm seeing extra messages when running bin/solaar -dd and a device not showing up.

2024-05-11 11:50:34,378,378     INFO [MainThread] hidapi.udev: Report Descriptor not processed for DEVICE /dev/hidraw0 BID 0003 VID 0000046D PID 0000C52B: open.<locals>.matchfn() takes 3 positional arguments but 5 were given

Kept the fileopen alias, now it will work again.

@MattHag MattHag force-pushed the code_improvements branch from e73c1ab to cbc45c5 Compare May 15, 2024 21:27
MattHag added 5 commits May 18, 2024 22:35
Replace NamedTuples with more flexible dataclass, which also support
type hints. Introduce enums to replace constant strings, which need to
be kept in sync. Also enhances interfaces by limiting it to the enum
values. Remove unused variables.
Replace unnecessary NamedInts in favour of default data types.
Simplify interfaces by reducing possible input from strings to members
of an enum.
@MattHag MattHag force-pushed the code_improvements branch from 8bc5af0 to b157412 Compare May 18, 2024 21:29
@MattHag MattHag requested a review from pfps May 19, 2024 07:56
Copy link
Collaborator

@pfps pfps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks acceptable

@pfps pfps merged commit c9dc232 into pwr-Solaar:master May 23, 2024
3 checks passed
rathann added a commit to rathann/Solaar that referenced this pull request Jan 6, 2025
It's imported unconditionally in:
lib/solaar/ui/about/presenter.py:19
lib/logitech_receiver/hidpp10.py:22
lib/logitech_receiver/hidpp20.py:35

Fixes 469c04f (committed as part of pwr-Solaar#2428).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants