Added manual dd update #104
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Manager
participant DeviceManager
participant RemoteDevice
User->>CLI: codexctl transfer <file> <destination> --port <port>
CLI->>Manager: call_func('transfer', args)
Manager->>DeviceManager: transfer_file_to_remote(file, destination)
DeviceManager->>RemoteDevice: SFTP upload file to destination (on port)
DeviceManager-->>Manager: Done
Manager-->>CLI: Print "Done!"
sequenceDiagram
participant User
participant CLI
participant Manager
participant DeviceManager
participant Analysis
participant RemoteDevice
User->>CLI: codexctl install <update_file> [--port <port>]
CLI->>Manager: call_func('install', args)
Manager->>Analysis: get_update_image(update_file, logger)
alt Manual downgrade needed
Manager->>DeviceManager: install_manual_update(update_image)
DeviceManager->>RemoteDevice: Upload image, run dd, reboot, reconnect, disable updates
else Normal install
Manager->>DeviceManager: install_sw_update or install_ohma_update
end
DeviceManager-->>Manager: Install complete
Manager-->>CLI: Print completion message
✨ Finishing Touches
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. 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.
| 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)
| 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)
codexctl/updates.py
Outdated
| 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}") |
codexctl/updates.py
Outdated
| 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
codexctl/__init__.py (2)
195-197: Fix typo in exception message.The exception message contains a typo: "alredy" should be corrected to "already".
324-329: Use exception chaining withraise ... from None.When re-raising exceptions within an
exceptblock, usefrom Noneto suppress the context of the original exception.codexctl/device.py (2)
169-169: Useis not NoneforNonecomparison.Replace
authentication != Nonewithauthentication is not Noneto follow Python best practices.
450-450: Fix undefined variableout_image_file.The variable
out_image_fileis used before it is defined, which will cause aNameError.
🧹 Nitpick comments (1)
codexctl/device.py (1)
434-444: Fix typo in docstring and LGTM on implementation.The method implementation is correct with proper SFTP usage and progress callback.
Apply this diff to fix the typo:
- Tranfers file at file_location to destination on devicec + Transfers file at file_location to destination on device
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
codexctl/__init__.py(9 hunks)codexctl/device.py(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
codexctl/__init__.py (3)
codexctl/analysis.py (1)
get_update_image(7-31)codexctl/device.py (4)
DeviceManager(18-762)transfer_file_to_remote(434-444)install_sw_update(540-640)install_manual_update(446-511)codexctl/server.py (1)
get_available_version(62-69)
🪛 Pylint (3.3.7)
codexctl/device.py
[refactor] 146-146: Too many branches (14/12)
(R0912)
🪛 Ruff (0.11.9)
codexctl/device.py
169-169: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
450-450: Undefined name out_image_file
(F821)
codexctl/__init__.py
327-329: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 Flake8 (7.2.0)
codexctl/device.py
[error] 169-169: comparison to None should be 'if cond is not None:'
(E711)
[error] 450-450: undefined name 'out_image_file'
(F821)
[error] 467-467: too many leading '#' for block comment
(E266)
codexctl/__init__.py
[error] 396-396: too many leading '#' for block comment
(E266)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build for macos-latest
- GitHub Check: Build for ubuntu-latest
- GitHub Check: Build for windows-latest
- GitHub Check: Build for remarkable
🔇 Additional comments (14)
codexctl/__init__.py (7)
101-101: LGTM: Proper integration of logger parameter.The addition of the
loggerparameter toget_update_imagecalls correctly aligns with the updated function signature inanalysis.py, improving logging consistency throughout the application.Also applies to: 134-134
205-205: LGTM: Proper port parameter integration.The addition of the
portparameter correctly passes the CLI argument toDeviceManager, enabling custom SSH port specification.
215-217: LGTM: Clean transfer command implementation.The transfer command implementation is straightforward and correctly utilizes the
transfer_file_to_remotemethod fromDeviceManager.
243-272: LGTM: Proper manual update detection logic.The manual update detection correctly identifies downgrade scenarios where the device uses the new update engine but the update file requires the old engine, enabling proper handling of version compatibility issues.
321-344: LGTM: Well-structured manual update implementation.The manual update flow is well-implemented with proper image extraction, installation, and cleanup. The logic correctly handles the downgrade scenario by extracting the
.imgfile and using the manual installation method.
386-392: LGTM: Proper CLI argument definition.The
--portargument is correctly defined with appropriate type, default value, and descriptive help text.
396-402: LGTM: Clear transfer subcommand definition.The transfer subcommand is well-defined with required file and destination arguments and clear help text.
codexctl/device.py (7)
20-20: LGTM: Proper port parameter integration in constructor.The port parameter is correctly added with appropriate default value and properly passed to connection methods.
Also applies to: 27-27, 33-33, 42-42
112-112: LGTM: Consistent port parameter usage.The port parameter is consistently used across reachability checks and connection methods, enabling custom SSH port configuration.
Also applies to: 118-118, 123-123, 163-163
142-144: Verify the exception handling change.The exception type was changed from
FileNotFoundErrorto a generic exception. For socket connection failures,ConnectionErrororOSErrorwould be more appropriate thanFileNotFoundError.
184-192: LGTM: Proper empty password authentication handling.The authentication logic correctly handles empty passwords by using
auth_nonefor passwordless SSH authentication, properly supporting Docker environments with no password authentication.
446-512: LGTM on overall manual update logic (pending fixes).The manual update implementation correctly handles both remote and local scenarios, with proper partition detection, dd execution, reboot handling, and service management. Once the variable issues are resolved, this provides a solid manual downgrade path.
596-596: LGTM: Consistent port usage in reconnection logic.The port parameter is correctly applied to all reconnection calls after device reboots, maintaining consistency with the custom port configuration.
Also applies to: 600-600, 714-714, 718-718
760-760: LGTM: Improved progress display formatting.The progress display formatting is enhanced with proper integer conversion for cleaner percentage output.
|
|
||
| print("Now running dd to overwrite the fallback partition") | ||
|
|
||
| _stdin, stdout, _stderr = self.client.exec_command(f"dd if={version_file} of=/dev/{fallback_partition}") |
There was a problem hiding this comment.
Fix incorrect variable usage in dd command.
The dd command uses version_file (local path) instead of out_image_file (remote path), which will fail since the file doesn't exist at that location on the remote device.
Apply this diff to fix the variable usage:
- _stdin, stdout, _stderr = self.client.exec_command(f"dd if={version_file} of=/dev/{fallback_partition}")
+ _stdin, stdout, _stderr = self.client.exec_command(f"dd if={out_image_file} of=/dev/{fallback_partition}")📝 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.
| _stdin, stdout, _stderr = self.client.exec_command(f"dd if={version_file} of=/dev/{fallback_partition}") | |
| _stdin, stdout, _stderr = self.client.exec_command(f"dd if={out_image_file} of=/dev/{fallback_partition}") |
🤖 Prompt for AI Agents
In codexctl/device.py at line 461, the dd command incorrectly uses the local
variable version_file instead of the remote file path variable out_image_file.
Replace version_file with out_image_file in the exec_command call to ensure the
command references the correct remote file path.
Todo
mountcommand instead of the env variablesSummary by CodeRabbit
New Features
transfercommand to enable transferring files from the host to the device, with support for specifying a custom SSH port.Improvements