Consolidate device type logic & add --hardware option to codexctl list
#141
Conversation
The device type (RM1, RM2, RMPP) was determined in multiple places across the code based on either the contents of the /sys/devices/soc0/machine file, or from the user-provided --hardware argument. This made it error-prone, and new commands or features also needed to come with their own logic for device type determination. This commit introduces the HardwareType enum, and its associated `parse` function, so that the logic to determine the device type lives in a single location.
WalkthroughThe changes replace string-based hardware type handling with a strongly typed Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant HardwareType
participant DeviceManager
participant UpdateManager
User->>CLI: list --hardware <type>
CLI->>HardwareType: parse(<type>)
HardwareType-->>CLI: HardwareType.<value> or None
CLI->>UpdateManager: get_latest_version(HardwareType)
UpdateManager-->>CLI: version info (filtered by hardware)
CLI->>User: display filtered version list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–25 minutes Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
codexctl/__init__.py (1)
58-61: Simplify conditional with ternary operatorThe static analysis tool correctly suggests simplifying this if-else block.
- if hw: - remarkable_version = HardwareType.parse(hw) - else: - remarkable_version = None + remarkable_version = HardwareType.parse(hw) if hw else None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
codexctl/__init__.py(5 hunks)codexctl/device.py(5 hunks)codexctl/updates.py(6 hunks)tests/test.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
codexctl/updates.py (1)
codexctl/device.py (4)
HardwareType(19-73)toltec_type(66-73)old_download_hw(36-43)new_download_hw(46-53)
tests/test.py (2)
codexctl/device.py (1)
HardwareType(19-73)codexctl/updates.py (1)
get_toltec_version(137-161)
🪛 Ruff (0.12.2)
codexctl/__init__.py
58-61: Use ternary operator remarkable_version = HardwareType.parse(hw) if hw else None instead of if-else-block
Replace if-else-block with remarkable_version = HardwareType.parse(hw) if hw else None
(SIM108)
🔇 Additional comments (5)
tests/test.py (1)
9-9: LGTM! Correct adoption of HardwareType enumThe test file has been properly updated to use the
HardwareTypeenum instead of string literals for device type specification. This change aligns perfectly with the PR's objective of consolidating device type logic.Also applies to: 181-183, 187-194
codexctl/__init__.py (1)
26-26: Excellent implementation of hardware type consolidationThe changes successfully integrate the
HardwareTypeenum throughout the file:
- Proper error handling and fallback logic for hardware type parsing
- Clean implementation of hardware-specific version filtering in the list command
- Correct replacement of string-based checks with enum comparisons
- Addition of the
--hardwareCLI option with appropriate aliasesThese changes significantly improve type safety and maintainability.
Also applies to: 54-57, 79-87, 91-91, 230-230, 495-502
codexctl/device.py (2)
28-28: Verify the "tests" alias for RM2Including "tests" as an alias for RM2 could be confusing and potentially cause issues if test environments need different handling. Is this intentional for test compatibility?
1-1: Well-structured HardwareType enum implementationExcellent consolidation of device type logic:
- The enum provides a single source of truth for hardware types
- Clear separation of concerns with different properties for various update contexts
- Proper error handling with descriptive ValueError messages for unsupported operations
- Clean integration throughout the DeviceManager class
The implementation successfully achieves the PR's goal of centralizing device type determination.
Also applies to: 19-74, 113-113, 444-444, 518-518
codexctl/updates.py (1)
14-14: Clean refactoring to use HardwareType enumThe updates module has been successfully refactored to use the
HardwareTypeenum:
- All string-based device type checks replaced with type-safe enum matching
- Proper use of hardware type properties for URL and filename construction
- Exhaustive match statements ensure all hardware types are handled
- The explicit string conversion for
download_folderimproves clarityThese changes align perfectly with the PR's objective and improve maintainability.
Also applies to: 118-135, 164-225
… clearer it can't return None
| self.hardware = "reMarkable1" | ||
| else: | ||
| self.hardware = "reMarkable2" | ||
| self.hardware = HardwareType.parse(machine_contents) |
There was a problem hiding this comment.
This appears to have changed handling, previously self.hardware would not be set if the file was not found, now it's defaulting to the rM2.
There was a problem hiding this comment.
I'm not sure I follow. I read this logic as:
- remote device:
with ftp.file(...) as file: throws aFileNotFoundErrorif the file is not found (no change from previous logic)- if the file is found,
machine_contentscontains its contents (no change from previous logic)
- local device:
- same as above but if the file is not found the exception is caught and
machine_contentswill be set to"tests"(no change from previous logic)
- same as above but if the file is not found the exception is caught and
HardwareType.parsewill either set the hardware type, or throw aValueErrorif it doesn't understand themachine_contents(changes from previous logic: previously, it would default to "reMarkable2")
In my eyes the only difference is that now the logic will throw a ValueError if it doesn't understand the machine_contents (which will be set as before). Do you want the code to go ahead with self.hardware unset, or set to None, or something else?
There was a problem hiding this comment.
If machine_contents was tests in the previous code, self.hardware would not be set. Now it's being set to HardwareType.RM2.
Likely the way it was previously was to avoid raising an exception when instantiating the DeviceManager instance, but still preserving that we don't know what device type it is.
There was a problem hiding this comment.
Hmm, the way I read it, previously if machine_contents was tests, if would fall under the else: branch of the if "reMarkable Ferrari" in machine_contents test and set self.hardware = "reMarkable2".
But either way, I agree with your other comment that off-device tests should be handled differently than by piggy-backing onto the hardware detection logic. I've pushed a commit that relies on a mock object instead, and no longer needs the "tests" hack.
There was a problem hiding this comment.
You are right, I seem to have misread that every single time as another elseif, whoops.
codexctl/device.py
Outdated
| def parse(cls, device_type: str) -> "HardwareType": | ||
| if device_type.lower() in ("pp", "pro", "rmpp", "ferrari", "remarkable ferrari"): | ||
| return cls.RMPP | ||
| elif device_type.lower() in ("2", "rm2", "remarkable 2", "tests"): |
There was a problem hiding this comment.
| elif device_type.lower() in ("2", "rm2", "remarkable 2", "tests"): | |
| elif device_type.lower() in ("2", "rm2", "remarkable 2"): | |
There was a problem hiding this comment.
"tests" was in there because the code in DeviceManager.__init__ will set machine_contents to "tests" (and pass it to HardwareType.parse) if /sys/devices/soc0/machine is not found, in the local device case. I assumed this was done to support running tests on a PC rather than on the reMarkable itself.
Removing this value causes tests/test.py to fail with this exception:
❯ python ./tests/test.py
Traceback (most recent call last):
File "/home/home/Code/misc/codexctl/./tests/test.py", line 13, in <module>
set_server_config = DeviceManager().set_server_config
~~~~~~~~~~~~~^^
File "/home/home/Code/misc/codexctl/codexctl/device.py", line 113, in __init__
self.hardware = HardwareType.parse(machine_contents)
~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^
File "/home/home/Code/misc/codexctl/codexctl/device.py", line 33, in parse
raise ValueError(f"Unknown hardware version: {device_type} (rm1, rm2, rmpp)")
ValueError: Unknown hardware version: tests (rm1, rm2, rmpp)
There was a problem hiding this comment.
There are no local tests outside of test.py or basic sanity checks that don't look to replicate running on device. @Jayy001 may have done some local testing by forcing some things, but that would not be good to leave enabled in the main application, and is better handled by either hiding it behind an environment variable to turn on, or build into test.py to trigger the explicit logic.
There was a problem hiding this comment.
Agreed, I've handled this in test.py by using a mock object.
Also add test for RMPP toltec update, which should fail. Co-authored-by: Nathaniel van Diepen <Eeems@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test.py (1)
62-77: Fix unused variable in exception handling.The context manager implementation is correct, but there's an unused variable assignment.
Apply this diff to fix the unused variable:
- except expected as e: + except expected: print("pass") return - except Exception as e: - got = e.__class__.__name__ + except Exception as ex: + got = ex.__class__.__name__codexctl/updates.py (1)
137-164: LGTM! Proper enum-based toltec handling.The method correctly uses the
HardwareType.toltec_typeproperty and handles the ValueError appropriately by converting it to SystemExit. This aligns with the previous review feedback.Consider using exception chaining for better error traceability:
- except ValueError as ex: - raise SystemExit(*ex.args) + except ValueError as ex: + raise SystemExit(*ex.args) from ex
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codexctl/updates.py(6 hunks)tests/test.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
codexctl/updates.py
150-150: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
tests/test.py
69-69: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (9)
tests/test.py (4)
9-9: LGTM! Import updated correctly.The addition of
HardwareTypeto the import statement properly supports the enum-based hardware type handling throughout the test file.
197-198: LGTM! Enum usage correctly implemented.The replacement of string literals with
HardwareTypeenum members properly aligns with the consolidated device type logic.
203-203: LGTM! Consistent enum usage.The
get_toltec_versioncalls correctly useHardwareTypeenum members instead of string literals.Also applies to: 208-208
211-212: LGTM! Proper exception testing added.The new test correctly verifies that
get_toltec_versionraisesSystemExitfor RMPP hardware, which is expected since toltec is not supported on ReMarkable Paper Pro.codexctl/updates.py (5)
14-14: LGTM! Import correctly added.The
HardwareTypeimport properly supports the enum-based refactoring throughout the file.
118-135: LGTM! Clean enum-based refactoring.The method signature and implementation correctly use the
HardwareTypeenum with a clean match statement for version selection. This eliminates string-based checks and improves type safety.
166-167: LGTM! Method signature properly updated.The parameter type change to
HardwareTypeenum and renaming tohardware_typecorrectly aligns with the consolidated device type logic.
181-188: LGTM! Explicit string conversion added.The explicit
str()conversion of the Path object ensures compatibility with downstream operations that expect string paths.
199-227: LGTM! Excellent enum-based download logic.The refactoring successfully replaces string-based conditionals with clean enum dispatch. Using
hardware_type.old_download_hwandhardware_type.new_download_hwproperties centralizes hardware-specific filename logic and improves maintainability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test.py (1)
66-81: Fix unused variable in exception handling.The context manager is well-implemented, but there's an unused variable
eon line 73 that should be addressed.Apply this diff to fix the unused variable:
- except expected as e: + except expected: print("pass") return
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
codexctl/__init__.py(5 hunks)codexctl/device.py(5 hunks)tests/test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- codexctl/init.py
- codexctl/device.py
🧰 Additional context used
🪛 Ruff (0.12.2)
tests/test.py
73-73: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (5)
tests/test.py (5)
6-6: LGTM! Import changes support the HardwareType refactoring.The addition of
NonCallableMockandHardwareTypeimports aligns perfectly with the PR's objective to consolidate device type logic using a strongly typed enum.Also applies to: 10-10
14-17: LGTM! Mock setup properly supports the refactored set_server_config method.The
NonCallableMockwith only theloggerfield and the class method reference correctly accommodate the change from instance method to class method call.
86-86: LGTM! Correctly updated to call set_server_config as a class method.The addition of
device_manageras the first argument properly reflects the refactoring from instance method to class method.
201-202: LGTM! Perfect implementation of HardwareType enum usage.The replacement of string literals ("reMarkable 1", "reMarkable 2") with
HardwareType.RM1andHardwareType.RM2perfectly aligns with the PR's objective to consolidate device type logic using a strongly typed enum.Also applies to: 207-207, 212-212
215-216: LGTM! Excellent addition of error handling test.This test case properly validates that
get_toltec_versionraisesSystemExitwhen called withHardwareType.RMPP, ensuring robust error handling for unsupported hardware types.
|
@laomaiweng Thank you so much ❤️ |
4 participants
The device type (RM1, RM2, RMPP) is determined in multiple places across the code based on either the contents of the
/sys/devices/soc0/machine file, or from the user-provided
--hardwareargument. This makes it error-prone, and new commands or features also need to come with their own logic for device type determination.This PR introduces the
HardwareTypeenum, and its associatedparsefunction, so that the logic to determine the device type lives in a single location.It also adds a
--hardwareoption tocodexctl list, to only list images for the specified hardware type.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests