Clarify USB media UI after host gadget fixes - #1
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis pull request refactors USB gadget initialization by extracting shared shell helpers, introducing a new Go USB control package, and integrating it across device and storage services. It adds virtual media device support to the web frontend alongside existing virtual disk and network options. ChangesUSB gadget initialization and control refactoring
Frontend virtual media device support
Sequence DiagramsequenceDiagram
participant App as Web App
participant VM as vm.Service
participant USB as usb.Package
participant Shell as Shell Init
App->>VM: UpdateVirtualDevice(media)
VM->>USB: SetVirtualMediaEnabled(true)
USB->>USB: WithDetachedUDC()
USB->>Shell: exec ActiveInitScript
Shell->>Shell: add_hid_function()
Shell->>Shell: bind_first_udc()
USB-->>VM: success
VM-->>App: device enabled
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors USB gadget/configfs handling to better separate “virtual image” media from the NanoKVM data disk, centralizes UDC rebinding + HID reopen logic, and updates both backend APIs and the UI to reflect the new device split.
Changes:
- Introduces a shared Go USB gadget helper (
server/service/usb/gadget.go) for UDC detach/attach, HID reopen coordination, and toggling virtual media / data disk / RNDIS. - Splits the UI and API surface so “Virtual Image” (disk0) is independent from “Virtual Disk” (disk1 / data storage export), with updated i18n strings.
- Adds Go + shell tests (including fake-configfs init-script tests) and wires them into
make check.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/desktop/menu/settings/device/virtual-devices.tsx | Adds a new “Virtual Image” toggle and supports media device updates. |
| web/src/pages/desktop/menu/image/index.tsx | Renames image mode labels/values to make ISO/CD-ROM explicit. |
| web/src/i18n/locales/en.ts | Adds media strings and updates disk description text. |
| web/src/i18n/locales/zh.ts | Adds media strings and updates disk description text. |
| tests/usb-init-scripts-test.sh | Adds init-script tests using a fake configfs/UDC environment. |
| support/sg2002/kvm_system/main/lib/system_state/system_state.cpp | Fixes HID/UDisk state detection by checking explicit configfs links. |
| server/service/vm/virtual-device.go | Extends virtual device API to include media and delegates toggles to the new USB helper. |
| server/service/usb/gadget.go | New shared gadget/configfs helper (UDC rebind, function toggles, LUN updates). |
| server/service/usb/gadget_test.go | New unit tests for gadget helper behavior with a fake gadget filesystem. |
| server/service/storage/image.go | Switches mount/unmount behavior to use the shared gadget helper. |
| server/service/hid/status.go | Uses shared USB constants/helpers for mode switching and PHY reset. |
| server/service/hid/status_test.go | Adds tests for HID mode detection via bcdDevice. |
| Makefile | Adds test-* targets and wires them into check. |
| kvmapp/system/init.d/S03usbdev | Shares boot-time USB/HID setup via S03usb-common and adds disk1 export. |
| kvmapp/system/init.d/S03usbhid | Shares boot-time USB/HID setup via S03usb-common. |
| kvmapp/system/init.d/S03usb-common | New shared init-script helper for repeated gadget setup steps. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mjc
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8443c41579
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![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
🤖 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 `@kvmapp/system/init.d/S03usb-common`:
- Around line 92-94: In the unbind_udc() function, replace the echo-based write
that writes a newline with an explicit empty write to the UDC file; use printf
'' > "${USB_GADGET_ROOT}/${USB_GADGET_NAME}/UDC" or shell truncation (e.g., >
"${USB_GADGET_ROOT}/${USB_GADGET_NAME}/UDC") so the UDC receives an actual empty
write rather than a newline.
- Around line 96-100: The restart_udc function currently uses "echo >" to clear
the UDC file which writes a newline; change this to use printf '' > to write an
empty string for proper unbind semantics: in restart_udc, replace the echo
redirection that targets "${USB_GADGET_ROOT}/${USB_GADGET_NAME}/UDC" with a
printf '' redirection so the UDC file receives an empty string (keep the
subsequent sleep and ls "${USB_UDC_CLASS}" | cat > step intact).
- Around line 24-32: In write_usb_config_string replace the conditional mkdir
calls with a single unconditional mkdir -p usage: remove the if/else block and
always run mkdir -p configs/c.1/strings/0x409 before writing the configuration
file so the directory is created safely in both test and normal modes.
In `@server/service/usb/gadget.go`:
- Around line 77-91: The RestartPHY function currently calls
exec.Command(...).Run() while holding the HID lock; switch to using
context.WithTimeout and exec.CommandContext with a reasonable timeout (e.g.,
usbPhyRestartTimeout) to prevent indefinite blocking, and ensure you do not hold
the HID lock while waiting for the external script: call h.CloseNoLock() while
locked, then h.Unlock() before creating and running the CommandContext, and only
proceed to call h.OpenNoLockWithRetry(hidReopenTimeout, hidReopenDelay) after
the command completes; reference RestartPHY, ActiveInitScript,
exec.CommandContext, context.WithTimeout, CloseNoLock, Lock, Unlock, and
OpenNoLockWithRetry when applying the change.
In `@support/sg2002/kvm_system/main/lib/system_state/system_state.cpp`:
- Around line 230-233: When fopen(USB_UDC_STATE, "r") fails you must preserve
the “unknown/unreadable” semantics by setting kvm_sys_state.hid_state and
kvm_sys_state.udisk_state to -1 (same as kvm_sys_state.usb_state) instead of 0;
update the error/early-return path in the function that opens USB_UDC_STATE so
that on fopen failure it assigns -1 to kvm_sys_state.hid_state and
kvm_sys_state.udisk_state and then returns, keeping the -1 contract from
config.h consistent.
In `@web/src/pages/desktop/menu/settings/device/virtual-devices.tsx`:
- Around line 85-97: Update the inline comment above the media toggle to match
the displayed i18n text: replace the "Virtual Media" comment with "Virtual
Image" (or a comment that mirrors t('settings.device.media')) so it aligns with
the rendered label; locate the block containing the Switch controlled by
isMediaEnabled and update('media') and change the comment text accordingly.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 9bff9faa-6a63-48d7-bbe9-d2a13d6dde2c
📒 Files selected for processing (16)
Makefilekvmapp/system/init.d/S03usb-commonkvmapp/system/init.d/S03usbdevkvmapp/system/init.d/S03usbhidserver/service/hid/status.goserver/service/hid/status_test.goserver/service/storage/image.goserver/service/usb/gadget.goserver/service/usb/gadget_test.goserver/service/vm/virtual-device.gosupport/sg2002/kvm_system/main/lib/system_state/system_state.cpptests/usb-init-scripts-test.shweb/src/i18n/locales/en.tsweb/src/i18n/locales/zh.tsweb/src/pages/desktop/menu/image/index.tsxweb/src/pages/desktop/menu/settings/device/virtual-devices.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31f8cc2f62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if image == "" { | ||
| return nil | ||
| } | ||
| return WriteString(LUNFile, image) |
There was a problem hiding this comment.
When an image is mounted and the device later reboots or rebuilds the gadget, S03usbdev restores lun.0/file from /boot/usb.media0, but this path only writes the selected image to the live configfs LUN and leaves the boot flag as the empty file created by EnsureFile. In that reboot/reset context the virtual media switch stays enabled but the selected ISO/img is lost and comes back as empty media, so the mount state shown to the host is not preserved; write the normalized image (and the corresponding mode flag if needed) into the persisted media flag when mounting.
Useful? React with 👍 / 👎.
| if err := EnsureFile(MassStorageFlag); err != nil { | ||
| return err | ||
| } | ||
| if err := ensureMassStorageLink(); err != nil { |
There was a problem hiding this comment.
When the active gadget is in HID-only mode, the image-mount API now reaches this helper and creates/links mass_storage.disk0 on demand, so opening the existing image menu or calling the storage endpoint can add a mass-storage function even though bcdDevice still reports HID-only. Previously the same request failed because the LUN path did not exist after a HID-only boot; this should check the current mode or active script and refuse media mutations while HID-only is selected.
Useful? React with 👍 / 👎.
| detachErr := DetachUDC() | ||
| mutateErr := mutate() |
There was a problem hiding this comment.
If clearing UDC fails in an environment where the gadget is still bound (for example because configfs/sysfs rejects the write or the path is temporarily unavailable), this still proceeds into mutate() and can change LUNs or config links while the host-visible gadget remains attached. The old command sequences stopped on a failed detach step, so this helper should return before running the mutation when detaching does not succeed.
Useful? React with 👍 / 👎.
31f8cc2 to
b781c1f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b781c1f6d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ls /sys/class/udc/ | cat > UDC | ||
| echo device > /proc/cviusb/otg_role | ||
| if [ -e "${USB_BOOT_DIR}/usb.disk0" ] |
There was a problem hiding this comment.
When upgrading from the previous script, a mounted image was persisted as a non-empty /boot/usb.disk0 and the old boot path copied that value into the media LUN. This new condition treats every existing usb.disk0 as the data-disk flag and the block below always backs it with /dev/mmcblk0p3, so users who had an ISO mounted before upgrading reboot into the data disk and lose their virtual media. Please migrate non-empty legacy usb.disk0 values to usb.media0 (or branch on empty vs non-empty) before enabling the data disk.
Useful? React with 👍 / 👎.
3bc09f5 to
e53288f
Compare
mjc
changed the title
e53288f to
b63331c
Compare
877aa7d to
288422b
Compare
288422b to
f16e3c2
Compare
b63331c to
e483fbc
Compare
f36ee45 to
295cffb
Compare
Context
This PR is the cleanup/UI branch stacked on top of the host-visible USB fixes:
The host-visible behavior changes live in those lower branches. This PR keeps the remaining user-facing cleanup: making the USB media controls describe the separated concepts clearly, adding tentative i18n strings for those labels, and wiring the full validation target that includes the web build.
What changed
test-webandchecktargets:make testcomes from the virtual-media branch and runs USB script tests plus Go server testsmake checkin this PR runsmake testplus the web TypeScript production buildScope table
test-webandcheckso the full stack can be validated with one target.Validation
Validated locally with the Nix dev shell from the companion
mjc/setup-nix-build-envbranch:The
make checktarget runs: