Added temperature sensors values in Powermeter #755

florinmihut · 2024-07-02T11:12:55Z

…mperature2)

- added in interface a variable containing the public OCMF key (publicKeyOcmf)
- added in config the following options: meter_timezone, meter_dst, SC, UV, UD
- added support for setting the following parameters: meter_dst and meter_timezone
- added support for V2 interface

Describe your changes

Issue ticket number and link

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

…emperature2) - added in interface a variable containing the public OCMF key (publicKeyOcmf) - added in config the following options: meter_timezone, meter_dst, SC, UV, UD - added support for setting the following parameters: meter_dst and meter_timezone - added support for V2 interface Signed-off-by: florinmihut <[email protected]>

Signed-off-by: florinmihut <[email protected]>

SirVer

only one nit for rust code.

modules/RsIskraMeter/src/main.rs

interfaces/powermeter.yaml

Pietfried · 2024-07-04T06:42:27Z

modules/LemDCBM400600/main/lem_dcbm_400600_controller.cpp

@@ -22,14 +23,33 @@ void LemDCBM400600Controller::init() {
    this->time_sync_helper->restart_unsafe_period();
 }

+std::vector<std::string> LemDCBM400600Controller::split(const std::string& str, char delimiter) {


This can be defined as a free function

modules/LemDCBM400600/main/lem_dcbm_400600_controller.hpp

Pietfried · 2024-07-04T08:06:08Z

modules/LemDCBM400600/main/lem_dcbm_400600_controller.hpp

    Conf config;
    std::unique_ptr<LemDCBMTimeSyncHelper> time_sync_helper;

+    std::vector<std::string> split(const std::string& str, char delimiter);
    void fetch_meter_id_from_device();
    void request_device_to_start_transaction(const types::powermeter::TransactionReq& value);
    void request_device_to_stop_transaction(const std::string& transaction_id);
    std::string fetch_ocmf_result(const std::string& transaction_id);
    void convert_livemeasure_to_powermeter(const std::string& livemeasure, types::powermeter::Powermeter& powermeter);


not part of the PR but could also be defined as a free function

Pietfried · 2024-07-04T08:14:20Z

types/powermeter.yaml

@@ -299,3 +299,10 @@ types:
          This is intended for meters that only support signing a collection of meter values.
        type: object
        $ref: /units_signed#/SignedMeterValue
+      temperature1:


This looks to LEM specific to me. Not sure which temperatures it actually measures, but would it make sense to you to add more generic properties like:

ambient_temperature

internal_temperature

conductor_temperature

...

I agree but since this is not defined by all the power meters, this has to cover everyone.
What if I have 2 cable temps? Or a high and low temp?
Starting to give them "actual functional" names might pollute the interface with a lot of unused variables.
I spoke with Cornelius as well and he proposed to use temp 1 and 2 and their meaning is Powermeter dependent. Since anyway it is - some have it, some have it not, some measure the ambient some measure the internal some measure the high and the low.
You get the picture.
So I would be in the favor of keeping them temp 1 and 2 and their meaning is Powermeter actual implementation dependent.

If it's really that opaque we could also just add an array of temperature values here, what if there's a third sensor for example :D

I agree but since this is not defined by all the power meters, this has to cover everyone.
Starting to give them "actual functional" names might pollute the interface with a lot of unused variables

For this we have optional properties. There are a lot of properties in the powermeter already defined this way.

What if I have 2 cable temps? Or a high and low temp?

As @hikinggrass pointed out, what if we have 3? That is indeed specific to the powermeter and should therefore not be covered in a generic interface like powermeter like this. An interface is meant to be required by other modules. How should other modules know what temp1 or temp2 or temp3 are, especially if they mean different things for different implementations. Defining it this way doesn't seem useful to me.

If we add it like this, Id agree with @hikinggrass and add an array of temperature values, but I would rather add more meaningful properties as described above, maybe defining a type for a temperature and adding optional low / high values to it as properties.

I think an array is a good idea

…d by reviewers Signed-off-by: florinmihut <[email protected]>

Signed-off-by: florinmihut <[email protected]>

Pietfried · 2024-07-08T11:53:01Z

modules/LemDCBM400600/main/lem_dcbm_400600_controller.cpp

+    } else {
+        for (auto& sensor : *powermeter.temperatures) {
+            if (sensor.identification == "temperatureL") {
+                sensor.temperature = data.at("temperatureL");
+            } else if (sensor.identification == "temperatureH") {
+                sensor.temperature = data.at("temperatureH");
+            }
+        }
+    }


the powermeter argument passed to this function is default initialized and the properties are then set in this function. Therefore this else block is never executed and can be dropped. Its also not required to check for has_value() in L202.

Apart from that I'd prefer a function signature like:
types::powermeter::Powermeter LemDCBM400600Controller::convert_livemeasure_to_powermeter(const std::string& livemeasure) that returns the powermeter object instead of passing it as an argument, but since this is legacy code im ok with keeping the signature like that

But isn't powermeter.temperatures an optional vector so that check is still needed?

maybe I miss something but afaics the temperatures property is always not set so the else block is never executed

because the passed powermeter is default initialized and passed to this function

ah you're right, then it's a good idea to drop the else block indeed!

You're right, I didn't check where the powermeter value comes from.
Since it is created by the calling function the else branch would never execute.
Thank you for catching that.
I made the changes, including the signature of the function since I am changing it again and the change is minor.

Signed-off-by: florinmihut <[email protected]>

* - added temperature sensors values in Powermeter (temperature1 and temperature2) - added in interface a variable containing the public OCMF key (publicKeyOcmf) - added in config the following options: meter_timezone, meter_dst, SC, UV, UD - added support for setting the following parameters: meter_dst and meter_timezone - added support for V2 interface * Fix tests and Rust implementation * Add temperatures as a optional vector, additional changes as requested by reviewers --------- Signed-off-by: florinmihut <[email protected]> Co-authored-by: Piet Gömpel <[email protected]>

florinmihut requested review from Pietfried, hikinggrass and corneliusclaussen as code owners July 2, 2024 11:12

florinmihut added 2 commits July 2, 2024 13:16

Fix coding style

d961603

Signed-off-by: florinmihut <[email protected]>

Fix tests and Rust implementation

60290e8

Signed-off-by: florinmihut <[email protected]>

florinmihut requested review from SirVer, golovasteek and dorezyuk as code owners July 3, 2024 16:39

Fix code formatting

0073b69

Signed-off-by: florinmihut <[email protected]>

SirVer approved these changes Jul 3, 2024

View reviewed changes

modules/RsIskraMeter/src/main.rs Outdated Show resolved Hide resolved

Pietfried requested changes Jul 4, 2024

View reviewed changes

hikinggrass changed the title ~~- added temperature sensors values in Powermeter (temperature1 an…~~ Added temperature sensors values in Powermeter Jul 8, 2024

florinmihut added 2 commits July 8, 2024 11:59

Add temperatures as a optional vector, additional changes as requeste…

1c8d6a7

…d by reviewers Signed-off-by: florinmihut <[email protected]>

Reformat code

5c7c2ef

Signed-off-by: florinmihut <[email protected]>

florinmihut requested a review from Pietfried July 8, 2024 11:43

Pietfried requested changes Jul 8, 2024

View reviewed changes

hikinggrass added the include-in-release Tag that this PR should be included in the current merge window for the upcoming release if possible label Jul 8, 2024

florinmihut and others added 5 commits July 8, 2024 15:47

Fixes as found by reviewers

ddebb79

Signed-off-by: florinmihut <[email protected]>

Fix formatting

858b632

Signed-off-by: florinmihut <[email protected]>

Again fix the formatting

5ade90b

Signed-off-by: florinmihut <[email protected]>

Merge branch 'main' into feature/lem

c4bfc10

Fix formatting problems

f5226d6

Signed-off-by: florinmihut <[email protected]>

florinmihut requested a review from Pietfried July 8, 2024 14:27

Pietfried approved these changes Jul 8, 2024

View reviewed changes

hikinggrass approved these changes Jul 8, 2024

View reviewed changes

hikinggrass merged commit eb5cbc4 into main Jul 8, 2024
7 of 8 checks passed

hikinggrass deleted the feature/lem branch July 8, 2024 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added temperature sensors values in Powermeter #755

Added temperature sensors values in Powermeter #755

florinmihut commented Jul 2, 2024 •

edited

Loading

SirVer left a comment

Pietfried Jul 4, 2024

Pietfried Jul 4, 2024

Pietfried Jul 4, 2024

florinmihut Jul 4, 2024

hikinggrass Jul 4, 2024

Pietfried Jul 4, 2024

corneliusclaussen Jul 5, 2024

Pietfried Jul 8, 2024

hikinggrass Jul 8, 2024

Pietfried Jul 8, 2024

Pietfried Jul 8, 2024

hikinggrass Jul 8, 2024

florinmihut Jul 8, 2024

Added temperature sensors values in Powermeter #755

Added temperature sensors values in Powermeter #755

Conversation

florinmihut commented Jul 2, 2024 • edited Loading

Describe your changes

Issue ticket number and link

Checklist before requesting a review

SirVer left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florinmihut commented Jul 2, 2024 •

edited

Loading