Sensor update and gas usage

[mhkrebs]

First, thanks so much for doing all this work! I recently got this to report the numbers in ESPHome.

I had a few questions about the C++ implementation:

1. Is there a reason to use PollingComponent to update the sensors periodically (it looks like it defaults to 5s), instead of immediately publishing to the sensors in parse_gas() and parse_water()?
2. It looks like it's not publishing the gas usage. Is there a reason, such as the numbers not being reliable?
3. The latest revision of the code didn't build for me. It looked like minor things like uint8 complaints or something. (I was able to build with an older revision so it's not an issue for me at the moment.)

Thanks again for this. I'm an experienced developer, but very inexperienced with ESPHome and ESP32, so I couldn't have done this without your work!

[htumanyan]

First, thanks so much for doing all this work! I recently got this to report the numbers in ESPHome.

I had a few questions about the C++ implementation:

1. Is there a reason to use PollingComponent to update the sensors periodically (it looks like it defaults to 5s), instead of immediately publishing to the sensors in parse_gas() and parse_water()?
2. It looks like it's not publishing the gas usage. Is there a reason, such as the numbers not being reliable?
3. The latest revision of the code didn't build for me. It looked like minor things like uint8 complaints or something. (I was able to build with an older revision so it's not an issue for me at the moment.)

Thanks again for this. I'm an experienced developer, but very inexperienced with ESPHome and ESP32, so I couldn't have done this without your work!

Hello and thank you for taking the time to review and share quite valuable comments!

Here are some thoughts:

Is there a reason to use PollingComponent to update the sensors periodically (it >looks like it defaults to 5s), instead of immediately publishing to the sensors in >parse_gas() and parse_water()?
parse_gas and parse_water are called every time when a packet is received, which is hundreds of times per second.

As you'd imagine, the parameters don't change that often. Updating sensors every time when we receive a measurement would cause an unnecessary processing overload on components in the chain between the device and home assistant. Hence I just picked 5s polling/update frequency as a good compromise between data velocity and the processing load.

2. It looks like it's not publishing the gas usage. Is there a reason, such as the numbers not being reliable?

Not exactly, I just didn't get a chance to put the code for updates. Just an artefact of this still being work in progress. Good catch though - need to fix.

3. The latest revision of the code didn't build for me. It looked like minor things like uint8 complaints or something. (I was able to build with an older revision so it's not an issue for me at the moment.)

Thank you and I see you've submitted a PR - reviewing...

[mhkrebs]

As you'd imagine, the parameters don't change that often. Updating sensors every time when we receive a measurement would cause an unnecessary processing overload on components in the chain between the device and home assistant.

They change for me a lot... I look at the BTU usage and water flow, in particular. Before I realized I could get them quickly, I waited the 5s between updates when I was watching the ESPHome web page. That is, I was turning on faucets to see when I hit the max for various flow rates (sink vs shower, etc.), and waiting 5s wasn't ideal for me. I really liked the real-time numbers. :)

I think I can answer my own question, but is it really that bad to send the measurement every packet? (I'm new to ESP and haven't delved into the guts of Home Assistant before.) Even if it is hundreds of times a second? Is there a best practices for ESPHome and/or Home Assistant for this?

I think the answer to that is in various posts, where the concern is Home Assistant recording sensor values too often in its database. I found things like https://community.home-assistant.io/t/frequently-update-a-sensor-value-for-usage-in-esphome-update-ha-less-often/653996 as a way to I think get what I want from the web page and yet not update Home Assistant very often. So that would update ESPHome in real-time, but then only update Home Assistant every second (or 5s).

[mhkrebs]

Thanks for considering these pull requests! I created two more to cover my remaining changes. So there are three now: #5, #6, and #7. They're each dependent on the one before them.

If you don't want to take them in, or you want to implement them a different way, feel free. I just figured I'd send back what I wrote in case it's useful to others.

[mhkrebs]

From #7 (comment):

Thanks to keep pushing this. I did not get a chance to reply earlier (sorry for that) as I went down the route of refactoring the code to allow better portability (moving to arduino based boards) as well as easier extendability for cases like the real time you've mentioned. I should admit it took me quite a bit longer than I thought - still not finished but I've pushed the almost complete version to the branch refactor_separate_proto - would you, please, take a look and, if ok with you, let's evolve that version and perhaps adjust your PR for it? I feel this would be a better long term path.

For the gas usage - give me a day or two, I will experiment too and share my discoveries.

I ended up just creating a new branch off of your "refactor_separate_proto", rather than try to rebase my existing ones. (I'm new to git, so I'm still figuring out my best practices, but that seemed like a reasonable thing to do.)

I created pull request #8, which corresponds to my prior PR #5. It's pretty much the same changes. I'm curious, does it build for you with the uint8 (vs uint8_t)? I don't know why that change is needed for me. Maybe the old-style uint8 is defined in your board's headers but not mine?

I can re-implement my real-time PR #6, but I wonder if it's worth it if you already have something in mind. Basically, I would just call publish_state() directly in Navien::on_gas() like what I did in PR #6.

[htumanyan]

BTW, I've added an optional real time update option based on your input and code - take a look at my latest commit (ace01ab714cb0d68060cf6e704ac2237e8630a18 and try it out please. Looks to be working for me, but I'd very much appreciate an independent validation.