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

Added basic fan status logging #65

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added basic fan status logging #65

wants to merge 1 commit into from

Conversation

FlorentLM
Copy link

Added basic fan status logging for use with external metrics collectors.

Example for Telegraf:

[[inputs.file]]
  files = ["/var/log/fanshim_status"]
  name_override = "fanshim_status"
  data_format = "value"
  data_type = "integer"

Added basic fan status logging for use with external metrics collectors (such as Telegraf)
@Gadgetoid
Copy link
Member

Thanks for taking the time to PR this.

It seems like a worthwhile addition, but should probably be paired with a new argument that disables logging by default and allows users to enable it. Something like:

parser.add_argument('--logging', action='store_true', help='Enable logging')

Additionally it could be worth using Python's logging module so log entries are implicitly timestamped, see: https://docs.python.org/3/library/logging.html

A complete implementation, I think, would use logging coupled with a configurable logging-level so that perhaps warnings are generated for fan on/off events and info messages are generated containing the info currently output when args.verbose is set.

If you're up for turning this PR into a more concerted learning effort (I'm making assumptions - based on this being your first PR - that Python might be new to you!) then I'm happy to guide you through these changes.

@Gadgetoid Gadgetoid self-assigned this Feb 24, 2020
@Gadgetoid Gadgetoid added the enhancement New feature or request label Feb 24, 2020
@FlorentLM
Copy link
Author

FlorentLM commented Feb 28, 2020

Sure, these are good points! This was initially very basic, as for my own purpose I really only needed to know when the fan was on or off for Telegram, I tried to add as little code as possible. But your ideas are definitely needed for a more public implementation, yes.

I'm totally up for improving this PR, I'll do that as soon as I have some time.

PS - no worries, Python is not new to me, it'll be fine :) But thanks!

@Gadgetoid Gadgetoid added this to the Overhaul automatic.py milestone Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants