feat: driver/sdwire: support unprogrammed FT200X EEPROM + macOS storage/mux fixes#748
Open
mmahut wants to merge 1 commit into
Open
feat: driver/sdwire: support unprogrammed FT200X EEPROM + macOS storage/mux fixes#748mmahut wants to merge 1 commit into
mmahut wants to merge 1 commit into
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRefactors SDWire device discovery with platform-specific helpers, adds polling/await helpers for host-side storage availability, updates host/DUT switching with macOS SMSC power-cycling and eject semantics, widens common storage helper types, and adds comprehensive unit tests and a README doctest tweak. ChangesSDWire Storage Discovery and Switching Refactor
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Member
|
@mangelajo assigned you for reviews as it seems to be macos focused which i can not really test |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py (1)
72-73: ⚡ Quick winAvoid silent exception swallowing during hub power-cycle.
Line 72-Line 73 hides USB control failures completely, which makes host re-enumeration timeouts hard to diagnose. Log the exception before continuing best-effort behavior.
Proposed refactor
- except Exception: - pass # best-effort; caller raises if the disk never appears + except Exception as e: + if logger: + logger.warning(f"failed to power-cycle SMSC hub port: {e}") + # best-effort; caller raises if the disk never appears🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py` around lines 72 - 73, The except Exception: pass in jumpstarter_driver_sdwire/driver.py (the hub power-cycle USB control block) silently swallows errors; change it to log the exception before continuing (e.g., use logging.getLogger(__name__).exception(...) or an existing module/class logger like self.logger.exception) with a clear message such as "hub power-cycle USB control failed" and then keep the best-effort behavior so the caller still handles timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@python/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py`:
- Around line 83-84: The failure isn't due to converting dev.bus/dev.address to
strings—pyudev accepts ints—so leave the
pyudev.Enumerator.match_attribute("busnum", dev.bus) / match_attribute("devnum",
dev.address") calls as-is, and instead remove the silent broad excepts in
_find_storage_device_linux and _power_cycle_smsc_port: replace "except
Exception: pass" with either narrowly typed exception handlers for the expected
errors or catch Exception but log the caught exception via the module logger
(e.g., logger.exception or logger.error with exception info) so failures are
observable and traceable.
---
Nitpick comments:
In
`@python/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py`:
- Around line 72-73: The except Exception: pass in
jumpstarter_driver_sdwire/driver.py (the hub power-cycle USB control block)
silently swallows errors; change it to log the exception before continuing
(e.g., use logging.getLogger(__name__).exception(...) or an existing
module/class logger like self.logger.exception) with a clear message such as
"hub power-cycle USB control failed" and then keep the best-effort behavior so
the caller still handles timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7df7584-c909-48e2-9056-c7654fe223e6
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-sdwire/README.mdpython/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.pypython/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver_test.pypython/packages/jumpstarter/jumpstarter/common/storage.py
mangelajo
requested changes
Jun 8, 2026
Member
mangelajo
left a comment
mangelajo
left a comment
There was a problem hiding this comment.
Thank you @mmahut this looks great, only a few comments, and we need to make the linter + ty + typos CI work.
I didn't even dare to fight the OSX battle for this, but I see there was more to it than I even expected. Thanks for making this better!
| for block in filter(lambda d: d.subsystem == "block", udevice.parent.children): | ||
| for storage_device in filter(lambda link: link.startswith("/dev/disk/by-diskseq/"), block.device_links): | ||
| return storage_device | ||
| except Exception: |
Member
There was a problem hiding this comment.
Can we be more explicit about Exception capture? this will also probably fail our linter.
| return node.get("serial_num") == serial | ||
| vid = node.get("vendor_id", "").lower() | ||
| pid = node.get("product_id", "").lower() | ||
| return ("0x04e8" in vid and "0x6001" in pid) or ("0x0403" in vid and "0x6015" in pid) |
Member
There was a problem hiding this comment.
can we create/use constants for the VID/PIDs ?
| # The disk node lives under the reader's nested "Media" list, not at top level. | ||
| vid = node.get("vendor_id", "").lower() | ||
| pid = node.get("product_id", "").lower() | ||
| if not ("0x0424" in vid and "0x4050" in pid): |
| try: | ||
| import json | ||
|
|
||
| out = subprocess.check_output( |
Member
|
@mmahut where can I ship you a bunch of Jumpstarter stickers? :D totally deserved! (ping me at majopela@redhat.com ) if you're interested. Cheers and thanks again! |
Contributor
Author
|
@mangelajo thank you for the great review! I have addressed these comments and force pushed. Also, I will never say no to free stickers! :D |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/packages/jumpstarter-driver-sdwire/README.md`:
- Line 25: Update the doctest path in the README where
ExporterConfigV1Alpha1DriverInstance.from_path(...) is used: change the YAML
path from "source/api-reference/drivers/sdwire.yaml" to
"source/reference/package-apis/drivers/sdwire.yaml" so the doctest points at the
actual driver YAML location; ensure the line containing
ExporterConfigV1Alpha1DriverInstance.from_path("...").instantiate() is updated
accordingly to match other driver READMEs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5bc76f3d-7af3-4f52-810a-5ec24837c0ad
📒 Files selected for processing (4)
python/packages/jumpstarter-driver-sdwire/README.mdpython/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.pypython/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver_test.pypython/packages/jumpstarter/jumpstarter/common/storage.py
✅ Files skipped from review due to trivial changes (1)
- python/packages/jumpstarter/jumpstarter/common/storage.py
🚧 Files skipped from review as they are similar to previous changes (2)
- python/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver_test.py
- python/packages/jumpstarter-driver-sdwire/jumpstarter_driver_sdwire/driver.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recently, I got an unprogrammed unit from 3mdeb, which did not work out of the box. It had the factory-default FTDI FT200X EEPROM (
0x0403/0x6015) rather than the Samsung VID/PID set bysd-mux-ctrl --init. Also, previously the driver was Linux-only (pyudev).To elaborate:
1. Unprogrammed FT200X support
Also matches
0x0403/0x6015, but only when an explicitserialis configured -- a bare FT200X has no reliable runtime signature, so requiring a serial avoids false-positive matches on unrelated FTDI devices. Also raises a clear error whe multiple programmed SD Wires are present with no serial (instead of silently binding the first).2. macOS storage discovery
effective_storage_device()now usessystem_profiler SPUSBDataTypeon Darwin (pyudev is Linux-only). The SD reader is correlated to its FT200X as a sibling under the same internal hub, and the disk node is read from the reader's nestedMedia[].bsd_name, so the right disk is targeted when several SD Wires are attached. Reads/writes retry discovery (USB re-enumerates after a switch) and fail with a clear error instead of aTypeError.3. macOS
dut()ejects before switchingThe mux only switches when the SD bus is idle;
diskutil eject(SCSI STOP UNIT) idles it. If the disk can't be determined,dut()aborts rather than risk corrupting a mounted volume.4. macOS
host()power-cycles the reader's hub portejectleaves the SMSC reader stopped, sohost()routes the card back first and then power-cycles port 1 of this SD Wire's internal hub (scoped by USB topology, so other attached devices aren't disturbed) to force a clean re-enumeration without a physical replug.