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

Setting a default error callback, two ideas #266

Open
radarsat1 opened this issue Oct 16, 2021 · 5 comments
Open

Setting a default error callback, two ideas #266

radarsat1 opened this issue Oct 16, 2021 · 5 comments

Comments

@radarsat1
Copy link
Contributor

Looking at #228, the idea is that it is problematic not to be able to provide an error handler before the API was initialized, making it impossible to catch and display warnings in some other context.

Two ideas to handle this:

  1. Provide a static default RtMidi::setDefaultErrorHandler which can be called early in the program and will be used if no RtMidi object has a specific handler set. This is implemented here: radarsat1@c5bff5e
  2. The other way I thought of is to specify such a callback as an argument to RtMidiIn and RtMidiOut constructors, so that they are available at the earliest moment. This is implemented here: radarsat1@5d7a494

To be honest I am not sure which is best, they both seem to have some advantages. For (2), the default value for the parameter is nullptr, so it is not required to give anything here and RtMidi will work just as before. Similar for (1), if setDefaultErrorHandler is not called. So neither changes the API.

I think it comes down to preference, so asking for opinions. Constructor arguments or static function?

@insolace
Copy link
Contributor

A constructor argument would be my preference. Would this potentially replace the try/catch statements for sending and connecting to ports?

@keinstein
Copy link

What is the benefit of an error handler over try/catch?
Actually you could implement your own error handler even with try/catch easily.

void exceptioncaught() {
  try {
    throw;
  } catch (const exception1 & e) {
    ...
  } catch (eonst exception2 & e) {
    ...
  }
}
void func1
  try {
    RtMidiIn in(...);
  } catch (...) {
    exceptioncaught()
  }
}

With the callback solution you just drop the type safety for exceptions.

On the other hand in the background threads you cannot throw exceptions and callbacks must be realtime ready. So you must not interact with the user in such a callback. Otherwise everything might break.

Error reporting starts to get interesting after a port is opened, because RtMidi cannot pass information it received before to the application. So checking if everything is ok can be done when the port is opened. And before you open a port there is plenty of time to install a callback.

Global callbacks have the additional drawback that you must think about a way to find out which object is affected when the callback is called. Note: you don't have access to the internal state of the API.

@radarsat1
Copy link
Contributor Author

@keinstein interestingly i was trying to implement your own idea here from #228 ;) So to be honest I am not sure, maybe exceptions are fine in fact, in the case of #228? There is the idea in RtAudio at least of moving away from the use of exceptions...

I completely open to the possibility that there is no need for this change.. but I guess with the existence of the setErrorCallback function it did seem to make sense to me that the constructor being the one place it cannot be used is a bit awkward.

@keinstein
Copy link

Moving away from exceptions would be moving away from C++.
There is ongoing activity to move RtMidi towards C. At least the C wrapper has to translate all exceptions back into something C can handle. Patches from that directions have usually more C flavour than C++ flavour.

OK. RtMidi never used the full power of C++ and its type checking algorithms (e. g. it uses void pointers to pass user data instead of using inheritence and virtual functions, same with the backend data). In the internet you can find statements that complain that RtMidi follows C philosophy.

Modern exception implementations are fast as long as there are no exceptions thrown. So from that point of view throwing them in functions that are called from user code can simplify the code a lot. On the other hand in backend callbacks (e.g. MIDI receive callbacks) the calling function usually doesn't know how to deal with exceptions. Thus they are dangerous in these callbacks as uncaught exceptions may kill the software. Here we need error callbacks for communication with the user code.

My concerns against putting the error callback into the constructor is that it bloats the constructor interface if we are adding more and more parameters to it. And it gets hard for the user code to handle them, as they have a strict linear order in which they can be omitted. If you have 10 optional parameters, the first optional parameter is nearly mandatory as it must be provided always when one optional parameter shall be changed. If we had a proper way to pass arbitrary parameters to the backends, then a realtime error callback parameter could be one of them. I personally miss the options to control the JACK server using RtMidi. In automated tests we would like to start it automatically, while in user applications this probably should be configurable by the end user.

The more I think about it, the more I think it's right to make a destinction between error callbacks for the realtime part and using exceptions for the non-realtime part.

@vananasun
Copy link

vananasun commented Nov 30, 2021

Bumping this :) I wouldn't opt against exceptions because there is a separate C++ API out there, and one could just manually set the error handler in the struct before the "constructor" was called.

To me, exceptions seem like the most elegant and consistent solution to this problem, because minimal changes to the code are needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants