-
Notifications
You must be signed in to change notification settings - Fork 117
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
Mqtt reworked API version #379
Conversation
I haven't forgotten about this PR, just trying to fix a few issues with #207 |
mqtt: publish max_current mqtt charge_current/set + max_current/set
mqtt: removed /manual_override/set /pilot/set mqtt: added /override & /claim + related setter ( /override/set [json] | delete | toggle && /claim/set [json] | delete ) m event: publishing new event override containing either json or 0.
…ne (instead of 0)
mqtt: changed delete method to clear/release
…: /claim/set/charge_current, /override/set/state )
fix typo updated doc
I need some helps on this one. Where should I start ? |
Just calling event_send should be all you need. That will then send the new values. The double polling/web socket is just a legacy thing, going to remove the polling at some point. |
Latest commit remove bulky settables, now merged props is ok. |
mqtt: some cleanup
mqtt: publish max_current mqtt charge_current/set + max_current/set
Possibly this could be an option, may need to be optional though, maybe more relevant for the values that are not evented every 30 seconds, but mostly they where targeted at the HTTP/WS API, IE you get the initial state via the
The 30 second loop is needed as the values are not evented from the EVSE module, guess we could keep a record of the last value and only send when they change. This may need to also be optional as so use cases it is good to get the regular updates, ie to make sure the device is still online.
This could be removed already, there may a few bugs, where values may not be evened, but sure these can be resolved. In reality this will probably just not be done when doing a new UI. I would raise a separate ticket for each of these. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code changes look good, will give it a test then can be merged.
On MQTT we already have the last will message to know if the device is connected or not so that shouldn't be a problem. Also we could add an heartbeat somewhere. |
Can you first merge the current_shaper pull request before this one ? |
Forget it, I've merged it anyway. |
Do you plan to merge those soon ? |
this 0b3c212 works even better than the polling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gave this a test, looks good, just one open query
src/evse_monitor.cpp
Outdated
StaticJsonDocument<128> event; | ||
event["pilot"] = _pilot; | ||
event_send(event); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this what you mean? did you intend this to be an event_send for voltage? That being said the voltage is not read by most OpenEVSEs, normally the voltage is a static value or read from MQTT, so eventing it maybe does not make sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think this is good to go, would at least be good to get it out there for wider comments. Thank you very much for your contribution, it is really appreciated
You're welcome. I'm laying around. There's also a pull request for the gui related to this one. |
Fixes #358
Fixes #376
Fixes #373
Following previous discussion here #369
Based on espidf4 branch.
Edit: all functional