-
Notifications
You must be signed in to change notification settings - Fork 19
process: update metamodel safety analysis #104
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
process: update metamodel safety analysis #104
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //src:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
|
@PandaeDo is this ready for review or still in draft? |
7a6985a to
97b50ce
Compare
97b50ce to
30dcf06
Compare
|
@MaximilianSoerenPollak The draft shall show that I still work on it. In aspect to that our remarks are also warm welcome. |
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some questions & some changes needed.
Any questions let me know.
|
If those things are changed. Here are the resulting errors: folder_templates/features/feature_name/requirements/index.rst:48: WARNING: feat_req__feature_name__some_title: parent need These then also need to be fixed I think. Though that should be us. I'm a bit unsure why it checks 'EXTERNAL' needs here.. that's a bit worrysome. |
Ref: closes eclipse-score#102
Ref: closes eclipse-score#54
Ref: closes eclipse-score#54
Ref: closes eclipse-score#54
src/extensions/score_metamodel/tests/rst/graph/test_metamodel_graph.rst
Outdated
Show resolved
Hide resolved
| :id: feat_req__parent__abcd | ||
|
|
||
| .. Checks if the child requirement has the at least the same safety level as the parent requirement. It's allowed to "overfill" the safety level of the parent. | ||
| .. ASIL decomposition is not foreseen in S-CORE. Therefore it's not allowed to have a child requirement with a lower safety level than the parent requirement as |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just hint, in this case we would need to check e.g. ASIL B -> QM(B) or ASIL A(B)
masc2023
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for now to test, but one comment, in current release we want only ASIL B, not ASIL D, so how can we find ASIL D requirements automatically and let them not be in the code. May needs some discussion
That should be an easy check, just checking if safety is ASIL_D in an requirements. |
aschemmel-tech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comments
| prefix: feat_saf_fmea__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__fault_models__[0-9a-z_]*$ | ||
| violation_id: ^.*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be "failure_mode"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed to be consitent accross the types if I remember correctly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned to the actual PR it's "fault_models". ](eclipse-score/process_description#54)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, consistent is nice, but according to #54 above the name of the attribute and the content to be written is completely inconsistent: ":violation_cause: "description of failure effect of the fault model on the element"" - the attribute asks for the "cause" and the description is about the "effect"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we discuss this then tomorrow?
| title: FMEA | ||
| prefix: feat_saf_fmea__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__fault_models__[0-9a-z_]*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should start with prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected it
| title: DFA | ||
| prefix: feat_plat_saf_dfa__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__dfa_failure_initiators__[0-9a-z_]*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should start with prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^feat_plat_saf_dfa__[0-9a-z_]*$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected it
| title: DFA | ||
| prefix: comp_saf_dfa__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__dfa_failure_initiators__[0-9a-z_]*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should start with prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected it
| title: FMEA | ||
| prefix: comp_saf_fmea__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__fault_models__[0-9a-z_]*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should start with prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected it
| prefix: comp_saf_fmea__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__fault_models__[0-9a-z_]*$ | ||
| violation_id: ^.*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be "failure_mode"
| mandatory_options: | ||
| id: ^gd_guidl__fault_models__[0-9a-z_]*$ | ||
| violation_id: ^.*$ | ||
| violation_cause: ^.*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be "failure_effect"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aligned to the actual PR it's "fault_models". ](eclipse-score/process_description#54)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above
| mandatory_options: | ||
| id: ^gd_guidl__fault_models__[0-9a-z_]*$ | ||
| violation_id: ^.*$ | ||
| violation_cause: ^.*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be "failure_effect"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After discussion I changed it to violation. If needed we shall discuss it in our round.
| violation_cause: ^.*$ | ||
| mitigation_issue: ^https://github.com/.*$ | ||
| sufficient: ^(yes|no)$ | ||
| argument: ^.+$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only occurence of "argument" - in the process templates it is in every DFA and FMEA. I am not sure if we discussed to remove this and put the in the description of the "need".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument should be removed as an attribute. It was decided the argument is inside the 'conent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this one. We deleted it in the metamodel and add a note into the templates in the actual PR that the argument is inside the contend and therefore its mandatory.
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall great work, thank you.
Some small comments, requests & questions from my side.
| title: DFA | ||
| prefix: feat_plat_saf_dfa__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__dfa_failure_initiators__[0-9a-z_]*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^feat_plat_saf_dfa__[0-9a-z_]*$
| violation_id: ^.*$ | ||
| violation_cause: ^.*$ | ||
| mitigation_issue: ^https://github.com/.*$ | ||
| sufficient: ^(yes|no)$ | ||
| status: ^(valid|invalid)$ | ||
| mandatory_links: | ||
| mitigates: ^(feat_req__.*|aou_req__.*|)$ | ||
| verifies: ^feat_arc_sta__[0-9a-z_]*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this and any of the other new need types.
Are ALL mentioned links & options/attributes mandatory ? Or are some optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the mitigation_issue to optional_links
| prefix: feat_saf_fmea__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__fault_models__[0-9a-z_]*$ | ||
| violation_id: ^.*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was changed to be consitent accross the types if I remember correctly?
| violation_cause: ^.*$ | ||
| mitigation_issue: ^https://github.com/.*$ | ||
| sufficient: ^(yes|no)$ | ||
| argument: ^.+$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument should be removed as an attribute. It was decided the argument is inside the 'conent
src/extensions/score_metamodel/tests/rst/options/test_options_options.rst
Show resolved
Hide resolved
Ref: closes eclipse-score#102
Ref: closes eclipse-score#102
aschemmel-tech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to discuss if we want "failure effect" documented at least for FMEA
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one more question from me, but I think this is ok.
From my side we can bring this in, depending on the verifies naming status?
src/extensions/score_metamodel/BUILD
Outdated
| args = [ | ||
| "-s", | ||
| "-vv", | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick:
Should be removed.
This can be done in next PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| sufficient: ^(yes|no)$ | ||
| status: ^(valid|invalid)$ | ||
| mandatory_links: | ||
| mitigates: ^(feat_req__.*|aou_req__.*)$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I saw in the process that 'mitigates' can be left empty.
If this is the case, then this should be a optional_link not madatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mitigation link can be open as long as there is no mitigation (gd_guidl__safety_analysis). To finish a safety analysis a sufficient and linked mitigation is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That then means though that this should be optional?
| prefix: feat_saf_fmea__ | ||
| mandatory_options: | ||
| id: ^gd_guidl__fault_models__[0-9a-z_]*$ | ||
| violation_id: ^.*$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we discuss this then tomorrow?
aschemmel-tech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok with merge to enable piloting
aschemmel-tech
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine for me to merge.
Signed-off-by: Volker Häussler <[email protected]>
MaximilianSoerenPollak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Ref: closes #102