BME680 Sensor Component#11695
Conversation
…res sensor connected
|
Please note that it is necessary to run the sensor update outside of the event loop in a thread if the gas sensor is enabled. There needs to be a consistent interval between the gas sensor reads to keep the readings stable. I tried many ways to do this but the only method that worked was to use a separate thread to read the sensor data at a fixed interval (every 1 second) and then replicate the data structure so that home-assistant can read it on the event loop. The thread is only used if the gas sensor is enabled (also required for Air Quality calculations), otherwise it is entirely run on the event loop. |
|
|
||
| # pylint: disable=import-error | ||
| @asyncio.coroutine | ||
| def async_setup_platform(hass, config, async_add_devices, discovery_info=None): |
There was a problem hiding this comment.
It's very inefficient to keep yielding to the executor pool all the time. Better to just run the whole setup in 1 worker.
You can either make the setup_platform sync (by changing name to setup_platform) or define a function that runs all the I/O commands and run that in the executor pool.
There was a problem hiding this comment.
@balloob I have refactored async_setup_platform to call a single function that runs the I/O commands.
Please review.
|
@balloob I have refactored async_setup_platform to call a single function that runs the I/O commands. Please review. |
|
Neat! Thanks 🐬 🌮 |
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Some minor things that would be good to adjust in a future PR. See below.
|
|
||
| Temperature, humidity, pressure and volitile gas support. | ||
| Air Qaulity calucaltion based on humidity and volatile gas. | ||
|
|
There was a problem hiding this comment.
Missing link to docs here. Look at other modules.
|
|
||
| sensor_handler = yield from hass.async_add_job(_setup_bme680, config) | ||
| if sensor_handler is None: | ||
| return False |
There was a problem hiding this comment.
Nothing is checking this return value, so just return here.
| pass | ||
|
|
||
| async_add_devices(dev) | ||
| return True |
| """Set up and configure the BME680 sensor.""" | ||
| from smbus import SMBus | ||
| import bme680 | ||
| from time import sleep |
There was a problem hiding this comment.
Standard library imports should be imported at the top of the module. Only platform specific imports need to be done inside functions.
| self.sensor_data.air_quality = self._calculate_aq_score() | ||
| time.sleep(1) | ||
| else: | ||
| return |
There was a problem hiding this comment.
You could move this to the initial check if not self._gas_sensor_running after inverting that. Then you could outdent most of the lines in this method.
| for variable in config[CONF_MONITORED_CONDITIONS]: | ||
| dev.append(BME680Sensor( | ||
| sensor_handler, variable, SENSOR_TYPES[variable][1], name)) | ||
| except KeyError: |
There was a problem hiding this comment.
This can't happen with config validation in place. You can remove the try... except.
|
@MartinHjelmare I have implemented the changes you requested in PR#: 11892. |
4 participants
Description:
BME680 Sensor Component
Related issue (if applicable): N/A
Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#4432
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
toxrun successfully. Your PR cannot be merged unless tests passREQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass