Closed arpiecodes closed 1 year ago
Cool way to go! I'll try to review later (also not an C developer by any means ;) ). Switchable Home Assistant auto discovery would be cool :)
Electric car is charging just fine with these additions to the code. No problems whatsoever and has been stable for over three days now while sending new MainsMeter and EVMeter updates from HA via MQTT to the SmartEVSE. So far so good.
Updated pull request description with latest changes.
Note that I did not take into consideration the battery current updating with the 'No Comm' error trigger once values were not updated for some time. I do not fully understand the battery logic nor the need for it if you already set the mains meter values through API, so I have not touched it.
Also added some configuration examples of how to integrate into Home Assistant. It's not complete, but with a little effort everything is possible. Setting values from automations should be very easy by using the mqtt.publish
service.
For my current use case the committed config is enough.
i like your updates.. but maybe for integration purposes it would be easier to have different functionality in different PR's? so one for mqtt support, one for charging failsafe, one for evmeter API, etc? That would probably make the small ones push through faster.
@daninden Yeah, that's probably best moving forward. First would like to hear from the maintainers if they're even interested in merging (one or more of) these additions before I spend time on that, though.
It's of course great if new functionality is added! The only problem is the project as a whole lacks any testing suites so it is a bit tricky of course as it could break someone else his installation. Smaller things we can easily merge in the master branch, bigger changes it depends of course how high the risk is and how easy we can find the time to properly review/test at our site if everything is still working.
If you have a lot of stuff in your mind of adding, we could go for a separate release branch in which multiple PR's can be merged and everything can be tested in a more controlled way before we merge it with the master.
Totally agree with that regard; we have to keep it maintainable/testable. This pull has totally blown out of proportions as I simply keep on adding changes/refactoring the code in the branch this pull is connected to for my own use-cases and testing/improvements.
I aim for stability with my mqtt branch right now. Let's re-visit the changes I've made with separate pull requests as soon as I think everything looks stable.
In the meantime if anyone wants to try out the code, be my guest but do know it can also contain some unstable elements.
Hi @arpiecodes how the stability testing going? Do you still using yours mqtt version? Are there some botlenecks? I woul love to use and help with this mqtt feature, plese contact me (contacts are in my profile) if you have time to discuss.
Thank you.
Hi @arpiecodes how the stability testing going? Do you still using yours mqtt version? Are there some botlenecks? I woul love to use and help with this mqtt feature, plese contact me (contacts are in my profile) if you have time to discuss.
Thank you.
Actually, I've been using it since october as my 'daily driver' and have had many stable charging sessions with it already. There's the occasional communications error, but it's very rare. It may also have something to do with my ported Sensorbox ESPHome firmware rather than the SmartEVSE itself.
As far as I'm concerned, the branch 'mqtt' on my fork works stable enough to use. The only problem I seem to have with the SmartEVSE in general is that the leds do not work properly. E.g. it always has the green coloured led on, even when access is denied/car is charging, etc. Not sure if you can help with that though. It also seems to happen with the original firmware on it.
I am actually contemplating forking the original SmartEVSE from @mstegen and re-writing the MQTT support into there, as I do not have a need for the REST API endpoints as long as MQTT works and I believe less is more with regards to code complexity on the controller itself. So I'm not entirely sure if the general heading of Serkri matches with that. I suppose merging it into Serkri also would mean a lot of updating/patching/checking any changes made. there, some that would possibly break things (I myself do not use) and I'm not sure if I have time for such undertaking myself. But maybe with some extra help and alignment, we could get it merged in Serkri and maybe also fix some other stuff in the meantime so we end up with one fork to rule them all.
What would you think of that plan?
Oh thats amazing news :-)
Basically, I own only smartevse, without the box, since my home has already smartmeter for FVE installed ( data are in HA, therfore I love idea API and more MQTT).
I would maybe suggest diferrent path. Create simple interface between evse code and "external custom network meter" -> API or MQTT. So the will be only one "code base" with same functionality web, rfid cards, buttons/leds, etc. Only diferrentiate will API/MQTT part. I think it will be beneficial to all, bugs/features in code will be fixed on one place there will be concetrated support an so on. The API and MQTT will have own files/classes to nicely diferentiate and maintain their needs. Maybe for quicker communication contact me :-)
I would love to help, Im good with programming, but I dont have many experience in C++.
@k-janssens or @dingo35 what do you think about one "interface" for "network meter" API or MQTT?
I would prefer to integrate the MQTT code into main base, as long as we don't run out of resources (cpu or memory), and even then we could make it configurable (REST or MQTT).
This would need to cleanup the @apriecodes codebase, there is some stuff in there that is not mqtt related (e.g. default access mode), when you rip that out only evse.cpp, evse.h and index.html are affected. Not sure what the "mqtt-edit-mode" code in index.html is supposed to do?
O and before I forget, I would require exporting / importing ALL variables that are also supported by the REST API, so that users don't get confused on what is / is not interfaced.
I would prefer to integrate the MQTT code into main base, as long as we don't run out of resources (cpu or memory), and even then we could make it configurable (REST or MQTT).
This would need to cleanup the @apriecodes codebase, there is some stuff in there that is not mqtt related (e.g. default access mode), when you rip that out only evse.cpp, evse.h and index.html are affected. Not sure what the "mqtt-edit-mode" code in index.html is supposed to do?
Thats totaly fine it can and it should be a part of codebase, but maybe some abstraction like common variables/function should be generalized and they will dont care, if data comes from API or MQTT :-)
I like the code being pretty compact, any reason not to use arduino mqtt library like https://github.com/arduino-libraries/ArduinoMqttClient or would that cause too much overhead?
I would prefer to integrate the MQTT code into main base, as long as we don't run out of resources (cpu or memory), and even then we could make it configurable (REST or MQTT). This would need to cleanup the @apriecodes codebase, there is some stuff in there that is not mqtt related (e.g. default access mode), when you rip that out only evse.cpp, evse.h and index.html are affected. Not sure what the "mqtt-edit-mode" code in index.html is supposed to do?
Thats totaly fine it can and it should be a part of codebase, but maybe some abstraction like common variables/function should be generalized and they will dont care, if data comes from API or MQTT :-)
Definitely. Last time I checked there were a lot of single-purpose functions in the codebase that could use some good old DRY applied to them.
I do agree that any solution exposed through MQTT would have to be on feature parity with the REST variant. Having said that, my OCD totally disagrees with some of the variable names used currently, but I can ignore that. :-)
If there's also some willingness to accept some further additions that would enable some extra use-cases (like the deny access on startup, which is absolutely mandatory for my case) as well as some refactoring of existing code, I suppose I can definitely partake in that. Of course, I'd open separate pull requests for those so we can discuss those individually.
This pull would have to be severely refactored to only include the MQTT stuff. But as @stevoh6 said it'd be nice if we can also refactor/DRY the functional code shared with the REST API as well.
No problem with accepting your default access code, as long as the default setting for simple users stays compatible with the old behaviour. Perhaps its time to introduce an "advanced settings" menu ....
I like the code being pretty compact, any reason not to use arduino mqtt library like https://github.com/arduino-libraries/ArduinoMqttClient or would that cause too much overhead?
I considered using that lib, but I continued searching for something more compact. Eventually came across https://github.com/fredilarsen/ReconnectingMqttClient which seemed to do everything that's required and with three times less code doing so. I also liked the multi-platform compatibility, just keeping it simple. If we ever switch from Arduino to IDF, it would still remain compatible.
But I suppose the actual library used can be anything and is easily changed out.
Not sure what the "mqtt-edit-mode" code in index.html is supposed to do?
Basically it was my solution to facilitate doing updates to the MQTT settings without interfering with other forms/buttons. Also because the standard AJAX updating would continually update values even when they were edited in the form itself. So had to work around that. :-)
Ok looks good. Talking about DRY: I have been trying to get rid of this line 3009 in evse.cpp DynamicJsonDocument doc(1200); // https://arduinojson.org/v6/assistant/
It takes up 1200 bytes just to call serializeJson, and every time we add a variable to the API we have to check if enough memory is allocated.
The shortcut would be something like this:
printf(R"DELIM({ "version":"v3serkri-%i", "mode":"NORMAL", "mode_id":1, "car_connected":true, "wifi":{"status":"WL_CONNECTED","ssid":"WiFi2020","rssi":-70,"bssid":"78:8A:20:5A:41:76","auto_connect":false,"auto_reconnect":true},"evse":{"temp":21,"temp_max":65,"connected":true,"access":true,"mode":0,"solar_stop_timer":0,"state":"Charging Stopped","state_id":9,"error":"Communication Error","error_id":2,"rfid":"Not Installed"},"settings":{"charge_current":0,"override_current":0,"current_min":6,"current_max":32,"current_main":32,"solar_max_import":0,"solar_start_current":4,"solar_stop_time":10,"enable_C2":"Always Off","mains_meter":"Sensorbox"},"home_battery":{"current":0,"last_update":0},"ev_meter":{"description":"Disabled","address":12,"import_active_energy":0,"total_kwh":0,"charged_kwh":0},"mains_meter":{"import_active_energy":0,"export_active_energy":0},"phase_currents":{"TOTAL":0,"L1":0,"L2":0,"L3":0,"last_data_update":0,"charging_L1":false,"charging_L2":false,"charging_L3":false,"original_data":{"TOTAL":0}}})DELIM",i);
where all the values should be replaced by %i and %s 's.
But the code would become totally unreadable. If we would have some struct that replaces all those variables, and would be easy to serialize, and would be comfortable for the MQTT code... any thoughts?
Ok looks good. Talking about DRY: I have been trying to get rid of this line 3009 in evse.cpp DynamicJsonDocument doc(1200); // https://arduinojson.org/v6/assistant/
It takes up 1200 bytes just to call serializeJson, and every time we add a variable to the API we have to check if enough memory is allocated. [...] But the code would become totally unreadable. If we would have some struct that replaces all those variables, and would be easy to serialize, and would be comfortable for the MQTT code... any thoughts?
Maybe replacing the Arduino JSON library with something like cJSON would improve the heap usage? Like you already stated; replacing it with a big printf would become unreadable/unmaintainable and it would also use quite the heap memory.
Hi, maybe we dont need to use json in mqtt. There are different formats, or just pass every value to its own topic as raw value - which is common in many mqtt integrations. (HA addon can easily handle adding autodiscovery or parsing data on his own)
Or maybe this library can help https://arduinojson.org/
Hi, maybe we dont need to use json in mqtt. There are different formats, or just pass every value to its own topic as raw value - which is common in many mqtt integrations. (HA addon can easily handle adding autodiscovery or parsing data on his own)
Or maybe this library can help https://arduinojson.org/
For the MQTT messages; this is true to a certain extend and simply pushing out values to topics is generally the way to go there. But if we'd like to implement HA AutoDiscovery, JSON is kinda required. Also, I suppose it would not do any harm if we could refactor the JSON encoder for the REST API part so we can 'free up' some heap size. The added MQTT configuration adds quite some vars to it, so I suppose @dingo35's point is we have to be careful with the current implementation.
I suppose we could also make two endpoints; one with the status and one with settings, instead of one big JSON document. But that'd kinda break backwards compatibility. Maybe we can do both but keep the old format in as well for some releases so people can migrate. Then it will only use the heap size when the legacy endpoint is requested.
Expanding on this, we can also make the inclusion of said 'legacy endpoint' an optional build constant so the user can choose whether to include the legacy endpoint or not. Going the new endpoints way would open up the possibility to adjust/reorder/improve the API/variables as well without breaking current implementations, following REST principles a bit better.
HA autodiscovery should be and can be done in HA integration plugin (https://github.com/dingo35/ha-SmartEVSEv3). We shoulnd tight this project to HA (there is lot of other great projects like Domovitz, Mqtt dashboard, ....)
So json isnt requirement for mqtt from my point of view.
Not sure if MQTT would need a separate autodiscovery, but the SmartEVSE device is already autodiscovered by the ha-smartevse integration, by using zeroconf.
So are you talking autodiscovering the SmartEVSE device/entities by the MQTT server, or autodiscovering the SmartEVSE MQTT entities by HA? Wouldn't that be handled automatically by the MQTT-ha integration?
Not sure if MQTT would need a separate autodiscovery, but the SmartEVSE device is already autodiscovered by the ha-smartevse integration, by using zeroconf.
So are you talking autodiscovering the SmartEVSE device/entities by the MQTT server, or autodiscovering the SmartEVSE MQTT entities by HA? Wouldn't that be handled automatically by the MQTT-ha integration?
If you want to HA to autodiscover MQTT entities, you need to push JSON "config" message to specific topic, so HA mqtt audodiscoery can find and setup entities. And this can done by ha integration after some development. I created bunch of them. And I still think, that this should by done by https://github.com/dingo35/ha-SmartEVSEv3 integration :) It will free some space in evse controller.
It was just an example and the only reason I could think of needing JSON for MQTT, as with MQTT usually the values are directly published 1:1 by topic. Sorry for the confusion.
HA MQTT auto-discovery is a feature of MQTT inside HA (https://www.home-assistant.io/integrations/mqtt/#mqtt-discovery) that can automatically set up devices/entities through MQTT without needing any integration. But yeah, you can also go the YAML sharing route. I don't think the logic/use-case of interfacing with/through HA is complex enough to warrant a custom HA integration for the smartevse as most can simply be done with some config effort.
EDIT: but I can certainly imagine that when you are going the REST API route a custom integration is preferred, as I see has already been done with ha-SmartEVSEv3. Custom REST APIs are a little bit more complicated to integrate well within the HA ecosystem without going through YAML hell.
It was just an example and the only reason I could think of needing JSON for MQTT, as with MQTT usually the values are directly published 1:1 by topic. Sorry for the confusion.
HA MQTT auto-discovery is a feature of MQTT inside HA (https://www.home-assistant.io/integrations/mqtt/#mqtt-discovery) that can automatically set up devices/entities through MQTT without needing any integration. But yeah, you can also go the YAML sharing route. I don't think the logic/use-case of interfacing with/through HA is complex enough to warrant a custom HA integration for the smartevse as most can simply be done with some config effort.
Np at all :) If there is enough compute power in evse its fine ;)
Well let's try to get all on the same page here: There IS already a HomeAssistant - SmartEVSE integration (written by me) that does autodiscovery. That integration is for users that don't want the hassle of running an additional MQTT server. However, if any additional MQTT logic would be needed we could add it to this integration; preferably the MQTT stuff should run without an integration, and without yaml-configuration; the autodiscovery facilities of HA sounds to me as the best way to go. If that means JSON stuff is needed in the firmware, so be it; it is already there, I just wanted to touch on the subject to see if we can streamline that piece of code to both MQTT and the REST API.
I think we should avoid building a second HA-SmartEVSE-MQTT integration, and that we should also avoid manual-yaml-configuration: the syntax of that stuff keeps changing as ninja2 keeps evolving, and thus is not very robust....
Ok this is the second time that the MQTT discussion comes to an agreement, but nobody adapts the PR. Is there any initiative going on to insert MQTT into an atomic PR or shall I close this subject?
Up to you :) I dont have enought time to fully adopt this right now, maybe little bit later, but definitely I would love to do it :)
Hello there!
An electric car is almost delivered to my doorstep, so I thought I'd share some of my hacked additions* to your code.
Note that I am by no means a C++ developer, so please forgive my code standards. It 'works on my machine' and that is about all the guarantees you will get, but I guess it is good enough to share so others could possibly improve on it and I can learn. :-)
Here is a screenshot of the values published into MQTT and the available things to set from MQTT;
As you can see, you can also publish/set some of the values. The values accepted are the same values as published on the MQTT topics (Yes/No, Allow/Deny, Smart/Normal/Solar, etc). The MainsMeter and EVMeter Set topics are a bit special. For MainsMeter all phase currents (don't forget to multiply these by 10) should be sent in one message with a
:
as delimiter, likephase1:phase2:phase3
. For EVMeter all phase currents, total live power consumed (in watts) and total energy consumed (in wh) should be sent in one message with a:
as delimiter, likephase1:phase2:phase3:power:wh
.All of the MQTT topics are published on a ~5 second interval.
I also added a check for value timeout when using MainsMeter API (very much like the modbus logic does). It should trigger a NO COMM error if the value has not been updated for 10 seconds straight. This should prevent overload situations to occur when API suddenly stops receiving values.
I also enabled 3-phase contactor to enable in Smart mode as the load balancing logic should deal with this already.
Last but not least I've added a new setting in the menu that allows you to set the default access upon a fresh start of SmartEVSE. In my case, I do not have a physical switch attached to the EVSE but I control everything from Home Assistant. I only want charging to be allowed when Home Assistant thinks this should be the case. Another example; if SmartEVSE reboots, I do not want to allow charging as a default but rather deny it. I know I could have just pretended I had a switch, but I think this solution is more elegant with the new API ways of control.
Let me know your feedback/comments and I will see what I can do to improve further.
New Swagger schema that incorporates the changes and elaborates on the MQTT settings;