Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notify in CLI if system is incompatible #401

Merged
merged 1 commit into from
Feb 20, 2024
Merged

Notify in CLI if system is incompatible #401

merged 1 commit into from
Feb 20, 2024

Conversation

13r0ck
Copy link
Contributor

@13r0ck 13r0ck commented Jun 7, 2023

Print out a error message if the system is not supported. Behaviour before this is a success that looks like the power profile was changed, which is incorrect.

@13r0ck 13r0ck requested review from a team June 7, 2023 19:41
@jacobgkau jacobgkau self-assigned this Jun 7, 2023
@13r0ck
Copy link
Contributor Author

13r0ck commented Jun 7, 2023

Do we want rustfmt to pass? It looks like other PRs have merged without that, so if I cargo fmt here it is going to change a lot of stuff

@jacobgkau
Copy link
Member

That's up to you/engineering. That check not passing isn't a regression from a QA standpoint since it's already not passing on master.

Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, I did already get an error message that somewhat explained what the problem was if I tried to switch graphics profiles on a non-switchable system.

system76@pop-os:~$ system76-power graphics nvidia
setting graphics to nvidia
daemon returned an error message: "does not have switchable graphics"
system76@pop-os:~$

Now, I get a longer message that may be more human-readable.

system76@pop-os:~$ system76-power graphics nvidia

System76 Power is not supported on this device.
This means that your device is either a desktop or doesn't have both an iGPU and dGPU.
System76 Power's daemon automatically stops on systems like this. There is no need
to uninstall nor any need for further configuration.

system76@pop-os:~$ 

However, this new message is also showing up if I try to switch power profiles (testing on lemp10.) All laptops should work with power profiles, not just the graphics-switchable ones. This change also breaks setting charging profiles (displaying the same message.)

@13r0ck
Copy link
Contributor Author

13r0ck commented Jun 7, 2023

I force pushed a change, hows it look now?

src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
jackpot51
jackpot51 previously approved these changes Jun 8, 2023
@13r0ck 13r0ck requested a review from a team June 8, 2023 21:40
Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Power profiles and charging profiles now work again on non-switchable laptops.

For desktops, do we want to consider also using the get_desktop method/message for charging profiles? Currently, it still returns Not running System76 firmware with charge threshold support.

src/client.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
jacobgkau
jacobgkau previously approved these changes Jun 15, 2023
Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "not switchable" string still seems pointless since it will never be displayed, but that's more engineering's discretion.

Desktop:

  • system76-power graphics returns a new message instead of functioning like a laptop.
  • system76-power profile returns a new message instead of functioning like a laptop.
  • system76-power charge-thresholds returns an existing daemon error message.

I noticed in the logs that the GNOME Shell extension still thinks the "balanced" profile is being loaded when it first starts. The top-right menu doesn't include the battery/power drop-down, so not sure what its behavior would be now.

Laptop (switchable):

  • system76-power graphics still works.
  • system76-power profile still works.
  • system76-power charge-thresholds still works.

Laptop (non-switchable):

  • system76-power graphics returns a new message instead of the old daemon error message.
  • system76-power profile still works.
  • system76-power charge-thresholds still works.

@13r0ck
Copy link
Contributor Author

13r0ck commented Jun 15, 2023

@jacobgkau

system76-power charge-thresholds returns an existing daemon error message.

That is a good point. I pushed a fix for that. Thank you

src/client.rs Outdated Show resolved Hide resolved
Copy link
Member

@jacobgkau jacobgkau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Desktop:

  • system76-power graphics returns a new message instead of functioning like a laptop.
  • system76-power profile returns a new message instead of functioning like a laptop.
  • system76-power charge-thresholds returns a new message instead of the old daemon error message.

Laptop (switchable):

  • system76-power graphics still works.
  • system76-power profile still works.
  • system76-power charge-thresholds still works.

Laptop (non-switchable):

  • system76-power graphics returns a new message instead of the old daemon error message.
  • system76-power profile still works.
  • system76-power charge-thresholds still works.

Print out a error message if the system is not supported.
Behaviour before this is a success that looks like the power
profile was changed, which is incorrect.
@mmstick mmstick merged commit 27192d2 into master Feb 20, 2024
16 checks passed
@mmstick mmstick deleted the notify branch February 20, 2024 19:55
@jacobgkau
Copy link
Member

jacobgkau commented Feb 20, 2024

I gave this a quick sanity check since it's been a while.

This is still working as expected on switchable and non-switchable laptops.

Testing in a virtual machine attempting to simulate a desktop, I found that by default, /sys/class/dmi/id/chassis_type was 1 (while the host laptop was 9), which doesn't match the 3 the code is looking for to detect desktops. Looking at DMI documentation, it appears 1 is "other" and 3 is indeed "desktop." In the virtual machine set to 1, I was visually able to switch between power profiles using the GNOME extension, but I got errors doing it on the CLI, which is not a regression.

In order to properly simulate running this on a desktop, I cloned this branch and modified the is_desktop check in graphics.rs to look for 1 instead of 3. After doing that, I saw similar behavior: the power profile was visually able to switch in the GUI, but attempting to switch in the CLI (or check what mode I'm in) now gives the notice that it's not supported. I do think the only reason the menu was showing up in the GUI was because VirtualBox is passing my laptop's battery info through to the VM.

We might be able to better hide the options in the GUI on desktops, but this isn't going to cause any crashes in the GUI (and it sounds like profiles weren't properly functioning on desktops anyway, from this PR's initial description).

Edit: @XV-02 did confirm that the GUI menu doesn't show up at all on desktops by default.

@CodeMan99
Copy link

CodeMan99 commented Feb 22, 2024

Behaviour before this is a success that looks like the power profile was changed, which is incorrect.

Is the "success" actually incorrect? I'm currently trying to debug what seems to be a hardware issue. Compiling a personal project caused hardware instability if I ran sudo system76-power profile performance. Compiling the same project after sudo system76-power profile battery was much slower, but stable.

OS: Pop!_OS 22.04 LTS x86_64
Host: Thelio Mira thelio-mira-b4.1
Kernel: 6.6.10-76060610-generic
Uptime: 8 mins
Packages: 1883 (dpkg), 33 (flatpak)
Shell: bash 5.1.16
Resolution: 1920x1200, 1920x1200
DE: GNOME 42.5
WM: Mutter
WM Theme: Pop
Theme: Pop-dark [GTK2/3]
Icons: Pop [GTK2/3]
Terminal: alacritty
CPU: Intel i9-14900K (32) @ 5.700GHz
GPU: Intel Device a780
GPU: AMD ATI 03:00.0 Device 7480
Memory: 3255MiB / 64063MiB

I'm now unsure what profile is active on my machine.

@mmstick
Copy link
Member

mmstick commented Feb 22, 2024

Power profiles do work on desktops, yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants