Added manual dd update #104
Conversation
WalkthroughThe pull request introduces several modifications across multiple files in the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Jayy remember to uncomment |
|
Extra additions
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
codexctl/__init__.py (1)
Line range hint
230-249: Simplify and validate version number extraction.The current method for extracting
version_numbercould be improved. Consider refactoring to handle cases where the version number is not present in the filename, and provide clear validation feedback to the user.codexctl/analysis.py (1)
Line range hint
7-29: Importerrnomodule for exception handling.The
errnomodule is used in the exception handling but is not imported, leading to aNameError.Add the import statement at the top of the file:
+import errno🧰 Tools
🪛 Ruff (0.8.0)
27-27: Undefined name
errno(F821)
codexctl/updates.py (1)
Line range hint
201-208: Refactor device type recognition for better maintainabilityThe current implementation uses multiple string comparisons. Consider using sets for more efficient and maintainable device type matching.
- if device_type in ("rm1", "reMarkable 1", "reMarkable1", "remarkable1"): - version_lookup = self.remarkable1_versions - elif device_type in ("rm2", "reMarkable 2", "reMarkable2", "remarkable2"): - version_lookup = self.remarkable2_versions - BASE_URL_V3 += "2" - elif device_type in ("rmpp", "rmpro", "reMarkable Ferrari", "ferrari"): - version_lookup = self.remarkablepp_versions - else: - raise SystemError("Hardware version does not exist! (rm1,rm2,rmpp)") + DEVICE_TYPES = { + 'rm1': {'aliases': {"rm1", "reMarkable 1", "reMarkable1", "remarkable1"}, + 'versions': self.remarkable1_versions, + 'url_suffix': ""}, + 'rm2': {'aliases': {"rm2", "reMarkable 2", "reMarkable2", "remarkable2"}, + 'versions': self.remarkable2_versions, + 'url_suffix': "2"}, + 'rmpp': {'aliases': {"rmpp", "rmpro", "reMarkable Ferrari", "ferrari"}, + 'versions': self.remarkablepp_versions, + 'url_suffix': ""} + } + + for device_key, device_info in DEVICE_TYPES.items(): + if device_type in device_info['aliases']: + version_lookup = device_info['versions'] + if device_info['url_suffix']: + BASE_URL_V3 += device_info['url_suffix'] + break + else: + supported_types = ", ".join(sorted(set.union(*[info['aliases'] for info in DEVICE_TYPES.values()]))) + raise SystemError(f"Hardware version does not exist! Supported types: {supported_types}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
README.md(1 hunks)codexctl/__init__.py(10 hunks)codexctl/analysis.py(2 hunks)codexctl/device.py(10 hunks)codexctl/updates.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
codexctl/analysis.py
27-27: Undefined name errno
(F821)
codexctl/__init__.py
314-316: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
codexctl/device.py
169-169: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
441-441: Undefined name out_image_file
(F821)
🔇 Additional comments (12)
codexctl/__init__.py (6)
98-98: Properly include logger in get_update_image function call.
The addition of logger=logger in the get_update_image function call ensures that logging is appropriately handled within the function.
131-131: Consistent use of logger in analysis functions.
Including logger=logger in the get_update_image call maintains consistent logging practices across the analysis functionalities.
168-169: Add support for the transfer command in call_func.
The call_func method now includes the transfer command, enabling file transfer capabilities to the device.
202-205: Implement file transfer functionality.
The code correctly calls remarkable.transfer_file_to_remote to transfer the specified file to the destination on the device.
370-376: Add --port argument to the CLI options.
The inclusion of the --port (-p) option allows users to specify a custom SSH port when connecting to the device, enhancing flexibility.
380-387: Introduce the transfer subcommand to the CLI.
The new transfer subcommand enables users to transfer files from the host to the device directly via the CLI.
codexctl/device.py (4)
20-27: Add port parameter to DeviceManager constructor and methods.
Including the port parameter supports connections to devices on custom SSH ports, improving connectivity options.
112-118: Use self.port in get_remarkable_address.
By using self.port when checking device reachability, the method respects the custom port specified by the user.
123-142: Update check_if_address_reachable to accept a port parameter.
The method now accepts remote_port, allowing for reachability checks on custom ports.
Line range hint 147-192: Update connect_to_device method to use the port parameter.
This change ensures that SSH connections use the specified port, enhancing flexibility.
🧰 Tools
🪛 Ruff (0.8.0)
169-169: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
README.md (1)
53-53: Update documentation to reflect the correct command-line option.
The option -hw has been changed to -d. Ensure all documentation and examples use the updated option for consistency.
codexctl/updates.py (1)
Line range hint 1-394: Verify alignment with PR objectives
The current changes focus on device type recognition and version handling, but don't appear to address the stated PR objectives:
- Using
mountcommand instead of environment variables - Updating the restore script
- Refactoring automatic updates into a function
- Testing the implementation
Please clarify if these objectives are being addressed in other files or if they should be handled in this file.
| else: | ||
| if function == "transfer": | ||
| raise SystemError("You can't transfer files alredy on your device!") |
There was a problem hiding this comment.
Fix typo in exception message.
The exception message contains a typo: "alredy" should be corrected to "already".
Apply this diff to fix the typo:
- raise SystemError("You can't transfer files alredy on your device!")
+ raise SystemError("You can't transfer files already on your device!")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| else: | |
| if function == "transfer": | |
| raise SystemError("You can't transfer files alredy on your device!") | |
| else: | |
| if function == "transfer": | |
| raise SystemError("You can't transfer files already on your device!") |
| raise ImportError( | ||
| "remarkable_update_image is required for this update. Please install it!" | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use exception chaining with raise ... from None.
When re-raising exceptions within an except block, use from None to suppress the context of the original exception. This provides clearer error messages to the user.
Apply this diff to include from None:
- raise ImportError(
- "remarkable_update_image is required for this update. Please install it!"
- )
+ raise ImportError(
+ "remarkable_update_image is required for this update. Please install it!"
+ ) from None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| raise ImportError( | |
| "remarkable_update_image is required for this update. Please install it!" | |
| ) | |
| raise ImportError( | |
| "remarkable_update_image is required for this update. Please install it!" | |
| ) from None |
🧰 Tools
🪛 Ruff (0.8.0)
314-316: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
| raise SystemError(f"Error: Device {remote_address} is not reachable!") | ||
|
|
||
| client = paramiko.client.SSHClient() | ||
| client.set_missing_host_key_policy(paramiko.AutoAddPolicy()) | ||
|
|
||
| if authentication: | ||
| if authentication != None: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use is not None for None comparison.
Replace authentication != None with authentication is not None to follow Python best practices.
Apply this diff:
- if authentication != None:
+ if authentication is not None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if authentication != None: | |
| if authentication is not None: |
🧰 Tools
🪛 Ruff (0.8.0)
169-169: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
| if self.client: | ||
| print(f"Uploading {version_file} image") | ||
|
|
||
| out_image_file = f"/tmp/{out_image_file}" |
There was a problem hiding this comment.
Fix undefined variable out_image_file.
The variable out_image_file is used before it is defined, which will cause a NameError.
Define out_image_file before using it in the formatted string. Apply this diff:
- out_image_file = f"/tmp/{out_image_file}"
+ out_image_file_name = os.path.basename(version_file)
+ out_image_file = f"/tmp/{out_image_file_name}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| out_image_file = f"/tmp/{out_image_file}" | |
| out_image_file_name = os.path.basename(version_file) | |
| out_image_file = f"/tmp/{out_image_file_name}" |
🧰 Tools
🪛 Ruff (0.8.0)
441-441: Undefined name out_image_file
(F821)
| @@ -197,9 +197,10 @@ def download_version( | |||
| BASE_URL = "https://updates-download.cloud.remarkable.engineering/build/reMarkable%20Device%20Beta/RM110" # Default URL for v2 versions | |||
| BASE_URL_V3 = "https://updates-download.cloud.remarkable.engineering/build/reMarkable%20Device/reMarkable" | |||
|
|
|||
| if device_type in ("rm1", "reMarkable 1", "remarkable1"): | |||
| print(device_type) | |||
There was a problem hiding this comment.
Remove debug print statement
The print statement appears to be debugging code that was accidentally left in. This should be replaced with proper logging if debugging is needed.
- print(device_type)
+ self.logger.debug(f"Processing device type: {device_type}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| print(device_type) | |
| self.logger.debug(f"Processing device type: {device_type}") |
| if int(version_minor) > 11 or update_version == "3.11.3.3": | ||
| version_external = True |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document and clarify version handling logic
The special case handling for version "3.11.3.3" needs documentation explaining why this version requires different treatment. Additionally, the condition could be structured more clearly.
- if int(version_minor) > 11 or update_version == "3.11.3.3":
+ # Version 3.11.3.3 is a special case that requires external provider handling
+ # despite being in the 3.11.x series
+ is_special_version = update_version == "3.11.3.3"
+ is_newer_than_3_11 = int(version_minor) > 11
+ if is_newer_than_3_11 or is_special_version:
version_external = True📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if int(version_minor) > 11 or update_version == "3.11.3.3": | |
| version_external = True | |
| # Version 3.11.3.3 is a special case that requires external provider handling | |
| # despite being in the 3.11.x series | |
| is_special_version = update_version == "3.11.3.3" | |
| is_newer_than_3_11 = int(version_minor) > 11 | |
| if is_newer_than_3_11 or is_special_version: | |
| version_external = True |
Todo
mountcommand instead of the env variablesSummary by CodeRabbit
New Features
codexctlutility.DeviceManagerclass.UpdateManagerclass.Bug Fixes
Documentation
README.mdfor clearer command usage examples.Refactor