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

Driver for the sensirion sht85 sensor #136

Merged
merged 6 commits into from
Nov 10, 2020
Merged

Conversation

konstantinmauer
Copy link
Contributor

Adds a driver for the sensirion sht85 sensor and a sensirion sensor bridge. It uses official drivers of sensirion to communicate with the sensor bridge.

Copy link
Collaborator

@odnarod odnarod left a comment

Choose a reason for hiding this comment

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

this thing is getting quite complicated for a temperature sensor 🙈
would like to hear what @DavidLP thinks about some of my comments..

basil/HL/SensirionBridgeDevice.py Show resolved Hide resolved
basil/HL/sensirion_sht85.py Show resolved Hide resolved
basil/HL/sensirion_sht85.py Outdated Show resolved Hide resolved
basil/HL/sensirion_sht85.py Outdated Show resolved Hide resolved
basil/HL/sensirion_sht85.py Show resolved Hide resolved
basil/TL/SensirionSensorBridge.py Outdated Show resolved Hide resolved
basil/TL/SensirionSensorBridge.py Outdated Show resolved Hide resolved
basil/TL/SensirionSensorBridge.py Outdated Show resolved Hide resolved
basil/TL/SensirionSensorBridge.py Outdated Show resolved Hide resolved
examples/lab_devices/sensirionSHT85.py Outdated Show resolved Hide resolved
@DavidLP
Copy link
Collaborator

DavidLP commented Nov 3, 2020

Pretty nice first commit into basil!

@DavidLP
Copy link
Collaborator

DavidLP commented Nov 3, 2020

I do not know the Sensirion box etc. so it is tough for me to judge this. From what I read, it works as the following:

  • there is a sensirion box that allows to have up to 2 ic2 devices connected to it. This is the TL SensirionSensorBridge
  • the i2 devices can be connected to it. A generic i2c device is SensirionBridgeI2CDevice
  • a specific i2c device are the temperature devices sensirionSHT85

I mean that is (as I can quickly judge) a very clean and generic implementation to support hardware we have and to support hardware we don't have ;-)

If we are only going to use the bridge with the temperature sensors, one could maybe just make one driver and skip the extra TL.
For lab devices I originally used the TL to specify the means of communication (RS232, TCP/IP, GPIB, USB and the library NI VISA, python.socket, ...). This makes a lot of sense for lab devices that have multiple interfaces (e.g. TTi, Keithleys, ...). I am not sure what we gain for having such generic TL/HL pattern, when the TL is not interchangeable, not configurable and fixed (@michaeldaas an me discussed that recently, too).

@DavidLP DavidLP self-requested a review November 9, 2020 15:30
@DavidLP
Copy link
Collaborator

DavidLP commented Nov 9, 2020

I recommend to configure your editor in a way that it takes care of the code style. Good editors (pydev, VStudio Code, ...) can do that. When you clean up the formatting issues https://travis-ci.org/github/SiLab-Bonn/basil/jobs/741928925#L5989 it is fine to be merged. You can check for code flaws by typing:

flake8-diff origin/development

@DavidLP
Copy link
Collaborator

DavidLP commented Nov 9, 2020

Also, you see that the CI failed due to code flaws

@coveralls
Copy link

coveralls commented Nov 10, 2020

Coverage Status

Coverage decreased (-1.1%) to 32.447% when pulling 52d54d3 on sensirion_sht85 into 968842a on development.

@konstantinmauer konstantinmauer merged commit 183a36f into development Nov 10, 2020
@themperek themperek deleted the sensirion_sht85 branch February 9, 2022 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants