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

Changes in manual override #656

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

KipK
Copy link
Collaborator

@KipK KipK commented May 23, 2023

edit:
some changes in manual override to match ui & button
remove "manual_override" status event

@jeremypoulter
Copy link
Collaborator

Hum... Does this make sense? The primary use case is pressing the physical button on the EVSE.

The existing behaviour is to just override to the opposite of what the current state is. Where as this will force it to the default state. But what if the EVSE is in that state? The button will do nothing right? Not sure that is desirable. If you wanted to bring some synergy with the UI you could toggle active/disabled/auto maybe?

@KipK
Copy link
Collaborator Author

KipK commented May 23, 2023

@jeremypoulter , I've only reverted the claim/release toggle behavior depending of the default state configured.
Should not change anything or have I missed something?

@jeremypoulter
Copy link
Collaborator

@KipK yeah the toggle looked at what the current state of the EVSE is and then switched to the opposite, as in if it was currently charging it would disable, if it is currently disabled, it would change to active. The default state is irrelevant to that.

@KipK
Copy link
Collaborator Author

KipK commented May 23, 2023

@KipK yeah the toggle looked at what the current state of the EVSE is and then switched to the opposite, as in if it was currently charging it would disable, if it is currently disabled, it would change to active. The default state is irrelevant to that.

@jeremypoulter, not sure it was, look, the isActive() was just checking if there's a claim from EvseClient_OpenEVSE_Manual, considering before override was only used for the state property so it fits.
return _evse->clientHasClaim(EvseClient_OpenEVSE_Manual)

I've replaced by

return _evse->getClaimProperties(EvseClient_OpenEVSE_Manual).getState() != EvseState::None;

Should do the same, without saying override is active when there's only a charge_current property but no state.

and then in ManualOverride::toggle(), it just now reverse the claim(), clear() needed depending of default_state as it can be set to disabled so this needs to be reversed in this case.

It also prevent the LCD to display manual override when it is supposed to not be the case
Perhaps I've missed something

@jeremypoulter
Copy link
Collaborator

Correct, isActive checks to see if the override is active or not, if active it clears it if not it makes a claim. It is the latter that then checks is the EVSE is active or not and does the opposite:

bool ManualOverride::claim()
{
  EvseProperties props(EvseState::Active == _evse->getState() ?  EvseState::Disabled : EvseState::Active);
  return claim(props);
}

@KipK
Copy link
Collaborator Author

KipK commented May 23, 2023

@jeremypoulter ok, so if we want to mimic the UI, does something like this would work ?

manual.toggle() should do:

if current state is disabled:
- override state set to Active
- charge_current override state set to 32A if not already set .This will ensure Divert is not controling charge_current when forced to manual active, currently UI set /divertmode to 1 ( = fast ), this could be removed ( but then the Eco will stil be displayed on UI even when not doing anything, or need some boilerplate code on UI to cheat that )

if current state is active:
- override state set to Disabled
- charge_current override state removed if not already set to another value than max_current_soft

UI would call manual.toggle() for Activate/Disable buttons ( needs an endpoint for that )
if Robot Icon is clicked: clear all override props.

for the button to mimic the same UX:

if short press: manual.toggle()

if long press: clear all overrides = set to Auto

@jeremypoulter
Copy link
Collaborator

I think that is correct, however I wouldn't worry about the charge current, should just set to max.

Also there is some more extensive work to do around the button so leave any long press handling till later

@KipK
Copy link
Collaborator Author

KipK commented May 23, 2023

I think that is correct, however I wouldn't worry about the charge current, should just set to max.

This is needed to keep the charge_current that was set previously ( i.e. from the slider in the main interface or from hass over mqtt/api )

@KipK KipK force-pushed the default_state branch 2 times, most recently from c9afb95 to e73f8a6 Compare May 24, 2023 20:22
@KipK KipK force-pushed the default_state branch from e73f8a6 to 5ac94fd Compare May 24, 2023 20:29
@KipK KipK requested a review from jeremypoulter May 24, 2023 20:29
@KipK KipK force-pushed the default_state branch 3 times, most recently from e282d50 to 46260b0 Compare May 24, 2023 22:55
@KipK KipK force-pushed the default_state branch 2 times, most recently from b8bc974 to e840e34 Compare May 25, 2023 10:27
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.

2 participants