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

C app interface #2691

Merged
merged 24 commits into from
Nov 12, 2024
Merged

C app interface #2691

merged 24 commits into from
Nov 12, 2024

Conversation

phlptp
Copy link
Member

@phlptp phlptp commented Nov 2, 2024

Summary

If merged this pull request will add an interface for the helicsApps in the C shared library

Proposed changes

@phlptp phlptp added the Interfaces Issue relating to one of the wrapper interfaces label Nov 2, 2024
@phlptp phlptp requested a review from afisher1 November 8, 2024 19:35
Copy link

@josephmckinsey josephmckinsey left a comment

Choose a reason for hiding this comment

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

I am sure I am missing something due to a lack of familiarity with the structure of the project.

@@ -239,6 +239,66 @@ const char* helicsEndpointGetDefaultDestination(HelicsEndpoint endpoint)
return str.c_str();
}

void helicsEndpointSendString(HelicsEndpoint endpoint, const char* message, HelicsError* err)

Choose a reason for hiding this comment

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

These functions are new and need to be included in the Python et al interfaces.

* @return An opaque value app object nullptr if the object creation failed.
*/
HELICS_EXPORT HelicsApp
helicsCreateApp(const char* appName, const char* appType, const char* configFile, HelicsFederateInfo fedInfo, HelicsError* err);

Choose a reason for hiding this comment

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

Why are these in a folder called backup?

Copy link
Member Author

Choose a reason for hiding this comment

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

If python is present and enabled they are automatically generated and the backups now used. But if not these files get used.

* @param[in,out] err An error object that will contain an error code and string if any error occurred during the execution of the function.

*
* @return An opaque value app object nullptr if the object creation failed.

Choose a reason for hiding this comment

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

These are the files we need to actually need to make available to other languages. This is a reminder to myself.


auto M = helicsEndpointGetMessage(epid2);
// ASSERT_TRUE (M);
ASSERT_EQ(helicsMessageGetByteCount(M), 500);

Choose a reason for hiding this comment

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

It would be nice to set a strange time for the message and then verify that exactly that time is propagated to the message object.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe that is tested in the main suite of message federate tests, here we are mainly testing that the api call works as intended.

int index{-2};
int valid{0};
};

Choose a reason for hiding this comment

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

Why is this only necessary now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Quick answer is no one needed it before now. There are some desired applications (by various programs) that require the connector to be able to execute in the same process as other applications, so it made the most sense to expose other apps in the same way as those will also likely be needed in the future.

@josephmckinsey
Copy link

I forgot to mention, but the build failure for helics looks real.

src/helics/shared_api_library/MessageFederate.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
src/helics/shared_api_library/helicsApps.h Outdated Show resolved Hide resolved
@phlptp phlptp merged commit 4ee0f9d into develop Nov 12, 2024
16 of 19 checks passed
@phlptp phlptp deleted the C_APP_INTERFACE branch November 12, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Interfaces Issue relating to one of the wrapper interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants