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

Expose mappings of requirements, provides and modules to modules #206

Merged
merged 25 commits into from
Oct 30, 2024

Conversation

hikinggrass
Copy link
Contributor

@hikinggrass hikinggrass commented Oct 6, 2024

EVerest modules and even individual interface implementations can have mappings
assigned to them. These mappings are inspired by the OCPP 3-tier model.

Modules can access the mapping information in the following ways depending on which specific information is required, all of these return an optional Mapping struct
If the mapping of a requirement is of interest it can be accessed via a get_mapping() function of a requirments:
r_name_of_the_requirement->get_mapping()

If the mapping of an interface implementation is of interest it can also be accessed via a get_mapping() function: p_name_of_an_implementation->get_mapping()

If the mapping of the current module is of interest it can be accessed via the module info:
this->info.mapping

Mapping information is also available in error reporting via "error.origin.mapping":

const auto error_handler = [this](const Everest::error::Error& error) {
    const auto evse_id = error.origin.mapping.has_value() ? error.origin.mapping.value().evse : 0;
};

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
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.

lgtm. Linting is currently failing

Tested with EVerest/everest-core#872

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Copy link
Contributor

@a-w50 a-w50 left a comment

Choose a reason for hiding this comment

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

I would like to request some documentation in the commit message about what has been implemented here.

if (mapping.has_value()) {
os << mapping.value();
} else {
os << "Mapping(charging station)";
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this default representation make any sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The feature (at the moment at least) is inspired by the 3-tier mappings from OCPP. So if you have no explicit mapping it's assumed that the module maps to the whole charging station, getting more specific by a mapping to an evse and/or connector

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Still I'm unsure, whether the string Mapping(charging station) on a text console will help users to understand what is meant.

include/utils/types.hpp Show resolved Hide resolved
include/utils/types.hpp Outdated Show resolved Hide resolved
@hikinggrass
Copy link
Contributor Author

I would like to request some documentation in the commit message about what has been implemented here.

Added to the PR description which will be in the squashed commit message, it's also already available in the documentation: https://everest.github.io/nightly/general/05_existing_modules.html#tier-module-mappings (the code snippets didn't render, should be fixed soon)

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Copy link
Contributor

@a-w50 a-w50 left a comment

Choose a reason for hiding this comment

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

See the comments, please address function signatures and if possible remove the mapping from the Requirement struct.

include/framework/ModuleAdapter.hpp Outdated Show resolved Hide resolved
include/framework/everest.hpp Outdated Show resolved Hide resolved
@@ -147,9 +147,9 @@ class Config {
json resolve_requirement(const std::string& module_id, const std::string& requirement_id) const;

///
/// \returns a list of Requirements for \p module_id
/// \returns a map of Fulfillments for \p module_id
Copy link
Contributor

Choose a reason for hiding this comment

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

I really doubt the value of these comments, you get exactly the same information if you look at the function signature.

include/utils/config.hpp Outdated Show resolved Hide resolved
include/utils/types.hpp Outdated Show resolved Hide resolved
lib/config.cpp Outdated Show resolved Hide resolved
lib/config.cpp Outdated Show resolved Hide resolved
Comment on lines 833 to 837
if (mapping.implementations.find(impl_id) == mapping.implementations.end()) {
return std::nullopt;
// if no specific implementation mapping is given, use the module mapping
return mapping.module;
}
return mapping.implementations.at(impl_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

This pattern happens quite often. It is usually better to use the iterator instead of doing the lookup twice:

const auto mapping_it = ...find(impl_id);
if (mapping_id == std::end(...)) {
  return default;
}
return *mapping_it;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess something like this would be quite readable:

const auto& mappings = module_tier_mappings.value();
const auto mapping_it = mappings.implementations.find(impl_id);
if (mapping_it == mappings.implementations.end()) {
    return mappings.module;
}
const auto& [key, mapping] = (*mapping_it);
return mapping;

The alternative doesn't quite feel as readable:

...
return (*mapping_it).second;

Or what do you think?

lib/everest.cpp Outdated Show resolved Hide resolved
lib/everest.cpp Outdated Show resolved Hide resolved
Functionality now provided via a free helper function with the same name

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
re-introduce get_requirements function

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
… a different type

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
lib/config.cpp Outdated Show resolved Hide resolved
lib/config.cpp Outdated Show resolved Hide resolved
include/utils/types.hpp Outdated Show resolved Hide resolved
lib/runtime.cpp Outdated Show resolved Hide resolved
…equirements and get_fulfillments

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
These can be used in (generated) user code to initialize requirements with their fulfillments and optional mappings

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
include/utils/types.hpp Show resolved Hide resolved
std::optional<Mapping> mapping;
};

using RequirementInitialization = std::map<std::string, std::vector<RequirementInitializer>>;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm uncertain with that name: RequirementInitialization sounds more like the result of the initialization of a requirement than a mapping from each requirement name to its initializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm open for suggestions 😄 that was the best I could come up with at the time 😅

lib/config.cpp Outdated Show resolved Hide resolved
lib/config.cpp Outdated Show resolved Hide resolved
lib/config.cpp Show resolved Hide resolved

for (const auto& [requirement, fulfillment] : this->resolve_requirements(module_id)) {
const auto& mapping = this->get_3_tier_model_mapping(fulfillment.module_id, fulfillment.implementation_id);
res[requirement.id].push_back({requirement, fulfillment, mapping});
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit tricky. We implicitly rely on the fact, that requirements are sorted within the map according to their index. This should be guaranteed, but still is not obvious to the reader. Having the requirement without it's index, but instead pointing at a vector of Fulfillments would probably make the code even simpler.

include/utils/types.hpp Outdated Show resolved Hide resolved
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass force-pushed the feature/expose-mappings branch 2 times, most recently from f0cd79b to 2f45d45 Compare October 29, 2024 10:12
Now all mappings are defined under a "mapping" key that contains a "module" and/or "implementations" key which in turn can have mappings for individual implementations defined

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass merged commit bb3d3a9 into main Oct 30, 2024
4 checks passed
@hikinggrass hikinggrass deleted the feature/expose-mappings branch October 30, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants