Add new OPAL-RT Orchestra node-type

[stv0g]

This PR is adding a new node-type for improving the interface between VILLASnode and OPAL-RT software platforms using OPAL-RT Orchestra Co-simulation framework.

We are using this new node-type both in the ENSURE and SEGuRo projects.

See:

• https://opal-rt.atlassian.net/wiki/spaces/PRD/pages/505348099/Orchestra+I+O
• https://opal-rt.atlassian.net/wiki/spaces/PRD/pages/143626248/Orchestra
• https://opal-rt.atlassian.net/wiki/spaces/PRD/pages/143787939/RT-LAB+API+Orchestra

[n-eiling]

Avoid exception

[al3xa23]

Hi, nice work. I added some minor comments since I am not yet that familiar with code reviews.
General question: I assume the tests are following in the next PR?

[stv0g]

Hi @al3xa23,

General question: I assume the tests are following in the next PR?

Good point. I dont think we can test this node-type via an integration test unfortunately, as it depends on an Orchestra framework which is provided by an OPAL-RT real-time simulation model. As this is closed-source software, we can not simply include it into our CI tests.

Do you think this qualifies for an exception?

[al3xa23]

Ah okay, I understand. It is fine from my side :)

[n-eiling]

Would be interesting to know what the license states specifically. Can we even publish this as open source? If yes, then at least the header needs to be open source as well, right? Or is there some clause specifying terms for use as a library?

[stv0g]

Would be interesting to know what the license states specifically. Can we even publish this as open source? If yes, then at least the header needs to be open source as well, right? Or is there some clause specifying terms for use as a library?

The libOpalOrchestra library and its header which are not licensed under an open-source license. This contribution includes neither the library itself or its header.

It merely uses it by dynamically linking against it.

I do not see an issue here as we are licensing the node-type itself (this contribution) under a permissive OSS license (Apache-2.0).

[stv0g]

@n-eiling I have addressed most your comments. I only kept using exceptions for now. Which is something I would like to reconsider and also discuss among other contributors (@pjungkamp, @windrad6).

Are you fine with the changes so far (ignoring the exceptions for now)?

[stv0g]

@al3xa23 Would you mind doing another round of review, and/or approving the PR if you are fine with it?

[pjungkamp]

Just a small nitpick from my side. You're using std::map which requires all entries to be sorted by the key when iterating. This leads most implementations to implement this as e.g. a red-black tree. See https://en.cppreference.com/w/cpp/container/map.html.

I'd prefer to see at least a std::unordered_map here, which is a hash table, since you don't seem to rely on the key ordering in std::map. See https://en.cppreference.com/w/cpp/container/unordered_map.html.

The tree structure of std::map leads to memory access patterns that are hostile to CPU caches for almost everything from lookup to traversal and should almost never be used.

[pjungkamp]

And another nitpick.

Please don't use const std::string & in new code. This much better served by either a plain std::string if you intend to store it somewhere (e.g. in constructors) or by a std::string_view if you're just inspecting/viewing it but don't want to own it.

---

example

class SomeItem {
private:
  std::string some_string;

public:
  explicit SomeItem(std::string some_string) : some_string(std::move(some_string)) {}
}

[pjungkamp]

Suggested change

	DataItem(const std::string &name) : name(name) {}
	explicit DataItem(std::string name) : name(std::move(name)) {}

[pjungkamp]

Suggested change

	BusItem(const std::string &name) : items(), name(name) {}
	explicit BusItem(std::string name) : items(), name(std::move(name)) {}

[pjungkamp]

Suggested change

	DataSet(const std::string &name) : items(), name(name) {}
	explicit DataSet(std::string name) : items(), name(std::move(name)) {}

[pjungkamp]

Suggested change

	std::shared_ptr<DataItem> DataSet::upsertItem(const std::string &path,
	bool &inserted) {
	return upsertItem(split(path), inserted);
	}
	std::shared_ptr<DataItem>
	DataSet::upsertItem(std::vector<std::string> pathComponents, bool &inserted) {
	auto &name = pathComponents.front();
	auto it = items.find(name);
	if (it == items.end()) { // No item with this name exists. Create a new one.
	if (pathComponents.size() == 1) { // No bus, just a signal.
	auto item = std::make_shared<DataItem>(name);
	items.emplace(name, item);
	inserted = true;
	return item;
	} else {
	auto bus = std::make_shared<BusItem>(name);
	items.emplace(name, bus);
	pathComponents.erase(pathComponents.begin());
	return bus->upsertItem(pathComponents, inserted);
	}
	} else {
	if (pathComponents.size() == 1) {
	auto item = std::dynamic_pointer_cast<DataItem>(it->second);
	if (!item) {
	throw RuntimeError("Item with name '{}' is not a data item",
	pathComponents.front());
	}
	inserted = false;
	return item;
	} else {
	// Item with this name exists. Check if it is a bus.
	auto bus = std::dynamic_pointer_cast<BusItem>(it->second);
	if (!bus) {
	throw RuntimeError("Item with name '{}' is not a bus",
	pathComponents.front());
	}
	pathComponents.erase(pathComponents.begin());
	return bus->upsertItem(pathComponents, inserted);
	}
	}
	}
	std::shared_ptr<DataItem>
	DataSet::upsertItem(std::string_view path, bool &inserted) {
	auto separator = path.find('/');
	auto name = std::string(path.substr(0, separator));
	auto it = items.find(name);
	if (it == items.end()) { // No item with this name exists. Create a new one.
	if (separator == std::string_view::npos) { // No bus, just a signal.
	auto item = std::make_shared<DataItem>(name);
	items.emplace(std::move(name), item);
	inserted = true;
	return item;
	} else {
	auto bus = std::make_shared<BusItem>(name);
	items.emplace(std::move(name), bus);
	return bus->upsertItem(path.substr(separator + 1), inserted);
	}
	} else {
	if (separator == std::string_view::npos) {
	auto item = std::dynamic_pointer_cast<DataItem>(it->second);
	if (!item) {
	throw RuntimeError("Item with name '{}' is not a data item", name);
	}
	inserted = false;
	return item;
	} else {
	// Item with this name exists. Check if it is a bus.
	auto bus = std::dynamic_pointer_cast<BusItem>(it->second);
	if (!bus) {
	throw RuntimeError("Item with name '{}' is not a bus", name);
	}
	return bus->upsertItem(path.substr(separator + 1), inserted);
	}
	}
	}

Why did you create that std::vector<std::string> overload for the path components? I don't see it being used anywhere.

This version does not allocate any memory unless explicitly requested by a std::string constructor without being more complicated. You can also remove the split function at the top of the file.

Of course it's just a suggestion. It's also not in a hot path, so the current version would also be fine. I just wanted to do some advertisement for std::string_view.