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

Master/Slave WebUI modifications #152

Merged
merged 6 commits into from
Jun 21, 2023
Merged

Conversation

jaroslawp
Copy link

This PR makes webui changes for master/slave(s) setups:

  • displays EVSE Load balancing mode (Master / Slave Node X/ None)
  • blocks control modifications on Slave Node(s)
    note: modifications via API are still possible.

in a master/slave(s) setup charging mode / amperage / etc modifications should be done only on master which propagates changes to slave(s) via modbus: current web interface allows to perform changes also on slaves .. which results in non-working and/or inconsistent setup of the system (communication errors/slave not charging connected EV .. etc)

@dingo35
Copy link

dingo35 commented Jun 5, 2023

Interesting PR, looks good!

But wouldn't it be a better idea to shut off the webserver on the slave devices? I can imagine the /update endpoint makes sense for updating the software, and the telnet debugging might make sense, but all other stuff doesn't? Why would you look at the webserver of the slave device anyways?

@jaroslawp
Copy link
Author

Yes /update is definitely needed but also the / and /settings endpoints are still quite useful: these allow to visually (or in home assistant integration) to check the status of the slave (car connected / charging stopped / waiting for solar .. etc). Master does not report slave status, so no other practical way of accessing this information other than looking at the display. [ On the other hand disabling part of the writeable /settings api on slave would make sense I think, but in order to do so it would be probably better to first have a method allowing to change the master/slave mode remotely ? ]

@dingo35
Copy link

dingo35 commented Jun 5, 2023

I make it a point to have installation dependent parameters only modifiable at the LCD display; like MaxMains, MaxCircuit etc.
So the SmartEVSE that is the Master, and which ones are the Slaves belongs to that IMHO. I can see no reason to swap Master and Slave dynamically?

Agree on keeping /settings, although a lot of them would be irrelevant...

From the webserver EVSE window I only see "Connected" and "State" as useful info. I think "OverrideCurrent" would not be valid for a Slave, perhaps StartTime/StopTime/Repeat although this should be tested on a slave...

Am I missing something?

@dingo35
Copy link

dingo35 commented Jun 5, 2023

For "dynamically" read "remotely" .

@jaroslawp
Copy link
Author

Fair enough, then in that case master/slave mode switch indeed shall be treated the same. No , no real reason to swap master/slaves remotely (except for some testing when I'm not in the garage .. but that is not really relevant)

I think displaying on the slave the current charging mode - even if disabled is still useful , the contactor 2 status too ? 'current details' and 'phase details' are showing same as on master. This is how it look with this MR now:
Slave (the Current dropdown is also deactivated but this does not show in firefox):
slave-smart-nocharging
Master:
master-smart-charging

@dingo35
Copy link

dingo35 commented Jun 7, 2023

Ok could you please adapt your PR so that it still exports loadbl to the REST API, and if loadbl >=2 then the webserver will only show
a) the EVSE box, with data:
Mode
Load Balance
Connected
State
Temp
StartTime
StopTime
Repeat
(Contactor2 setting is not allowed when loadbalance is enabled).

b) the Control box , with Actions reboot/update/raw data.

I think that is what we agree on, right?

@djoenez
Copy link

djoenez commented Jun 7, 2023

This looks promising! I do have a question:
Does it have the function to switch off one or the other? For instance:

We have 2 electric cars which are both completely drained and both hooked up to their EVSEs.
The car on EVSE2 needs charging for the next day, so I would like to set SMART on and disable EVSE1 (so that EVSE1 can charge the next day on sun power).

Is above function possible? Or is it with this PR that both EVSES will be in the same mode, so that both cars are charging (which results in lower charge rate, since i'm fuse limited).

@dingo35
Copy link

dingo35 commented Jun 7, 2023

All EVSE's are always in the same mode (Normal, Smart, Solar), but I think it is possible to put a EVSE "OFF" (=access denied) while the other is charging.

You would have to test this yourself!

@djoenez
Copy link

djoenez commented Jun 7, 2023

Will do! Thanks for your reply, i'll see what i can make in HA to do some "smart" selecting.

@dingo35
Copy link

dingo35 commented Jun 7, 2023

@jaroslawp This would mean we need an on/off button in the webinterface of the slave...

@jaroslawp
Copy link
Author

Let me check this - it should work (I can switch master to normal/off while slave stays what master told it to be - solar/smart .. but not sure if this really works as expected for charging .. need to test that - and other way around too)

@jaroslawp
Copy link
Author

Ok could you please adapt your PR so that it still exports loadbl to the REST API, and if loadbl >=2 then the webserver will
[...]

Sure, will have a look at this, additional question: conntactor 2 should be visible on master or there too it cannot be used in master/slave config ?

@dingo35
Copy link

dingo35 commented Jun 7, 2023

No, Contactor2 cannot be used when loadbl != 0 (so master/slave config)

@jaroslawp
Copy link
Author

OK, so here is an updated version (please dont mind commented out lines in index.html .. to be removed before finalizing)
master:
Screenshot from 2023-06-21 07-20-46
slave:
Screenshot from 2023-06-21 07-20-53

Just removing some trailing spaces/tabs, this sometimes gets in the way of people using IDE's.
Copy link

@dingo35 dingo35 left a comment

Choose a reason for hiding this comment

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

Thanks for your work, removed some trailing spaces but all fine!

@dingo35 dingo35 merged commit ce6a311 into serkri:master Jun 21, 2023
@rvdgaag rvdgaag mentioned this pull request Oct 25, 2023
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.

3 participants