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

Unlock in states A/B instead of B/C #939

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

Conversation

ydankner
Copy link
Contributor

@ydankner ydankner commented Oct 30, 2024

Describe your changes

This change is necessary for us for two reasons:

  • The connector should never unlock in state C, as in this state there is live electricity, and unlocking in this state violates safety standards and regulations
  • The connector should unlock in state A, as we see no reason for why it didn't until now. If there was any reason, we would like to know :)

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

Signed-off-by: Yonatan Dankner <yda@qwello.eu>
@ydankner
Copy link
Contributor Author

@corneliusclaussen as this was your code, I would kindly request your input on if this makes sense to you or breaks anything. Thanks!

@lategoodbye
Copy link

I'm not familiar with this specific code, but I agree the connector shouldn't be unlock in energized state but CP state isn't a reliable state for this. In order to ensure safety you should check the state of the EVSE contactor. The whole point of force unlock is to help the EV owner to get the cable back (even the CP detector is broken or the EV is simplified according to IEC 61851 which doesn't have CP state B).

@@ -505,7 +505,8 @@ void IECStateMachine::connector_force_unlock() {
cp = cp_state;
}

if (cp == RawCPState::B or cp == RawCPState::C) {
// Unlocking in state C violates regulations and risks the user.
Copy link
Contributor

Choose a reason for hiding this comment

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

I can only speculate but the reasoning why its only force unlocked in B and C is because we already unlock (without the force) when we enter state A (see same file)

Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that in general force unlock should always unlock but before doing so open the contactors (and wait for state A or B)

@corneliusclaussen
Copy link
Contributor

I think there is a bit of confusion what the force_unlock command should do, we probably need to improve the documentation in the interface for this one.

The use case is the following: If an EV driver calls the hotline because the cable does not unlock, the CSMS should be able to send an unlock command to unlock, no matter which state the EVSE is currently in. This also includes state C, as there may be a problem with the EV (or it is a simplified charging EV). After all, state C is issued from the vehicle and beyond our control.

It should of course only do so if the relay is open, which is what we can control.
As per OCPP 1.6 specs, the unlock command should not stop the transaction by itself, that has to be done before, so to successfully unlock from an ongoing charging session you would need to 1) stop the transaction and 2) force_unlock (The current implementation also allows to do it in reverse order).

Following this, the current implementation should:

  • State A: always unlocked anyway, forcing has no effect
  • State B: normally locked, a force unlock should normally unlock right away (as the relays are usually open)
  • State C/D: normally locked. A force unlock should be noted and executed once the relays are confirmed to be open (checked in check_connector_lock())
  • State E: always unlocked anyway, forcing has no effect
  • State F: unlock if in state F for more than 5 seconds (to avoid unlocking/locking during T_step_EF sequences)

If it does not behave like this, we should fix it, but I'm not aware of bugs here at the moment.

You stated unlocking in state C violates safety standards and regulations. Can you be more specific about this? I have not found any violation with IEC61851-1.

Signed-off-by: Yonatan Dankner <yda@qwello.eu>
@ydankner
Copy link
Contributor Author

ydankner commented Nov 6, 2024

Thank you for your quick responses. I understand the reasoning with unlocking in state C, but I would like to add state A as well - we have cases where the socket can be somehow locked in state A (if the lock didn't open properly previously), which makes it useful to force unlock it anyway. Is there a good reason not to force unlock in state A? I updated the branch accordingly.
@corneliusclaussen

@lategoodbye
Copy link

In our legacy charging stack unlock works regardless of the current CP or lock sensor state. The whole reason behind the OCPP unlock is that the mechanics / sensors behind plug locks are not 100% reliable. There are situations (e.g. very cold temperatures) were it seems that the cable has been disconnected, but it's still stuck. In this case it's last chance to get the cable back.

@ydankner
Copy link
Contributor Author

ydankner commented Nov 7, 2024

@lategoodbye that sounds like another reason to add the unlock for state A as well :)

@corneliusclaussen
Copy link
Contributor

corneliusclaussen commented Nov 7, 2024

I don't think your change actually does what you want. Did you test the code? It allows to set the force_unlock flag to true in state A. There are multiple issues here:

  • That flag is not cleared anymore. This means probably the next charging session won't lock at all.
  • Setting the flag has no effect, since it is already unlocked when entering state A
  • I agree with @lategoodbye on how it should be. However it is not like that, neither the current main nor your with your changes

The current logic in check_connector_lock() ensures a signal_lock() or signal_unlock() is only send on lock state changes. That means forcing unlocking in A will NOT generate a new unlock attempt as this was already done when entering A.

I would propose the following: Rework of the logic so that a force_unlock will send a new unlock attempt command to the interface even if it is already unlocked. Independent of the CP state, but doing it only if the relays are verfied to be open.

This however has nothing to do with the changes proposed in this PR, so maybe we can close this one and open a new one with the behaviour that @lategoodbye described.

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.

4 participants