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

[BUG] Apple M CPU Generation not displayed #678

Closed
masiboss opened this issue Dec 5, 2023 · 8 comments · Fixed by #679
Closed

[BUG] Apple M CPU Generation not displayed #678

masiboss opened this issue Dec 5, 2023 · 8 comments · Fixed by #679
Assignees
Labels
bug Something isn't working

Comments

@masiboss
Copy link
Contributor

masiboss commented Dec 5, 2023

Describe the bug

The CPU name is displayed as Apple M, while on a Apple M2
i belive it is read here

size_t size = sizeof(buffer);
if (sysctlbyname("machdep.cpu.brand_string", &buffer, &size, NULL, 0) < 0) {
Logger::error("Failed to get CPU name");
return name;
}
name = string(buffer);

however
sysctl -a | grep machdep.cpu.brand_string
returns
machdep.cpu.brand_string: Apple M2

To Reproduce

Install on a Apple Silicon Mac

Expected behavior

Displays Apple M2

Screenshots

image

Info (please complete the following information):

  • btop++ version: 1.2.13
  • Architecture: arm64
  • Platform: OSX
  • (OSX/FreeBSD) Os release version: 14.1.1
  • Terminal used: iTerm2 3.4.22
@masiboss masiboss added the bug Something isn't working label Dec 5, 2023
@imwints
Copy link
Contributor

imwints commented Dec 6, 2023

Could you add a

#include <iostream>

at the top of the file and a

std::cerr << name << '\n';

below name = ..., then compile it and post whatever is in log if you run btop 2> log?

Edit:
Or just use this

#include <iostream>
#include <string>

#include <sys/sysctl.h>

int main() {
	char buffer[1024];
	std::size_t size = sizeof buffer;
	sysctlbyname("machdep.cpu.brand_string", &buffer, &size, nullptr, 0);
	std::cout << std::string { buffer } << '\n';
}

@masiboss
Copy link
Contributor Author

masiboss commented Dec 7, 2023

i don't know c++ but i had to make a minor change to get it to compile

#include <iostream>
#include <string>

#include <sys/sysctl.h>

int main() {
	char buffer[1024];
	std::size_t size = sizeof buffer;
	sysctlbyname("machdep.cpu.brand_string", &buffer, &size, nullptr, 0);
	std::cout << std::string(buffer) << '\n';
}

Apple M2

@masiboss
Copy link
Contributor Author

masiboss commented Dec 7, 2023

ok i just launched btop to have another look and its actually an issue with the text length and different scaling, so i assume here

const string cpu_title = uresize((custom.empty() ? Cpu::cpuName : custom) , b_width - 14);

it should rather be cutting off "Apple" in
string get_cpuName() {

but this would still be to long on a M2 Ultra as that's 8 characters and it currently seems to be cut to 7
image
image

@aristocratos
Copy link
Owner

@masiboss
The issue isn't in Cpu::get_cpuName().
In most cases the current cpu frequency is detected and showed next to the cpu name.
When that happens the cpu name needs to be trimmed for both to fit.

To fix this the following line need to be edited:

const string cpu_title = uresize((custom.empty() ? Cpu::cpuName : custom) , b_width - 14);

And needs to check if cpu frequency is available, if not, the -14 needs to be changed to a more appropriate size.

@aristocratos
Copy link
Owner

There is also a config variable that should be checked Config::getB("show_cpu_freq").
So the logic would need to check if the config variable is false, else if true if the the output of Cpu::get_cpuHz() is empty.

I think the size would be b_width - 4 instead if previous conditions are true.

@aristocratos
Copy link
Owner

aristocratos commented Dec 7, 2023

Something like:

const string cpu_title = uresize(
	(custom.empty() ? Cpu::cpuName : custom),
	b_width - (Config::getB("show_cpu_freq") and not Cpu::get_cpuHz().empty() ? 14 : 4)
);

This would need to be tested on all platforms though since the Cpu::get_cpuHz() call here would incur a latency cost every time the sizes of the boxes are updated.

@aristocratos
Copy link
Owner

aristocratos commented Dec 7, 2023

Maybe a better way to go about it would be to have it like:

static const bool hasCpuHz = not Cpu::get_cpuHz().empty()

const string cpu_title = uresize(
	(custom.empty() ? Cpu::cpuName : custom),
	b_width - (Config::getB("show_cpu_freq") and hasCpuHz ? 14 : 4)
);

@masiboss
Copy link
Contributor Author

masiboss commented Dec 7, 2023

went ahead and added that to my pull request, i still think stripping out "Apple" is correct for consistency with the other CPU brands

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants