Streets MOSAIC integration: CDASimAdapter register with Simulation ambassador and send sensors information #554

dan-du-car · 2023-07-25T19:02:07Z

PR Details

Description

Update the CDASimAdapter registration message to include the sensors information.
Read the sensors information from local file and store the sensors information in the CDASimAdapter
plugin.

Related Issue

NA

Motivation and Context

CDA research

How Has This Been Tested?

unit test
local integration test

Types of changes

Defect fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that cause existing functionality to change)

Checklist:

I have added any new packages to the sonar-scanner.properties file
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
V2XHUB Contributing Guide
I have added tests to cover my changes.
All new and existing tests passed.

paulbourelly999 · 2023-07-26T12:36:45Z

.devcontainer/docker-compose-vscode.yml

@@ -24,6 +24,8 @@ services:
      - SIM_V2X_PORT=5757
      - V2X_PORT=8686
      - INFRASTRUCTURE_ID=1
+      - SENSOR_JSON_FILE_PATH=/var/www/plugins/MAP/sensors.json
+      - KAFKA_BROKER_ADDRESS=127.0.0.1:9092


We no longer need this configuration parameter. It is captured in the CARMA-Streets Plugin configuration.

Which one? The json file path or the kafka broker?

The Kafka broker

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp

paulbourelly999 · 2023-07-26T12:59:26Z

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp

+        }
+    }
+
+    Json::Value CDASimConnection::get_sensor_by_id(std::string &sensor_id)


Just out of curiosity, is there a reason you opted against making sensor its own data type that handles serialization/deserialization internally. This works to but it seems to be that it may be a cleaner implementation of we keep JSON serialization/deserialization and data type definition separate from this plugin.

Do we need to create another data type for sensor in JSON representation? This function is to find the sensor Json::Value from the sensors of type Json::Value which is sent for registration using sensor id. As we will need this later to modify the detected object.

I think Json::Value data type is sufficient for this case. It allows more flexibility to modify the sensor JSON, and no overhead defining another strong sensor object type, then implement our own serialization/deserialization logic.

My issue is that it contaminates our Plugin Logic with the logic for serializing/deserializing/retreiving data from the sensor object. Ideally the entire Infrastructure Registration JSON message would have its own class so we can modify it without directly having to modify our plugin logic. I think this concept is referred to as encapsulation : "the bundling of data, along with the methods that operate on that data, into a single unit". I will leave it to you whether you believe it is worth addressing.

I think we are back to our definition of detected object. As we forwarding detected object as JSON string to TMX bus, and subscribe the JSON string as it. I do not think we need strong type and operation here.

paulbourelly999 · 2023-07-26T13:39:37Z

src/tmx/TmxUtils/src/simulation/SimulationEnvUtils.h

+        /**
+         * @brief sensors file location
+        */
+        constexpr inline static const char *SENSOR_JSON_FILE_PATH = "SENSOR_JSON_FILE_PATH";


What was your reasoning for including this as an environment variable instead of a plugin configuration similar to location. Just curious, I think we need to define a clear line between what we use as environment variables and what ew configure in the plugin. Simulation configurations are good to have in as environment variables because they do not require configuration and can be read easily at startup. They are also unlikely to change dynamically. I think that makes sense for the sensor configuration file as well.

Did you answer your own question? Yeah, I agree that the file path for sensors shall not change constantly.

Yeah just wanted to bring it up, that we give thought to what is an environment variable and what is a configuration parameter.

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp

paulbourelly999 · 2023-07-26T13:42:45Z

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp

+       populate_json_with_string(ss.str(), sensors_json_v);
+    }
+
+    void CDASimConnection::populate_json_with_string(const std::string& json_str, Json::Value& json_v){


Same comment as above about using void return by reference. What is your reasoning here?

stop the execution flow and return the control to the caller

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp

paulbourelly999 · 2023-07-26T13:44:11Z

src/v2i-hub/CDASimAdapter/src/include/CDASimConnection.hpp

@@ -142,6 +163,9 @@ namespace CDASimAdapter {
            uint _v2x_port;
            tmx::utils::Point _location;
            bool _connected = false;
+            std::string _sensor_json_file_path;
+            //Global variable to store the sensors information
+            Json::Value _sensors_json_v;


Why not make a sensor class

I think Json::Value is sufficient for this. Do you have specific reason that we need a sensor class?

I would be against handling any JSON object directly especially if we plan to used the stored data. There is no indication in the code of what structure the JSON has to have, additionally if we pass this JSON anywhere else in the code there will be not structure defined there either.

I can add comments to refer to the sensors.json, so we know what json structure we are expecting if that helps to indicate the sensor Json message.

paulbourelly999 · 2023-07-26T13:46:07Z

src/v2i-hub/CDASimAdapter/test/sensors.json

@@ -0,0 +1,32 @@
+[
+  {
+    "id": "SomeID",


Can we change this to be sensorId. I think id conflicts with Interaction (MOSIAC RTI) Id field.

sensorId instead of sensor_id to be consistant with naming convention.

paulbourelly999 · 2023-07-27T13:05:52Z

src/v2i-hub/CDASimAdapter/test/TestCARMASimulationConnection.cpp

+        if(in_strm.is_open())
+        {
+            ASSERT_EQ(connection->get_handshake_json("4566", "127.0.0.1", 4567, 4568, location), 
+            "{\n   \"infrastructureId\" : \"4566\",\n   \"location\" : {\n      \"x\" : 1000.0,\n      \"y\" : 38.954999999999998,\n      \"z\" : -77.149000000000001\n   },\n   \"rxMessageIpAddress\" : \"127.0.0.1\",\n   \"rxMessagePort\" : 4568,\n   \"sensors\" : [\n      {\n         \"location\" : {\n            \"x\" : 0.0,\n            \"y\" : 0.0,\n            \"z\" : 0.0\n         },\n         \"orientation\" : {\n            \"x\" : 0.0,\n            \"y\" : 0.0,\n            \"z\" : 0.0\n         },\n         \"sensor_id\" : \"SomeID\",\n         \"type\" : \"SematicLidar\"\n      },\n      {\n         \"location\" : {\n            \"x\" : 1.0,\n            \"y\" : 2.0,\n            \"z\" : 0.0\n         },\n         \"orientation\" : {\n            \"x\" : 23.0,\n            \"y\" : 0.0,\n            \"z\" : 0.0\n         },\n         \"sensor_id\" : \"SomeID2\",\n         \"type\" : \"LIDAR\"\n      }\n   ],\n   \"timeSyncPort\" : 4567\n}\n");


"sensorId" instance of "sensor_id" so we are consistent with naming convention

paulbourelly999 · 2023-07-27T13:09:08Z

If you fix the sensorId field this looks good to me. Looking into the code smells now.

paulbourelly999 · 2023-07-27T13:11:07Z

src/v2i-hub/CDASimAdapter/test/TestCARMASimulationConnection.cpp

@@ -21,15 +22,16 @@ namespace CDASimAdapter {
    class TestCARMASimulationConnection : public ::testing::Test {
        protected:
            void SetUp() override {
-                // Initialize CARMA Simulation connection with (0,0,0) location.
+                // Initialize CARMA Simulation connection with (0,0,0) location and mock kafka producer.


small issue but there is no mock kafka producer here.

paulbourelly999 · 2023-07-27T13:12:47Z

src/v2i-hub/CDASimAdapter/scripts/udp_socket_server.py

@@ -0,0 +1,12 @@
+import socket
+


Add small commented about what script is for and how to use it?

It is a UDP server script to launch a server for testing the CDASimAdapter plugin registration. Run this script first, then start the CDASimAdapter plugin, this server shall print out the registration message.

paulbourelly999 · 2023-07-27T14:53:47Z

src/v2i-hub/CDASimAdapter/test/TestCARMASimulationConnection.cpp

+        if(in_strm.is_open())
+        {
+            ASSERT_EQ(connection->get_handshake_json("4566", "127.0.0.1", 4567, 4568, location), 
+            "{\n   \"infrastructureId\" : \"4566\",\n   \"location\" : {\n      \"x\" : 1000.0,\n      \"y\" : 38.954999999999998,\n      \"z\" : -77.149000000000001\n   },\n   \"rxMessageIpAddress\" : \"127.0.0.1\",\n   \"rxMessagePort\" : 4568,\n   \"sensors\" : [\n      {\n         \"location\" : {\n            \"x\" : 0.0,\n            \"y\" : 0.0,\n            \"z\" : 0.0\n         },\n         \"orientation\" : {\n            \"x\" : 0.0,\n            \"y\" : 0.0,\n            \"z\" : 0.0\n         },\n         \"sensorId\" : \"SomeID\",\n         \"type\" : \"SematicLidar\"\n      },\n      {\n         \"location\" : {\n            \"x\" : 1.0,\n            \"y\" : 2.0,\n            \"z\" : 0.0\n         },\n         \"orientation\" : {\n            \"x\" : 23.0,\n            \"y\" : 0.0,\n            \"z\" : 0.0\n         },\n         \"sensorId\" : \"SomeID2\",\n         \"type\" : \"LIDAR\"\n      }\n   ],\n   \"timeSyncPort\" : 4567\n}\n");


Lastly orientation should be

{ "yaw":0.0, "pitch":0.0, "roll":0.0

paulbourelly999 · 2023-07-27T14:54:20Z

src/v2i-hub/CDASimAdapter/test/sensors.json

+      "z": 0.0
+     },
+     "orientation": {
+       "x": 0.0,


See comment about about yaw, pitch, roll

paulbourelly999 · 2023-07-27T14:55:06Z

src/v2i-hub/CDASimAdapter/test/sensors.json

+  },
+  {
+    "sensorId": "SomeID2",
+    "type": "LIDAR",


We will only support SemanticLidar type for right now.

dan-du-car and others added 4 commits July 25, 2023 18:57

init

d0a5fbc

Merge branch 'develop' into register_cdasim_adapter

c96db25

update

c4474a8

add sensor json

950a211

dan-du-car requested a review from paulbourelly999 July 25, 2023 20:02

dan-du-car added 4 commits July 25, 2023 20:23

CI build fix

297e147

CI build fix

4a76dd0

update file path

a7ffaa6

update file path

680b94e

paulbourelly999 reviewed Jul 26, 2023

View reviewed changes

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp Outdated Show resolved Hide resolved

paulbourelly999 reviewed Jul 26, 2023

View reviewed changes

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp Show resolved Hide resolved

paulbourelly999 reviewed Jul 26, 2023

View reviewed changes

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp Outdated Show resolved Hide resolved

paulbourelly999 reviewed Jul 26, 2023

View reviewed changes

src/v2i-hub/CDASimAdapter/src/CDASimConnection.cpp Outdated Show resolved Hide resolved

paulbourelly999 reviewed Jul 26, 2023

View reviewed changes

dan-du-car added 10 commits July 26, 2023 14:01

update PR

f9b2190

update

f5840ff

add unit test

7a1fc37

address comments

44685f3

address comments

9ae8603

reafactor func

c1c334f

code smell

85280e4

address comments

9abb47b

address comments

55221e3

fix unit test failure

e1825e7

paulbourelly999 reviewed Jul 27, 2023

View reviewed changes

dan-du-car added 4 commits July 27, 2023 13:44

address comments

646168a

address comments

b2ba7b9

address comments

d3c8d53

add comments to python script

0db0b59

paulbourelly999 reviewed Jul 27, 2023

View reviewed changes

dan-du-car added 4 commits July 27, 2023 14:58

add comments to python script

fa44458

update unit test

f438b05

update unit test

11a36d6

update unit test

0c615eb

paulbourelly999 approved these changes Jul 27, 2023

View reviewed changes

paulbourelly999 merged commit 678784c into develop Jul 27, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Streets MOSAIC integration: CDASimAdapter register with Simulation ambassador and send sensors information #554

Streets MOSAIC integration: CDASimAdapter register with Simulation ambassador and send sensors information #554

dan-du-car commented Jul 25, 2023 •

edited

Loading

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 26, 2023

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 27, 2023

paulbourelly999 Jul 26, 2023

dan-du-car Jul 26, 2023

paulbourelly999 Jul 27, 2023

dan-du-car Jul 27, 2023

paulbourelly999 Jul 27, 2023

paulbourelly999 commented Jul 27, 2023

paulbourelly999 Jul 27, 2023

dan-du-car Jul 27, 2023

paulbourelly999 Jul 27, 2023

dan-du-car Jul 27, 2023

paulbourelly999 Jul 27, 2023

paulbourelly999 Jul 27, 2023

paulbourelly999 Jul 27, 2023

dan-du-car Jul 27, 2023

Streets MOSAIC integration: CDASimAdapter register with Simulation ambassador and send sensors information #554

Streets MOSAIC integration: CDASimAdapter register with Simulation ambassador and send sensors information #554

Conversation

dan-du-car commented Jul 25, 2023 • edited Loading

PR Details

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

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

paulbourelly999 commented Jul 27, 2023

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

dan-du-car commented Jul 25, 2023 •

edited

Loading