Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align model with template #1

Merged
merged 93 commits into from
Jul 11, 2023
Merged

Align model with template #1

merged 93 commits into from
Jul 11, 2023

Conversation

ClemensLinnhoff
Copy link
Contributor

@ClemensLinnhoff ClemensLinnhoff commented Mar 24, 2023

Add a description
This PR aligns the camera model with the template inkl. documentation structure and CI test pipeline.

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

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

Sorry, something went wrong.

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff ClemensLinnhoff added bug Something isn't working documentation Improvements or additions to documentation labels Mar 24, 2023
ClemensLinnhoff and others added 4 commits March 24, 2023 10:52

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>
…hlich die doCalc Funktion und alle Funktionen, die daraus aufgerufen werden. Dabei am besten den Code direkt auch etwas aufräumen. Da ist recht viel auskommentiert und die Funktionen sind teilweise etwas lang.

• Dateinamen anpassen (Aktuell HelloWorldSensor.cpp und HelloWorldSensor.h)
• Die modelDescription.in.xml rüberkopieren und ggf. anpassen
• Die CMakeLists.txt ggf. anpassen (Wenn keine neue Code-Dateien von euch dazukommen müsste das eigentlich nur der Modellname und die Dateinamen sein. Alle Libraries (FMI, OSI etc. sind schon richtig eingebunden.)
• Die Teile aus der alten Readme in die neue Readme Struktur kopieren.
• Die zur Readme gehörigen Bilder in den doc Ordner ziehen und in der Readme richitg verlinken (Das Video könntet ihr dabei in ein gif umwandeln und dann direkt in die Readme integrieren.
• Den Namen der Modell FMU in der build.yml anpassen, wie ganz oben im Readme Template beschrieben. Dann sollte die Testpipeline auch schon durchlaufen können.
doc/README.md Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
@MarzenaFranek
Copy link
Contributor

MarzenaFranek commented Apr 14, 2023 via email

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>
format

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>
format

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>
format

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>
@ClemensLinnhoff
Copy link
Contributor Author

Looks very good! I tried to fix the last clang-format remarks, but in my opinion the formatting is fine.

The DCO sign-off check fails, as not all commit are signed off. But since your last ones are, and we are doing a squash merge anyways, I am fine with ignoring it. Therefore, I give my approval to the PR.
Before merging, we need one more review from another maintainer.

workflow_call:

env:
FMU_FILE_NAME: OSMPCameraSensor.fmu #TODO: Adjust to file name of your FMU
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ClemensLinnhoff this is outdated now, as we use the repo name as FMU name, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but let's merge this PR first, then adapt to naming the FMU according to the repo. This requirement did not exist, when @MarzenaFranek startet, so I would like to keep it separate.

if(EXISTS "${CMAKE_SOURCE_DIR}/../.git")
set(default_build_type "Debug")
endif()
project(OSMPCameraSensor)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ClemensLinnhoff would this also need to be adapted to the repo name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily, but as above, I would adapt this in a separate PR.

@jdsika jdsika added the enhancement New feature or request label Apr 17, 2023
@jdsika
Copy link

jdsika commented Apr 17, 2023

For the sake of showing the use of the MPL 2.0 OSMP sensor packaging in combination with a model implementation and looking at the weak copy left of MPL, shouldn't we split the function into a different file? In this case the function will also be MPL but we had the diuscussion about how this could be used in combination with other licenses? @ClemensLinnhoff

@ClemensLinnhoff
Copy link
Contributor Author

For the sake of showing the use of the MPL 2.0 OSMP sensor packaging in combination with a model implementation and looking at the weak copy left of MPL, shouldn't we split the function into a different file? In this case the function will also be MPL but we had the diuscussion about how this could be used in combination with other licenses? @ClemensLinnhoff

Yes, I think this would generally be a good idea! Would it be possible for you @MarzenaFranek to do this? So put the model contents (pretty much everything in the doCalc function) into a separate file? Then we would have a clean separation between model and OSMP.

@MarzenaFranek
Copy link
Contributor

MarzenaFranek commented Apr 20, 2023 via email

@ClemensLinnhoff
Copy link
Contributor Author

I just added the separation to the sl-1-0-sensor-model-repository-template. Now there is an OSMP.cpp file with the packaging and a MySensorModel.cpp file with the actual model in the src directory.
@MarzenaFranek you can basically copy this concept to the camera model.

MarzenaFranek and others added 13 commits July 4, 2023 10:44

Verified

This commit was signed with the committer’s verified signature. The key has expired.
…in combination with a model implementation and looking at the weak copy left of MPL, we split the function into a different file

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Signed-off-by: Marzena Franek <[email protected]>

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Signed-off-by: Marzena Franek <[email protected]>

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Signed-off-by: Marzena Franek <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature. The key has expired.
Signed-off-by: Marzena Franek <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>

Verified

This commit was signed with the committer’s verified signature.
ClemensLinnhoff Clemens Linnhoff
Signed-off-by: ClemensLinnhoff <[email protected]>
@github-actions
Copy link

Cpp-Linter Report ⚠️

Some files did not pass the configured checks!

clang-tidy reports: 22 concern(s)
  • src/OSMP.cpp

    /src/OSMP.cpp:60:5: warning: [cppcoreguidelines-pro-type-member-init]

    uninitialized record type: 'myaddr'

        union Addrconv
        ^

    /src/OSMP.cpp:69:12: warning: [cppcoreguidelines-pro-type-union-access]

    do not access members of unions; use (boost::)variant instead

        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

        myaddr.base.hi = hi;
               ^

    /src/OSMP.cpp:71:12: warning: [cppcoreguidelines-pro-type-reinterpret-cast]

    do not use reinterpret_cast

        return reinterpret_cast<void*>(myaddr.address);
               ^

    /src/OSMP.cpp:71:43: warning: [cppcoreguidelines-pro-type-union-access]

    do not access members of unions; use (boost::)variant instead

        return reinterpret_cast<void*>(myaddr.address);
                                              ^

    /src/OSMP.cpp:82:5: warning: [cppcoreguidelines-pro-type-member-init]

    uninitialized record type: 'myaddr'

        union Addrconv
        ^

    /src/OSMP.cpp:91:12: warning: [cppcoreguidelines-pro-type-union-access]

    do not access members of unions; use (boost::)variant instead

        myaddr.address = reinterpret_cast<unsigned long long>(ptr);
               ^

    /src/OSMP.cpp:91:22: warning: [cppcoreguidelines-pro-type-reinterpret-cast]

    do not use reinterpret_cast

        myaddr.address = reinterpret_cast<unsigned long long>(ptr);
                         ^

    /src/OSMP.cpp:92:17: warning: [cppcoreguidelines-pro-type-union-access]

    do not access members of unions; use (boost::)variant instead

        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

        lo = myaddr.base.lo;
                    ^

    /src/OSMP.cpp:231:18: warning: [readability-convert-member-functions-to-static]

    method 'DoStart' can be made static

    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

    fmi2Status OSMP::DoEnterInitializationMode()
                     ^

    /src/OSMP.cpp:297:18: warning: [readability-convert-member-functions-to-static]

    method 'DoTerm' can be made static

    fmi2Status OSMP::DoTerm()
                     ^

    /src/OSMP.cpp:308:1: warning: [cppcoreguidelines-pro-type-member-init]

    constructor does not initialize these fields: boolean_vars_, integer_vars_, real_vars_

    OSMP::OSMP(fmi2String theinstance_name,
    ^

    /src/OSMP.cpp:468:24: warning: [cppcoreguidelines-pro-bounds-constant-array-index]

    do not use array subscript when the index is not an integer constant expression

                value[i] = real_vars_[vr[i]];
                           ^

    /src/OSMP.cpp:492:24: warning: [cppcoreguidelines-pro-bounds-constant-array-index]

    do not use array subscript when the index is not an integer constant expression

                value[i] = integer_vars_[vr[i]];
                           ^

    /src/OSMP.cpp:509:24: warning: [cppcoreguidelines-pro-bounds-constant-array-index]

    do not use array subscript when the index is not an integer constant expression

                value[i] = boolean_vars_[vr[i]];
                           ^

    /src/OSMP.cpp:526:24: warning: [cppcoreguidelines-pro-bounds-constant-array-index]

    do not use array subscript when the index is not an integer constant expression

                value[i] = string_vars_[vr[i]].c_str();
                           ^

    /src/OSMP.cpp:543:13: warning: [cppcoreguidelines-pro-bounds-constant-array-index]

    do not use array subscript when the index is not an integer constant expression

                real_vars_[vr[i]] = value[i];
                ^

    /src/OSMP.cpp:560:13: warning: [cppcoreguidelines-pro-bounds-constant-array-index]

    do not use array subscript when the index is not an integer constant expression

                integer_vars_[vr[i]] = value[i];
                ^

    /src/OSMP.cpp:577:13: warning: [cppcoreguidelines-pro-bounds-constant-array-index]

    do not use array subscript when the index is not an integer constant expression

                boolean_vars_[vr[i]] = value[i];
                ^

    /src/OSMP.cpp:594:13: warning: [cppcoreguidelines-pro-bounds-constant-array-index]

    do not use array subscript when the index is not an integer constant expression

                string_vars_[vr[i]] = value[i];
                ^

Have any feedback or feature suggestions? Share it here.

@ClemensLinnhoff
Copy link
Contributor Author

ClemensLinnhoff commented Jul 11, 2023

Data Cluster Maintainer Decision: Merge as is, update CI pipeline according to template in new PR.

@ClemensLinnhoff ClemensLinnhoff merged commit 59c8b08 into main Jul 11, 2023
@ClemensLinnhoff ClemensLinnhoff deleted the align-with-template branch July 11, 2023 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants