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

Add 1-Wire sensor monitoring #3822

Merged
merged 1 commit into from Jun 15, 2018
Merged

Add 1-Wire sensor monitoring #3822

merged 1 commit into from Jun 15, 2018

Conversation

@dspinellis
Copy link
Contributor

@dspinellis dspinellis commented Jun 14, 2018

Auto-detect and display data from multiple 1-Wire temperature sensors.
Sensor names are user-configurable.

@dspinellis
Copy link
Contributor Author

@dspinellis dspinellis commented Jun 14, 2018

1-wire

Here is an image of the resultant dashboard.

Loading

'absolute', 1, 10])
self.definitions['temp']['lines'] = lines
return len(self.probes) > 0
except Exception as error:
Copy link
Member

@ilyam8 ilyam8 Jun 14, 2018

Choose a reason for hiding this comment

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

Some suggestions:

  1. Make exception more specific
  2. Remove all code from try/except block which can not cause an exception

Like:

try:
    filenames = os.listdir(W1_DIR)
except OSError as err:  # i believe we only need OSError here, but i am not sure
    self.error(err)
    return None

...
...

Loading

Copy link
Contributor Author

@dspinellis dspinellis Jun 14, 2018

Choose a reason for hiding this comment

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

Done.

Loading

value = round(int(matched.group(1)) / 1000., 1)
value = int(value * 10)
data['w1sensor_temp_' + identifier] = value
except Exception as error:
Copy link
Member

@ilyam8 ilyam8 Jun 14, 2018

Choose a reason for hiding this comment

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

please make exception more specific

Loading

Copy link
Contributor Author

@dspinellis dspinellis Jun 14, 2018

Choose a reason for hiding this comment

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

Done.

Loading

@ilyam8
Copy link
Member

@ilyam8 ilyam8 commented Jun 14, 2018

@dspinellis thanks for the contribution 👍

Loading

@dspinellis
Copy link
Contributor Author

@dspinellis dspinellis commented Jun 14, 2018

Thanks! I pushed a commit addressing your comments. I can then squash the commits when the PR passes code review.

Loading

value = round(int(matched.group(1)) / 1000., 1)
value = int(value * 10)
data['w1sensor_temp_' + identifier] = value
except OSError as err:
Copy link
Member

@ilyam8 ilyam8 Jun 14, 2018

Choose a reason for hiding this comment

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

add IOError here

Loading

Copy link
Contributor Author

@dspinellis dspinellis Jun 14, 2018

Choose a reason for hiding this comment

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

Right, done!

Loading

ilyam8
ilyam8 approved these changes Jun 14, 2018
@dspinellis
Copy link
Contributor Author

@dspinellis dspinellis commented Jun 14, 2018

I squashed my commits following your approval of the changes.

Loading

# default module values (can be overridden per job in `config`)
update_every = 5
priority = 60000
retries = 60
Copy link
Member

@Ferroin Ferroin Jun 14, 2018

Choose a reason for hiding this comment

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

Unless I'm mistaken, you're using the same number of retries as the default, in which case this line can probably be removed.

Loading

Copy link
Member

@ilyam8 ilyam8 Jun 14, 2018

Choose a reason for hiding this comment

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

Yes, retries and priority can be removed since it is current default

Loading

Copy link
Contributor Author

@dspinellis dspinellis Jun 14, 2018

Choose a reason for hiding this comment

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

Thank you; I removed them.

Loading

Copy link
Member

@Ferroin Ferroin left a comment

Beyond the one codre comment I left (which isn't really critical), looks good to me.

Loading

@ilyam8
Copy link
Member

@ilyam8 ilyam8 commented Jun 14, 2018

@dspinellis please do a rebase

Loading

Multiple 1-Wire temperature sensors are auto-detected and monitored.
Displayed sensor names are user-configurable.
@dspinellis
Copy link
Contributor Author

@dspinellis dspinellis commented Jun 14, 2018

I rebased and squashed the commit to remove the unneeded default values.

Loading

@ktsaou
Copy link
Member

@ktsaou ktsaou commented Jun 15, 2018

Well done!
Thank you @dspinellis !

Loading

@ktsaou ktsaou merged commit 9598db2 into netdata:master Jun 15, 2018
3 checks passed
Loading
@Ferroin Ferroin mentioned this pull request Aug 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants