serkri / SmartEVSE-3

Smart Electric Vehicle Charging Station (EVSE)
MIT License
68 stars 28 forks source link

MQTT: Add mainsmeter and EV meter entities #177

Closed hmmbob closed 1 year ago

hmmbob commented 1 year ago

Subject says it all.

arpiecodes commented 1 year ago

I think most of the EV meter values are already in the current code. Exception being the totals and the total kwh counters.

hmmbob commented 1 year ago

Let me know which ones, because I mapped them to all entities exposed by the HA integration and thought these were still missing.

arpiecodes commented 1 year ago

https://github.com/serkri/SmartEVSE-3/blob/2aa41ba0c6da6102b4720d30ce1d5593412f27c1/SmartEVSE-3/src/evse.cpp#L2483 https://github.com/serkri/SmartEVSE-3/blob/2aa41ba0c6da6102b4720d30ce1d5593412f27c1/SmartEVSE-3/src/evse.cpp#L2484

These should actually be put under the if(EVMeter) conditional.

Also, I think the renaming of the existing EVMeter values is unnecessary. Rest looks nice though. I'd send in values in Wh and W though, not kW and kWh.

hmmbob commented 1 year ago

I'm ok with either direction, I was just trying to create "feature parity" with your HACS component. Those totals are in there too....

The doubling of topics was an honest mistake, and the renaming of topics was intended to being consistency in the sensors - not a 'lighthearted change'

I'm not sure if your comments are intended the way I perceive them (rather negative), but I'm just a guy trying to help. Help on building out a feature that I thought is great and could be a great alternative to the hacs component.

Anyways, as with previous PRs happy to make adjustments but they won't be until after the weekend as I am away from my system.

dingo35 commented 1 year ago

@hmmbob I surely appreciate your contributions, dont want to discourage you, but wanting to encourage to uphold the quality of your earlier contributions! Especially on API/interface issues I like to walk a straight line, because as our firmware grows it is so easy to get entangled in growing, non-orthogonal definitions. As you state correctly, we already grew into that with the REST API, it keeps on growing on a weekly basis and cutting back to the essence would mean massive incompatibility with earlier versions.

So please keep up the good work, none of us is as smart as all of us!

dingo35 commented 1 year ago

As to kW, kWh and W, Wh I do not like showing numbers in higher accuracies than they are actually measured. Most of our kWh meters used are Class 1, which mean they are measuring with 1% accuracy. Showing values in W and Wh suggest accuracies of 1%% or more... So I suggest staying with the rounding/accuracies of the REST API.

hmmbob commented 1 year ago

I think I did that, right? Got the code directly from the JSON generator down in the code.

If that's not the correct code, please drop a line on how it should be, happy to change. Did not change anything there functionally.

dingo35 commented 1 year ago

Yup my comment was meant as a comment to @arpiecodes who stated he likes W and Wh more than kW and kWh.

So,we're on the same side on this one 😁

arpiecodes commented 1 year ago

I don't like publishing totals (e.g. Mains Current Total), because it seems to me that a broker should publish basic information and derived values can/should be computed and the subscribing client. The cost in our use case would be that in HA people should have to add a few lines to configuration.yaml to compute totals, which is less user friendly then having MQTT publish and HA discover automatically. Now I'm not an MQTT specialist, @arpiecodes what's your opinion on this?

I suppose it's OK to present calculated values for the sake of convenience. But it should be useful in such case. I suppose in the sense of load balancing etc, it's not really useful as that's done across phases. Then again, someone someday will probably have a use-case for it, I just can't think of one now.

As to kW, kWh and W, Wh I do not like showing numbers in higher accuracies than they are actually measured. Most of our kWh meters used are Class 1, which mean they are measuring with 1% accuracy. Showing values in W and Wh suggest accuracies of 1%% or more... So I suggest staying with the rounding/accuracies of the REST API.

Interesting take! For me, sending in values in certain precision would never indicate to me that device is capable of measurements of a certain precision. I suppose I have a very different priority here, namely to avoid floating point calculations to values at any cost inside computers/microcontrollers, as they simply suck at that. I also don' think these values are meant to be interpreted by humans directly, but always through a client like HA which is smart enough to do conversions on its own. Lastly, I like to also be able to retrieve from the device what you set; e.g. the MQTT set topics are all in Wh/W/smallest value (and the internals in EVSE also store the Modbus values in W/Wh etc) - so why convert something up/down into less precision and return back a completely different value?

hmmbob commented 1 year ago

(and the internals in EVSE also store the Modbus values in W/Wh etc) - so why convert something up/down into less precision and return back a completely different value?

I like this take. If the value comes into the evse in W/Wh, we probably should stick with that.

So, you're saying the modbus values are W/Wh, or are they calculated?

dingo35 commented 1 year ago

"Interesting take!"

It's not my take, in physics it is the basic scientific approach on accuracy, precision and significance of numbers.... https://phys.libretexts.org/Bookshelves/College_Physics/Book%3A_College_Physics_1e_(OpenStax)/01%3A_The_Nature_of_Science_and_Physics/1.03%3A_Accuracy_Precision_and_Significant_Figures

arpiecodes commented 1 year ago

"Interesting take!"

It's not my take, in physics it is the basic scientific approach on accuracy, precision and significance of numbers.... https://phys.libretexts.org/Bookshelves/College_Physics/Book%3A_College_Physics_1e_(OpenStax)/01%3A_The_Nature_of_Science_and_Physics/1.03%3A_Accuracy_Precision_and_Significant_Figures

This probably has a lot of meaning in the scientific world or if you come from a scientific background, but to me is completely irrelevant. I just like consistency and clear logic - and I don't think it's up to the EVSE to take sides here on what's the correct value (and actually changing it in the process - due to floating point imprecision).

There are meters that are more accurate than class 1. It is an assumption most meters are not. And we shouldn't assume is my opinion.

Someone chose using Wh/W units inside the code of EVSE, probably also because of the floating point 'issue'. Most meters deliver values in Wh/W or even more precise than that. Why not simply adhere to that as a base to work from for programmatic input/output and make it more difficult than it has to be in this case?

dingo35 commented 1 year ago

"This probably has a lot of meaning in the scientific world or if you come from a scientific background, but to me is completely irrelevant."

Yeah its not that measuring current, power and energy is physics 🤣

arpiecodes commented 1 year ago

"This probably has a lot of meaning in the scientific world or if you come from a scientific background, but to me is completely irrelevant."

Yeah its not that measuring current, power and energy is physics 🤣

It may be physics, but what we're working on right now is engineering/software. ;)

dingo35 commented 1 year ago

You're absolutely right, let me sleep over this!

dingo35 commented 1 year ago

Ok guys you got me convinced, lets keep the power / energy stuff in W and Wh for the following reasons: -MQTT doesn´t really handle floats, it handles it like strings -we actually lose accuracy when converting to floating point -my main problem is that from time to time I get large "spikes" in energy usage; I think it has to do with the "total_increasing" state class in HA, and when (by accuracy errors) a reading slightly LOWER then the previous reading occurs, the total yearly usage is added to the statistics; but I cannot attribute this exactly to this point, also I think since MQTT is broader then HA I shouldnt try to solve HA problems in the MQTT interface.

When overthinking this it occurred to me that we might need to make some "publish" statements (more) conditional, in the sense that we only publish topics when they deviate from the default value. E.g. the energy counters are set to 0 by default and only when a reading comes in they are updated, IMHO the 0 value should not be published to the MQTT server. It might be the cause of my statistics problem...

arpiecodes commented 1 year ago

When overthinking this it occurred to me that we might need to make some "publish" statements (more) conditional, in the sense that we only publish topics when they deviate from the default value. E.g. the energy counters are set to 0 by default and only when a reading comes in they are updated, IMHO the 0 value should not be published to the MQTT server. It might be the cause of my statistics problem...

Agreed with this. I think we should have a centralised 'setter' function that is used for all supported measurement inside the code and pair it up with a 'getter' (at least for measurement consumption in the API and MQTT code). This way we also don't have to do the conversions in-situ whenever we access the vars, and we can make the MQTT publishing even faster/more optimised (only send values on-change, etc). The code already contains such getter/setter methods for other stuff, like menu settings and Modbus readings.

arpiecodes commented 1 year ago

@hmmbob I think my recently opened pull request may conflict with yours. Sorry about that.

hmmbob commented 1 year ago

Vacation time, let me redo this work later (and solve the conflicts)