Enable compile warnings (#728)

* CMake: add compiler warnings

Currently, no compiler warnings are active at all, which is absolutely not
recommended. Warnings can uncover bugs, but also wrong intentions.

Add "-Wall", but silence warnings for unused functions, as the generated
interface implementations are full of them, as they serve as libraries.

* API: avoid warnings

Re-order an initialization list.

* Auth: avoid warnings

Re-order an initialization list.
Drop assignment of an unused return value.

* DPM1000: avoid warnings

Drop assignment of an unused return value.

* EvseManager: avoid warnings

Re-order an initialization list.

* Example, ExampleUser: avoid warnings by regenerating using ev-cli

* OCPP201: hint at enum warning causes

* OCPPExtensionExample: avoid warnings

* PacketSniffer: avoid warnings by regenerating using ev-cli

* PersistentStore: avoid warnings by regenerating using ev-cli

* PN532TokenProvider: avoid warnings by regenerating using ev-cli

* PN532TokenProvider: avoid warnings

* Setup: avoid warnings

Drop unused variable.

* Store: avoid warnings by regenerating using ev-cli

* lib/staging/slac: avoid warnings by using modern iterator

Signed-off-by: Moritz Barsnick <[email protected]>

* Add EVEREST_COMPILE_OPTIONS list that is passed to the module targets if EVEREST_ENABLE_COMPILE_WARNINGS is set to "ON"

By default EVEREST_ENABLE_COMPILE_WARNINGS is set to OFF

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

* EnergyManager: avoid warnings

Re-order an initialization list.
Use suggested brackets.
Use valid loop index types.

* EnergyNode: avoid warnings

Use suggested brackets.

* SerialCommHub: avoid warnings

Fix incorrect intent of `<<` stream vs. fmt::format.

* ExampleErrorRaiser: avoid warnings

Use valid loop index types.

Signed-off-by: Moritz Barsnick <[email protected]>

* Add EVEREST_ENABLE_GLOBAL_COMPILE_WARNINGS option

With this option (set to OFF by default) you can globally enable the compile warnings set in the EVEREST_COMPILE_OPTIONS variable

* Handle Unknown SessionInfo State in API module

* Explicitly fallthrough SessionEventEnums in AuthHandler

These are not handled by design

* Handle MutexDescription Undefined in EvseManager scoped_lock to_string()

* Iterate over tx_start_stop_points with const ref

* Enable compile warnings in compile.sh

* Fix some more compiler warnings

* Add missing newline

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

---------

Signed-off-by: Moritz Barsnick <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
Co-authored-by: Kai-Uwe Hermann <[email protected]>
Co-authored-by: Piet Gömpel <[email protected]>

.ci/build-kit/compile.sh

@@ -10,6 +10,7 @@ cmake \
     -DISO15118_2_GENERATE_AND_INSTALL_CERTIFICATES=OFF \
     -DCMAKE_INSTALL_PREFIX="$EXT_MOUNT/dist" \
     -DWHEEL_INSTALL_PREFIX="$EXT_MOUNT/dist-wheels" \
-    -DBUILD_TESTING=ON
+    -DBUILD_TESTING=ON \
+    -DEVEREST_ENABLE_COMPILE_WARNINGS=ON
 
 ninja -j$(nproc) -C "$EXT_MOUNT/build"

CMakeLists.txt

@@ -42,6 +42,14 @@ option(ISO15118_2_GENERATE_AND_INSTALL_CERTIFICATES "Automatically generate and
 option(EVEREST_ENABLE_RUN_SCRIPT_GENERATION "Enables the generation of run scripts (convenience scripts for starting available configurations)" ON)
 option(${PROJECT_NAME}_BUILD_TESTING "Build unit tests, used if included as dependency" OFF)
 option(BUILD_TESTING "Build unit tests, used if standalone project" OFF)
+option(EVEREST_ENABLE_COMPILE_WARNINGS "Enable compile warnings set in the EVEREST_COMPILE_OPTIONS flag" OFF)
+option(EVEREST_ENABLE_GLOBAL_COMPILE_WARNINGS "Enable compile warnings set in the EVEREST_COMPILE_OPTIONS flag globally" OFF)
+# list of compile options that are passed to modules if EVEREST_ENABLE_COMPILE_WARNINGS=ON
+# generated code has functions often not used
+set(EVEREST_COMPILE_OPTIONS "-Wall;-Wno-unused-function" CACHE STRING "A list of compile options used for building modules")
+if(EVEREST_ENABLE_GLOBAL_COMPILE_WARNINGS)
+    add_compile_options(${EVEREST_COMPILE_OPTIONS})
+endif()
 if((${CMAKE_PROJECT_NAME} STREQUAL ${PROJECT_NAME} OR ${PROJECT_NAME}_BUILD_TESTING) AND BUILD_TESTING)
     set(EVEREST_CORE_BUILD_TESTING ON)
 endif()

cmake/everest-generate.cmake

@@ -535,6 +535,13 @@ function (ev_add_cpp_module MODULE_NAME)
                     ${ATOMIC_LIBS}
             )
 
+            if(EVEREST_ENABLE_COMPILE_WARNINGS)
+                message(STATUS "Building ${MODULE_NAME} with the following compile options: ${EVEREST_COMPILE_OPTIONS}")
+                target_compile_options(${MODULE_NAME}
+                    PRIVATE ${EVEREST_COMPILE_OPTIONS}
+                )
+            endif()
+
             add_dependencies(${MODULE_NAME} generate_cpp_files)
 
             install(TARGETS ${MODULE_NAME}

lib/staging/slac/fsm/ev/src/states/others.cpp

@@ -57,8 +57,8 @@ void InitSlacState::enter() {
     std::mt19937 rng(rnd_dev());
     std::uniform_int_distribution<std::mt19937::result_type> dist256(0, 255);
 
-    for (auto i = 0; i < sizeof(run_id); ++i) {
-        run_id[i] = dist256(rng);
+    for (auto& id : this->run_id) {
+        id = dist256(rng);
     }
 }
 

lib/staging/slac/fsm/ev/src/states/sounding.cpp

@@ -103,8 +103,8 @@ bool SoundingState::do_sounding() {
         std::mt19937 rng(rnd_dev());
         std::uniform_int_distribution<std::mt19937::result_type> dist256(0, 255);
 
-        for (auto i = 0; i < sizeof(msg.random); ++i) {
-            msg.random[i] = dist256(rng);
+        for (auto& random : msg.random) {
+            random = dist256(rng);
         }
 
         ctx.send_slac_message(session_parameters.evse_mac, msg);

modules/API/API.cpp

@@ -9,12 +9,12 @@ namespace module {
 static const auto NOTIFICATION_PERIOD = std::chrono::seconds(1);
 
 SessionInfo::SessionInfo() :
-    state(State::Unknown),
     start_energy_import_wh(0),
     end_energy_import_wh(0),
-    latest_total_w(0),
     start_energy_export_wh(0),
-    end_energy_export_wh(0) {
+    end_energy_export_wh(0),
+    latest_total_w(0),
+    state(State::Unknown) {
     this->start_time_point = date::utc_clock::now();
     this->end_time_point = this->start_time_point;
 
@@ -146,6 +146,8 @@ void SessionInfo::update_state(const types::evse_manager::SessionEventEnum event
 
 std::string SessionInfo::state_to_string(SessionInfo::State s) {
     switch (s) {
+    case SessionInfo::State::Unknown:
+        return "Unknown";
     case SessionInfo::State::Unplugged:
         return "Unplugged";
     case SessionInfo::State::Disabled:

modules/Auth/Auth.cpp

@@ -18,7 +18,7 @@ void Auth::init() {
 
     for (const auto& token_provider : this->r_token_provider) {
         token_provider->subscribe_provided_token([this](ProvidedIdToken provided_token) {
-            std::thread t([this, provided_token]() { const auto res = this->auth_handler->on_token(provided_token); });
+            std::thread t([this, provided_token]() { this->auth_handler->on_token(provided_token); });
             t.detach();
         });
     }

modules/Auth/include/Connector.hpp

@@ -28,9 +28,9 @@ struct Connector {
     explicit Connector(int id) :
         id(id),
         transaction_active(false),
-        reserved(false),
+        state_machine(ConnectorState::AVAILABLE),
         is_reservable(true),
-        state_machine(ConnectorState::AVAILABLE){};
+        reserved(false){};
 
     int id;
 

modules/Auth/lib/AuthHandler.cpp

@@ -232,7 +232,7 @@ TokenHandlingResult AuthHandler::handle_token(const ProvidedIdToken& provided_to
     types::authorization::ValidationResult validation_result = {types::authorization::AuthorizationStatus::Unknown};
     if (!validation_results.empty()) {
         bool authorized = false;
-        int i = 0;
+        std::vector<ValidationResult>::size_type i = 0;
         // iterate over validation results
         while (i < validation_results.size() && !authorized && !referenced_connectors.empty()) {
             validation_result = validation_results.at(i);
@@ -604,6 +604,39 @@ void AuthHandler::handle_session_event(const int connector_id, const SessionEven
         this->connectors.at(connector_id)->connector.is_reservable = true;
         this->connectors.at(connector_id)->connector.reserved = false;
         break;
+    /// explicitly fall through all the SessionEventEnum values we are not handling
+    case SessionEventEnum::Authorized:
+        [[fallthrough]];
+    case SessionEventEnum::Deauthorized:
+        [[fallthrough]];
+    case SessionEventEnum::AuthRequired:
+        [[fallthrough]];
+    case SessionEventEnum::PrepareCharging:
+        [[fallthrough]];
+    case SessionEventEnum::ChargingStarted:
+        [[fallthrough]];
+    case SessionEventEnum::ChargingPausedEV:
+        [[fallthrough]];
+    case SessionEventEnum::ChargingPausedEVSE:
+        [[fallthrough]];
+    case SessionEventEnum::WaitingForEnergy:
+        [[fallthrough]];
+    case SessionEventEnum::ChargingResumed:
+        [[fallthrough]];
+    case SessionEventEnum::StoppingCharging:
+        [[fallthrough]];
+    case SessionEventEnum::ChargingFinished:
+        [[fallthrough]];
+    case SessionEventEnum::ErrorCleared:
+        [[fallthrough]];
+    case SessionEventEnum::PermanentFaultCleared:
+        [[fallthrough]];
+    case SessionEventEnum::ReplugStarted:
+        [[fallthrough]];
+    case SessionEventEnum::ReplugFinished:
+        [[fallthrough]];
+    case SessionEventEnum::PluginTimeout:
+        break;
     }
     this->connectors.at(connector_id)->event_mutex.unlock();
 }

modules/Auth/lib/CMakeLists.txt

@@ -29,3 +29,8 @@ PRIVATE
 # needs c++ 14
 target_compile_features(auth_handler PRIVATE cxx_std_14)
 
+if(EVEREST_ENABLE_COMPILE_WARNINGS)
+    target_compile_options(auth_handler
+        PRIVATE ${EVEREST_COMPILE_OPTIONS}
+    )
+endif()

modules/DPM1000/main/can_broker.cpp

@@ -99,7 +99,7 @@ void CanBroker::set_state(bool enabled) {
     write_to_can(frame);
 
     // Do an extra module ON command as sometimes the bits in the header are not enough to actually switch on
-    auto status = set_data_int(dpm1000::def::SetValueType::SWITCH_ON_OFF_SETTING, (enabled ? 0 : 1));
+    set_data_int(dpm1000::def::SetValueType::SWITCH_ON_OFF_SETTING, (enabled ? 0 : 1));
 }
 
 CanBroker::AccessReturnType CanBroker::dispatch_frame(const struct can_frame& frame, uint16_t id,

modules/EnergyManager/BrokerFastCharging.cpp

@@ -56,11 +56,11 @@ bool BrokerFastCharging::trade(Offer& _offer) {
 
         // in each timeslot: do we want to import or export energy?
         if (slot_type[i] == SlotType::Undecided) {
-            bool can_import = !(total_power_import.has_value() && total_power_import.value() == 0. ||
-                                max_current_import.has_value() && max_current_import.value() == 0.);
+            bool can_import = !((total_power_import.has_value() && total_power_import.value() == 0.) ||
+                                (max_current_import.has_value() && max_current_import.value() == 0.));
 
-            bool can_export = !(total_power_export.has_value() && total_power_export.value() == 0. ||
-                                max_current_export.has_value() && max_current_export.value() == 0.);
+            bool can_export = !((total_power_export.has_value() && total_power_export.value() == 0.) ||
+                                (max_current_export.has_value() && max_current_export.value() == 0.));
 
             if (can_import) {
                 slot_type[i] = SlotType::Import;

modules/EnergyManager/Market.cpp

@@ -149,7 +149,7 @@ ScheduleReq Market::get_max_available_energy(const ScheduleReq& request) {
                 break;
             }
             auto tp_r_2 = Everest::Date::from_rfc3339((*(ir + 1)).timestamp);
-            if (tp_a >= tp_r_1 && tp_a < tp_r_2 || (ir == request.begin() && tp_a < tp_r_1)) {
+            if ((tp_a >= tp_r_1 && tp_a < tp_r_2) || (ir == request.begin() && tp_a < tp_r_1)) {
                 r = ir;
                 break;
             }
@@ -191,7 +191,7 @@ ScheduleReq Market::get_max_available_energy(const ScheduleReq& request) {
 
 ScheduleReq Market::get_available_energy(const ScheduleReq& max_available, bool add_sold) {
     ScheduleReq available = max_available;
-    for (int i = 0; i < available.size(); i++) {
+    for (ScheduleReq::size_type i = 0; i < available.size(); i++) {
         // FIXME: sold_root is the sum of all energy sold, but we need to limit indivdual paths as well
         // add config option for pure star type of cabling here as well.
 
@@ -222,7 +222,7 @@ ScheduleReq Market::get_available_energy_export() {
 
 Market::Market(types::energy::EnergyFlowRequest& _energy_flow_request, const float __nominal_ac_voltage,
                Market* __parent) :
-    _nominal_ac_voltage(__nominal_ac_voltage), _parent(__parent), energy_flow_request(_energy_flow_request) {
+    energy_flow_request(_energy_flow_request), _parent(__parent), _nominal_ac_voltage(__nominal_ac_voltage) {
 
     // EVLOG_info << "Create market for " << _energy_flow_request.uuid;
 
@@ -290,7 +290,7 @@ static void schedule_add(ScheduleRes& a, const ScheduleRes& b) {
     if (a.size() != b.size())
         return;
 
-    for (int i = 0; i < a.size(); i++) {
+    for (ScheduleRes::size_type i = 0; i < a.size(); i++) {
         if (b[i].limits_to_root.ac_max_current_A.has_value()) {
             a[i].limits_to_root.ac_max_current_A =
                 b[i].limits_to_root.ac_max_current_A.value() + a[i].limits_to_root.ac_max_current_A.value_or(0);

modules/EnergyManager/Offer.cpp

@@ -53,7 +53,7 @@ static void apply_limits(ScheduleReq& a, const ScheduleReq& b) {
         EVLOG_error << fmt::format("apply_limits: a({}) and b({}) do not have the same size.", a.size(), b.size());
         return;
     }
-    for (int i = 0; i < a.size(); i++) {
+    for (ScheduleReq::size_type i = 0; i < a.size(); i++) {
         // limits to leave are already merged to the root side, so we dont use them here
         apply_one_limit_if_smaller(a[i].limits_to_root.ac_max_current_A, b[i].limits_to_root.ac_max_current_A);
         apply_one_limit_if_smaller(a[i].limits_to_root.ac_max_phase_count, b[i].limits_to_root.ac_max_phase_count);

modules/EnergyNode/energy_grid/energyImpl.cpp

@@ -155,7 +155,7 @@ void energyImpl::merge_price_into_schedule(std::vector<types::energy::ScheduleRe
         auto tp_schedule = Everest::Date::from_rfc3339(next_entry_schedule.timestamp);
         auto tp_price = Everest::Date::from_rfc3339(next_entry_price.timestamp);
 
-        if (tp_schedule < tp_price && it_schedule != schedule.end() || it_price == price.end()) {
+        if ((tp_schedule < tp_price && it_schedule != schedule.end()) || it_price == price.end()) {
             currently_valid_entry_schedule = next_entry_schedule;
             auto joined_entry = currently_valid_entry_schedule;
 
@@ -168,7 +168,7 @@ void energyImpl::merge_price_into_schedule(std::vector<types::energy::ScheduleRe
             continue;
         }
 
-        if (tp_price < tp_schedule && it_price != price.end() || it_schedule == schedule.end()) {
+        if ((tp_price < tp_schedule && it_price != price.end()) || it_schedule == schedule.end()) {
             currently_valid_entry_price = next_entry_price;
             auto joined_entry = currently_valid_entry_schedule;
             joined_entry.price_per_kwh = currently_valid_entry_price;

modules/EvseManager/Charger.cpp

@@ -28,9 +28,9 @@ Charger::Charger(const std::unique_ptr<IECStateMachine>& bsp, const std::unique_
                  const types::evse_board_support::Connector_type& connector_type, const std::string& evse_id) :
     bsp(bsp),
     error_handling(error_handling),
-    r_powermeter_billing(r_powermeter_billing),
     connector_type(connector_type),
-    evse_id(evse_id) {
+    evse_id(evse_id),
+    r_powermeter_billing(r_powermeter_billing) {
 
 #ifdef EVEREST_USE_BACKTRACES
     Everest::install_backtrace_handler();

modules/EvseManager/scoped_lock_timeout.hpp

@@ -94,6 +94,8 @@ enum class MutexDescription {
 
 static std::string to_string(MutexDescription d) {
     switch (d) {
+    case MutexDescription::Undefined:
+        return "Undefined";
     case MutexDescription::Charger_signal_loop:
         return "Charger.cpp: error_handling->signal_loop";
     case MutexDescription::Charger_signal_error:

modules/LemDCBM400600/main/lem_dcbm_400600_controller.hpp

@@ -94,8 +94,8 @@ class LemDCBM400600Controller {
     std::string version;
     bool v2_capable = false;
     bool trasaction_is_ongoing = false;
-    Conf config;
     std::unique_ptr<LemDCBMTimeSyncHelper> time_sync_helper;
+    Conf config;
 
     void fetch_meter_id_from_device();
     void request_device_to_start_transaction(const types::powermeter::TransactionReq& value);

modules/LemDCBM400600/main/lem_dcbm_time_sync_helper.hpp

@@ -43,11 +43,11 @@ class LemDCBMTimeSyncHelper {
     }
 
     explicit LemDCBMTimeSyncHelper(ntp_server_spec ntp_spec, timing_config tc) :
-        timing_constants(tc),
         ntp_spec(std::move(ntp_spec)),
-        unsafe_period_start_time({}),
+        timing_constants(tc),
         meter_timezone(""),
-        meter_dst("") {
+        meter_dst(""),
+        unsafe_period_start_time({}) {
     }
 
     virtual ~LemDCBMTimeSyncHelper() = default;

modules/OCPP201/OCPP201.cpp

@@ -42,7 +42,7 @@ std::set<TxStartStopPoint> get_tx_start_stop_points(const std::string& tx_start_
         csv.push_back(str);
     }
 
-    for (const auto tx_start_stop_point : csv) {
+    for (const auto& tx_start_stop_point : csv) {
         if (tx_start_stop_point == "ParkingBayOccupancy") {
             tx_start_stop_points.insert(TxStartStopPoint::ParkingBayOccupancy);
         } else if (tx_start_stop_point == "EVConnected") {
@@ -712,6 +712,8 @@ void OCPP201::process_session_event(const int32_t evse_id, const types::evse_man
         this->process_deauthorized(evse_id, connector_id, session_event);
         break;
     }
+
+        // missing AuthRequired, PrepareCharging and many more
     }
 
     // process authorized event which will inititate a TransactionEvent(Updated) message in case the token has not yet

modules/OCPPExtensionExample/OCPPExtensionExample.cpp

@@ -51,7 +51,7 @@ void OCPPExtensionExample::ready() {
     component_variables.push_back({{""}, {"KeyThatIsNotConfigured"}});
 
     std::vector<types::ocpp::GetVariableRequest> get_variables_requests;
-    for (const auto component_variable : component_variables) {
+    for (const auto& component_variable : component_variables) {
         get_variables_requests.push_back({component_variable});
     }
 

modules/PN532TokenProvider/PN532TokenProvider.hpp

@@ -5,7 +5,7 @@
 
 //
 // AUTO GENERATED - MARKED REGIONS WILL BE KEPT
-// template version 1
+// template version 2
 //
 
 #include "ld-ev.hpp"
@@ -27,8 +27,8 @@ class PN532TokenProvider : public Everest::ModuleBase {
     PN532TokenProvider(const ModuleInfo& info, std::unique_ptr<auth_token_providerImplBase> p_main, Conf& config) :
         ModuleBase(info), p_main(std::move(p_main)), config(config){};
 
-    const Conf& config;
     const std::unique_ptr<auth_token_providerImplBase> p_main;
+    const Conf& config;
 
     // ev@1fce4c5e-0ab8-41bb-90f7-14277703d2ac:v1
     // insert your public definitions here

modules/PN532TokenProvider/pn532_serial/PN532Serial.cpp

@@ -55,29 +55,28 @@ void PN532Serial::resetDataRead() {
 
 void PN532Serial::readThread() {
     uint8_t buf[2048];
-    int n;
 
     resetDataRead();
 
     while (true) {
         if (readThreadHandle.shouldExit()) {
             break;
         }
-        n = read(fd, buf, sizeof buf);
+        auto n = read(fd, buf, sizeof buf);
         if (n == 0) {
             continue;
         }
 
         if (this->debug) {
             std::stringstream data_stream;
-            for (size_t i = 0; i < n; i++) {
+            for (ssize_t i = 0; i < n; i++) {
                 data_stream << "0x" << std::setfill('0') << std::setw(2) << std::right << std::hex << (int)buf[i]
                             << " ";
             }
 
             EVLOG_info << "Received bytes: " << data_stream.str();
         }
-        for (size_t i = 0; i < n; i++) {
+        for (ssize_t i = 0; i < n; i++) {
             if (!preamble_seen) {
                 if (preamble_start_seen && buf[i] == 0xff) {
                     preamble_seen = true;
@@ -137,7 +136,7 @@ void PN532Serial::readThread() {
                 tfi = buf[start_of_packet + 2];
                 command_code = buf[start_of_packet + 3];
                 first_data = false;
-                for (size_t i = start_of_packet + 4; i < n; i++) {
+                for (ssize_t i = start_of_packet + 4; i < n; i++) {
                     data.push_back(buf[i]);
                 }
             }
@@ -243,7 +242,7 @@ void PN532Serial::parseInListPassiveTargetResponse() {
 
             if (data.size() >= 6 + target_data.nfcid_length) {
                 response.valid = true;
-                for (size_t i = 6; i < 6 + target_data.nfcid_length; i++) {
+                for (ssize_t i = 6; i < 6 + target_data.nfcid_length; i++) {
                     target_data.nfcid.push_back(data.at(i));
                 }