Skip to content

Fix(slac): slac inline enum declaration#1898

Open
ThatsLucas wants to merge 1 commit intoEVerest:mainfrom
ThatsLucas:add-slac-types
Open

Fix(slac): slac inline enum declaration#1898
ThatsLucas wants to merge 1 commit intoEVerest:mainfrom
ThatsLucas:add-slac-types

Conversation

@ThatsLucas
Copy link
Contributor

@ThatsLucas ThatsLucas commented Feb 27, 2026 •
edited
Loading

Added types/slac.yaml to fix #1323 - The SLAC interface uses the unsupported inline enum declaration

Updated everything to work with the new types.

Describe your changes

Issue ticket number and link

#1323

Checklist before requesting a review

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

@cburandt cburandt self-assigned this Feb 27, 2026
mhei
mhei reviewed Feb 27, 2026
View reviewed changes
modules/Simulation/SlacSimulator/evse/slacImpl.cpp Outdated

void slacImpl::ready() {
publish_state(state_to_string(state));
publish_state(types::slac::string_to_state(state_to_string(state)));
Copy link
Contributor

@mhei mhei Feb 27, 2026

Choose a reason for hiding this comment

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

Converting forth and back again to string look complicated. Maybe we should get rid of the utils/state.{c,hpp} in SlacSimulator completely now that we have a nice type?

Copy link
Contributor Author

@ThatsLucas ThatsLucas Feb 27, 2026

Choose a reason for hiding this comment

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

hmm yea you'r right, i'll take a look at this

Copy link
Contributor Author

@ThatsLucas ThatsLucas Mar 2, 2026

Choose a reason for hiding this comment

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

It is better now ?

@ThatsLucas ThatsLucas force-pushed the add-slac-types branch 2 times, most recently from 4f4c3cc to 2594854 Compare February 28, 2026 00:47
SebaLukas
SebaLukas requested changes Mar 2, 2026
View reviewed changes
Copy link
Member

@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.

Looking good 👍
See my comments

modules/EV/EvManager/main/car_simulation.hpp Outdated
Comment on lines +77 to +79
void set_slac_state(const std::string& slac_state) {
sim_data.slac_state = slac_state;
}
Copy link
Member

@SebaLukas SebaLukas Mar 2, 2026

Choose a reason for hiding this comment

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

Why did you change std::move to a const ref?

Copy link
Contributor Author

@ThatsLucas ThatsLucas Mar 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I saw that the function was not modifying slac_state so i added const ref.
And from what i understand its not logical to move a const value.

If you think it was better before i can change it back :p

Copy link
Member

@SebaLukas SebaLukas Mar 2, 2026

Choose a reason for hiding this comment

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

In the previous function defintion it was not a const ref.
So I think move a std::string with std::move is valid. So please revert it back 👍

Copy link
Contributor Author

@ThatsLucas ThatsLucas Mar 2, 2026

Choose a reason for hiding this comment

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

I put the old definition back as it was. But about std::move im not sure how to do it.
Now that im catching a throw, i need slac_state in EVLOG_error and at this time, it might be empty with an std::move.

modules/EV/EvManager/main/car_simulation.hpp Outdated
}

void set_slac_state(types::slac::State slac_state) {
sim_data.slac_state = types::slac::state_to_string(slac_state);
Copy link
Member

@SebaLukas SebaLukas Mar 2, 2026

Choose a reason for hiding this comment

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

If we have now this nice types::slac::State type, then it should be used as well in the EvManager and not a std::string.

Copy link
Contributor Author

@ThatsLucas ThatsLucas Mar 2, 2026

Choose a reason for hiding this comment

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

Yup you'r right gonna do that shortly

Copy link
Contributor Author

@ThatsLucas ThatsLucas Mar 2, 2026

Choose a reason for hiding this comment

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

Should be good now

modules/EVSE/EvseSlac/main/slacImpl.cpp Outdated
callbacks.signal_dlink_ready = [this](bool value) { publish_dlink_ready(value); };

callbacks.signal_state = [this](const std::string& value) { publish_state(value); };
callbacks.signal_state = [this](const std::string& value) { publish_state(types::slac::string_to_state(value)); };
Copy link
Member

@SebaLukas SebaLukas Mar 2, 2026

Choose a reason for hiding this comment

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

My only problem with using string_to_state() that catching an exception is not handled here. As well in EvSlac module.

Copy link
Contributor Author

@ThatsLucas ThatsLucas Mar 2, 2026

Choose a reason for hiding this comment

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

I didn't thought of that, now im catching it.

I just have a question here :

        try {
            publish_state(types::slac::string_to_state(value));
        } catch (const std::exception& e) {
            EVLOG_error << fmt::format("Tried to publish unknown SLAC state '{}'. Error: {}", value, e.what());
            publish_dlink_ready(false);
        }

Should i keep publishing dlink false ? I guess it should never happen but just to be sure

cburandt
cburandt requested changes Mar 2, 2026
View reviewed changes
@ThatsLucas ThatsLucas force-pushed the add-slac-types branch 2 times, most recently from 4afb301 to 99fe7e7 Compare March 2, 2026 13:23
cburandt
cburandt requested changes Mar 3, 2026
View reviewed changes
Copy link
Contributor

@cburandt cburandt left a comment

Choose a reason for hiding this comment

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

The code in wrapper.[hc]pp looks almost fine. The exception message texts where sort of mixed up, but that was my bad, since the example I pointed you at, had the same confusion. If you apply my suggestions below, this will be fixed.

@ThatsLucas
Update SLAC state handling.
f3fb931 
Added types/slac.yaml to fix EVerest#1323 - The SLAC interface uses the unsupported inline enum declaration

Updated everything to work with the new types.

Signed-off-by: ThatsLucas <lucasmailpro9@gmail.com>
@ThatsLucas
Copy link
Contributor Author

ThatsLucas commented Mar 3, 2026

Do i have to do something about Codacy ?

@barsnick
Copy link
Contributor

barsnick commented Mar 3, 2026

Do i have to do something about Codacy ?

These checks?
https://github.com/EVerest/everest-core/pull/1898/checks?check_run_id=65529027064

No, you cannot fix those, and they do not block the PR (unlike other failing checks).

@ThatsLucas
Copy link
Contributor Author

ThatsLucas commented Mar 3, 2026

ok great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mhei mhei mhei left review comments

@SebaLukas SebaLukas SebaLukas requested changes

@cburandt cburandt cburandt requested changes

@Pietfried Pietfried Awaiting requested review from Pietfried Pietfried is a code owner

@corneliusclaussen corneliusclaussen Awaiting requested review from corneliusclaussen corneliusclaussen is a code owner

@james-ctc james-ctc Awaiting requested review from james-ctc james-ctc is a code owner

@mlitre mlitre Awaiting requested review from mlitre mlitre is a code owner

@a-w50 a-w50 Awaiting requested review from a-w50 a-w50 is a code owner

@hikinggrass hikinggrass Awaiting requested review from hikinggrass hikinggrass is a code owner

@djchhp djchhp Awaiting requested review from djchhp djchhp is a code owner

@florinmihut florinmihut Awaiting requested review from florinmihut florinmihut is a code owner

@andistorm andistorm Awaiting requested review from andistorm andistorm is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@cburandt cburandt

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The SLAC interface uses the unsupported inline enum declaration

5 participants

@ThatsLucas @barsnick @mhei @cburandt @SebaLukas