Conversation
WalkthroughThe pull request refactors several components in the flashing driver packages. In the flashers package, code formatting has been streamlined by condensing multiline declarations, adjusting blank lines, and updating method calls. The U-Boot console handling is modified to remove redundant object creation and to instantiate it if absent. In addition, tests have been reformatted and a standalone U-Boot module was removed. The opendal package also sees minor formatting updates. A new dependency on Changes
Sequence Diagram(s)sequenceDiagram
participant Flasher as BaseFlasher
participant Children as Device Children
participant Uboot as UbootConsole
Flasher->>Children: Check for "uboot" key
alt "uboot exists"
Children-->>Flasher: Return existing UbootConsole
else "uboot missing"
Flasher->>Uboot: Instantiate UbootConsole with power & serial
Uboot-->>Children: Assign as "uboot"
end
Flasher->>Uboot: Call reboot_to_console()
Uboot-->>Flasher: Reboot acknowledged
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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:
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 (
|
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
I verified that it works, although I guess you want to go for the better uboot :) |
Indeed, especially around console handling. Right now the same pexpect instance is passed around, and it can cause problems especially with builtin serial adapters that's powered by the board. |
|
Rebased. TODO:
|
| bytes_diff = current_bytes - last_pos | ||
| speed_mb = (bytes_diff / (1024*1024)) / elapsed | ||
| total_mb = current_bytes/(1024*1024) | ||
| speed_mb = (bytes_diff / (1024 * 1024)) / elapsed |
There was a problem hiding this comment.
what do you pass to reformat those files? should we set it in ruff linting?
There was a problem hiding this comment.
Nothing, just uv run ruff format && uv run ruff check --fix --unsafe-fixes
There was a problem hiding this comment.
ah uv run ruff format thanks! :)
|
@NickCao this also works!, But one note, as all the execution in uboot now requires opening streams all the time with .pexpect() ... it's noticeably slower (like 1-2 seconds per uboot command). Can we make it accept an open stream instead? |
Yet we should recreate the pexpect instance everytime, since we make extensive use of the I think a good design is to make |
I tried with this change and it performs much better: Could we do that? I understand that if something has sent command or something that will output a prompt, it could get confused... but there is a very noticeable change in environments with latency... |
|
Also, another request, UbootConsoleClient - INFO - Running DHCP to obtain network configuration... Can we make it not write the "; echo $?" artifact for the checked commands? it's an implementation detail that only adds noise to the output :) Anyway, great work, I love the new driver! :) |
|
Try jumpstarter-dev/jumpstarter#348, this should bring us back to the original speed, also prohibits misuse (like, forgetting to call reboot_to_console) |
NickCao
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/jumpstarter-driver-flashers/pyproject.toml (2)
21-21: Dependency Addition in Dependencies List
The new dependency declaration"jumpstarter-driver-uboot"has been successfully added to the dependencies list as part of the U-Boot driver integration. Consider whether a version constraint should be specified if the package eventually leaves workspace mode, but if the intention is to manage it exclusively via a workspace, this setup is acceptable.
37-39: Workspace Source Configuration for U-Boot Driver
The new[tool.uv.sources]section correctly declares thejumpstarter-driver-ubootas a workspace dependency, aligning with the objective of integrating a standalone U-Boot driver. Please consider either removing or documenting the commented-out line (#asyncio_mode = "auto") so that the configuration remains clean and understandable.packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
27-540: Address potential performance concerns mentioned in PR discussionThe PR discussions mentioned by mangelajo highlight a performance issue with the execution of U-Boot commands taking an additional 1-2 seconds per command due to frequent opening of streams with
.pexpect(). While the current implementation has improved by using the context manager pattern, additional optimization might be needed.I notice that a follow-up PR (#348) was mentioned that aims to restore the original speed. Consider implementing a solution where the pexpect instance is reused more efficiently to minimize overhead, while still ensuring any unwanted data from the
beforeattribute is properly cleared.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py(2 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(13 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py(4 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py(3 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py(0 hunks)packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/uboot.py(0 hunks)packages/jumpstarter-driver-flashers/pyproject.toml(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py(2 hunks)packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py(1 hunks)
💤 Files with no reviewable changes (2)
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/test_bundle.py
- packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/uboot.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-utils Dockerfile.utils)
🔇 Additional comments (35)
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/driver.py (1)
198-198: Formatting looks good.Adding a blank line between method definitions enhances code readability and follows good Python style practices.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver_test.py (4)
24-26: Improved code spacing looks good.The reformatting of the pytest fixture decorator spacing enhances readability.
42-42: Good code simplification.Condensing the BaseFlasher instantiation into a single line makes the code more concise while maintaining readability.
48-48: Good code simplification.Condensing the BaseFlasher instantiation into a single line is consistent with the similar change above.
56-56: Good code simplification.Condensing the initram variable assignment improves code conciseness.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/bundle.py (2)
12-12: Improved readability with consistent blank lines.Adding blank lines between class definitions improves code organization and readability.
Also applies to: 18-18, 25-25
31-31: Good code simplification.Condensing the login field declaration into a single line makes the code more concise while maintaining readability.
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/driver.py (8)
7-7: Good addition of standalone UbootConsole import.This import aligns with the PR objective of making use of a standalone U-Boot driver in the flasher.
17-18: Consistent docstring formatting.Removing the space after opening triple quotes maintains consistency in docstring formatting.
Also applies to: 202-203
21-22: Consistent attribute formatting.Standardizing the formatting of the tftp_dir and http_dir attributes improves code consistency.
30-36: Consistent formatting for conditional checks.The reformatting of the code for checking and initializing "tftp" and "http" children improves readability.
39-47: Improved error message formatting.The reformatted error messages for missing "serial" and "power" children enhance readability.
49-55: Excellent implementation of the standalone UbootConsole.This code automatically initializes the "uboot" child if not present, which is a key part of making use of the standalone U-Boot driver. This approach ensures backward compatibility while integrating the new driver.
59-59: Improved comment formatting.The reformatted comment about the default dtb improves readability.
101-104: Enhanced error message readability.The reformatted ValueError message is now more readable with proper line breaks.
packages/jumpstarter-driver-opendal/jumpstarter_driver_opendal/client.py (5)
47-47: Added blank line for separationThe added blank line improves readability by clearly separating functions.
49-50: Improved docstring formattingThe docstring formatting has been standardized by removing the extra space after the opening triple quotes.
55-58: Standardized string quotation style and improved formattingChanged single quotes to double quotes for string literals and improved the formatting of the URL parsing and operator creation for better readability.
60-60: Enhanced tuple unpacking readabilityThe return statement uses the star operator for unpacking more clearly, which improves code readability.
529-529: Simplified method signature formattingThe
dumpmethod signature has been reformatted from multi-line to single-line, maintaining the same parameters while improving code compactness.packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (15)
44-44: Added TODO comment for future enhancementGood practice to document the need to set console debug on the uboot client in the future.
64-65: Simplified bootloader shell implementation using standalone uboot driverThe code now directly uses the context manager from the standalone uboot driver instead of a separate UbootConsole class, aligning with the PR objective to "Make use of standalone uboot driver in flasher".
99-103: Improved thread creation readabilityThe thread creation code has been reformatted to improve readability by spreading the arguments across multiple lines.
198-199: Clarified variable naming with MB unitsThe variables now consistently use
_mbsuffix to clearly indicate they represent values in megabytes.
232-240: Enhanced method signature readabilityThe
_transfer_bg_threadmethod signature has been reformatted to improve readability by placing each parameter on its own line.
301-307: Improved JSON structure readabilityThe JSON creation code has been reformatted to improve readability by structuring the dictionary across multiple lines.
321-321: Standardized regex pattern with double quotesChanged single quotes to double quotes for consistency with the rest of the codebase.
376-380: Integrated standalone uboot driver for console handlingThe code now uses the context manager pattern with
self.uboot.reboot_to_console()and directly callsself.uboot.setup_dhcp()instead of managing these operations manually. This change addresses the performance concerns mentioned in the PR discussions by potentially reducing the frequency of opening streams.
385-385: Simplified environment configuration with uboot driverNow using the
set_env_dictmethod from the uboot driver to handle environment configuration, which encapsulates the implementation details within the driver.
392-392: Streamlined command execution with uboot driverCommand execution is now handled through the
run_commandmethod from the uboot driver, providing a consistent interface and encapsulating implementation details.Also applies to: 397-397, 402-403
404-409: Enhanced console interaction with pexpectImproved the console interaction code by directly using the serial's pexpect method and added conditional debug output based on the console_debug flag.
441-441: Standardized operator unpackingUpdated the unpacking to consistently use the pattern seen elsewhere in the codebase, including capturing the operator scheme.
464-481: Improved CLI options readabilityThe CLI command options have been reformatted for better readability by giving each option its own line, which makes the code easier to maintain and extend.
489-494: Enhanced flash method call readabilityThe flash method call has been reformatted for better readability by placing each argument on its own line.
535-539: Streamlined decompression command logicImproved the formatting of the decompression command logic and standardized on double quotes for string literals throughout the code.
2 participants
Summary by CodeRabbit
New Features
Refactor
Style