feat: driver/sdwire: support unprogrammed FT200X EEPROM + macOS storage/mux fixes#748
feat: driver/sdwire: support unprogrammed FT200X EEPROM + macOS storage/mux fixes#748mmahut wants to merge 1 commit into
Conversation
|
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 as they are similar to previous changes (3)
📝 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 |
|
@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}
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
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: |
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) |
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( |
|
@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! |
|
@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 |
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
|
@mangelajo with regards to the failing CI right now:
|
|
We have been bumping a whole bunch of dependencies last/this week. It also increased the nitpickyness of the typos checker, check warnings is related to a sphinx version bump and addressed in #778, e2e flakiness are likely failures in either hooks which @evakhoni is looking at or some transient infrastructure related issues (AWS was having trouble last week, GitHub CI in the beginning of this week) - after all it looks good to me |
kirkbrauer
left a comment
kirkbrauer
left a comment
There was a problem hiding this comment.
I think this looks really interesting, could be helpful for these macOS edge cases that can be tough to get out of without a Linux device lying around 😄
As I'm waiting for a replacement of my framework laptop, this was indeed my case. 🥲 |
|
I will run a quick check here, Linux + macOS, then merge |
|
I will test on Linux today, I was having some issues in macOS I will try to fix in a separate PR, since this works for you it’s a step forward :-) |
4 participants
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.