Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose config and config updates through MQTT #634

Merged
merged 2 commits into from
May 22, 2023

Conversation

mathieucarbou
Copy link
Contributor

No description provided.

@mathieucarbou
Copy link
Contributor Author

mathieucarbou commented May 9, 2023

@KipK : I've created this PR to monitor config and config changes with MQTT. This is useful to display dashboard components accordingly on HA, especially to get some config like power ratio, max_current_soft, etc.

Note: this introduces a code duplication that needs to be solved - just please let me know here would be the good candidate for putting the common code between web and mqtt to generate the json payload.

thanks ;-)

@mathieucarbou
Copy link
Contributor Author

@mathieucarbou
Copy link
Contributor Author

Note: when a RAPI command is sent through MQTT or debug mode (ie. $SA 190 0), the config does not update.

Which is OK IMO

src/web_server_config.cpp Outdated Show resolved Hide resolved
@mathieucarbou mathieucarbou force-pushed the config branch 3 times, most recently from 71fa0d9 to 7e393db Compare May 9, 2023 16:58
@mathieucarbou mathieucarbou requested a review from KipK May 9, 2023 16:59
src/mqtt.cpp Outdated Show resolved Hide resolved
src/app_config.h Outdated Show resolved Hide resolved
src/app_config.cpp Outdated Show resolved Hide resolved
src/mqtt.cpp Outdated Show resolved Hide resolved
src/mqtt.cpp Outdated Show resolved Hide resolved
@mathieucarbou
Copy link
Contributor Author

PR rebased and updated to push the config each 30sec with the new time

src/mqtt.cpp Outdated Show resolved Hide resolved
src/mqtt.cpp Outdated Show resolved Hide resolved
src/app_config.cpp Outdated Show resolved Hide resolved
@KipK
Copy link
Collaborator

KipK commented May 10, 2023

PR rebased and updated to push the config each 30sec with the new time

config should only be pushed at mqtt connection with retain flag, and thereafter only when there is an update ( i.e when config_version has changed ) . No need to push this each 30sec, just push it after a config change.

@mathieucarbou
Copy link
Contributor Author

h 30sec, just push it aft

that is what I did initially. But once mqtt connects, I guess the evse manager is not ready yet ? Because all properties returnes from evse like firmware, max_current_soft, offset, etc are all equals to empty strings or 0.

So I did that to get a second push to ensue the config gets sent at each time update

@mathieucarbou mathieucarbou reopened this May 12, 2023
@mathieucarbou
Copy link
Contributor Author

(sorry - mistake - closed and reopened)

@mathieucarbou

This comment was marked as resolved.

Copy link
Collaborator

@jeremypoulter jeremypoulter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few small changes

src/evse_monitor.cpp Outdated Show resolved Hide resolved
src/app_config.cpp Outdated Show resolved Hide resolved
src/app_config.cpp Outdated Show resolved Hide resolved
src/app_config.cpp Show resolved Hide resolved
src/app_config.cpp Show resolved Hide resolved
@LavredisG
Copy link

I've read that RAPI will soon be removed (probably) from the MQTT and I have some questions on it:

  1. Does this affect working with HTTP too, or in this case only http is used as api?
  2. Is there any difference in general while working with either HTTP or MQTT?

@jeremypoulter
Copy link
Collaborator

Yeah the intention has always been to remove RAPI. I have created a ticket for this, will probably be part of the upcoming v5 release: #643

Let's move any discussion about removing RAPI there so we didn't lose any comments

@mathieucarbou
Copy link
Contributor Author

Rebased!

@glynhudson
Copy link
Collaborator

@mathieucarbou could you add the new MQTT API to the docs as part of this PR? Thanks

Your HA integration looks great, have you published this?

@mathieucarbou
Copy link
Contributor Author

mathieucarbou commented May 16, 2023

@mathieucarbou could you add the new MQTT API to the docs as part of this PR? Thanks

done!

Your HA integration looks great, have you published this?

I am maintaining it there: https://gist.github.com/mathieucarbou/92a3d5e0dc38d6b68aa1bdaf153a80c5

Screenshot 2023-05-16 at 14 49 19

I am always improving it.

Some notes (fyi @KipK and @jeremypoulter):

The integration still relies on some stuff not yet merged:

  • MQTT /config
  • MQTT /restart (for evse)
  • MQTT /config/set: in one of my branch, I didn't PR it yet because I know you guys fear that

HA is strongly integrated with MQTT and MQTT is pretty efficient. Going through HTTP API calls, even if this is towards what OpenEVSE is going, is highly inefficient with HA, especially when binding some UI components because, of the lags, http overhead and polling requirements. This is not a push system and HA does not have a great websocket support to bind components.

There are some config settings, like max current soft, min charging time, max power for shaper, divert settings etc that are worth exposing in read-write mode. The reason is that a Home Assistant software is supposed to centralise management and monitoring. And some settings are tightly coupled with improving the energy consumption of the charge (i.e. divert). For example, in summer or winter, I might not use the same settings. Also, since days now, I am tweaking the values to find the best calibration and numbers that work here for solar divert and also to compute a correct voltage because the EVSE does not have a voltmeter.

And to do that, I am also looking at some graph in Home Assistant.
I don't want to have to deal with many UIs if I can have everything I need and tweak tweak the values directly from HA.

/config/set is not about providing the ability to automate things. This is to provide the ability to mutate some key attributes that I think could be exposed from HA like I did in my integration.

image
image
image

src/mqtt.cpp Outdated
Comment on lines 520 to 522
String fulltopic = mqtt_topic + "/config_version";
String payload = String(config_version());
mqttclient.publish(fulltopic, payload, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this, config_version is evented via event_send when the version number is incremented

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need this, config_version is evented via event_send when the version number is incremented

We need it when the first publication comes from the mqtt layer. Version is not upgraded at startup. I initially didn't have it but if we remove it, only the config is pushed not the version, and it takes a config change coming from the UI to get it published.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But with it here it is published twice. It may be better to handle explicitly in the MQTT connection code. I would also assume the same bug exists for all the xxx_version, that being said are these useful for MQTT if you are exposing the resources directly? They are meant so the version number is evented over MQTT or Websocket, and then you would fetch the resource over HTTP. With the config, etc being sent over MQTT directly the version is irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But with it here it is published twice.

Yes I knew that...

It may be better to handle explicitly in the MQTT connection code.

Thought about that and no because the version number and config might not be published at mqtt connection time if evse is not started. So the initial publish will come from the loop.

I would also assume the same bug exists for all the xxx_version, that being said are these useful for MQTT if you are exposing the resources directly?

Not at all... There is no point in having have these *_version exposed in mqtt (IMO). Only exception is if someone wants to know hen a config change happened...

They are meant so the version number is evented over MQTT or Websocket, and then you would fetch the resource over HTTP. With the config, etc being sent over MQTT directly the version is irrelevant.

I agree.

So to keep the behaviour backward compatible but still avoid this duplicate publish, what I can do is check the version number is the mqtt publish function. If it is 1, it means it has to be published because this is the initial publish. If it is greater than 1, then there has been a config change, and the event will be responsible to publish.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just pushed ad95246 as a proposal to remove the duplication

@mathieucarbou
Copy link
Contributor Author

rebased on master just now

src/mqtt.cpp Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants