Averaging Measurements #241

SNSubramanya · 2023-11-02T13:43:18Z

No description provided.

marcemmers

Can you please take another look to remove all the unrelated changes, debug stuff etc?

Also take a closer look at the mutex uses especially since the AlignedData has its own mutex now.

I am wondering if AlignedData is the best name. It says something currently about what it is used for, not what it is. I normally think that naming something what it is, is a bit clearer and allows for more general use of classes.

SNSubramanya · 2023-11-07T16:01:32Z

Can you please take another look to remove all the unrelated changes, debug stuff etc?

Also take a closer look at the mutex uses especially since the AlignedData has its own mutex now.

I am wondering if AlignedData is the best name. It says something currently about what it is used for, not what it is. I normally think that naming something what it is, is a bit clearer and allows for more general use of classes.

Not sure what that means... you mean rename it to averaging?

marcemmers · 2023-11-09T11:08:09Z

lib/ocpp/v201/evse.cpp

@@ -221,6 +221,16 @@ MeterValue Evse::get_meter_value() {
    return this->meter_value;
 }

+MeterValue Evse::get_idle_meter_value() {
+    std::lock_guard<std::recursive_mutex> lk(this->meter_value_mutex);


Still some mutexes here that are no longer needed ;)

marcemmers · 2023-11-09T11:09:07Z

lib/ocpp/v201/evse.cpp

@@ -112,16 +112,13 @@ void Evse::open_transaction(const std::string& transaction_id, const int32_t con
    if (aligned_data_tx_updated_interval > 0s) {
        transaction->aligned_tx_updated_meter_values_timer.interval_starting_from(
            [this] {
-                if (this->device_model.get_optional_value<bool>(ControllerComponentVariables::AlignedDataSendDuringIdle)


We should probably still check on the AlignedDataSendDuringIdle flag

marcemmers · 2023-11-09T11:11:27Z

lib/ocpp/v201/charge_point.cpp

            const auto meter_value =
-                get_latest_meter_value_filtered(this->get_meter_value(), ReadingContextEnum::Sample_Clock,
+                get_latest_meter_value_filtered(this->aligned_data_evse0.get_values(), ReadingContextEnum::Sample_Clock,


Are you clearing the evse0 averaging as well?

Maybe you can still do the clearing in the get so you don't always have to group a get and clear function together. Only get is probably not the best name then since you will be changing the internal state which you normally don't do on a get

marcemmers · 2023-11-09T11:13:34Z

lib/ocpp/v201/average_meter_values.cpp

+}
+
+void AverageMeterValues::set_values(const MeterValue& meter_value) {
+    EVLOG_debug << " Setting the aligned values";


Maybe remove these log lines? Even for debugging this is not really useful IHMO. You'd just get spammed with this line, maybe even a couple of times a second with a fast meter.

At the very least remove the log from the constructor. It is assumed a constructor just works so does not really need logging like this.

marcemmers · 2023-11-09T11:15:16Z

lib/ocpp/v201/average_meter_values.cpp

+bool AverageMeterValues::is_avg_meas(const SampledValue& sample) {
+
+    // TODO: check up on location values and how they impact the averaging
+    if (sample.measurand.has_value() && sample.phase.has_value()) {


What if we have a single phase system and there is no phase set? Or we want to average the total power value which also does not have a phase?

I think a more close look is needed at this still

i agree this is a bit tricky at the moment

marcemmers · 2023-11-13T15:56:13Z

include/ocpp/v201/average_meter_values.hpp

+        // Define a comparison operator for the struct
+        bool operator<(const MeterValueMeasurands& other) const {
+
+            return measurand < other.measurand || location.value() < other.location.value() ||


Did you test this with measurands that don't have a location or a phase?

Also, they prefer or to ||

marcemmers · 2023-11-13T15:59:29Z

lib/ocpp/v201/average_meter_values.cpp

+    // avg all the possible measurerands
+    for (auto element : meter_value.sampledValue) {
+        if (is_avg_meas(element)) {
+            MeterValueCalc temp =


If you make this a reference you can edit the value inside the map instead of having to copy it twice

marcemmers · 2023-11-13T16:01:53Z

lib/ocpp/v201/average_meter_values.cpp

+            return true;
+    } else {
+        return false;
+    }


I'm assuming you'd get a warning of this as well, but you have a return without value path here I think?

marcemmers · 2023-11-13T16:03:56Z

lib/ocpp/v201/charge_point.cpp

                                                    ControllerComponentVariables::AlignedDataMeasurands);

                if (!meter_value.sampledValue.empty()) {
                    // J01.FR.14 this is the only case where we send a MeterValue.req
                    this->meter_values_req(evse_id, std::vector<ocpp::v201::MeterValue>(1, meter_value));
+                    // clear the values
+                    evse->clear_idle_meter_values();


Shouldn't we clear regardless of if we actually have filtered data?

…f a transaction Signed-off-by: Marc Emmers <[email protected]>

Signed-off-by: Marc Emmers <[email protected]>

Signed-off-by: Soumya Subramanya <[email protected]>

Signed-off-by: Soumya Subramanya <[email protected]> fix rebasign errors Signed-off-by: Soumya Subramanya <[email protected]> fix formatting Signed-off-by: Soumya Subramanya <[email protected]> intermidiate commit Signed-off-by: Soumya Subramanya <[email protected]> PR changes Signed-off-by: Soumya Subramanya <[email protected]> remove fake values Signed-off-by: Soumya Subramanya <[email protected]> put back optional location and phase values Signed-off-by: Soumya Subramanya <[email protected]> remove fake values Signed-off-by: Soumya Subramanya <[email protected]> fix formatting Signed-off-by: Soumya Subramanya <[email protected]> fix some more PR issues Signed-off-by: Soumya Subramanya <[email protected]> revert unexpected changes Signed-off-by: Soumya Subramanya <[email protected]> Pr changes

marcemmers · 2023-11-20T10:48:13Z

lib/ocpp/v201/evse.cpp

+}
+
+void Evse::clear_idle_meter_values() {
+    std::lock_guard<std::recursive_mutex> lk(this->meter_value_mutex);


No mutex needed here right?

marcemmers · 2023-11-20T10:49:28Z

lib/ocpp/v201/average_meter_values.cpp

+                EVLOG_debug << " loc: " << element.location.value();
+            }
+
+            EVLOG_debug << " sum:" << temp.sum << "#num" << temp.num_elements << "AVG: " << element.value;


Can we please remove these debug lines? I get that they are useful to test with. But this makes it impossible to enable debug on a system without getting spammed. And now it is working we can remove these right?

Signed-off-by: Soumya Subramanya <[email protected]>

SNSubramanya force-pushed the clock-aligned-values branch 2 times, most recently from c9eda59 to 4c7e6a1 Compare November 3, 2023 12:02

SNSubramanya requested review from marcemmers and Pietfried November 7, 2023 14:52

SNSubramanya marked this pull request as ready for review November 7, 2023 14:52

SNSubramanya force-pushed the clock-aligned-values branch from b5efa1c to 87a5e4d Compare November 7, 2023 14:56

marcemmers requested changes Nov 7, 2023

View reviewed changes

marcemmers requested changes Nov 9, 2023

View reviewed changes

SNSubramanya force-pushed the clock-aligned-values branch from 3ac3851 to aa2b88e Compare November 13, 2023 15:03

marcemmers reviewed Nov 13, 2023

View reviewed changes

SNSubramanya force-pushed the clock-aligned-values branch from a43d9f5 to b52a31d Compare November 16, 2023 12:57

marcemmers and others added 9 commits November 16, 2023 14:01

Use database to store transaction metervalues to be sent at the end o…

68878a0

…f a transaction Signed-off-by: Marc Emmers <[email protected]>

Fix linting issue

9cf09d0

Signed-off-by: Marc Emmers <[email protected]>

Averaging of idle values

72411b9

Signed-off-by: Soumya Subramanya <[email protected]>

updated averages work

2b8b2bf

Signed-off-by: Soumya Subramanya <[email protected]>

move clear values to inside lambda function

488ac4f

Signed-off-by: Soumya Subramanya <[email protected]>

Add voltage and power measurands

133d24b

Signed-off-by: Soumya Subramanya <[email protected]>

add evseId 0 averaging

6c7e5cf

Signed-off-by: Soumya Subramanya <[email protected]>

rename aligned data class

8b026d4

Signed-off-by: Soumya Subramanya <[email protected]>

SNSubramanya force-pushed the clock-aligned-values branch from 7c8bb1b to f71b134 Compare November 16, 2023 13:01

SNSubramanya requested a review from marcemmers November 17, 2023 10:56

marcemmers reviewed Nov 20, 2023

View reviewed changes

PR changes

6aaa9c1

Signed-off-by: Soumya Subramanya <[email protected]>

marcemmers approved these changes Nov 20, 2023

View reviewed changes

SNSubramanya merged commit 8a4a971 into main Nov 20, 2023

SNSubramanya deleted the clock-aligned-values branch November 20, 2023 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Averaging Measurements #241

Averaging Measurements #241

SNSubramanya commented Nov 2, 2023

marcemmers left a comment

SNSubramanya commented Nov 7, 2023

marcemmers Nov 9, 2023

marcemmers Nov 9, 2023

marcemmers Nov 9, 2023

marcemmers Nov 9, 2023

marcemmers Nov 9, 2023

SNSubramanya Nov 9, 2023

marcemmers Nov 13, 2023

marcemmers Nov 13, 2023

marcemmers Nov 13, 2023

marcemmers Nov 13, 2023

marcemmers Nov 20, 2023

marcemmers Nov 20, 2023

SNSubramanya Nov 20, 2023

Averaging Measurements #241

Averaging Measurements #241

Conversation

SNSubramanya commented Nov 2, 2023

marcemmers left a comment

Choose a reason for hiding this comment

SNSubramanya commented Nov 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment