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

Bug/729 add remove variables and components from schema for device model #740

Conversation

maaikez
Copy link
Contributor

@maaikez maaikez commented Aug 8, 2024

Describe your changes

With this pull request, it is possible to remove and add variables (and their attributes and characteristics) and components for all other components than only EVSE and Connector.

It also removed the use of config.json, so everything is now added to the component schema's, which are renamed to component config. The values from the 'default config' are now added to the new 'component config' json files.

There is one TODO left in the ocpp_types file. If the changes are ok there, I can also change the generator to do exactly that. Otherwise I have to solve it another way.

I also made a pull request in everest core (EVerest/everest-core#822) and are still busy with some changes in everest utils (https://github.com/EVerest/everest-utils/tree/feature/715-combine-component-schemas-and-config-in-one-file).

Issue ticket number and link

#729
#715

Checklist before requesting a review

…only evse and connector.

Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
…ymore but only schema's, in which the config values are set.

Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
…that are required have a (default) value set.

Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
…rity checks.

Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
… type in for loops. Fix schema's for unit tests.

Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
@Pietfried Pietfried self-assigned this Aug 12, 2024
Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
Copy link
Contributor

@Pietfried Pietfried left a comment

Choose a reason for hiding this comment

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

Looks nice. Just added a couple of suggestions. Make sure to update the dependency in EVerest/everest-core#822 once this is merged

Comment on lines 1211 to 1212
/// or
/// changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// or
/// changed.
/// or changed.

@@ -1866,9 +1866,28 @@ void from_json(const json& j, VariableAttribute& k) {
if (j.contains("type")) {
k.type.emplace(conversions::string_to_attribute_enum(j.at("type")));
}

// TODO mz change this in the generator (if everyone agrees)!!
Copy link
Contributor

Choose a reason for hiding this comment

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

You mean in the generate_component_config.py right? Not sure if this still works and if we still need it. It was based on a modified version of the dm_component_vars.csv that we haven't made public due to uncertainty about the license

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you don't generate it anymore?
(I just want to be sure it is not removed when the file is regenerated)

software, it will:
- Check if there are Components in the database that are not in the component config's. Those will be removed.
- Check if there are Components in the component config's that are not in the database. Those will be added.
- Check if anything has changed inside the Component (`Variable`, `Characteristics` or `Attributes`).
Those will be removed, changed or added. Those will be removed, changed or added to the database as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Those will be removed, changed or added. Those will be removed, changed or added to the database as well.
Those will be removed, changed or added to the database as well.

Comment on lines 498 to 500
if (attribute.value.has_value() ||
(attribute.type.has_value() && (attribute.type.value() == AttributeEnum::Actual) &&
default_actual_value.has_value())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could put this into a separate function so this reads a little better. You could also reusue it within L604

Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
Signed-off-by: Maaike Zijderveld, iolar <git.mail@iolar.nl>
@maaikez maaikez force-pushed the bug/729-add-remove-variables-and-components-from-schema-for-device-model branch from 4a5626a to fa11168 Compare August 19, 2024 09:45
@maaikez maaikez merged commit 47eee62 into main Aug 19, 2024
3 of 4 checks passed
@maaikez maaikez deleted the bug/729-add-remove-variables-and-components-from-schema-for-device-model branch August 19, 2024 09:56
@acnixvkh
Copy link

acnixvkh commented Sep 9, 2024

@maaikez, I encountered some difficulty adapting my configuration when changes from this pull request appeared in my copy of EVerest. Although it is working again, I am unsure if my approach is correct and would appreciate your confirmation.

Scenario: I want to change a value defined in the standardized schemas or define a value for a variable that has no default value in the standardized schemas. To do this, I create a new schema in the sub-directory component_config/custom/. This new schema repeats much of the code from the original schema in component_config/standardized/ with only the "value" element added or changed.

Example: To set ContractCertificateInstallationEnabled from component_config/standardized/ISO15118Ctrlr.json to true, I create component_config/custom/config.json with the following content:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "name": "ISO15118Ctrlr",
  "properties": {
    "ContractCertificateInstallationEnabled": {
      "variable_name": "ContractCertificateInstallationEnabled",
      "characteristics": {
        "supportsMonitoring": true,
        "dataType": "boolean"
      },
      "attributes": [
        {
          "type": "Actual",
          "mutability": "ReadWrite",
          "value": true
        }
      ],
      "description": "If this variable is true, then ISO 15118 contract certificate installation/update as described by use case M01 - Certificate installation EV and M02 - Certificate Update EV is enabled. If this variable is false, then ISO 15118 contract certificate installation/update as described by use case M01 - Certificate installation EV and M02 - Certificate Update EV is disabled.",
      "type": "boolean"
    }
  }
}

Question: Is this the correct approach to adapt configurations in libocpp, or is there a more efficient method to avoid repeating large portions of code?

@maaikez
Copy link
Contributor Author

maaikez commented Sep 9, 2024

@acnixvkh Ah never thought of this way. No you should just change the ISO15118Ctrlr.json and set the correct value there. Although this might just work, but I am not sure.

The default values are confusing, I will remove them in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants