Skip to content

Improve Session Management for Charge Point Connections #2908

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.

Sign up for GitHub

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

Closed
Sn0w3y wants to merge 5 commits into from
Closed

Improve Session Management for Charge Point Connections #2908

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f94ac47
Update EvcsOcppServer.java
Sn0w3y Dec 3, 2024
7213b92
Update MyJsonServer.java
Sn0w3y Dec 3, 2024
abb988c
Checkstyle Fix
Sn0w3y Dec 3, 2024
25e7484
Fix super class missing
Sn0w3y Dec 3, 2024
c0a9afa
Merge branch 'OpenEMS:develop' into fix-ocpp-server
Sn0w3y Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 20 additions & 8 deletions io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/EvcsOcppServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,21 +110,33 @@ protected void removeEvcs(Evcs evcs) {
return;
}
var ocppEvcs = (AbstractManagedOcppEvcsComponent) evcs;
var evcss = this.activeEvcsSessions.get(ocppEvcs.getSessionId());
if (evcss != null) {
if (evcss.size() < 2) {
this.activeEvcsSessions.remove(ocppEvcs.getSessionId());
} else {
this.activeEvcsSessions.get(ocppEvcs.getSessionId()).remove(ocppEvcs);
var sessionId = ocppEvcs.getSessionId();
if (sessionId != null) {
var evcss = this.activeEvcsSessions.get(sessionId);
if (evcss != null) {
evcss.remove(ocppEvcs);
if (evcss.isEmpty()) {
this.activeEvcsSessions.remove(sessionId);
// Also remove from ocppSessions if needed
Copy link
Contributor

@katsuya katsuya Dec 10, 2024

Choose a reason for hiding this comment

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

In actual production environments, we rarely remove components, so we went ahead with weekend testing without including this commit. However, I don’t see any issues with the code itself.

Copy link
Collaborator Author

@Sn0w3y Sn0w3y Dec 16, 2024

Choose a reason for hiding this comment

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

Could you maybe still test it ?

var ocppIdToRemove = "";
for (Entry<String, UUID> entry : this.ocppSessions.entrySet()) {
if (entry.getValue().equals(sessionId)) {
ocppIdToRemove = entry.getKey();
break;
}
}
if (!ocppIdToRemove.isEmpty()) {
this.ocppSessions.remove(ocppIdToRemove);
}
}
}
}
this.ocppEvcss.remove(ocppEvcs.getConfiguredOcppId());
ocppEvcs.lostSession();
}

public EvcsOcppServer() {
super(OpenemsComponent.ChannelId.values() //
);
super(OpenemsComponent.ChannelId.values());
}

@Activate
Expand Down
29 changes: 26 additions & 3 deletions io.openems.edge.evcs.ocpp.server/src/io/openems/edge/evcs/ocpp/server/MyJsonServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ public class MyJsonServer {
/**
* The JSON OCPP server.
*
* <p>
* Responsible for sending and receiving OCPP JSON commands.
*
* <p>Responsible for sending and receiving OCPP JSON commands.
*/
private final JSONServer server;

Expand Down Expand Up @@ -90,6 +90,23 @@ public void newSession(UUID sessionIndex, SessionInformation information) {

var ocppIdentifier = information.getIdentifier().replace("/", "");

// Check if there is already a session for this ocppIdentifier
Copy link
Contributor

@katsuya katsuya Dec 10, 2024

Choose a reason for hiding this comment

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

Over the weekend, we tested both our patch(#2908 (comment)) and this one, and both appeared to work properly. However, I’m concerned about the following point in this patch. What do you think?

UUID oldSessionIndex = MyJsonServer.this.parent.ocppSessions.get(ocppIdentifier);
if (oldSessionIndex != null && !oldSessionIndex.equals(sessionIndex)) {
// Remove old session
MyJsonServer.this.logDebug("Removing old session [" + oldSessionIndex + "] for ocppIdentifier ["
+ ocppIdentifier + "]");
List<AbstractManagedOcppEvcsComponent> oldEvcss = MyJsonServer.this.parent.activeEvcsSessions
.get(oldSessionIndex);
if (oldEvcss != null) {
for (AbstractManagedOcppEvcsComponent ocppEvcs : oldEvcss) {
ocppEvcs.lostSession();
}
MyJsonServer.this.parent.activeEvcsSessions.remove(oldSessionIndex);
}
}

// Update the ocppSessions with the new sessionIndex
MyJsonServer.this.parent.ocppSessions.put(ocppIdentifier, sessionIndex);

var presentEvcss = MyJsonServer.this.parent.ocppEvcss.get(ocppIdentifier);
Expand Down Expand Up @@ -122,10 +139,16 @@ public void lostSession(UUID sessionIndex) {
for (Entry<String, UUID> session : MyJsonServer.this.parent.ocppSessions.entrySet()) {
Copy link
Contributor

@katsuya katsuya Dec 10, 2024

Choose a reason for hiding this comment

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

To address the issue mentioned below, we added the following changes. However, these modifications might not be necessary if our only concern is the brief window between establishing the new session and discarding the old one, as they make the code more complex.

When the old connection is disconnected, it should ensure that the lostSession for the charge point referenced by the new connection is not invoked.

        if (sessionEvcss != null) {
          for (AbstractManagedOcppEvcsComponent ocppEvcs : sessionEvcss) {
            var ocppId = ocppEvcs.getConfiguredOcppId();
            UUID latest = MyJsonServer.this.parent.ocppSessions.get(ocppId);
            /**
             * Since multiple sessions are connected with the same ocppId, if
             * `lostSession` is executed with an old session, the `sessionId` of the
             * component corresponding to the latest session would be set to null.
             */
            if (latest != sessionIndex) {
              MyJsonServer.this.logWarn("skip lostSession of old session [" + sessionIndex +
                  "] for ocppIdentifier [" + ocppId + "]");
              continue;
            }
            ocppEvcs.lostSession();
            MyJsonServer.this.logWarn("lostSession [" + sessionIndex + "] for ocppIdentifier [" + ocppId + "]");
          }
          MyJsonServer.this.parent.activeEvcsSessions.remove(sessionIndex);
          MyJsonServer.this.logWarn("remove activeEvcsSessions [" + sessionIndex + "]");
        }

if (session.getValue().equals(sessionIndex)) {
ocppId = session.getKey();
break;
}
}

MyJsonServer.this.parent.ocppSessions.remove(ocppId);
// Before removing, check if the sessionIndex is still the one mapped to ocppId
Copy link
Contributor

@katsuya katsuya Dec 10, 2024

Choose a reason for hiding this comment

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

This code also deals with the window time between establishing the new session and discarding the old one inside newSession, so it might not be necessary after all. During our weekend tests, we couldn’t hit this code path without artificially introducing sleeps, which suggests we may not need it.

UUID currentSessionIndex = MyJsonServer.this.parent.ocppSessions.get(ocppId);
if (currentSessionIndex != null && currentSessionIndex.equals(sessionIndex)) {
MyJsonServer.this.parent.ocppSessions.remove(ocppId);
}

MyJsonServer.this.parent.activeEvcsSessions.remove(sessionIndex);
}

Expand Down