Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough
Priority: ⬇️ Low Change: Feature Merge Risk: 🔵 Low · up to
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: f9b41fef-98a3-450a-8cec-3864c8e4117c
📒 Files selected for processing (5)
FanMeter.cFanMeter.hMakefile.amlinux/Platform.clinux/Platform.h
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| if (dfd == -1) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove braces from the three single-statement bodies.
Remove the braces at lines 451-453, 464-466, and 514-516. Keep braces around the cleanup-plus-control-flow bodies at lines 469-471 and 502-504.
The repository guidance requires omitting braces around simple single statements.
|
Hi, just adding some context that might be useful here for this PR While working on #2076 I also drafted a follow-up branch to make the hardware sensor code backend-independent and add direct Linux hwmon support as a fallback when libsensors is unavailable: https://github.com/massimomazzariol/htop/tree/feature/hwmon-fallback The draft currently discovers hwmon temperature, fan, PWM and voltage sensors and feeds them through the same generic The main hwmon backend commit is here: I deliberately kept this out of #2076 because I did not want to expand the scope again after the sensor meter implementation had gone through review. Since #2107 is now adding fan support through hwmon, I thought it was worth mentioning in case parts of that draft or the abstraction could be useful, or if the two approaches should be aligned before both implementations diverge. If you want, feel free to adapt the draft depending on which direction you prefer |
| this->values[residueIndex] = residuePercentage; | ||
| } | ||
|
|
||
| static int Platform_getFanSpeedFromHwmonDevice(int parent_dfd, char const* hwmon_device) { |
There was a problem hiding this comment.
Using const char * here is fine and overall more common in the codebase.
| static int Platform_getFanSpeedFromHwmonDevice(int parent_dfd, char const* hwmon_device) { | |
| static int Platform_getFanSpeedFromHwmonDevice(int parent_dfd, const char *hwmon_device) { |
| DIR* dirp; | ||
| int dfd; | ||
|
|
||
| dfd = openat(parent_dfd, hwmon_device, O_RDONLY | O_CLOEXEC | O_DIRECTORY); |
| continue; | ||
| } | ||
|
|
||
| fclose(fp); |
|
|
||
| int fd = openat(dfd, dir->d_name, O_RDONLY | O_CLOEXEC); | ||
| if (fd == -1) | ||
| continue; |
There was a problem hiding this comment.
Some blank line after the if statement doesn't hurt.
| continue; | |
| continue; | |
| /* | ||
| Traverse hwmon sensors, find first fan, read and return its speed. | ||
| Returns -1 on failure. | ||
| */ |
There was a problem hiding this comment.
| /* | |
| Traverse hwmon sensors, find first fan, read and return its speed. | |
| Returns -1 on failure. | |
| */ | |
| /* Traverse hwmon sensors, find first fan, read and return its speed. */ |
| FileDescriptorMeter.h \ | ||
| FunctionBar.h \ | ||
| GPUMeter.h \ | ||
| FanMeter.h \ |
There was a problem hiding this comment.
File order is sorted alphabetically
|
|
||
| linux_platform_sources = \ | ||
| GPUMeter.c \ | ||
| FanMeter.c \ |
There was a problem hiding this comment.
File order is sorted alphabetically
|
@foxpy No problem. Sometimes PRs take a while to ripen. One of my earliest PRs to htop took more then 6 months and it mostly cleaned up some existing code to make it be shared by all platforms. Given the long-term maintainability is important with a huge code-base, having several approaches to a problem to compare what works best is a clear plus. This applies similarly to using synergies with existing code, thus re-using existing infrastructure to query information or realize a feature is much preferred. @coderabbitai Removing braces around single-statement bodies is not required, but (strongly) encouraged. It's totally fine to have them, e.g. if the statements itself are somewhat complicated, but for most simple cases (i.e. just an assignment), the variant without braces is preferred/recommended. Furthermore: blank openat without @massimomazzariol Your PR #2076 is still on my list of PRs to review, but this might still take some time. As noted above: Having a shared infrastructure for accessing hwmon information without duplicating logic unnecessarily sounds good. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
|
@BenBE thank you for your review. I am afraid @massimomazzariol 's work in PR #2076 is more useful and we both should focus on reviewing it instead of spending extra time on what I did. What do you think? |
|
As noted earlier in this thread, I prefer if we can re-use infrastructure to fetch information from the system, thus going is #2076 is the probably the best move to go forward. The comments in my previous review are for reference, but also what would need to be done if we were to use this PR instead. Unfortunately I'm currently a bit short on spare time for timely reviews, but I try to get around to them eventually as time permits. That said: I still have #2076 marked for reviewing on my todo list … |
BenBE
added
Linux 🐧
|
Alright, I fixed the issues that you mentioned. I don't know how you would like to proceed, but feel free to close this PR and move on with #2076 if you think that's a better way to go. If you would still like to proceed with this PR, I have a question: do you want me to squash all commits here and force push, so we don't have this dangling "review fixes" commit lying around? |
|
Huh… I think the implementation of openat accepts fd as its first argument, but in |
The type of In the header you will thus find this block: #ifdef HAVE_OPENAT
typedef int openat_arg_t;
static inline void Compat_openatArgClose(openat_arg_t dirfd) {
close(dirfd);
}
static inline int Compat_openat(openat_arg_t dirfd, const char* pathname, int flags) {
return openat(dirfd, pathname, flags);
}
#else /* HAVE_OPENAT */
typedef const char* openat_arg_t;
static inline void Compat_openatArgClose(openat_arg_t dirpath) {
(void)dirpath;
}
int Compat_openat(openat_arg_t dirpath, const char* pathname, int flags);
#endif /* HAVE_OPENAT */which forwards the Thus when using #ifdef HAVE_OPENAT
int cpuDirFd = openat(xDirfd(dir), entry->d_name, O_DIRECTORY | O_PATH | O_NOFOLLOW);
if (cpuDirFd < 0)
continue;
#else
char cpuDirFd[4096];
xSnprintf(cpuDirFd, sizeof(cpuDirFd), "/sys/devices/system/cpu/%s", entry->d_name);
#endifHope this helps with your confusion. |
Yes, it helps a lot! And also tells that I've written grossly incorrect code :) |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Require an exact fanN_input match. · Platform.c:472
linux/Platform.c:472
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire an exact
fanN_inputmatch.
sscanfcan return one assignment after parsing the fan number, even when the_inputsuffix does not match. A numericfanN_minorfanN_faultfile can therefore be read first and returned as the fan speed.FanMeter_updateValuesdisplays every non-negative result as RPM, so this produces an incorrect display. The impact is display-only, so this is a minor issue rather than a major one.Suggested fix
int fan_num; - if (sscanf(dir->d_name, "fan%d_input", &fan_num) != 1) + int name_length; + if (sscanf(dir->d_name, "fan%d%n", &fan_num, &name_length) != 1 || + !String_eq(dir->d_name + name_length, "_input")) continue;
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
e964acce-b409-4a5c-8714-5ff6597cadb1
📒 Files selected for processing: 1
linux/Platform.c
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
3 participants
This PR is a replacement for #2009, which implemented the same feature, but using a vendor-specific interface, only supporting Thinkpads running Linux. The new version should support all Linux devices, because it uses hwmon.
It only reports the speed of the first fan it finds, which is not great, but hey, it's something :)
@BenBE sorry for ignoring #2009 for the entire summer, I've been busy with personal stuff and completely forgot about this :D
I remember that we talked about adding support for minimum and maximum fan speed reporting, which I also decided to keep aside for now.