Feature/656 device model initialization in cpp #681

maaikez · 2024-06-20T16:14:48Z

Describe your changes

Python script to initialize and insert device model database is now moved to C++. This also allows is to add more Connectors and EVSE's and change Variables of Connectors and EVSE's.

Not everything is done yet:

There are some todo's left (if you have an opinion on them, please add to the draft as well)
Master needs to be merged in.

Issue ticket number and link

#656

Checklist before requesting a review

I have performed a self-review of my code
I have made corresponding changes to the documentation
If OCPP 2.0.1: I have updated the OCPP 2.0.1 status document
I read the contribution documentation and made sure that my changes meet its requirements

…thon. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…ion. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

… (install directory) so it can be copied to the target. Move sql script to separate 'migration' folder. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…raints, change foreign key of variable characteristics so we can add an ON DELETE constraint, which prevents dangling characteristics. Get components from db and check if they are in the schema, remove if they are not in the schema files but are existing in the database (only for Connectors and EVSE's). Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…eck if anything changed with components and variables and update variables. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…ributes, delete variables. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

… call so it also works with in memory databases. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…commented code. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

… so the value source is / can be set when an attribute value is changed. Add test for insert config and default values and fix some bugs. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…me functions to not return a boolean, because they just throw if something is going wrong. Add more verbose information to exception. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…d new tests. Fix almost all tests. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…ning when default value is an array or object). Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…ing foreign key pointing to it. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

… does not work anymore. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez · 2024-07-03T08:37:06Z

lib/ocpp/v201/init_device_model_db.cpp

+    return config_values;
+}
+
+bool InitDeviceModelDb::check_config_integrity(


Should we add a check for the source of the value as well and add a log message if it is not 'default' (so the value is not changed)? And then maybe remove it from the list so we don't get more warnings about it.

sounds like a good idea

Hmm because it is now a vector, if there is any error in the vector it will throw. So maybe it is not the best idea after all :). I think I keep it like this.

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

include/ocpp/v201/init_device_model_db.hpp

lib/ocpp/v201/charge_point.cpp

lib/ocpp/v201/device_model_storage_sqlite.cpp

lib/ocpp/v201/init_device_model_db.cpp

hikinggrass · 2024-07-03T11:43:51Z

lib/ocpp/v201/init_device_model_db.cpp

+                EVLOG_error << "Could not remove database " << database_path.u8string();
+                throw InitDeviceModelDbError("Could not remove database " + database_path.u8string());


EVLOG_AND_THROW

Changed it (and all the others). Shouldn't we use EVLOG_AND_THROW always instead of only throw?

I would say if you want to log an error that the user should see then yes, but if it's just an internal exception that can be handled without the user needed to know it's perfectly fine to just throw

hikinggrass · 2024-07-03T12:57:35Z

lib/ocpp/v201/init_device_model_db.cpp

I think the "errors" output variable should be handled in a different way (preferable with a return value)

The whole string as return value you mean?

Yes, or a struct/vector with the individual errors might also be an idea

…port migrations yet. Fix some config / schema inconsistencies. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…fault config value could not be set because it has already changed by another source. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…components / variables / attributes that are not consistent with the database. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

… at the check integrity function. Put inserting in database inside transactions, otherwise it is very slow. Add test for the default device model schemas / config. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez · 2024-07-04T10:09:53Z

lib/ocpp/v201/init_device_model_db.cpp

+            InitDeviceModelDbError("Config not consistent with device model component schema's: \n" + output));
+    }
+
+    std::unique_ptr<common::DatabaseTransactionInterface> transaction = database->begin_transaction();


Starting a transaction here and ending it at the end of the function. If anything throws in between, transaction will not be committed. I think that's the idea but I can of course also add a try/catch there. What do you think?

Pietfried

Looks good and works well! Added a few remarks

Pietfried · 2024-07-04T12:12:25Z

config/v201/device_model_migrations/1_up-initial.sql

  MUTABILITY_ID INTEGER,
  PERSISTENT INTEGER,
  CONSTANT INTEGER,
  TYPE_ID INTEGER,
+  VALUE_SOURCE TEXT DEFAULT "default",


I'd prefer to leave this empty and use config for the initialization (by the config file) because its more verbose. There might be variables in the database without a value for the variable attribute

doc/device_model_initialization.md

lib/ocpp/v201/charge_point.cpp

Pietfried · 2024-07-04T12:46:51Z

lib/ocpp/v201/device_model_storage_sqlite.cpp

+    if (init_db) {
+        if (db_path.empty() || migration_files_path.empty() || schemas_path.empty() || config_path.empty()) {
+            EVLOG_AND_THROW(
+                DeviceModelStorageError("Can not initialize device model storage: one of the paths is empty."));
+        }
+        InitDeviceModelDb init_device_model_db(db_path, migration_files_path);
+        init_device_model_db.initialize_database(schemas_path, false);
+        init_device_model_db.insert_config_and_default_values(schemas_path, config_path);
+    }


You could also remove this constructor and move the content inside the ChargePoint constructor and leave the other DeviceModelStorageSqlite constructor as is.

I personally dont like a constructor that relies only on a certain combination of arguments to be valid. This would also avoid this odd initialzation L45

Yes that constructor thing is a bit strange indeed
Problem is that I have to do the device model initialization before creating DeviceModelStorageSqlite.

I'll think about it (suggestions welcome)

I removed the odd looking constructor and added default parameters to the remaining constructor. Because we do so much code in the constructors, I don't know how else to do it, because like I said the code has to be called before the code in the chargepoint constructor is called, which creates a device model instance. But all the migration and inserting stuff should have been done at that point already.

Pietfried · 2024-07-04T13:49:53Z

include/ocpp/v201/init_device_model_db.hpp

+    /// \param r Second componentKey to compare with the other ComponentKey
+    /// \return True if 'l' is 'smaller' than r (should be placed before 'r').
+    ///
+    friend bool operator<(const ComponentKey& l, const ComponentKey& r);


Looks like using unordered_map(s) could be more suitable

Good point. I tried to change it but for an unordered_map you need a hash function.

lib/ocpp/v201/init_device_model_db.cpp

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

…e and use default values. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez · 2024-07-05T09:54:52Z

Looks good and works well! Added a few remarks

They should be fixed now. If possible.

include/ocpp/common/cistring.hpp

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

hikinggrass · 2024-07-05T12:03:00Z

config/v201/config.json

@@ -52,12 +52,6 @@
                    "Actual": true
                }
            },
-            "EVSEPower": {


Just wondering why these variables were removed?

They have been removed because they are not configuration but telemetry variables, so there is no reason to have them in the config.

include/ocpp/common/cistring.hpp

…oes otherwise not work. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

config/v201/CMakeLists.txt

Signed-off-by: Piet Gömpel <[email protected]>

maaikez added 12 commits June 5, 2024 18:01

Add files and start working on init device model in c++ instead of py…

dd4b760

…thon. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add function to add components and its properties etc to the database.

aedf06f

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Make more structs from json data.

b08d964

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Implement all to_json funcitons. Start with the 'insert config' funct…

0f03258

…ion. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Implement 'insert variable' and default values.

4d9c8b9

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Some changes to cmakelists to add the schemas to the correct location…

a0a9753

… (install directory) so it can be copied to the target. Move sql script to separate 'migration' folder. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Change some structs for already existing structs. Add functions to ch…

743b803

…eck if anything changed with components and variables and update variables. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

For Connector and / or EVSE Component: change characteristics and att…

66932c3

…ributes, delete variables. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Create test for init device model db class. Add flag to database open…

5348678

… call so it also works with in memory databases. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add doxygen documentation. Solme reshuffling of functions. Uncomment …

09207cf

…commented code. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add value source to Variable Attribute in database. Change interfaces…

472b247

… so the value source is / can be set when an attribute value is changed. Add test for insert config and default values and fix some bugs. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

hikinggrass self-assigned this Jun 24, 2024

maaikez added 2 commits June 28, 2024 10:52

Merge branch 'main' into feature/656-device-model-initialization-in-cpp

7403144

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Fix build and warnings from buildserver

d9b5b0e

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/656-device-model-initialization-in-cpp branch from 8637579 to d9b5b0e Compare June 28, 2024 12:21

maaikez added 2 commits June 28, 2024 16:38

Add exception and throw that specific exception from class. Change so…

29b33a0

…me functions to not return a boolean, because they just throw if something is going wrong. Add more verbose information to exception. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Fix unit tests and paths. Update unit test device model database.

dbe1291

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/656-device-model-initialization-in-cpp branch from 6b3e007 to dbe1291 Compare June 28, 2024 16:53

maaikez added 4 commits June 29, 2024 18:07

Get device model from database and check integrity of config file. Ad…

b2354f5

…d new tests. Fix almost all tests. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Clangformat. Fix failing smart charging unittest.

a7e1fd5

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add some documentation.

99f6e14

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Make cmake for device model migrations work. Do another todo (add war…

e5a500c

…ning when default value is an array or object). Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/656-device-model-initialization-in-cpp branch from f228821 to e5a500c Compare July 1, 2024 13:33

maaikez added 4 commits July 1, 2024 16:16

Remove unnecessary todo's

484aa3f

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Restrict removeing monitor config type if there is a variable monitor…

7645b2e

…ing foreign key pointing to it. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Add some documentation to the unittests.

8b52ae4

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Removed some todo's and outcommented code. Removed python script that…

e17c704

… does not work anymore. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez commented Jul 3, 2024

View reviewed changes

Split function so it is hopefully more readable.

1fcf825

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/656-device-model-initialization-in-cpp branch from 52809d4 to 1fcf825 Compare July 3, 2024 09:51

hikinggrass requested changes Jul 3, 2024

View reviewed changes

Review comments: readable error when database is old and does not sup…

2db7648

…port migrations yet. Fix some config / schema inconsistencies. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/656-device-model-initialization-in-cpp branch from a0216cd to 9ea7186 Compare July 3, 2024 15:31

Review comments

654a121

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez force-pushed the feature/656-device-model-initialization-in-cpp branch from 9ea7186 to 654a121 Compare July 3, 2024 15:42

maaikez added 3 commits July 3, 2024 17:45

Remove values from config that should not be set. Do not warn if a de…

d968c11

…fault config value could not be set because it has already changed by another source. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Change check_config_integrity so it returns a vector of strings with …

6fd006d

…components / variables / attributes that are not consistent with the database. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez commented Jul 4, 2024

View reviewed changes

Pietfried requested changes Jul 4, 2024

View reviewed changes

maaikez added 2 commits July 4, 2024 17:51

Review comments

a85d9cd

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Fix some warnings. Remove constructor from device model storage sqlit…

1fc6fed

…e and use default values. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

maaikez marked this pull request as ready for review July 5, 2024 09:53

maaikez requested a review from marcemmers as a code owner July 5, 2024 09:53

maaikez commented Jul 5, 2024

View reviewed changes

include/ocpp/common/cistring.hpp Show resolved Hide resolved

maaikez added 2 commits July 5, 2024 12:46

Merge main into feature branch

a67abf4

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Fix update config where there is no default source set.

2f424dc

Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

hikinggrass requested changes Jul 5, 2024

View reviewed changes

Move check for old database to a place before the migrations, as it d…

85e90cb

…oes otherwise not work. Signed-off-by: Maaike Zijderveld, iolar <[email protected]>

Pietfried reviewed Jul 6, 2024

View reviewed changes

config/v201/CMakeLists.txt Outdated Show resolved Hide resolved

Update config/v201/CMakeLists.txt

7697e88

Signed-off-by: Piet Gömpel <[email protected]>

Pietfried approved these changes Jul 6, 2024

View reviewed changes

hikinggrass approved these changes Jul 8, 2024

View reviewed changes

maaikez merged commit 981925c into main Jul 8, 2024
3 of 4 checks passed

Pietfried mentioned this pull request Jul 24, 2024

Device Model Database initialization: Move from python to C++ #656

Closed

the-bay-kay mentioned this pull request Oct 22, 2024

Simulator improvements EVerest/everest-demo#74

Closed

maaikez deleted the feature/656-device-model-initialization-in-cpp branch February 17, 2025 11:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/656 device model initialization in cpp #681

Feature/656 device model initialization in cpp #681

maaikez commented Jun 20, 2024 •

edited

Loading

maaikez Jul 3, 2024

hikinggrass Jul 4, 2024

maaikez Jul 5, 2024

hikinggrass Jul 3, 2024

maaikez Jul 3, 2024

hikinggrass Jul 4, 2024

hikinggrass Jul 3, 2024

maaikez Jul 3, 2024 •

edited

Loading

hikinggrass Jul 3, 2024

maaikez Jul 4, 2024

Pietfried left a comment

Pietfried Jul 4, 2024

Pietfried Jul 4, 2024

maaikez Jul 4, 2024 •

edited

Loading

maaikez Jul 5, 2024

Pietfried Jul 4, 2024

maaikez Jul 4, 2024

maaikez commented Jul 5, 2024

hikinggrass Jul 5, 2024

Pietfried Jul 6, 2024

		EVLOG_error << "Could not remove database " << database_path.u8string();
		throw InitDeviceModelDbError("Could not remove database " + database_path.u8string());

Feature/656 device model initialization in cpp #681

Feature/656 device model initialization in cpp #681

Conversation

maaikez commented Jun 20, 2024 • edited Loading

Describe your changes

Issue ticket number and link

Checklist before requesting a review

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

maaikez Jul 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pietfried left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maaikez Jul 4, 2024 • edited Loading

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

maaikez commented Jul 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maaikez commented Jun 20, 2024 •

edited

Loading

maaikez Jul 3, 2024 •

edited

Loading

maaikez Jul 4, 2024 •

edited

Loading