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 manual modes , also Reorganising MQTT topics #373

Closed
KipK opened this issue Jun 6, 2022 · 8 comments · Fixed by #379
Closed

MQTT manual modes , also Reorganising MQTT topics #373

KipK opened this issue Jun 6, 2022 · 8 comments · Fixed by #379

Comments

@KipK
Copy link
Collaborator

KipK commented Jun 6, 2022

Let's discuss about #369 (comment) here, as it involve bigger changes.

Here is my proposal

So when MQTT set something that is not a configuration data, it should be a claim, that could be considered on mqtt side as such "manual_override" , let's call it "manual" then, to not be confusing with manual_override from the api ( using the highest priority manual level ).

/manual[0,1] (0: auto/no manual 1: either manual charge_current or status are active)
/manual/set 0 (0: delete all related claims, can't set 1, there's no purpose I think?)
/manual/charge_current (0: auto/no claim, other value in Amp )
/manual/charge_current/set ( set to 0 delete the charge_current claim, other value in Amp )
/manual/status ( 0: auto/no claim, 1: enabled/start, 2: disabled/sleep )
/manual/status/set ( set to 0 delete the enabled/disabled claim, 1: enabled/start, 2: disabled/sleep )

Considering this, we could also reorganize a bit the mqtt topics and remove duplicated values.
Something like that ?

/announce
/announce/[id]

/state [0,1,...]
/state/colour [0,1,..]
/state/flags [int]
/state/vehicule [0,1]
/state/manual_override [0,1]

/smartmode [0,1] are smart features enabled or not ( could be solar divert / Grid +i-E / other incoming modes]
/smartmode/set [0,1] enable/disable smart features. Replace divertmode/set( could be solar divert / Grid +i-E / other incoming modes]
/smartmode/divert_update
/smartmode_grid_ie
/smartmode/available_current
/smartmode/smoothed_available_current
/smartmode/charge_rate ( should be deleted, just rounded available_current, confusing with pilot )

/charge/pilot
/charge/current
/charge/session_kwh
/charge/total_kwh

/system/temp
/system/tempwifi
/system/voltage
/system/max_current
/system/max_current/set
/system/srssi
/system/freeram

@jeremypoulter
Copy link
Collaborator

Thanks for your comments, I was also pondering this while reviewing you PR. As far as renaming the topics go, I am a little reluctant to do this for backwards compatibility reasons. I am not totally ruling it out, but I do want to be consistent across MQTT, WebSockets and HTTP API. @glynhudson / @chris1howell do you have any thoughts on this?

Now names aside, the other thing you bring up is the claims, I wonder if rather than having separate topics for each property there should be a single topic .../claim and .../claim/set that you then read/write JSON blocks as with the HTTP API. I think this may be better as this then represents what the MQTT module claim is on the system vs the actual derived values that are the current topics. Does that make sense?

Also I think the manual_override should remain controlling the same manual override as the button/API to allow getting/setting of that higher level concept, same with the divert_mode.

So what I would propose is:

  • .../divert_mode/.../divert_mode/set read/set the eco/divert mode
  • .../manual_override/.../manual_override/set read/set the manual override, possible values active/disabled/toggle/clear (use the manual API rather than the claiming/releasing though)
  • .../claim/.../claim/set read/write the MQTT claim on the EVSE, should be a JSON blob that follows the same scheme as making a claim via the HTTP API or null, false and/or empty to clear the claim

@KipK
Copy link
Collaborator Author

KipK commented Jun 7, 2022

You join me on this one, I was exploring this possibility too and it make sense to match the api.

override ( list overrides in json )
/override/set {json doc | toggle | delete }

/claim ( claim from mqtt client id in json )
/claim/set [json doc] update claim
/claim/set delete (delete claim)

Do we need to expose other clients claims ? I'm not sure it has sense here.

/schedule ( get all schedules )
/schedule/set ( batch update schedule )

/status ( get status json): For this one I'm not sure we should export the full json as it would need a full refresh for just one value update.

So there should we organise a bit those data perhaps between /status /system /config ?

About backward compatibility, considering the big change involved in the V4 and its logic, perhaps it's time to reorg / remove old stuff like RAPI over mqtt/http , & unused variable and start from a clean basis for a V4.1. I think old way and new way in the same codebase brings lot of confusion. Look at all those guides and libraries online pushing to use RAPI commands, peoples won't be aware of new changes if it's still works partially on the new codebase.

@KipK
Copy link
Collaborator Author

KipK commented Jun 7, 2022

Also I would rename the enable/disable divert mode setting to something more generic in preparation to new smart features. ( /smart_mode enable/disable ? )
For now I use divertmode in grid mode without solar panel to handle dynamic charge_current load balancing and it works well, but I plan to make a dedicated service with some added features.

@KipK
Copy link
Collaborator Author

KipK commented Jun 7, 2022

I have started to integrate that. There's something that tickle me in the way mqtt is handled in the current code.

There's no need to loop mqtt publishing every 30 sec.

We can publish the data with "retain=true", and publish them allinone only once at mqtt server connection..
Then we just need to update retained values when it's needed using event only. Considering we have the MQTT Last Man Standing, other clients knows OpenEVSE is disconnected and can ignore those values if it's offline.

That's what I'm starting to do with /override and /claim topics. They're already updated with retain each time there's a /override/set | claim/set , and published first at mqtt connection only.

Other point, It seems we don't have yet an event engine for claims/override firing updates like for evse data ( also manual_override value is not updated live on mqtt if changed from http ). So they are updated on mqtt only each 30sec, if triggered from another client than mqtt.

@KipK
Copy link
Collaborator Author

KipK commented Jun 7, 2022

I've pushed a WIP on this branch based on ispidfv4: KipK@d9979dd

I have a problem with #376

@jeremypoulter
Copy link
Collaborator

Sorry for not getting back sooner.

I think I would prefer to keep the manual API fairly basic, it really should be limited to a users direct intervention rather than any automated process so keeping it fairly basic would encourage use of the claims for all the more advanced use cases.

I think the claim bits are fine, I would perhaps use 'clear' or 'release' over 'delete' as this is more natural. DELETE is only used for the HTTP API as that is the defined verb in HTTP.

On nesting the topics, I think this will have a lot of compatibility implications, eg won't work with EmonCMS via MQTT, and if we keep the same mapping across HTTP/MQTT/WebSockets there will be a lot of code to rework in the GUI and other stuff like the Home Assistant integration. We certainly could look at renaming some topics (as in duplicate them and deprecate the old one), but lets do that as a separate task.

Specifically on renaming divert_mode to smart_mode, I don't think it makes sense to do this, PV diversion is only one 'smart' mode there are others, eg Ohm, and will be more that are not mutually exclusive so bundling turning them on/off under the same control doesn't make sense. If you need to just switch them all on/off for some reason, that is what the manual override is for.

Hopefully this doesn't sound too negative, your contribution is very much appreciated and very good work so far.

@KipK
Copy link
Collaborator Author

KipK commented Jun 11, 2022

no problem with your comments. I've used Delete matching the http api to keep the main logic only, let's use "clear" "relase", whatever ;)

about #373 (comment) , I'm trying to fire events on mqtt for claims change/add/release. I see the HTTP interface receiving those updates in real time, but not on MQTT. But http page is polling for that.
Should I do the same for MQTT, or shouldn't we try to add that to current event engine and make it more generic like other data updates ?

Renaming are not urgent, and for now can just be handled on client side for me.
Works neat:

evse

@KipK
Copy link
Collaborator Author

KipK commented Jun 26, 2022

Closed by #379

@KipK KipK closed this as completed Jun 26, 2022
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 a pull request may close this issue.

2 participants