Fix logging

[ClemensLinnhoff]

Reference to a related issue in the repository

69

Add a description Fixes private logging

Take this checklist as orientation for yourself, if this PR is ready for Maintainer Review

• [x] My suggestion follows the governance rules.
• [x] All commits of this PR are signed.
• [x] My changes generate no errors when passing CI tests.
• [x] I updated all documentation (readmes incl. figures) according to my changes.
• [x] I have successfully implemented and tested my fix/feature locally.
• [x] Appropriate reviewer(s) are assigned.

[ClemensLinnhoff]

@MarcelKe could your check the public logging functionality with CarMaker?

[MarcelKe]

@MarcelKe could your check the public logging functionality with CarMaker?

Public logging is yet not working. The variable names above have to be adjusted in case of the OSMP.h:

In case of the MySensorModel.h it should not be able to find the required variables, since they are private variables of the OSMP class. I quick-fixed it by adding the required variables as private variables to the MySensorModel class as well and defining them with a copy of the corresponding OSMP variables during model initialization:

In this way, there is of course some duplicate code in the template and one has to hand over three parameters to the model initialization just for logging. A nicer alternative might be to decouple the OSMP interface from the logger or to forego public logging for the sensor model. What do you think?

[MarcelKe]

One minor remark: I also recognized that the OSMPVERSION is not yet defined in the uppermost CMakeLists. Hence, it is not automatically set in the resulting ModelDescribtion.xml - I just recognized because Carmaker is checking for the OSMP version of the FMU.

[ClemensLinnhoff]

@MarcelKe Could you commit your fixes here? Then I can get a better understanding of what's necessary to enable public logging.

[ClemensLinnhoff]

One minor remark: I also recognized that the OSMPVERSION is not yet defined in the uppermost CMakeLists. Hence, it is not automatically set in the resulting ModelDescribtion.xml - I just recognized because Carmaker is checking for the OSMP version of the FMU.

You are right. Although I am not sure, if the OSMP version is even set correctly, when the variable is defined. Which one does carmaker need? Is it this line?

<Tool name="net.pmsf.osmp" xmlns:osmp="http://xsd.pmsf.net/OSISensorModelPackaging"><osmp:osmp version="@OSMPVERSION@" osi-version="@OSIVERSION@"/></Tool>

Because currently the OSMPVERSION is also set as version directly under "<fmiModelDescription" but I think this should be the version of the model itself, right?

I added a commit with this differentiation.

[MarcelKe]

@MarcelKe Could you commit your fixes here? Then I can get a better understanding of what's necessary to enable public logging.

@ClemensLinnhoff unfortunately, I don't have permission to push my public logging fix branch (I checked it out from the logging-fix)

[MarcelKe]

One minor remark: I also recognized that the OSMPVERSION is not yet defined in the uppermost CMakeLists. Hence, it is not automatically set in the resulting ModelDescribtion.xml - I just recognized because Carmaker is checking for the OSMP version of the FMU.

You are right. Although I am not sure, if the OSMP version is even set correctly, when the variable is defined. Which one does carmaker need? Is it this line?

<Tool name="net.pmsf.osmp" xmlns:osmp="http://xsd.pmsf.net/OSISensorModelPackaging"><osmp:osmp version="@OSMPVERSION@" osi-version="@OSIVERSION@"/></Tool>

Because currently the OSMPVERSION is also set as version directly under "<fmiModelDescription" but I think this should be the version of the model itself, right?

I added a commit with this differentiation.

@ClemensLinnhoff Carmaker expects 1.0.0 - If I set the OSMPVERSION variable to it within the CmakeLists it was transferred correctly to the final modelDescription.xml and accepted by Carmaker. I am not quite sure which line Carmaker actually reads, since I have yet only changed it globally - But I would also expect the second one. I agree - For me it would also make more sense if the "version" is utilized for the model version.

[ClemensLinnhoff]

@MarcelKe Could you commit your fixes here? Then I can get a better understanding of what's necessary to enable public logging.

@ClemensLinnhoff unfortunately, I don't have permission to push my public logging fix branch (I checked it out from the logging-fix)

You should have access now. I added you to the contributors teams.

[MarcelKe]

@MarcelKe Could you commit your fixes here? Then I can get a better understanding of what's necessary to enable public logging.

@ClemensLinnhoff unfortunately, I don't have permission to push my public logging fix branch (I checked it out from the logging-fix)

You should have access now. I added you to the contributors teams.

Thank you, I pushed the branch.

[MarcelKe]

One minor remark: I also recognized that the OSMPVERSION is not yet defined in the uppermost CMakeLists. Hence, it is not automatically set in the resulting ModelDescribtion.xml - I just recognized because Carmaker is checking for the OSMP version of the FMU.

You are right. Although I am not sure, if the OSMP version is even set correctly, when the variable is defined. Which one does carmaker need? Is it this line?

<Tool name="net.pmsf.osmp" xmlns:osmp="http://xsd.pmsf.net/OSISensorModelPackaging"><osmp:osmp version="@OSMPVERSION@" osi-version="@OSIVERSION@"/></Tool>

Because currently the OSMPVERSION is also set as version directly under "<fmiModelDescription" but I think this should be the version of the model itself, right? I added a commit with this differentiation.

@ClemensLinnhoff Carmaker expects 1.0.0 - If I set the OSMPVERSION variable to it within the CmakeLists it was transferred correctly to the final modelDescription.xml and accepted by Carmaker. I am not quite sure which line Carmaker actually reads, since I have yet only changed it globally - But I would also expect the second one. I agree - For me it would also make more sense if the "version" is utilized for the model version.

I tested it quickly - Carmaker is as expected checking only the above mentioned line in the xml, therefore the "version" specifier can be used for the model version as planned

[ClemensLinnhoff]

I merged your branch into this one @MarcelKe. Is this now ready to be merged into main?

[github-actions[bot]]

Cpp-Linter Report :warning:

Some files did not pass the configured checks!

clang-tidy reports: 34 concern(s)

- src/MySensorModel.cpp /src/MySensorModel.cpp:17:25: warning: [cppcoreguidelines-avoid-non-const-global-variables] > variable 'private_log_file' is non-const and globally accessible, consider making it const ```cpp ofstream MySensorModel::private_log_file; ^ ``` - src/MySensorModel.h /src/MySensorModel.h:62:21: warning: [cppcoreguidelines-avoid-non-const-global-variables] > variable 'private_log_file' is non-const and globally accessible, consider making it const ```h static ofstream private_log_file; ^ ``` /src/MySensorModel.h:94:21: warning: [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers] > 1024 is a magic number; consider replacing it with a named constant ```h char buffer[1024]; ^ ``` /src/MySensorModel.h:98:27: warning: [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers] > 1024 is a magic number; consider replacing it with a named constant ```h vsnprintf(buffer, 1024, format, arg); ^ ``` - src/OSMP.cpp /src/OSMP.cpp:50:16: warning: [cppcoreguidelines-avoid-non-const-global-variables] > variable 'private_log_file' is non-const and globally accessible, consider making it const ```cpp ofstream OSMP::private_log_file; ^ ``` /src/OSMP.cpp:60:5: warning: [cppcoreguidelines-pro-type-member-init] > uninitialized record type: 'myaddr' ```cpp union Addrconv ^ ``` /src/OSMP.cpp:69:12: warning: [cppcoreguidelines-pro-type-union-access] > do not access members of unions; use (boost::)variant instead ```cpp myaddr.base.lo = lo; ^ ``` /src/OSMP.cpp:70:12: warning: [cppcoreguidelines-pro-type-union-access] > do not access members of unions; use (boost::)variant instead ```cpp myaddr.base.hi = hi; ^ ``` /src/OSMP.cpp:71:12: warning: [cppcoreguidelines-pro-type-reinterpret-cast] > do not use reinterpret_cast ```cpp return reinterpret_cast(myaddr.address); ^ ``` /src/OSMP.cpp:71:43: warning: [cppcoreguidelines-pro-type-union-access] > do not access members of unions; use (boost::)variant instead ```cpp return reinterpret_cast(myaddr.address); ^ ``` /src/OSMP.cpp:82:5: warning: [cppcoreguidelines-pro-type-member-init] > uninitialized record type: 'myaddr' ```cpp union Addrconv ^ ``` /src/OSMP.cpp:91:12: warning: [cppcoreguidelines-pro-type-union-access] > do not access members of unions; use (boost::)variant instead ```cpp myaddr.address = reinterpret_cast(ptr); ^ ``` /src/OSMP.cpp:91:22: warning: [cppcoreguidelines-pro-type-reinterpret-cast] > do not use reinterpret_cast ```cpp myaddr.address = reinterpret_cast(ptr); ^ ``` /src/OSMP.cpp:92:17: warning: [cppcoreguidelines-pro-type-union-access] > do not access members of unions; use (boost::)variant instead ```cpp hi = myaddr.base.hi; ^ ``` /src/OSMP.cpp:93:17: warning: [cppcoreguidelines-pro-type-union-access] > do not access members of unions; use (boost::)variant instead ```cpp lo = myaddr.base.lo; ^ ``` /src/OSMP.cpp:231:18: warning: [readability-convert-member-functions-to-static] > method 'DoStart' can be made static ```cpp fmi2Status OSMP::DoStart(fmi2Boolean tolerance_defined, fmi2Real tolerance, fmi2Real start_time, fmi2Boolean stop_time_defined, fmi2Real stop_time) ^ ``` /src/OSMP.cpp:236:18: warning: [readability-convert-member-functions-to-static] > method 'DoEnterInitializationMode' can be made static ```cpp fmi2Status OSMP::DoEnterInitializationMode() ^ ``` /src/OSMP.cpp:295:18: warning: [readability-convert-member-functions-to-static] > method 'DoTerm' can be made static ```cpp fmi2Status OSMP::DoTerm() ^ ``` /src/OSMP.cpp:306:1: warning: [cppcoreguidelines-pro-type-member-init] > constructor does not initialize these fields: boolean_vars_, integer_vars_, real_vars_ ```cpp OSMP::OSMP(fmi2String theinstance_name, ^ ``` /src/OSMP.cpp:466:24: warning: [cppcoreguidelines-pro-bounds-constant-array-index] > do not use array subscript when the index is not an integer constant expression ```cpp value[i] = real_vars_[vr[i]]; ^ ``` /src/OSMP.cpp:490:24: warning: [cppcoreguidelines-pro-bounds-constant-array-index] > do not use array subscript when the index is not an integer constant expression ```cpp value[i] = integer_vars_[vr[i]]; ^ ``` /src/OSMP.cpp:507:24: warning: [cppcoreguidelines-pro-bounds-constant-array-index] > do not use array subscript when the index is not an integer constant expression ```cpp value[i] = boolean_vars_[vr[i]]; ^ ``` /src/OSMP.cpp:524:24: warning: [cppcoreguidelines-pro-bounds-constant-array-index] > do not use array subscript when the index is not an integer constant expression ```cpp value[i] = string_vars_[vr[i]].c_str(); ^ ``` /src/OSMP.cpp:541:13: warning: [cppcoreguidelines-pro-bounds-constant-array-index] > do not use array subscript when the index is not an integer constant expression ```cpp real_vars_[vr[i]] = value[i]; ^ ``` /src/OSMP.cpp:558:13: warning: [cppcoreguidelines-pro-bounds-constant-array-index] > do not use array subscript when the index is not an integer constant expression ```cpp integer_vars_[vr[i]] = value[i]; ^ ``` /src/OSMP.cpp:575:13: warning: [cppcoreguidelines-pro-bounds-constant-array-index] > do not use array subscript when the index is not an integer constant expression ```cpp boolean_vars_[vr[i]] = value[i]; ^ ``` /src/OSMP.cpp:592:13: warning: [cppcoreguidelines-pro-bounds-constant-array-index] > do not use array subscript when the index is not an integer constant expression ```cpp string_vars_[vr[i]] = value[i]; ^ ``` - src/OSMP.h /src/OSMP.h:134:21: warning: [cppcoreguidelines-avoid-non-const-global-variables] > variable 'private_log_file' is non-const and globally accessible, consider making it const ```h static ofstream private_log_file; ^ ``` /src/OSMP.h:164:21: warning: [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers] > 1024 is a magic number; consider replacing it with a named constant ```h char buffer[1024]; ^ ``` /src/OSMP.h:168:27: warning: [cppcoreguidelines-avoid-magic-numbers,readability-magic-numbers] > 1024 is a magic number; consider replacing it with a named constant ```h vsnprintf(buffer, 1024, format, arg); ^ ``` - src/OSMPConfig.in.h /src/OSMPConfig.in.h:7:2: error: [clang-diagnostic-error] > invalid preprocessing directive ```h #cmakedefine PUBLIC_LOGGING ^ ``` /src/OSMPConfig.in.h:8:2: error: [clang-diagnostic-error] > invalid preprocessing directive ```h #cmakedefine PRIVATE_LOG_PATH "@PRIVATE_LOG_PATH@" ^ ``` /src/OSMPConfig.in.h:9:2: error: [clang-diagnostic-error] > invalid preprocessing directive ```h #cmakedefine VERBOSE_FMI_LOGGING ^ ``` /src/OSMPConfig.in.h:10:2: error: [clang-diagnostic-error] > invalid preprocessing directive ```h #cmakedefine DEBUG_BREAKS ^ ```

Have any feedback or feature suggestions? Share it here.