Fixing transactionData omitted in StopTransaction.req

[movhdi]

Describe your changes

A boolean variable added inside transaction class as a flag for existing signed meter values.
The flag has been set in both on_transaction_started and on_transaction_stopped methods.
The flag has been used to fall in the main if statement in get_filtered_transaction_data method in which meter values are populated in filtered_transaction_data_vec.
I think this works although I am afraid I have meddled in transaction class.

Issue ticket number and link

issue number #472

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

[movhdi]

The tests failed and it seems it is in part because has_signed_meter_value is private and it is a fault, however is it the right approach at all?

[movhdi]

The other approach is to avoid meddling in TransactionData class and accordingly avoid playing around with mutexes and lock_guards, by just adding the following lines of code in get_filtered_transaction_data method:

    std::vector<TransactionData> transaction_data_vec = transaction->get_transaction_data();
    
    bool has_singed_meter_values =
        std::find_if(
            transaction_data_vec.begin(), transaction_data_vec.end(), [](TransactionData const& TransactionData_Item) {
                return (std::find_if(TransactionData_Item.sampledValue.begin(), TransactionData_Item.sampledValue.end(),
                                     [](SampledValue const& SampledValue_item) {
                                         return SampledValue_item.format.value_or(ValueFormat::Raw) ==
                                                ValueFormat::SignedData;
                                     }) != TransactionData_Item.end());
            }) != transaction_data_vec.end();

    if (!stop_txn_sampled_data_measurands.empty() or !stop_txn_aligned_data_measurands.empty() or 
        has_singed_meter_values) { ...

This way we make use of two nested std::find_if algorithm in order to define has_signed_meter_values flag for falling into the main if statement.

[movhdi]

Sorry for making too much noise, but another approach I think is to delete std::lock_guard from Transaction::set_has_signed_meter_values and change Transaction::add_meter_value method as follows, this way populating meter values and setting the flag of signed meter values in a single block of code guarded with meter_values_mutex, but it comes at a cost of processing all sampled_values every single time that add_meter_value is called:

void Transaction::add_meter_value(MeterValue meter_value) {
    if (this->active) {
        std::lock_guard<std::mutex> lock(this->meter_values_mutex);
        this->meter_values.push_back(meter_value);

        if (std::find_if(meter_value.sampledValue.begin(), meter_value.sampledValue.end(),
                         [](SampledValue const& SampledValueItem) {
                             return SampledValueItem.format == ValueFormat::SignedData;
                         }) != meter_value.sampledValue.end()) {
            this->set_has_signed_meter_values();
        }
    }
}

However, if we can define a signed_meter_value_flag defaulted to false as an argument for Transaction::add_meter_value(MeterValue meter_value, bool signed_meter_value = false) and only pass it as true whenever the optional signed_meter_value.has_value() (like in on_transaction_stopped and on_transaction_started) we can avoid this extra process.

void Transaction::add_meter_value(MeterValue meter_value, bool signed_meter_value = false) {
    if (this->active) {
        std::lock_guard<std::mutex> lock(this->meter_values_mutex);
        this->meter_values.push_back(meter_value);

        if (signed_meter_value) {
            this->set_has_signed_meter_values();
        }
    }
}

So in charge_point_impl.cpp we will have:

void ChargePointImpl::on_transaction_started(const int32_t& connector, const std::string& session_id,
                                             const std::string& id_token, const int32_t& meter_start,
                                             std::optional<int32_t> reservation_id, const ocpp::DateTime& timestamp,
                                             std::optional<std::string> signed_meter_value) {
    ...
    if (bool singed_meter_value_flag = signed_meter_value.has_value(); signed_meter_value_flag) {
        const auto meter_value =
            this->get_signed_meter_value(signed_meter_value.value(), ReadingContext::Transaction_Begin, timestamp);
        transaction->add_meter_value(meter_value, has_signed_meter_value);
    }
    ...
}

[thenaserov]

has_signed_meter_values is checking a state and letting start transaction happen
Great job Mahdi!

[thenaserov]

You need to get the checks reviewed and see what the reason behind this disapproval is!

[thenaserov]

I see no issue in this request!

[movhdi]

Dear @thenaserov , it is a good_first_issue, it is much better if solved with a new contributor. You give it a shot.

[movhdi]

Dear @Pietfried could you please guide me on the changes in the commits and changes proposed in the comments? May I have your supervision about the approaches?

[Pietfried]

Hi @movhdi , since this PR was in Draft state we missed it. Thanks for the contribution! Please run clang format so we can merge it.

[movhdi]

Dear @Pietfried, I runned clang-format. Thanks for your comment.