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

Read more temperature data from hwmon on linux #827

Merged

Conversation

darnuria
Copy link
Contributor

@darnuria darnuria commented Aug 31, 2022

May supplant #823, may help in #826.

  • Pro: Read more
  • cons: refactoring, do some magic to comply with Component/ComponentExt traits (labels and max)

Read things that are not used now, maybe doing a Linux specific Hwmon richer trait?

Not exhaustive checklist of what is done:

  • One monomorphised function to read just an number in file
  • Use option to denote non mandatory data
  • Possibility to read tempN_highest value or computed max (may need testing?)
  • Rework append_files
  • squash maybe a little bit?
  • Backport some doc?

@darnuria darnuria force-pushed the exploration/moar-hwmon-linux branch from 6208a51 to 79f2e16 Compare August 31, 2022 12:14
@darnuria
Copy link
Contributor Author

small forcepush to rebase onto master.

@darnuria
Copy link
Contributor Author

Thanks for early review, sorry for the hwmon thing noise, learned a lot while toying around, i did a big comment but can split if it's worth it, had a big design hesitation while doing it.

src/linux/component.rs Outdated Show resolved Hide resolved
src/linux/component.rs Outdated Show resolved Hide resolved
src/linux/component.rs Outdated Show resolved Hide resolved
src/linux/component.rs Outdated Show resolved Hide resolved
@darnuria
Copy link
Contributor Author

darnuria commented Sep 2, 2022

Made some change to fix CI. Had issue with format!("{var}") on 1.54.

src/traits.rs Outdated Show resolved Hide resolved
src/traits.rs Outdated Show resolved Hide resolved
src/traits.rs Outdated
Comment on lines 1507 to 1510
/// ## Linux hwmon
///
/// Read current Maximum value encountered may be computed by sysinfo
/// or from kernel in case of an errors, returns `f32::NAN`.
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
/// ## Linux hwmon
///
/// Read current Maximum value encountered may be computed by sysinfo
/// or from kernel in case of an errors, returns `f32::NAN`.
/// ## Linux
///
/// Returns `f32::NAN` if an error occurred.

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to found a common ground on about to explain the behaviour without being too verbose.

src/traits.rs Outdated Show resolved Hide resolved
src/traits.rs Outdated Show resolved Hide resolved
src/traits.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@darnuria darnuria left a comment

Choose a reason for hiding this comment

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

Pushed some fixup! to address review feedback. On some to end up bikesheding I tried to formulate a consensus.

On some I simply droped the info because either we went something possibly false or exposing implementation details to dev-user.

src/traits.rs Outdated
Comment on lines 1507 to 1510
/// ## Linux hwmon
///
/// Read current Maximum value encountered may be computed by sysinfo
/// or from kernel in case of an errors, returns `f32::NAN`.

This comment was marked as outdated.

src/traits.rs Outdated Show resolved Hide resolved
darnuria and others added 7 commits September 3, 2022 00:55
What it does?

Expose new things: (Read only)

- Expose a label that mimic `sensors` own (breaking change)
- Sensor type
- Allow reading of everything with a temp sensor.
- Highest form kernel/chip
  maximum historical read value, is read from hardware OR from computation.
- Thresolds max, critical, low

Also fully remove raspberry-pi special code.

Not everything is used, maybe a HwmonLinux specific trait can be done and
use a degraded view to comply with existing cross-os `Component`.

Future work? Read Fan, Voltage, Intrusion alert?
@GuillaumeGomez
Copy link
Owner

Thanks a lot for all these improvements!

@darnuria
Copy link
Contributor Author

darnuria commented Sep 3, 2022

No problem that was fun! Don't hesitate to @-me in case of regression, bugs or any issue about it. Also opened #826 for discussing reading more data from hwmon.

@darnuria darnuria deleted the exploration/moar-hwmon-linux branch September 3, 2022 12:26
@darnuria
Copy link
Contributor Author

Just in case of breakage: We removed the "special case code" for raspbian (raspberry pi plateform) it was relate to #328 and #327.

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.

2 participants