serkri / SmartEVSE-3

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

evse.cpp: Further build MQTT capability #173

Closed hmmbob closed 1 year ago

hmmbob commented 1 year ago

Based on the great work by @arpiecodes, adding MQTT in #159, I've:

Works locally, but conflicts with #172

image

hmmbob commented 1 year ago

Gotcha, will work on it.

OFF to Off is a functional change btw - otherwise the select didn't work for some reason. Will try to figure out if I can work around this.

hmmbob commented 1 year ago

@dingo35 ready for review.

Questions:

dingo35 commented 1 year ago

-sorry on the OFF/Off issue, now I get the problem. I like to stick to the spelling of StrMode, which is "Smart", "Normal", so lets stick to "Off" like in your original code. -for version, in the JSON string it is defined like this: doc["version"] = String(VERSION);

hmmbob commented 1 year ago

OK, think I'm done for now 😉

Don't like the line length of 2359 and 2369, but simply doing a += on the next line errored out. Don't have the C experience to know why? I have no modem or PV sensors here, so can't check that part.

image

stevoh6 commented 1 year ago

I agree with this sentence "I don't like moving towards the supported abbreviations of HA; MQTT is a universal protocol, which people might use to interface to other software than HA; "

I was plannig to provide "autodiscovery" feature for HA separatly from this project, as we have now for API. So If you doesnt wsa to edit yaml files in HA, you will simple intall HACS addoms, which will only push autodiscovery topics to mqtt (something like this) It will be much more cleaner and not tight to one platform, since there is lot of others.

hmmbob commented 1 year ago

We could opt to make "home assistant discovery feature" optional/configurable in the SmartEVSE frontend. So, an extra checkbox or something that tells the SmartEVSE to publish the discovery topics to HA.

in that way one could always publish the 'normal' MQTT topics (MQTTPrefix + topicname) for consumption by any mqtt client, and only the HA magical discovery topics when the user configured this. Then you won't need an extra component through HACS to just push the auto discovery items.

stevoh6 commented 1 year ago

Yeah, this will be good, I love the idea. But I see two problems (and therefore Im against it :D)

  1. resources cost of esp32 ( I wul like to keep space for more useffull stuff)
  2. what if other projects woul like to have native support of "autodiscovery" ? It will also cost recources of esp32.
arpiecodes commented 1 year ago

The whole point of the current discovery feature is so we do not require any HACS/custom add-ons anymore for it to function. I think it's also very unnecessary to introduce yet another add-on to do basic stuff. Even though it consumes space in the code, the extra resource usage on the device is minimal (as it basically just gets published once). Having said that, the abbreviations would only help marginally here, and I'd prefer the non-abbreviated variants just for clarity.

I also don't think yet another configuration option would make sense here, seen the actual gain (e.g. extra available resources) isn't really worth it imo.

I would however definitely be all for putting the code into separate source files, so it gets less of a hot mess like it is now. But this would be a pretty big undertaking as there are a lot of such things in the current source code that could use some organisation/DRY'ing.

Also, I'm of the opinion that certain platforms deserve deeper integrations than others, if they are popular enough and a large enough group of users are helped by it. In this case, I see most people use the MQTT feature with HA (but I could be wrong, hard to say) so for these people it really helps.

Of course, people that do not use HA don't have to use it. This is why I tried to keep the topics and logic generic and non-HA specific. If there even is such a thing. :-)

hmmbob commented 1 year ago

Agreeing on wat @arpiecodes says. Zooming in on 1 specific topic:

I also don't think yet another configuration option would make sense here, seen the actual gain (e.g. extra available resources) isn't really worth it imo.

On the other hand, a user would need to config the MQTT server anyways, just ticking a "Enable Home Assistant Discovery" box would be easy and prevents users that are not using Home Assistant from seeing those homeassistant/{{domain}}/{{entity_id}}/config topics pop up in their broker.

arpiecodes commented 1 year ago

On the other hand, a user would need to config the MQTT server anyways, just ticking a "Enable Home Assistant Discovery" box would be easy and prevents users that are not using Home Assistant from seeing those homeassistant/*/config topics pop up in their broker.

I am genuinely wondering if this is actually a thing? Quickly looking into what goes in/out my own MQTT broker it's basically wild west in there. This is why you should only subscribe to topics of interest on the client side.

While I was taking a peek, I tried to find something that may be used to identify installations where HA is active, but couldn't find an open door. Since the messages are sent once and as long as they are not retained, I really don't see the issue.

dingo35 commented 1 year ago

I agree, this is not a real problem. MQTT is a broker service, and a broker should have all the offers it can get, so clients can subscribe to them or not. If someone has a good proposal for generalizing the topic announcements then lets open a new PR for that, and close this one. O it already is :-)