Use native Matlab functions to display log messages.

[bcoconni]

In this PR, the logging feature is used to display log messages using mexPrintf and similar native Matlab functions. This ensures that the messages are displayed in the Matlab command window rather than in the terminal (if there is one).

I have used the function ssSetErrorStatus for fatal errors which, according to the docs, seems the most appropriate. Let me know if another function would be more appropriate.

Turn off the display of spaces in the diff to review the code because there are quite a lot of trailing spaces that have been removed.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 25.22%. Comparing base (077e835) to head (c79e0cd).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1409   +/-   ##
=======================================
  Coverage   25.22%   25.22%           
=======================================
  Files         169      169           
  Lines       18633    18633           
=======================================
  Hits         4701     4701           
  Misses      13932    13932           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[agodemar]

Well done @bcoconni
OK for me to merge this PR

[seanmcleod70]

Hmm, especially given the recent discussion in #1407 regarding exception handling I was wondering what the situation regarding exceptions is when Matlab/Simulink call JSBSim's s-functions and JSBSim potentially throws an exception.

Does Matlab/Simulink handle C++ exceptions coming from s-function implementations? Do they start a separate process so that any unhandled C++ exceptions, or OS exceptions don't take down the main Matlab/Simulink process?

I don't have a copy of Matlab/Simulink installed, so I can't really test.

So taking a look at the Matlab/Simulink documentation:

Handle Errors in S-Functions

It looks like they support fairly fine-grained in terms of allowing the s-function developer to specify which s-functions are exception free and which ones aren't, and Matlab/Simulink then take the appropriate action, e.g. wrapping the call in the try/catch before calling if the function hasn't been marked as exception free.

You can avoid simulation overhead by ensuring that your C MEX S-function contains entirely exception free code. Exception free code refers to code that never long-jumps.

Setting this option increases the performance of your S-function by allowing the Simulink engine to bypass the exception-handling setup that is usually performed prior to each S-function invocation. You must take extreme care to verify that your code is exception free when using SS_OPTION_EXCEPTION_FREE_CODE. If your S-function generates an exception when this option is set, unpredictable results occur.

[bcoconni]

Not sure about what Matlab is exactly doing regarding C++ exceptions. However it seems to me that this PR and the current code in master comply with the guidance given in the link you have provided: their recommandation is to use ssSetErrorStatus (or ssSetLocalErrorStatus if we want to be thread-safe ?) then to return from the S-function to Simulink.

I have checked in the code of our S-function and could not find any occurrence of ssSetOptions that would have declared the code as exception free, and there are no try/catch statements. So my understanding is that Simulink handles the exceptions thrown by JSBSim. And since PR #1251 the logging carried by LogException is processed when the exception is caught i.e., in this case, ssSetErrorStatusis called when Simulink handles the exception, which complies to the intent, no ?

[seanmcleod70]

Yep, so first off Matlab/Simulink support and handle exceptions, so it's an example as in the discussion with @AirshipSim of a host program handling exceptions that may propagate from a library that it loads and calls, in this case an s-function wrapper of JSBSim. Although Matlab/Simulink do also offer, for performance reasons they state, an option for the library to state it won't propagate exceptions back to Matlab/Simulink.

I also took a look at the time and didn't see JSBSim specifying that it was exception free, which means we don't need to worry about exceptions not being caught in JSBSim's s-functions, they can simply propagate back to Matlab/Simulink. So all good.

their recommendation is to use ssSetErrorStatus then to return from the S-function to Simulink.

the logging carried by LogException is processed when the exception is caught i.e., in this case, ssSetErrorStatus is called when Simulink handles the exception, which complies to the intent, no ?

There is a subtle difference. Since we don't have a try\catch within our s-function, it means that the LogException will actually propagate back to the caller of our s-function, and then call ssSetErrorStatus, as opposed to calling ssSetErrorStatus and then returning from the s-function.

Now I'm guessing it may not make a practical difference. But without an installation of Matlab/Simulink I can't currently test it.

I noticed that all the s-functions (mdlInitializeConditions, mdlUpdate etc.) return void. So they can't return a specific return value to indicate an error. If they don't throw an exception then the way to indicate an error is to call ssSetErrorStatus and then return immediately, e.g.

void mdlUpdate()
{
   if(!checkInputs()) {
      ssSetEErrorStatus();
      return;
   }

   if(!doUpdate()) {
      ssSetEErrorStatus();
      return;
   } 
}

The other option is to throw an exception and let that propagate back to Matlab/Simulink's code that calls the s-functions.

[bcoconni]

I entirely agree with your statements but I am not sure where I should go from there ? Granted that the exception management of the S-function could most certainly be improved. But that is not the point of this PR.

If the concern is that the code returns and then it calls ssErrorStatus in some Matlab function where it should not then I can replace it with a call to mexPrintf or std::cout... or even do nothing because we are unsure of the functions that are allowed to be called from Simulink's exception catcher:

   case LogLevel::FATAL:
     break;

If the point is that this PR should fix the exception management then I am afraid I will not be able to do that since I do not have access to a Matlab license either. In that case, I suggest to discard this PR and wait for someone with a Matlab license to volunteer and fix that.

[bcoconni]

So I have given some further thoughts to your comments @seanmcleod70 and revisited my code.

It occurred to me that when we are in Simulink's exception handler, it is preferable not to call a function that may throw. I don't know for mexPrintf but std::cout is not noexcept i.e. it can throw. So the best course of action I could imagine is to do nothing in LogMatlab::Flush when the error level is FATAL (knowing that buffer.clear() is guaranteed noexcept by the C++11 standard).

But since we need to display the error messages, all the calls to JSBSim functions in JSBSimInterface have been moved in try/catch blocks that intercept the occurrences of LogException before they reach Simulink's exception handler. And even if LogException instances would leak to Simulink's exception handler then nothing will happen (see above).

Most of the JSBSimInterface methods return a bool that is tested by the S-function. When these methods fail (i.e. return false), the S-function call ssErrorStatus and return, per the recommendations of the Matlab/Simulink docs. For these methods the catch block calls mexPrintf to display the error message and return false so that the error gets processed upstream in the S-function.

The JSBSimInterface constructors and the method JSBSimInterface::Update do not return an error code, so the catch block calls ssErrorStatus and re-throws. The exception is cast to BaseException before rethrowing but it is by an abundance of precaution because LogMatlab::Flush no longer do anything when the error level is FATAL.

[seanmcleod70]

The latest commit looks like the safer option.

[bcoconni]

@agodemar any comments about the update ?

[agodemar]

@bcoconni your last thoughts and commit look reasonable to me. Ok to merge

[bcoconni]

PR merged. Thanks.