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

 }

+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

    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

+            // send evseID = 0 values
            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

                    // 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 <m.emmers@alfen.com>

Signed-off-by: Marc Emmers <m.emmers@alfen.com>

Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com>

Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> fix rebasign errors Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> fix formatting Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> intermidiate commit Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> PR changes Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> remove fake values Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> put back optional location and phase values Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> remove fake values Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> fix formatting Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> fix some more PR issues Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> revert unexpected changes Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com> 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 <s.subramanya@alfen.com>

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 Pietfried and marcemmers 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 <m.emmers@alfen.com>

Fix linting issue

9cf09d0

Signed-off-by: Marc Emmers <m.emmers@alfen.com>

Averaging of idle values

72411b9

Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com>

updated averages work

2b8b2bf

Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com>

move clear values to inside lambda function

488ac4f

Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com>

Add voltage and power measurands

133d24b

Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com>

add evseId 0 averaging

6c7e5cf

Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com>

rename aligned data class

8b026d4

Signed-off-by: Soumya Subramanya <s.subramanya@alfen.com>

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 <s.subramanya@alfen.com>

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

Averaging Measurements #241

Averaging Measurements #241

Uh oh!

Conversation

SNSubramanya commented Nov 2, 2023

Uh oh!

marcemmers left a comment

Choose a reason for hiding this comment

Uh oh!

SNSubramanya commented Nov 7, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants