Adding a macro to remove fmi logging code in the callback function.

[davidhjp01]

The current fmi logging callback function is slowing down the simulation (even if it is not printing anything) for high frequency sampling applications. For example, the logging function uses 16% of cpu times in a particular simulation (without proxyfmu):

This update provides an option to remove the logging code using the macro option (via conan).

[kyllingstad]

Mostly looks good, just some comments about naming and defaults.

[kyllingstad]

I think the C++ preprocessor macro should be called LIBCOSIM_NO_FMI_LOGGING instead, so that logging is enabled by default. (Practically, this just means that the #ifdef tests should be replaced with #ifndef.)

Note that I think it's fine that the CMake variable is called LIBCOSIM_FMI_LOGGING, since its default value is defined by the option() command. That's not the case for macros, they are undefined by default.

[davidhjp01]

They are updated.

[kyllingstad]

Maybe call this fmi_logging instead?

[davidhjp01]

changed to no_fmi_logging with a default False

[kyllingstad]

LGTM. Thanks!

[markaren]

Before merging, should we discuss how this can be used in practice?
What if users of the demo app or cli want to disable logging?

Would it be better to use a runtime option, so that everyone can make use of the feature?

[davidhjp01]

I made an update to pass an argument to cosim::default_model_uri_resolver and cosim::fmi::importer::create(), so that the logging can be disabled during runtime.

I'm not sure I agree that it should be a runtime thing or, if it is, that this is the right way to do it. But I don't have time to re-review right now, so I'll just retract my approval for now and try to get back to it later.

[davidhjp01]

LIBCOSIM_NO_FMI_LOGGING is now read from the environment variable.

[kyllingstad]

Disabling logging by passing a no-op logger function looks like a good solution.

However, as a general rule, I don't think libraries should take their settings from environment variables. We don't know which applications libcosim will be embedded in, and this could potentially cause user environment variables to break third-party applications. The library should be under full control of the application that uses it, which means that this setting must be controllable via the library API—if it's really needed, that is. (See my question at the end.)

So then the question is where to put it. In the FMI subsystem (cosim::fmi), the natural place seems to be in cosim::fmi::fmu::instantiate_slave(), for example something like this:

virtual std::shared_ptr<slave_instance> instantiate_slave(
    std::string_view instanceName,
    bool disable_logging = false) = 0;

This could be easily implemented in terms of @davidhjp01's no-op logger function.

But our current applications typically don't use this API directly, they use the orchestration layer (in cosim/orchestration.hpp). And there, we don't make a difference between FMU slaves and other kinds of slaves (e.g. proxyfmu). Does it make sense to have a dedicated disable_logging parameter added to cosim::model::instantiate()? I'm not sure.

There are other ways to deal with this, though. For example, (and I think I would prefer this,) it could be limited to the FMI subsystem by a modification to cosim::fmu_file_uri_sub_resolver, e.g. as a resolver-level parameter passed to the constructor:

class fmu_file_uri_sub_resolver : public model_uri_sub_resolver
{
public:
    fmu_file_uri_sub_resolver(disable_logging = false);
    ...

From there, it could be transferred on to each particular cosim::fmu_model instance, and then on to cosim::fmi::fmu::instantiate_slave() from within cosim::fmu_model::instantiate.

But I still don't feel like we've really discussed the necessity of all this. @markaren only asked the question, he didn't say it was 100% necessary. Do anyone else have opinions?

[davidhjp01]

Anyone has further opinion on this? I am fine with either approach but do think the runtime option would be more flexible to use.

[davidhjp01]

I reverted the commits back to use the macro for disabling logging. If no one has other opinions I will merge this version when @kyllingstad accepts.

[davidhjp01]

I will merge this branch soon if no-one has further comments.

[kyllingstad]

LGTM