Add support for additional sensors from BME680 #288
Conversation
|
I think the failed tests are a result of the " Temperature" string added to the names, but I'm not 100% sure. |
| @@ -370,7 +370,7 @@ async def async_setup_optional_sensors(coordinator, entry, async_add_entities): | |||
| desc = MoonrakerSensorDescription( | |||
| key=f"{split_obj[0]}_{split_obj[1]}", | |||
| status_key=obj, | |||
| name=split_obj[1].replace("_", " ").title(), | |||
| name=split_obj[1].replace("_", " ").title() + " Temperature", | |||
There was a problem hiding this comment.
This creates not ideal naming, also the reason why test are failing
There was a problem hiding this comment.
I'm adding the " Temperature" since the sensor can provide more than just temperature. For me, in Klipper I've got the sensor named "Enclosure", so the names of all the Home Assistant sensors become "Enclosure Temperature", "Enclosure Humidity", etc.
Without adding " Temperature" to the temp sensor, I either have the temperature labelled "Enclosure" and the humidity labelled "Enclosure Humidity" or with your example "MCU Temp" and "MCU Temp Humidity".
I suppose I could not add " Humidity" etc. to any of the names and just refer to the units, but that makes searching entities in HASS harder.
There was a problem hiding this comment.
I think if we want to go that path, you would need to remove the _temp from all of them
so it goes, mcu_temperature, mcu_humidity
There was a problem hiding this comment.
For my enclosure, there is no _temp as I did not include it in the name. My MCU doesn't report temp, but I guess _temp is automatically added by moonraker? If so, I can strip that out if present.
So basically instead of
name=split_obj[1].replace("_", " ").title() + " Temperature",
we want
name=split_obj[1].removesuffix("_temp").replace("_", " ").title() + " Temperature",
right?
There was a problem hiding this comment.
actually,
do we want to keep the format, so it introduce no change for existing users.
example
name=split_obj[1].removesuffix("_temp").replace("_", " ").title() + "_temp",
and for the others
name=split_obj[1].removesuffix("_temp").replace("_", " ").title() + "_humidity",
etc
There was a problem hiding this comment.
We could do that. I don't love the shorted "temp" instead of "temperature", but it seems a reasonable tradeoff to get the new functionality without damaging existing installs.
That said, I think the name field is the display name (hence the replace("_", " "). I guess the sensor names are derived from that display name?
marcolivierarsenault
force-pushed
the
bme280_additional_sensors
branch
from
2c8be8d to
a792d32
Compare
marcolivierarsenault
force-pushed
the
bme280_additional_sensors
branch
from
9422ec1 to
e660274
Compare
|
@BenBergman just need to update testing to make sure you have coverage and it should be good |
marcolivierarsenault
force-pushed
the
bme280_additional_sensors
branch
from
e660274 to
5448af7
Compare
|
I was out of town for a bit, but back now. Will try to get coverage updated later this week. |
BenBergman
force-pushed
the
bme280_additional_sensors
branch
from
e458eb0 to
bb78e80
Compare
marcolivierarsenault
merged commit 593461d
into
marcolivierarsenault:main
This PR addresses issue #277. I also fixed a typo for
Print Projected Total Duration. With all the additional sensors added, I opted to append the sensor type to the name of the sensor from Moonraker, including the temperature. While that works better for my BME680 multisensor, I could see why it might be undesirable for other temperature sensors.Something I've left unaddressed is the precision of the new sensors. I'm new to Home Assistant integrations and I don't do much Python. I'm seeing way too many digits right now. I can manually set it in Home Assistant, but it would be better to set it in the integration.