Skip to content

Update to disco to be more compliant with FOM module loading #68

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.

Sign up for GitHub

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

Open
jarrodgreene wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update to disco to be more compliant with FOM module loading #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion codebase/src/java/disco/org/openlvc/disco/configuration/RprConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,7 +491,9 @@ private void loadDefaultModules()
"hla/rpr2/RPR-Enumerations_v2.0.xml",
"hla/rpr2/RPR-Physical_v2.0.xml",
"hla/rpr2/RPR-SIMAN_v2.0.xml",
"hla/rpr2/RPR-Warfare_v2.0.xml"
"hla/rpr2/RPR-Warfare_v2.0.xml",
"hla/rpr2/RPR-Aggregate_v2.0.xml",
"hla/rpr2/RPR-SE_v2.0.xml",
Copy link
Contributor

@michaelrfraser michaelrfraser May 30, 2024

Choose a reason for hiding this comment

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

Are these extra files already included in the codebase?

Copy link
Contributor Author

@jarrodgreene jarrodgreene May 31, 2024

Choose a reason for hiding this comment

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

Yeah... I don't know why they weren't included (perhaps we just never used anything in them?)

Copy link
Member

@timpokorny timpokorny May 31, 2024

Choose a reason for hiding this comment

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

These shouldn't be needed by Disco and should be removed. Any module that is not 100% required (because there is a mapper that uses data from it) should be excluded. If other federates/applications need these models they can add them within the scope of their applications.

};

if( getRtiProvider() == RtiProvider.Mak )
Expand Down
7 changes: 5 additions & 2 deletions codebase/src/java/disco/org/openlvc/disco/connection/rpr/model/InteractionClass.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,11 @@ public void addParameter( ParameterClass parameter )
// make sure we don't already have one by this name
if( parameters.containsKey(parameter.getName()) )
{
throw new DiscoException( "Add Parameter Failed: Class [%s] already has parameter with name [%s]",
name, parameter.getName() );
// FIXME - we'll assume that this parameter has the same definition - which makes this fine
// but if it were defined any differently we should error out.
return;
Copy link
Contributor

@michaelrfraser michaelrfraser May 30, 2024

Choose a reason for hiding this comment

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

Ideally this should have equivalency checks on the parameters as per portico. Are we just treating this current change as a stop-gap to get current work out of the way?

Copy link
Contributor Author

@jarrodgreene jarrodgreene May 31, 2024

Choose a reason for hiding this comment

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

Yep exactly. I think the way DISCO is doing this merging would need to change somewhat fundamentally for us to be able to do such equivalency checks - as far as I can tell - the only info we have about parameters at this point is the name, so would need to rework so that we have the full definition of the existing and the incoming parameters.

Copy link
Member

@timpokorny timpokorny May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Disco isn't doing any FOM merging. It is reading the collective set of provided FOM modules to cache information from it so that it can reference it later. The only information required to be stored for Interactions is the name I believe, because it's the only thing from the FOM that Disco uses in operation (gets used in pubsub). I believe this check by itself should just be enough.

Copy link
Member

@timpokorny timpokorny May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No, I lie - Parameters are also needed (for handle lookup).

This will need to expand to just loop over all params and add them to the set of cached parameters. If the RTI allows the set of FOM modules then you know they should comply with any HLA merging rules, so there should be no issue with just cumulatively adding to the store of parameters. That is effectively what happens, so it should be OK.

// throw new DiscoException( "Add Parameter Failed: Class [%s] already has parameter with name [%s]",
// name, parameter.getName() );
}

// store the parameter
Expand Down