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

Mqtt reworked API version #379

Merged
merged 49 commits into from Jul 29, 2022
Merged

Mqtt reworked API version #379

merged 49 commits into from Jul 29, 2022

Conversation

KipK
Copy link
Collaborator

@KipK KipK commented Jun 11, 2022

Fixes #358
Fixes #376
Fixes #373

Following previous discussion here #369
Based on espidf4 branch.

Edit: all functional

@jeremypoulter
Copy link
Collaborator

I haven't forgotten about this PR, just trying to fix a few issues with #207

@KipK
Copy link
Collaborator Author

KipK commented Jun 16, 2022

I need some helps on this one.
I'd like to integrate real time update of modified values on mqtt instead of the 30sec loop pushing the whole dataset.
For ex, "pilot" value should be notified when setting a charge_current override or claim. waiting 30 sec for feedback is inconvenient.
Also publishing the whole dataset each time is spamming the smart home solution database unnecessarily.

Where should I start ?
event_send(data) is already modified to push event to mqtt.
What would be the best way to solve those missing event notifications ?
This also would allow to remove the 5s status fetch on http interface, using only the web socket. ( I was wondering why 2 communication channels here? )
This will will prolly involve also this #376

@jeremypoulter
Copy link
Collaborator

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.

- removed independent property settable topics.
- merge claim properties with previous one when updating a claim/override
- clear a property by sending "clear" as parameter ( /claim/set {"charge_current": "clear" }
@KipK
Copy link
Collaborator Author

KipK commented Jun 20, 2022

Latest commit remove bulky settables, now merged props is ok.

@jeremypoulter
Copy link
Collaborator

jeremypoulter commented Jun 26, 2022

2/ publish everything has "retained", and push only updated data as soon as they have changed. ( need point 3 ). Current implementation spam smart home solution database uselessly.

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 /status endpoint then updates are sent via the WebSocket.

3/ remove the 30sec loop, we need a better event driven status system to publish data when a value has changed. some values like "pilot" are not evented taking too long to update on MQTT side. This will also allows to remove the status polling on the web ui. . I have no idea of the change involved, I'd rather let you guys taking care of this one. I'm still blurry with the main codebase.

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 will also allows to remove the status polling on the web ui.

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.

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.

Code changes look good, will give it a test then can be merged.

@KipK
Copy link
Collaborator Author

KipK commented Jun 26, 2022

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.

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.

@KipK
Copy link
Collaborator Author

KipK commented Jun 29, 2022

Can you first merge the current_shaper pull request before this one ?
I will then add the mqtt retain setting. Can't merge it now, I have too much conflicts from my branch with booth :)

mqtt retain setting fixes
updated web_static
@KipK
Copy link
Collaborator Author

KipK commented Jun 30, 2022

Can you first merge the current_shaper pull request before this one ? I will then add the mqtt retain setting. Can't merge it now, I have too much conflicts from my branch with booth :)

Forget it, I've merged it anyway.
It also needs OpenEVSE/openevse_wifi_gui#80

@KipK
Copy link
Collaborator Author

KipK commented Jul 15, 2022

Do you plan to merge those soon ?
Maintaining 2 branches for those pull requests and a third one with booth, for my usage, is getting somehow confusing. :)

@jeremypoulter
Copy link
Collaborator

@KipK really sorry, yes, definitely intended on reviewing/merging this as soon as #350 is complete but I have to focus on that as it is now blocking sales in the UK

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

KipK commented Jul 28, 2022

this 0b3c212 works even better than the polling.

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.

Gave this a test, looks good, just one open query

Comment on lines 507 to 510
StaticJsonDocument<128> event;
event["pilot"] = _pilot;
event_send(event);

Copy link
Collaborator

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

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.

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

@KipK
Copy link
Collaborator Author

KipK commented Jul 29, 2022

You're welcome. I'm laying around. There's also a pull request for the gui related to this one.

@jeremypoulter jeremypoulter merged commit 29e6a19 into OpenEVSE:master Jul 29, 2022
@KipK KipK deleted the mqtt_ispedfv4 branch July 29, 2022 19:25
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.

claim engine should deep merge properties MQTT manual modes , also Reorganising MQTT topics MQTT 'API'
4 participants