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

Add simple EvAPI and extend EvManager with simplistic SoC calculation #891

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

hikinggrass
Copy link
Contributor

Describe your changes

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

Signed-off-by: Kai-Uwe Hermann <[email protected]>
…nnectors"

This allows both the API and EvAPI modules to run simultaneously

Signed-off-by: Kai-Uwe Hermann <[email protected]>
This allows an EV implementation to report more detailed information such as SoC

Signed-off-by: Kai-Uwe Hermann <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
@hikinggrass hikinggrass marked this pull request as ready for review November 6, 2024 19:07
@SebaLukas
Copy link
Contributor

Maybe we can combine the new ev_manager interface with the existing car_simulator interface. I see no problem with moving the cmds and var from the car_simulator interface to the ev_manager interface.

Copy link
Contributor

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

In general, things are looking good. See my two comments.

modules/EvAPI/EvAPI.hpp Outdated Show resolved Hide resolved
modules/EvAPI/EvAPI.hpp Outdated Show resolved Hide resolved
modules/EvAPI/EvAPI.cpp Outdated Show resolved Hide resolved

std::list<std::unique_ptr<EvSessionInfo>> info;

const std::string api_base = "everest_api/";
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps make this a static inline constexpr std::string_view

modules/EvAPI/EvAPI.cpp Outdated Show resolved Hide resolved
Comment on lines 48 to 60
std::string var_base = ev_base + "/var/";

std::string var_ev_info = var_base + "ev_info";
ev->subscribe_ev_info([this, &ev, var_ev_info](types::evse_manager::EVInfo ev_info) {
json ev_info_json = ev_info;
this->mqtt.publish(var_ev_info, ev_info_json.dump());
});

std::string var_session_info = var_base + "session_info";
ev->subscribe_bsp_event([this, var_session_info, &session_info](const auto& bsp_event) {
session_info->update_state(types::board_support_common::event_to_string(bsp_event.event));
this->mqtt.publish(var_session_info, *session_info);
});
Copy link
Member

Choose a reason for hiding this comment

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

I think it could improve readability if those blocks are refactored to static functions like publish_ev_info and publish_session_info

modules/EvManager/main/car_simulation.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulation.cpp Outdated Show resolved Hide resolved
modules/EvManager/main/car_simulation.hpp Outdated Show resolved Hide resolved
Signed-off-by: Kai-Uwe Hermann <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
This is was only used for AC, where we now get it via the config

Signed-off-by: Kai-Uwe Hermann <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
…s cmd the EvManager goes to state ISO_CHARGING_REGULATED and then the new simulate_soc() works also for DC

Signed-off-by: Sebastian Lukas <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
@hikinggrass
Copy link
Contributor Author

Maybe we can combine the new ev_manager interface with the existing car_simulator interface. I see no problem with moving the cmds and var from the car_simulator interface to the ev_manager interface.

I can open a follow-up PR for that

if (not state_has_changed and
(sim_data.state == SimState::CHARGING_REGULATED or sim_data.state == SimState::CHARGING_FIXED or
sim_data.state == SimState::ISO_CHARGING_REGULATED)) {
simulate_soc();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this should already be triggered at startup, right now the ev_info is only published when a charging session is ongoing, but the SoC information is available all the time

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think you could do this and only sim_data.battery_charge_wh will be increased during charging.

@SirVer
Copy link
Contributor

SirVer commented Nov 21, 2024

I am not sure why I was added into this code review. @hikinggrass This does not seem to touch Rust or Bazel?

@hikinggrass
Copy link
Contributor Author

I am not sure why I was added into this code review. @hikinggrass This does not seem to touch Rust or Bazel?

Doesn't touch rust or bazel, review request were triggered by accident from a botched merge of main into this branch. I've removed the accidentally added reviewers again

Copy link
Contributor

@SebaLukas SebaLukas left a comment

Choose a reason for hiding this comment

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

👍

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.

6 participants