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

Feature/209 add of initial class for cefx template and study template for default subsystems #221

Open
wants to merge 27 commits into
base: development
Choose a base branch
from

Conversation

DimitriDiantos
Copy link

To accomplish this task, we followed the steps outlined below:

Created the CEF X package.
Developed various classes in accordance with the principles of the CEF Template and implemented them.
Incorporated the different components of the template into the template Menu to ensure visibility.
Updated the "Create DLR CEFX Equipment" command.
Addressed and resolved any errors encountered during the process.

Default subsystems encompasses:
Power
Structure
AOCS
Payload
DataHandling.

@franzTobiasDLR
Copy link
Member

franzTobiasDLR commented Mar 14, 2024 via email

Copy link

codecov bot commented Mar 14, 2024

Codecov Report

Merging #221 (ddddf31) into development (993494a) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff               @@
##             development     #221   +/-   ##
==============================================
  Coverage          88.00%   88.00%           
  Complexity           518      518           
==============================================
  Files                 74       74           
  Lines               1750     1750           
  Branches             217      217           
==============================================
  Hits                1540     1540           
  Misses               134      134           
  Partials              76       76           

@DimitriDiantos
Copy link
Author

Hi Dimitri, didn’t we close this PR? I thought we decided that all features of this PR are also in this: #209 Why do we have it here again? Cheers Tobi Von: Dimitri Ngatcha Pokouane @.> Gesendet: Donnerstag, 14. März 2024 10:55 An: virtualsatellite/VirtualSatellite4-CEF @.> Cc: Franz, Tobias @.>; Review requested @.> Betreff: Re: [virtualsatellite/VirtualSatellite4-CEF] Feature/209 add of initial class for cefx template and study template for default subsystems (PR #221) @DimitriDiantoshttps://github.com/DimitriDiantos requested your review on: #221<#221> Feature/209 add of initial class for cefx template and study template for default subsystems. — Reply to this email directly, view it on GitHub<#221 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AL2YSL4K3MC55H3IFX5OK43YYFXYBAVCNFSM6AAAAABEVYJ5JOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGEYTKNJWGQ2TOMA. You are receiving this because your review was requested.Message ID: @.@.>>

Hi Dimitri, didn’t we close this PR? I thought we decided that all features of this PR are also in this: #209 Why do we have it here again? Cheers Tobi Von: Dimitri Ngatcha Pokouane @.> Gesendet: Donnerstag, 14. März 2024 10:55 An: virtualsatellite/VirtualSatellite4-CEF @.> Cc: Franz, Tobias @.>; Review requested @.> Betreff: Re: [virtualsatellite/VirtualSatellite4-CEF] Feature/209 add of initial class for cefx template and study template for default subsystems (PR #221) @DimitriDiantoshttps://github.com/DimitriDiantos requested your review on: #221<#221> Feature/209 add of initial class for cefx template and study template for default subsystems. — Reply to this email directly, view it on GitHub<#221 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AL2YSL4K3MC55H3IFX5OK43YYFXYBAVCNFSM6AAAAABEVYJ5JOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGEYTKNJWGQ2TOMA. You are receiving this because your review was requested.Message ID: @.@.>>

Hi Dimitri, didn’t we close this PR? I thought we decided that all features of this PR are also in this: #209 Why do we have it here again? Cheers Tobi Von: Dimitri Ngatcha Pokouane @.> Gesendet: Donnerstag, 14. März 2024 10:55 An: virtualsatellite/VirtualSatellite4-CEF @.> Cc: Franz, Tobias @.>; Review requested @.> Betreff: Re: [virtualsatellite/VirtualSatellite4-CEF] Feature/209 add of initial class for cefx template and study template for default subsystems (PR #221) @DimitriDiantoshttps://github.com/DimitriDiantos requested your review on: #221<#221> Feature/209 add of initial class for cefx template and study template for default subsystems. — Reply to this email directly, view it on GitHub<#221 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AL2YSL4K3MC55H3IFX5OK43YYFXYBAVCNFSM6AAAAABEVYJ5JOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGEYTKNJWGQ2TOMA. You are receiving this because your review was requested.Message ID: @.@.>>

Because i couldn't push the update on github. I got a message like the failed. You can see the picture attached.
Screenshot 2024-03-12 144701

@dellerDLR
Copy link
Contributor

dellerDLR commented Mar 14, 2024

Hi Dimitri, didn’t we close this PR? I thought we decided that all features of this PR are also in this: #209 Why do we have it here again? Cheers Tobi Von: Dimitri Ngatcha Pokouane @.> Gesendet: Donnerstag, 14. März 2024 10:55 An: virtualsatellite/VirtualSatellite4-CEF _@**._> Cc: Franz, Tobias _@.>; Review requested @._> Betreff: Re: [virtualsatellite/VirtualSatellite4-CEF] Feature/209 add of initial class for cefx template and study template for default subsystems (PR #221) @DimitriDiantoshttps://github.com/DimitriDiantos requested your review on: #221<#221> Feature/209 add of initial class for cefx template and study template for default subsystems. — Reply to this email directly, view it on GitHub<#221 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AL2YSL4K3MC55H3IFX5OK43YYFXYBAVCNFSM6AAAAABEVYJ5JOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGEYTKNJWGQ2TOMA. You are receiving this because your review was requested.Message ID: _@.@.**_>>

Hi Dimitri, didn’t we close this PR? I thought we decided that all features of this PR are also in this: #209 Why do we have it here again? Cheers Tobi Von: Dimitri Ngatcha Pokouane @.> Gesendet: Donnerstag, 14. März 2024 10:55 An: virtualsatellite/VirtualSatellite4-CEF _@**._> Cc: Franz, Tobias _@.>; Review requested @._> Betreff: Re: [virtualsatellite/VirtualSatellite4-CEF] Feature/209 add of initial class for cefx template and study template for default subsystems (PR #221) @DimitriDiantoshttps://github.com/DimitriDiantos requested your review on: #221<#221> Feature/209 add of initial class for cefx template and study template for default subsystems. — Reply to this email directly, view it on GitHub<#221 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AL2YSL4K3MC55H3IFX5OK43YYFXYBAVCNFSM6AAAAABEVYJ5JOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGEYTKNJWGQ2TOMA. You are receiving this because your review was requested.Message ID: _@.@.**_>>

Hi Dimitri, didn’t we close this PR? I thought we decided that all features of this PR are also in this: #209 Why do we have it here again? Cheers Tobi Von: Dimitri Ngatcha Pokouane @.> Gesendet: Donnerstag, 14. März 2024 10:55 An: virtualsatellite/VirtualSatellite4-CEF _@**._> Cc: Franz, Tobias _@.>; Review requested @._> Betreff: Re: [virtualsatellite/VirtualSatellite4-CEF] Feature/209 add of initial class for cefx template and study template for default subsystems (PR #221) @DimitriDiantoshttps://github.com/DimitriDiantos requested your review on: #221<#221> Feature/209 add of initial class for cefx template and study template for default subsystems. — Reply to this email directly, view it on GitHub<#221 (comment)>, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AL2YSL4K3MC55H3IFX5OK43YYFXYBAVCNFSM6AAAAABEVYJ5JOVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJSGEYTKNJWGQ2TOMA. You are receiving this because your review was requested.Message ID: _@.@.**_>>

Because i couldn't push the update on github. I got a message like the failed. You can see the picture attached. Screenshot 2024-03-12 144701

Hey Dima,
did you try to pull (resolve conflicts if needed) and push again?
The same you have to do with this branch (if we proceed here) to make it up to date with the dev branch

@dellerDLR
Copy link
Contributor

From a code point of view it looks good to me.
But the template button is grayed out, eventhough the needed concepts are activated:
image

@dellerDLR
Copy link
Contributor

Also, dont forget to update this branch, so it has the current changes of development

@dellerDLR
Copy link
Contributor

Okay, the problem was that the CommandHelper is using the new discipline API.
@DimitriDiantos Do not forget to use the release target platform, since this feature will be included in the 4.16.2 release.
As a result you have to set the discipline name as follows ``newDiscipline.setUser(UserRegistry.getInstance().getUserName());`.

Furthermore the ElementDefinitions do not have PowerEquipment parameters.
A bonus would be also to have a Systemmodes Subsystem. But we should focus on the things above first

@DimitriDiantos
Copy link
Author

DimitriDiantos commented Mar 14, 2024

From a code point of view it looks good to me. But the template button is grayed out, eventhough the needed concepts are activated: image

Hello Dennis,

Thanks for the feedback, but I just tried again and everything is working fine on my end. Please take a look at the attached photo. Maybe you'll need to try again or re-import or check out the branch again.

@DimitriDiantos
Copy link
Author

Okay, the problem was that the CommandHelper is using the new discipline API. @DimitriDiantos Do not forget to use the release target platform, since this feature will be included in the 4.16.2 release. As a result you have to set the discipline name as follows ``newDiscipline.setUser(UserRegistry.getInstance().getUserName());`.

Furthermore the ElementDefinitions do not have PowerEquipment parameters. A bonus would be also to have a Systemmodes Subsystem. But we should focus on the things above first

Thank you for the feedback. Initially, I used the target release platform and made the modifications accordingly. However, when I pushed to GitHub, an error occurred indicating that 'setUser' did not exist. Consequently, I had to switch to the target development platform. If you could verify this, you will see.

@DimitriDiantos
Copy link
Author

DimitriDiantos commented Mar 14, 2024

Okay, the problem was that the CommandHelper is using the new discipline API. @DimitriDiantos Do not forget to use the release target platform, since this feature will be included in the 4.16.2 release. As a result you have to set the discipline name as follows ``newDiscipline.setUser(UserRegistry.getInstance().getUserName());`.

Furthermore the ElementDefinitions do not have PowerEquipment parameters. A bonus would be also to have a Systemmodes Subsystem. But we should focus on the things above first

@DimitriDiantos
Copy link
Author

From a code point of view it looks good to me. But the template button is grayed out, eventhough the needed concepts are activated: image

Hello Dennis,

Thanks for the feedback, but I just tried again and everything is working fine on my end. Please take a look at the attached photo. Maybe you'll need to try again or re-import or check out the branch again.

CEFX

@DimitriDiantos
Copy link
Author

Okay, the problem was that the CommandHelper is using the new discipline API. @DimitriDiantos Do not forget to use the release target platform, since this feature will be included in the 4.16.2 release. As a result you have to set the discipline name as follows ``newDiscipline.setUser(UserRegistry.getInstance().getUserName());`.

Furthermore the ElementDefinitions do not have PowerEquipment parameters. A bonus would be also to have a Systemmodes Subsystem. But we should focus on the things above first

Thanks for your comment. Initially, I used the target release platform and made the modifications accordingly. However, when I pushed to GitHub, an error occurred indicating that 'setUser' did not exist. Consequently, I had to switch to the target development platform. See the picture attached below.
bug

EC:Equipment in the CT. Also exchanging positions between PT and CT.
Copy link
Member

@franzTobiasDLR franzTobiasDLR left a comment

Choose a reason for hiding this comment

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

Thanks! Looking good :)

…CEFX_template-and_study_template_for_default_subsystems
Copy link
Contributor

@dellerDLR dellerDLR left a comment

Choose a reason for hiding this comment

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

The looks good to me.
Following things I noticed:

  1. The Subsystem under the CT:System has PowerParameters. I think we agreed on only adding mass parameters by default. same with SystemPowerParameters.
  2. When adding a Subsystem or Equipment Power as well as TemperatureParameters are getting added. Should this be the default or only the mass parameters?
  3. An Equipment can be added on Subsystemlevel. But I guess this should be possible due to the dynamic structure of the Product Structure Concept? What do you think @franzTobiasDLR

Despite that I think it can be merged :)

ngat_di added 2 commits March 20, 2024 15:10
…study_template_for_default_subsystems' of https://github.com/virtualsatellite/VirtualSatellite4-CEF.git into feature/209-add_of_initial_class_for_CEFX_template-and_study_template_for_default_subsystems
@DimitriDiantos
Copy link
Author

The looks good to me. Following things I noticed:

  1. The Subsystem under the CT:System has PowerParameters. I think we agreed on only adding mass parameters by default. same with SystemPowerParameters.
  2. When adding a Subsystem or Equipment Power as well as TemperatureParameters are getting added. Should this be the default or only the mass parameters?
  3. An Equipment can be added on Subsystemlevel. But I guess this should be possible due to the dynamic structure of the Product Structure Concept? What do you think @franzTobiasDLR

Despite that I think it can be merged :)

Okay Dennis :). i modified it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants