Implemented ASPM and auto camera disable before sleep - #6
Open
lakotamm wants to merge 13 commits into
Open
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved correctness and security findings affect camera/ASPM state handling and suspend safety.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds coordinated PCIe ASPM and FaceTimeHD camera control, including boot and suspend integration.
Changes:
- Adds camera and ASPM helper scripts.
- Adds boot-time and suspend-time handling.
- Updates the tray application and installation documentation.
File summaries
| File | Review summary |
|---|---|
src/main.rs |
Uses helper scripts, but records camera state before asynchronously verifying pkexec and command success. |
README.md |
Documents setup and ASPM behavior; restore the original repository URL and Hyprland instructions, and fix step numbering. |
facetimehd-camera-on.sh |
Apply ASPM changes before loading the module and restore state if loading fails. |
facetimehd-camera-off.sh |
Avoid masking unload failures so ASPM is not re-enabled while the camera remains loaded. |
facetimehd-aspm-set.sh |
Restrict register updates to the ASPM bits instead of clearing the unrelated bit 2. |
facetimehd-sleep.sh |
Avoid the predictable /tmp state-file symlink risk and propagate camera-off failures. |
facetimehd-aspm-boot.service |
Adds boot-time ASPM configuration. |
Review details
Suppressed comments (4)
README.md:159
systemctl enableonly creates the boot symlink; it does not run this oneshot during the current boot. Since the instructions already rebooted in step 3 and launch the applet immediately afterward, ASPM will remain at its default state until another reboot. Useenable --nowor explicitly start the service here.
sudo systemctl enable facetimehd-aspm-boot.service
README.md:133
- The new binary hard-codes the two
/usr/local/bin/facetimehd-camera-*.shhelpers, but the release workflow uploads onlytarget/release/facetimehd_toggle(.github/workflows/release.yml:30-34). Users choosing the documented release-binary path cannot obtain these required files from the release, so every toggle fails unless they separately clone the source. Include the scripts in the release artifact or document explicit download/install commands.
6. Install the ASPM scripts
README.md:133
- These helpers invoke
lspciandsetpci, but the setup instructions do not install or mention thepciutilspackage for any supported distribution. On a minimal installation, the new toggle fails with a missing command despite following the documented setup. Add the appropriate distro-specificpciutilsdependency.
6. Install the ASPM scripts
facetimehd-aspm-boot.service:3
- This unit is enabled through
multi-user.targetbut ordered after that target, so it may run after graphical/user services have started. A tray click during that window can be undone when this oneshot unloads the module and enables ASPM. Order this boot initialization before the target (or explicitly before graphical startup).
After=multi-user.target
- Files reviewed: 7/7 changed files
- Comments generated: 10
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| local current desired | ||
| current=$(setpci -s "$addr" "${byte_addr}.b") | ||
| current=$(printf "%X" "0x${current}") | ||
| desired=$(printf "%X" "$(( (0x${current} & ~0x7) | ASPM_SETTING ))") |
| # Ignore "module not loaded" errors - already off is a success case, both | ||
| # for the GUI (idempotent toggle) and the sleep hook (camera may already | ||
| # be off when the lid closes). | ||
| modprobe -r facetimehd 2>/dev/null || true |
Comment on lines
+24
to
+25
| modprobe facetimehd | ||
| "$ASPM_SET_SCRIPT" "$ROOT_COMPLEX" "$ENDPOINT" 0 |
Comment on lines
+20
to
+23
| if grep -q facetimehd /proc/modules 2>/dev/null; then | ||
| echo "was_enabled" > "${STATE_FILE}.pre_sleep" | ||
| else | ||
| echo "was_disabled" > "${STATE_FILE}.pre_sleep" |
| echo "was_disabled" > "${STATE_FILE}.pre_sleep" | ||
| fi | ||
|
|
||
| "$CAMERA_OFF" |
Comment on lines
+53
to
+56
| match run_command("pkexec", &["/usr/local/bin/facetimehd-camera-on.sh"]) { | ||
| Ok(_) => { | ||
| write_camera_state(true); | ||
| println!("Camera enabled - icon will update shortly"); | ||
| println!("Camera enabled, ASPM disabled - icon will update shortly"); |
Comment on lines
+66
to
+69
| match run_command("pkexec", &["/usr/local/bin/facetimehd-camera-off.sh"]) { | ||
| Ok(_) => { | ||
| write_camera_state(false); | ||
| println!("Camera disabled - icon will update shortly"); | ||
| println!("Camera disabled, ASPM re-enabled - icon will update shortly"); |
Owner
|
@lakotamm don't worry about the copiolt stuff. Since I have not been engaged with this project for a long time I don't remember much of the stuff I did. Just wanted get a review from copilot. I will look into these myself and let you know soon. Thanks a lot for the contribution. Most probably I will be able to merge this within 2 days or so. |
Author
|
All good. Just check whether the ASPM stuff works for you and whether you want it (in this form). |
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
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.
Hello!
I have been using your facetimehd-toggle app for a while, however after some attempts to integrate power saving on the camera as a stand-alone script, I came to the conclusion that it is not possible and I need to modify the app itself.
The thing is that:
When ASPM is enabled, CPU can reach C6/C7 states (assuming that the rest of the system is setup well/has the right components), however the camera develops artifacts.
When ASPM is disabled, CPU is stuck in C2/C3, and the camera works great.
Unfortunately one cannot have both. So I have to switch ASPM at the same time as I enable/disable the camera.
And at the same time:
So the result is a bunch of scripts - working together with your switch.
Feel free to integrate whatever you feel like!
(Just a warning, I can see that I also included in the pull request changes to the README - like removal of Hyperland and change of the link to the repo which I did not intend to. Sorry about that.)