feat(charging): provide the charging stations within a certain area #446

kschrab · 2024-12-12T10:42:22Z

Description

New method to provide a list of available charging stations within the vicinity of the vehicle (only for Electric Vehicles).

Issue(s) related to this PR

Resolves internal issue 950

Affected parts of the online documentation

Changes in the documentation required?

Definition of Done

Prerequisites

You have read CONTRIBUTING.md carefully.
You have signed the Contributor License Agreement.
Your GitHub user id is linked with your Eclipse Account.

Required

The title of this merge request follows the scheme type(scope): description (in the style of Conventional Commits)
You have assigned a suitable label to this pull request (e.g., enhancement, or bugfix)
origin/main has been merged into your Fork.
Coding guidelines have been followed (see CONTRIBUTING.md).
All checks on GitHub pass.
All tests on Jenkins pass.

Requested (can be enforced by maintainers)

New functionality is covered by unit tests or integration tests. Code coverage must not decrease.
If a bug has been fixed, a new unit test has been written (beforehand) to prove misbehavior
There are no new SpotBugs warnings.

Special notes to reviewer

...n/java/org/eclipse/mosaic/fed/application/ambassador/util/EventNicenessPriorityRegister.java

.../main/java/org/eclipse/mosaic/interactions/electricity/ChargingStationDiscoveryResponse.java

hoelger · 2024-12-12T16:22:22Z

...java/org/eclipse/mosaic/interactions/electricity/VehicleChargingStationDiscoveryRequest.java

+    }
+
+    @Serial
+    private static final long serialVersionUID = 1L;


Why do we have the serial number here? If it's an interaction-specific thing, why don't we use it in the response as well?

Interaction implements Serializable, if we would do distributed simulation in some way or another and serialize/deserialize interactions between remote hosts, this UID would be used to assure that all sides have the same class version loaded. When adding new Interactions giving a serialVersionUID of 1 is good, indicating version 1.

Now what we should consider though is that we update this ID whenever we change Interactions in a meaningful manner.

(see https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/io/Serializable.html)

I hope this suffices as a explanation 😅

A specific version is needed especially when storing the serialized objects in files or databases and reading them later. Then the version plays a crucial role to determine if a previously stored object is compatible for deserialization or not. Since we don't do such things, we decided long ago that we use 1 always and everywhere where we implement Serializable.

We transfer interactions between PHABMACS and MOSAIC by using this object-serialization, thus we need to implement the Serializable interface for all interactions, and for all objects which are used by interactions. Nevertheless, since we serialize/deserialize the objects just for the transport from/to PHABMACS, we don't need to care about versioning, thus using version 1 again everywhere.

...ation/src/main/java/org/eclipse/mosaic/fed/application/ambassador/ApplicationAmbassador.java

schwepmo · 2024-12-17T10:28:10Z

.gitignore

+.tiles
+/logs/


This should not be necessary, please remove

@iwillitried yes, there is another .gitignore in mosaic-starter which ignores the logs directory. That should suffice and you can remove the /logs/ entry here.

This is still not resolved!

.../main/java/org/eclipse/mosaic/fed/application/ambassador/simulation/ElectricVehicleUnit.java

schwepmo · 2024-12-17T10:39:59Z

...java/org/eclipse/mosaic/interactions/electricity/VehicleChargingStationDiscoveryRequest.java

+    }
+
+    @Serial
+    private static final long serialVersionUID = 1L;


Interaction implements Serializable, if we would do distributed simulation in some way or another and serialize/deserialize interactions between remote hosts, this UID would be used to assure that all sides have the same class version loaded. When adding new Interactions giving a serialVersionUID of 1 is good, indicating version 1.

Now what we should consider though is that we update this ID whenever we change Interactions in a meaningful manner.

(see https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/io/Serializable.html)

I hope this suffices as a explanation 😅

.../main/java/org/eclipse/mosaic/interactions/electricity/ChargingStationDiscoveryResponse.java

.../main/java/org/eclipse/mosaic/fed/application/app/api/os/ElectricVehicleOperatingSystem.java

…#965)

...n/java/org/eclipse/mosaic/fed/application/ambassador/util/EventNicenessPriorityRegister.java

...ic-objects/src/main/java/org/eclipse/mosaic/lib/objects/electricity/ChargingStationData.java

.../main/java/org/eclipse/mosaic/fed/application/app/api/os/ElectricVehicleOperatingSystem.java

...pse/mosaic/fed/application/ambassador/simulation/electric/objects/ChargingStationObject.java

...pse/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationTree.java

hoelger · 2025-01-23T13:59:39Z

...pse/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationTree.java

+public class ChargingStationTree extends ChargingStationIndex {
+    private final int bucketSize;
+
+    private KdTree<ChargingStationObject> chargingStationTree;


I don't find the code where this Tree is updated... can you help me? The tree should be updated (= take all data from indexedChargingStations and put it into the tree) latest every time before a search is issued?

In my understanding, the Tree uses the object references provided by the indexedChargingStations class variable. The tree just points at these objects

Ok, maybe the tree uses the references, this would mean that you continue to have a correct behavior even when CS data is updated.
But as I read the code, you start to have a wrong behavior when CS stations are added or removed.

The code how it is written now has following assumptions:

All addChargingStation calls are done before the first updateChargingStation call

You have at least one updateChargingStation call before the first "find in tree" call.
Both are things that might work now, but you cannot assume for the future!

I propose the following, which does not blow up the performance:

Firstly, write tests, which show that the current code is not working as expected. This way we can prevent regressions in the future.

Set the flag recreateTree = true inside add (and theoretically inside remove)

On every search call, check if you need to recreate the tree. If recreateTree==true then build the tree new, and reset the flag to false.

Make sure in a test, that a call to update is correctly shown in the tree later

Please create new bug-issues in gitlab for the code parts where you copied this behavior from.

The KD-Tree implementation used here is not designed to have objects added or removed after initialization, which maybe is a different problem.

Also as far as I see it, we don't actually need to ever update anything in this tree as for all intents and purposes we are just interested in the position and id of the charging stations, which is already contained within the registration.

In the traffic light tree we used the updateTrafficLights as a hook to initialize the tree (which is hacky in the first place), as we can be sure the all traffic light registrations have been received before the first update. I'm not sure this can be applied for the charging stations as well. However, I'm not certain how we can know if all charging station registrations have been received. 🤷

If we actually see the requirement to recreate the tree, we should use a different data structure as recreation scales really poorly.

...n/java/org/eclipse/mosaic/fed/application/ambassador/util/EventNicenessPriorityRegister.java

hoelger · 2025-01-23T15:20:54Z

...pse/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationTree.java

+    }
+
+    @Override
+    public void onChargingStationUpdate() {


I know that for e.g. TrafficLightTree this does not exist, but please add tests for the functions in ChargingStationIndex and ChargingStationTree

please check if the test ApplicationAmbassadorTest.processInteraction_ChargingStationRegistration() is enough or if we need an explicit test for updating the data of the object

How I read the processInteraction_ChargingStationRegistration it just shows that the interaction is processed without error, but you don't even have a check which looks if this add is executed correctly, and if you can then find/search/update the CS later.

So no, unfortunately you have to add more specific and detailed testing.

Yes we should have a separate Test for the ChargingStationTree.
The SimplePerceptionModule Test contains some implicit testing of the indexes, however I agree we should also have separate tests for all perception indexes.

- Merged contents of ChargingStationTree into ChargingStationIndex - ChargingStationIndex is not abstract anymore - small changes regarding function naming and removal of empty lines

kschrab

Minor changes required. Also, please check if all unit tests are green, currently, ApplicationAmbassadorTest.processInteraction_ChargingStationRegistration fails

kschrab · 2025-02-03T11:37:24Z

...se/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationIndex.java

+                .setChargingStationData(chargingStationData);
+    }
+
+    public List<ChargingStationObject>getChargingStationsInCircle(GeoCircle circle) {


Code style: missing space before method name

kschrab · 2025-02-03T11:40:18Z

...se/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationIndex.java

+    public void onChargingStationUpdate() {
+        if (chargingStationTree == null) { // initialize before first update is called
+            List<ChargingStationObject> allChargingStations = new ArrayList<>(indexedChargingStations.values());
+            chargingStationTree = new KdTree<>(new SpatialObjectAdapter<>(), allChargingStations, bucketSize);
+            treeTraverser = new SpatialTreeTraverser.InRadius<>();
+        }
+    }


Does this need to be public? I would make that private, rename it to initSearchTree() and move it right below updateChargingStation method where it is needed

kschrab · 2025-02-03T11:40:47Z

...se/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationIndex.java

+    public void initialize() {
+        // initialization at first update
+    }


Why do we have this method if we don't do anything here?

kschrab · 2025-02-03T11:41:25Z

.../main/java/org/eclipse/mosaic/fed/application/ambassador/simulation/ElectricVehicleUnit.java

+    public List<ChargingStationData> getChargingStationsInArea(GeoCircle searchArea) {
+        return SimulationKernel.SimulationKernel.getChargingStationIndex().getChargingStationsInCircle(searchArea)
+                .stream().map(ChargingStationObject::getChargingStationData)
+                .collect(Collectors.toList());


use toList() instead of .collect(Collectors.toList());

kschrab · 2025-02-03T11:42:00Z

...pplication/src/main/java/org/eclipse/mosaic/fed/application/ambassador/SimulationKernel.java

+        }
+
+        return this.chargingStationIndex;
+    }


code style: missing blank line after this method.

…ingStationRegistration(...)

hoelger · 2025-02-10T12:54:28Z

...se/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationIndex.java

+import java.util.List;
+import java.util.Map;
+
+public class ChargingStationIndex {


After merging Index and Tree, I think we should stick with naming "Tree". If I merge the two files from "Car extends Vehicle" then also what remains is rather a "Car" than a "Vehicle".

hoelger · 2025-02-10T13:21:39Z

...pse/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationTree.java

+public class ChargingStationTree extends ChargingStationIndex {
+    private final int bucketSize;
+
+    private KdTree<ChargingStationObject> chargingStationTree;


Ok, maybe the tree uses the references, this would mean that you continue to have a correct behavior even when CS data is updated.
But as I read the code, you start to have a wrong behavior when CS stations are added or removed.

The code how it is written now has following assumptions:

All addChargingStation calls are done before the first updateChargingStation call

You have at least one updateChargingStation call before the first "find in tree" call.
Both are things that might work now, but you cannot assume for the future!

I propose the following, which does not blow up the performance:

Firstly, write tests, which show that the current code is not working as expected. This way we can prevent regressions in the future.

Set the flag recreateTree = true inside add (and theoretically inside remove)

On every search call, check if you need to recreate the tree. If recreateTree==true then build the tree new, and reset the flag to false.

Make sure in a test, that a call to update is correctly shown in the tree later

Please create new bug-issues in gitlab for the code parts where you copied this behavior from.

hoelger · 2025-02-10T13:23:41Z

...pse/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationTree.java

+    }
+
+    @Override
+    public void onChargingStationUpdate() {


How I read the processInteraction_ChargingStationRegistration it just shows that the interaction is processed without error, but you don't even have a check which looks if this add is executed correctly, and if you can then find/search/update the CS later.

So no, unfortunately you have to add more specific and detailed testing.

schwepmo · 2025-02-10T13:33:24Z

.gitignore

+.tiles
+/logs/


This is still not resolved!

schwepmo · 2025-02-10T13:38:32Z

...ation/src/main/java/org/eclipse/mosaic/fed/application/ambassador/ApplicationAmbassador.java

@@ -368,6 +376,9 @@ private void process(final ServerRegistration serverRegistration) {

    private void process(final ChargingStationRegistration chargingStationRegistration) {
        UnitSimulator.UnitSimulator.registerChargingStation(chargingStationRegistration);
+        String id = chargingStationRegistration.getMapping().getName();
+        GeoPoint position = chargingStationRegistration.getMapping().getPosition();
+        SimulationKernel.SimulationKernel.chargingStationIndex.addChargingStation(id, position);


Use SimulationKernel.SimulationKernel.getChargingStationIndex() here

schwepmo · 2025-02-10T13:38:57Z

...ation/src/main/java/org/eclipse/mosaic/fed/application/ambassador/ApplicationAmbassador.java

@@ -449,6 +460,8 @@ private void process(final ChargingStationUpdate chargingStationUpdate) {
        final AbstractSimulationUnit simulationUnit =
                UnitSimulator.UnitSimulator.getUnitFromId(chargingStationData.getName());

+        SimulationKernel.SimulationKernel.chargingStationIndex.updateChargingStation(chargingStationUpdate.getUpdatedChargingStation());


Use SimulationKernel.SimulationKernel.getChargingStationIndex() here.
Also why don't you just use the chargingStationData as input declared in line 459?

schwepmo · 2025-02-10T13:40:30Z

...ation/src/main/java/org/eclipse/mosaic/fed/application/ambassador/ApplicationAmbassador.java

@@ -458,6 +471,7 @@ private void process(final ChargingStationUpdate chargingStationUpdate) {
                chargingStationData,
                EventNicenessPriorityRegister.UPDATE_CHARGING_STATION
        );
+


Why did you add this blank line? This makes this method look different from all other in this file

schwepmo · 2025-02-10T13:42:06Z

...ation/src/main/java/org/eclipse/mosaic/fed/application/ambassador/ApplicationAmbassador.java

@@ -158,6 +160,12 @@ public ApplicationAmbassador(AmbassadorParameter ambassadorParameter) {
            SimulationKernel.SimulationKernel.setCentralPerceptionComponent(centralPerceptionComponent);
        }

+        if (SimulationKernel.SimulationKernel.chargingStationIndex == null) {
+            // use same bucketsize as TrafficLightTree (see: CPercetion.java) (bucketsize := number of direct children per tree node)
+            ChargingStationIndex chargingStationIndex = new ChargingStationIndex(20);


Maybe define the 20 as a constant in the ChargingStationIndex, the way it defined right now is a magic number.

schwepmo · 2025-02-10T13:45:39Z

.../main/java/org/eclipse/mosaic/fed/application/ambassador/simulation/ElectricVehicleUnit.java

@@ -103,4 +109,10 @@ public void sendChargingStopRequest() {
        );
        sendInteractionToRti(vehicleChargingStopRequest);
    }
+
+    public List<ChargingStationData> getChargingStationsInArea(GeoCircle searchArea) {


Shouldn't this return ChargingStationObjects?

schwepmo · 2025-02-10T13:48:09Z

...pse/mosaic/fed/application/ambassador/simulation/electric/objects/ChargingStationObject.java

+    /**
+     * The data object that stores all static and dynamic information of the charging station.
+     */
+    private ChargingStationData chargingStationData;


This field stores references to the objects contained within the ChargingStationUpdates, which is potentially dangerous. I believe we should copy the relevant information from the ChargingStationData and store it in these objects

Looking into the ChargingStationData, I feel like we don't require it at all as we currently are just interested in the position of the charging stations. As far as I see we don't need any info on the charging spots?!

schwepmo · 2025-02-10T13:54:42Z

...se/mosaic/fed/application/ambassador/simulation/electric/providers/ChargingStationIndex.java

+     */
+    public void updateChargingStation(ChargingStationData chargingStationData) {
+        initSearchTree();
+        indexedChargingStations.get(chargingStationData.getName())


is there actually anything we want to update about charging stations?

schwepmo · 2025-02-10T13:55:23Z

.../main/java/org/eclipse/mosaic/fed/application/app/api/os/ElectricVehicleOperatingSystem.java

+     *
+     * @param searchArea The area where the charging stations are searched
+     */
+    List<ChargingStationData> getChargingStationsInArea(GeoCircle searchArea);


Why isn't this returning ChargingStationObjects?

provide the charging stations within a certain area

e934a58

hoelger suggested changes Dec 12, 2024

View reviewed changes

schwepmo reviewed Dec 16, 2024

View reviewed changes

...ation/src/main/java/org/eclipse/mosaic/fed/application/ambassador/ApplicationAmbassador.java Outdated Show resolved Hide resolved

schwepmo requested changes Dec 17, 2024

View reviewed changes

ChargingStations discovery is synchronous (related to mosaic-extended…

434bb9a

…#965)

kschrab commented Jan 21, 2025

View reviewed changes

kschrab added the enhancement New feature or request label Jan 21, 2025

kschrab marked this pull request as ready for review January 21, 2025 12:58

resolving pull request issues

b7ad846

hoelger reviewed Jan 23, 2025

View reviewed changes

hoelger suggested changes Jan 23, 2025

View reviewed changes

iwillitried added 2 commits January 29, 2025 11:12

Merge branch 'main' into 950-Provide-Charging-Stations-around-a-GeoPoint

83137fa

- Removed the file: ChargingStationTree.java

a0cf89b

- Merged contents of ChargingStationTree into ChargingStationIndex - ChargingStationIndex is not abstract anymore - small changes regarding function naming and removal of empty lines

kschrab commented Feb 3, 2025

View reviewed changes

iwillitried added 3 commits February 4, 2025 12:08

resolving PR issues

0d05e63

Fixing Unit tests: ApplicationAmbassadorTest.processInteraction_Charg…

fb7ce5a

…ingStationRegistration(...)

fixing PR issues

a94ffd6

hoelger reviewed Feb 10, 2025

View reviewed changes

schwepmo requested changes Feb 10, 2025

View reviewed changes

		.tiles
		/logs/

		.tiles
		/logs/

feat(charging): provide the charging stations within a certain area #446

Are you sure you want to change the base?

feat(charging): provide the charging stations within a certain area #446

Conversation

kschrab commented Dec 12, 2024 • edited Loading

Description

Issue(s) related to this PR

Affected parts of the online documentation

Definition of Done

Special notes to reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschrab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kschrab commented Dec 12, 2024 •

edited

Loading