Tilt Pi integration #139726
Tilt Pi integration #139726
Conversation
There was a problem hiding this comment.
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
| "data": { | ||
| "name": "Tilt Pi Friendly Name", | ||
| "host": "Tilt Pi Hostname or IP Address", | ||
| "port": "Tilt Pi Port" |
There was a problem hiding this comment.
Camel-case "TiltPi" or not? – Below you used that.
For the rest: Everything except the name must be sentence-cased in the three strings above.
There was a problem hiding this comment.
Thanks for the feedback!
The official naming seems to be "Tilt Pi". I have updated the strings and other places to reflect that more clearly where it comes to the string representation bed25e0
|
I believe this is just waiting for a review, if not please let me know if something is missing. |
|
|
||
| try: | ||
| await client.get_hydrometers() | ||
| except Exception as e: |
There was a problem hiding this comment.
Only catch bare exceptions in the config flow, we should be specific here
| session = async_get_clientsession(hass) | ||
| client = TiltPiClient( | ||
| host=entry.data[CONF_HOST], | ||
| port=entry.data[CONF_PORT], | ||
| session=session, | ||
| ) | ||
|
|
||
| try: | ||
| await client.get_hydrometers() | ||
| except Exception as e: |
There was a problem hiding this comment.
when running await coordinator.async_config_entry_first_refresh, the coordinator already tries to connect, and if it raised UpdateFailed, it will automatically raise ConfigEntryNotReady, so we can remove this here. I would also recommend to just move the client creation to the coordinator
| try: | ||
| await coordinator.async_config_entry_first_refresh() | ||
| entry.runtime_data = coordinator | ||
| except ConfigEntryNotReady: | ||
| await coordinator.async_shutdown() | ||
| raise |
| await coordinator.async_shutdown() | ||
| raise | ||
|
|
||
| hass.data.setdefault(DOMAIN, {})[entry.entry_id] = coordinator |
There was a problem hiding this comment.
you already store it in the entry runtime data
| """Unload a config entry.""" | ||
| if unload_ok := await hass.config_entries.async_unload_platforms(entry, PLATFORMS): | ||
| coordinator: TiltPiDataUpdateCoordinator = hass.data[DOMAIN].pop(entry.entry_id) | ||
| await coordinator.async_shutdown() |
There was a problem hiding this comment.
this is also already called, you can just return the first line
| "dependencies": [], | ||
| "documentation": "https://www.home-assistant.io/integrations/tilt_pi", | ||
| "homekit": {}, | ||
| "iot_class": "local_polling", | ||
| "quality_scale": "bronze", | ||
| "requirements": [], | ||
| "ssdp": [], | ||
| "zeroconf": [] |
| @@ -0,0 +1,14 @@ | |||
| { | |||
| "domain": "tilt_pi", | |||
There was a problem hiding this comment.
since we already have the BLE one, we should create a brand for this
| SENSOR_TYPES: Final[list[TiltEntityDescription]] = [ | ||
| TiltEntityDescription( | ||
| key=ATTR_TEMPERATURE, | ||
| name="Temperature", |
There was a problem hiding this comment.
| name="Temperature", |
It will now use device clas translations
| name="Gravity", | ||
| native_unit_of_measurement="SG", | ||
| icon="mdi:water", |
There was a problem hiding this comment.
please use translations for this
| async_add_entities: AddConfigEntryEntitiesCallback, | ||
| ) -> None: | ||
| """Set up Tilt Hydrometer sensors.""" | ||
| coordinator: TiltPiDataUpdateCoordinator = config_entry.runtime_data |
There was a problem hiding this comment.
| coordinator: TiltPiDataUpdateCoordinator = config_entry.runtime_data | |
| coordinator = config_entry.runtime_data |
Can be removed as it already can be inferred
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
We do still have the rule that any device or service specific code should be packaged in its own library. It's our way of giving back to the python community and this way we make sure our codebase isn't HUGE. This way we also split concerns and make stuff like testing way easier. |
|
If its ready for review again, please use the button to mark it as such |
Breaking change
Proposed change
Adds a new integration for Tilt Pi devices, which are devices such as Raspberry Pi or similar devices running the Tilt Pi image. The purpose of Tilt Pi is to communicate with Tilt Hydrometers, which are devices used in monitoring brewing temperature and specific gravity.
Tilt Hydrometer BLE is already a supported integration. Unfortunately for my use case, my Tilt Hydrometers are not within any Bluetooth device range, and therefore the Tilt Pi provides a bridge between the Tilt Hydrometer data and use cases that rely on that data.
With this integration, we merely have to add the host of the Tilt Pi and it will automatically create temperature and gravity entities of the Tilt Hydrometers that it finds in its range.
I have been tested this personally, to fit my use case, which replaces a lot of custom RESTful sensors that are not sustainable to configure manually in my opinion.
Here are some screenshots of the working integration:
One thing to note in the review is that I have created an
api.pymodule, which does not fully adhere to this requirement on the development checklist:I would argue that this is fine for the following reasons:
apiimplementation is there to promote re-use of existing constructs and centralize maintenance and development of API interactionsapimodule described here.Thank you for being receptive to my first contribution.
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: