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

Replace fmi3GetXXXStatus functions #484

Open
pmai opened this Issue Nov 6, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@pmai
Collaborator

pmai commented Nov 6, 2018

The fmi3GetXXXStatus functions of the Co-Simulation API are somewhat of an exception to the general API design of FMI1/2: Instead of having proper functions for each functionality, they try to create generic functions for getting any and all kinds of Status information from the slave FMU:

fmi2Status fmi2GetStatus       (fmi2Component c, const fmi2StatusKind s, fmi2Status* value);
fmi2Status fmi2GetRealStatus   (fmi2Component c, const fmi2StatusKind s, fmi2Real* value);
fmi2Status fmi2GetIntegerStatus(fmi2Component c, const fmi2StatusKind s, fmi2Integer* value);
fmi2Status fmi2GetBooleanStatus(fmi2Component c, const fmi2StatusKind s, fmi2Boolean* value);
fmi2Status fmi2GetStringStatus (fmi2Component c, const fmi2StatusKind s, fmi2String* value);

typedef enum { fmi2DoStepStatus,   fmi2PendingStatus,   fmi2LastSuccessfulTime,   fmi2Terminated }
fmi2StatusKind;

However this has obvious problems:

  • There are still only 4 pieces of information defined (with 4 different types), yet we need 5 functions, one of which is never used (fmi2GetIntegerStatus).
  • The different status kinds that can be enquired differ in the states in which they can actually be inquired, yet, since they are enquired with the generic functions, we cannot express this succinctly in the state machine (which is why the Status functions are currently allowed in more states than they make sense.
  • The status being enquired is defined as one enum, hence IDEs/linters do not complain about e.g. calling fmi2GetStringStatus(c,fmi2LastSuccessfulTime,...), which is illegal (i.e. you need to consult the standard to find out the allowed combinations).
  • To keep the set complete, we now would need to add fmi3GetBinaryStatus, without having a use case for that.
  • With the changeover to the new numeric types in FCP-017 we would need to rename fmi3GetIntegerStatus to fmi3GetInt32Status, for no obvious gain, since that function is unused in any case.

So I think this part of the standard should not be kept as is:

  1. I think at a minimum we should get rid of fmi3GetIntegerStatus, if there is no apparent need for this function.
  2. I think even more preferable would be to just rename the functions to match the things they enquire, and do away with a genericity that has not appeared to be used at all:
fmi3Status fmi3GetPendingDoStepStatus(fmi3Component c, fmi3Status* value);
fmi3Status fmi3GetPendingDoStepMessage(fmi3Component c, fmi3String* value);
fmi3Status fmi3GetLastSuccesfulTime(fmi3Component c, fmi3Real* value);
fmi3Status fmi3GetDiscardedDoStepTerminated(fmi3Component c, fmi3Boolean* value);
  1. Probably even better, turn them into two functions, since the information is actually related:
fmi3Status fmi3GetPendingDoStepStatus(fmi3Component c, fmi3Status* status, fmi3String* message);
fmi3Status fmi3GetDiscardedDoStepStatus(fmi3Component c, fmi3Boolean* terminated, fmi3Real* lastSuccesfulTime);

For both functions the calling environment can pass in NULL to any of the pointers, in order to not retrieve that information (if that is wanted).

  1. Do nothing

What is the overall opinion on this?

@pmai pmai added this to the FMI 3.0 milestone Nov 6, 2018

@t-sommer

This comment has been minimized.

Collaborator

t-sommer commented Nov 6, 2018

  1. Sounds good but I suggest to call the functions fmi3GetDoStepPendingStatus and fmi3GetDoStepDiscardedStatus
@pmai

This comment has been minimized.

Collaborator

pmai commented Nov 6, 2018

Agreed, those names roll a bit more easily off the tongue...

@t-sommer

This comment has been minimized.

Collaborator

t-sommer commented Nov 6, 2018

I've added this issue to the agenda for the next FMI Steering Committee meeting

@KarlWernersson

This comment has been minimized.

Collaborator

KarlWernersson commented Nov 7, 2018

Should we perhaps reconsider support for asynchronous doSetp completely for Fmi 3.0?
As far as I know there is no implementation and no testing of the features at all?

These function has also been discusses within the hybrid co-sim fcp
Finding some unified solution there with the other return data used in that FCP could be preferable.
Especially the last successful time and terminated should be easy to merge.

@pmai

This comment has been minimized.

Collaborator

pmai commented Nov 7, 2018

@KarlWernersson I agree that stripping out the Async stuff is something to consider (we actually implemented this in FMIBench both for import and export, and you can even use FMIBench to turn a non-async FMU into an async one and vice-versa, but as you point out, we haven’t run into actual real-world use of that, and stripping that code path out would make me quite happy).

@andreas-junghanns

This comment has been minimized.

Contributor

andreas-junghanns commented Nov 7, 2018

I do not agree here: asynchronous coupling is always useful in the real-time world. We should ask @APillekeit before we conclude this too eagerly.

@pmai

This comment has been minimized.

Collaborator

pmai commented Nov 7, 2018

@andreas-junghanns We should not rush this, I agree (which is why I currently only propose to make the GetXXXStatus stuff nicer, which is useful regardless of whether we do away with async step).

And obviously asynchronous coupling itself is definitely useful in the real-time world, however I fear the way it is done in FMI1/2 does not lend itself easily to this: It more or less presupposes that the FMU has a mechanism to spawn off its own thread, which means the FMU has control over threading. I think the new FMI3 activation mode is more to the liking of the RT guys, since that leaves control over threading to the master…

Again I agree that we should gather comments on the usefulness of the async stuff prior to any decision to remove it (and we need a seperate issue for that in any case I think).

@pmai

This comment has been minimized.

Collaborator

pmai commented Nov 7, 2018

@KarlWernersson I agree, the new functions should be aligned with the ones needed for the hybrid co-simulation stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment