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

Cleanup/remove js slac yeti #887

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

corneliusclaussen
Copy link
Contributor

Describe your changes

Removes JS versions of slac and yeti SIL as they have been replaced with C++ versions.
Depends on PR #862 which needs to be merged before.

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code
  • I have made corresponding changes to the documentation
  • I read the contribution documentation and made sure that my changes meet its requirements

MarzellT and others added 2 commits October 2, 2024 12:22
Signed-off-by: MarzellT <tobias.marzell@pionix.de>
Signed-off-by: Cornelius Claussen <cc@pionix.de>
Comment on lines +14 to +15
PowermeterData();
int64_t time_stamp;
Copy link
Contributor

Choose a reason for hiding this comment

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

The PowermeterData needs a custom constructor only because of the int64_t timestamp. In general it is best practice to leave these POD type structs with all their default constructors etc.
What could be done here is to create a TimeStamp class, which by itself has a default constructor that does exactly what right now the PowermeterData constructor is doing. This Timestamp class could have a conversion operator to int64_t or a get_value method (it could also be templated on the underlying data type).
This way the responsibility of construction is moved to the correct place.

// SPDX-License-Identifier: Apache-2.0
// Copyright Pionix GmbH and Contributors to EVerest

#pragma once
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistency, some files use #ifdef .., some use #pragma once

void MqttHandler::handle_mqtt_payload(const std::string& payload) {
const auto e = nlohmann::json::parse(payload);

const auto raise = e["raise"] == "true";
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that e["raise"] will implicitly create a raise key if it does not exists and then you're relying on its behavior that it defaults to false. For accessing elements, e.at("raise") is usually always the best thing to do.
Furthermore, the pattern here can be implemented easier via const auto raise = e.value("raise", false);

Comment on lines +295 to +303
void ErrorHandler::error_lock_ConnectorLockUnexpectedClose(bool raise) {
if (raise) {
const auto error = p_board_support->error_factory->create_error("connector_lock/ConnectorLockUnexpectedClose",
"", "Simulated fault event", Severity::High);
p_connector_lock->raise_error(error);
} else {
p_connector_lock->clear_error("connector_lock/ConnectorLockUnexpectedClose");
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these functions look the same, therefore it might be helpful to figure out if this can be generalized.

  • the error object is created in every function, it is probably better to create that error function beforehand and pass that to the generic function
  • the error type connector_lock/ConnectorLockUnexpectedClose" is used twice as a raw string literal (this might lead to copy paste etc. errors, better use error.type the second time
  • the only thing left in these functions is the specific handle for raising/clearing the error like p_connector_lock, this would need to be templated

this would yield a generic function like

template <typename HandleType> void forward_error(HandleType& handle, const Error& error, bool raise) {
  if (raise) {
    handle->raise_error(error);
  } else {
    handle->clear_error(error.type);
  }
}

Next thing that is somehow generic, is the create_error call. You could create a custom struct like:

struct ErrorDefintion {
  const char* type;
  const char* sub_type {""};
  const char* message {""};
  Everest::Error::Severity {Severity::High};
};

With this you can predefine all of these errors like

namespace error_definitions {
inline const auto connector_lock_unexpected_close = ErrorDefinition{"connector_lock/ConnectorLockUnexpectedClose"};
...
}

With this you're able to create a helper function like

auto create_error(Everest::error::ErrorFactory& error_factory, const ErrorDefinition& def) {
  return error_factory.create_error(def.type, def.sub_type, def.message, def.severity);
}

Now you can turn your handle_mqtt_payload into a function which returns for example a tuple like

std::tuple<bool, ErrorDefinition> parse_error_type(const json& data) {
  // parsing, getting raise
  const std::string& error_type = e.at("error_type");
  if (error_type == "lock_ConnectorLockUnexpectedClose") {
    return {raise, error_definitions::connector_lock_unexpected_close};
  } else if ... {
    //
  } else {
    // this case either needs to use exceptions or you need an return type which encapsulates the information that no error was found
  }
};

With all of this setup, the main flow should be now like:

const auto [raise, error_definition] = parse_error_type(json_data);
const auto error = create_error(p_board_support->error_factory, error_definition);

forward_error(handle, error_definition, raise);

Note, I forgot, that handle also needs to be deduced. Probably the ErrorDefinition struct needs to be enhanced by an enum class like error_destination (although the routing isn't really of this PODs concern), having items like ConnectorLock, RCD etc. This way you can deduce the correct handle.

Note that all of the above functions are free functions and hence can be easily tested. There is also much less code duplication (the templates) and more cohesion (the definition of the various error types).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These comments are for PR #862. This PR is just a draft based on the 862 branch to delete the JS versions once the C++ version from 862 is merged. So no code changes should end up here.

Copy link
Contributor Author

@corneliusclaussen corneliusclaussen Oct 14, 2024

Choose a reason for hiding this comment

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

Once 862 is merged this needs to be rebased again

@a-w50 a-w50 mentioned this pull request Oct 13, 2024
3 tasks
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